Remove redundant async call in FRU collection threads #588

souvik1914581 · 2025-01-15T07:32:12Z

This commit removes the redundant std:async call in the detached thread
launched for parsing and publishing the VPD for an individual FRU.
Since we have a dedicated detached thread for each FRU, we can do VPD
parse and publish in a synchronous manner from the detached thread
itself, instead of launching a asynchronous task which adds unnecessary
performance cost.

This commit addresses issue #558.

Test:

1. Install bitbaked image on Everest (ever6bmc)
2. After initial boot, check:
	- BMC State Ready
	- vpd-manager's CollectionStatus property should be "Completed"
	busctl get-property com.ibm.VPD.Manager
	/com/ibm/VPD/Manager com.ibm.VPD.Manager
	CollectionStatus
	s "Completed"
	- vpd-manager status should be running with no restarts
	- vpd-manager should have only 1 thread running:
	  check "ls -la /proc/<vpd-manager PID>/task"
3. Reboot the BMC several times and repeat Step 2.

Change-Id: I603c64dc9b5057429a2288f0edfde6086755b851
Signed-off-by: Souvik Roy [email protected]

This commit removes the redundant std:async call in the detached thread launched for parsing and publishing the VPD for an individual FRU. Since we have a dedicated detached thread for each FRU, we can do VPD parse and publish in a synchronous manner from the detached thread itself, instead of launching a asynchronous task which adds unnecessary performance cost. Test: ``` 1. Install bitbaked image on Everest (ever6bmc) 2. After initial boot, check: - BMC State Ready - vpd-manager's CollectionStatus property should be "Completed" busctl get-property com.ibm.VPD.Manager /com/ibm/VPD/Manager com.ibm.VPD.Manager CollectionStatus s "Completed" - vpd-manager status should be running with no restarts - vpd-manager should have only 1 thread running: check "ls -la /proc/<vpd-manager PID>/task 3. Reboot the BMC several times and repeat Step 2. ``` Change-Id: I603c64dc9b5057429a2288f0edfde6086755b851 Signed-off-by: Souvik Roy <[email protected]>

This commit removes the redundant std:async call in the detached thread launched for parsing and publishing the VPD for an individual FRU. Since we have a dedicated detached thread for each FRU, we can do VPD parse and publish in a synchronous manner from the detached thread itself, instead of launching a asynchronous task which adds unnecessary performance cost. Test: ``` 1. Install bitbaked image on Everest (ever6bmc) 2. After initial boot, check: - BMC State Ready - vpd-manager's CollectionStatus property should be "Completed" busctl get-property com.ibm.VPD.Manager /com/ibm/VPD/Manager com.ibm.VPD.Manager CollectionStatus s "Completed" - vpd-manager status should be running with no restarts - vpd-manager should have only 1 thread running: check "ls -la /proc/<vpd-manager PID>/task" 3. Reboot the BMC several times and repeat Step 2. ``` Change-Id: I603c64dc9b5057429a2288f0edfde6086755b851 Signed-off-by: Souvik Roy <[email protected]>

This commit removes the redundant std:async call in the detached thread launched for parsing and publishing the VPD for an individual FRU. Since we have a dedicated detached thread for each FRU, we can do VPD parse and publish in a synchronous manner from the detached thread itself, instead of launching a asynchronous task which adds unnecessary performance cost. This commit addresses issue ibm-openbmc#558. Test: ``` 1. Install bitbaked image on Everest (ever6bmc) 2. After initial boot, check: - BMC State Ready - vpd-manager's CollectionStatus property should be "Completed" busctl get-property com.ibm.VPD.Manager /com/ibm/VPD/Manager com.ibm.VPD.Manager CollectionStatus s "Completed" - vpd-manager status should be running with no restarts - vpd-manager should have only 1 thread running: check "ls -la /proc/<vpd-manager PID>/task" 3. Reboot the BMC several times and repeat Step 2. ``` Change-Id: I603c64dc9b5057429a2288f0edfde6086755b851 Signed-off-by: Souvik Roy <[email protected]>

branupama · 2025-01-16T13:39:46Z

vpd-manager should have only 1 thread running

once collection is completed, there will be no thread running for parseAndPublishVPD.
this scenario will be same without any changes.
Commit message needs update I guess.

souvik1914581 · 2025-01-16T13:59:55Z

once collection is completed, there will be no thread running for parseAndPublishVPD.
this scenario will be same without any changes.
Commit message needs update I guess.

Yes, what I meant is there will be a single thread (the "main" thread) running for vpd-manager process, which we can see in /proc/<vpd-manager PID>/task directory.

vpd-manager/src/worker.cpp

This commit removes the redundant std:async call in the detached thread launched for parsing and publishing the VPD for an individual FRU. Since we have a dedicated detached thread for each FRU, we can do VPD parse and publish in a synchronous manner from the detached thread itself, instead of launching a asynchronous task which adds unnecessary performance cost. This commit addresses issue ibm-openbmc#558. Test: ``` 1. Install bitbaked image on Everest (ever6bmc) 2. After initial boot, check: - BMC State Ready - vpd-manager's CollectionStatus property should be "Completed" busctl get-property com.ibm.VPD.Manager /com/ibm/VPD/Manager com.ibm.VPD.Manager CollectionStatus s "Completed" - vpd-manager status should be running with no restarts - vpd-manager should have only 1 thread running: check "ls -la /proc/<vpd-manager PID>/task" 3. Reboot the BMC several times and repeat Step 2. ``` Change-Id: I603c64dc9b5057429a2288f0edfde6086755b851 Signed-off-by: Souvik Roy <[email protected]>

This commit removes the redundant std:async call in the detached thread launched for parsing and publishing the VPD for an individual FRU. Since we have a dedicated detached thread for each FRU, we can do VPD parse and publish in a synchronous manner from the detached thread itself, instead of launching a asynchronous task which adds unnecessary performance cost. This commit also handles any exception thrown while launching the detached thread for a FRU. Incase launching detached thread for a FRU fails, we log a PEL and continue launching threads for other FRUs. This commit addresses issue ibm-openbmc#558. Test: ``` 1. Install bitbaked image on Everest (ever6bmc) 2. After initial boot, check: - BMC State Ready - vpd-manager's CollectionStatus property should be "Completed" busctl get-property com.ibm.VPD.Manager /com/ibm/VPD/Manager com.ibm.VPD.Manager CollectionStatus s "Completed" - vpd-manager status should be running with no restarts - vpd-manager should have only 1 thread running: check "ls -la /proc/<vpd-manager PID>/task" 3. Reboot the BMC several times and repeat Step 2. ``` Change-Id: I603c64dc9b5057429a2288f0edfde6086755b851 Signed-off-by: Souvik Roy <[email protected]>

PriyangaRamasamy · 2025-01-17T15:41:11Z

vpd-manager/src/worker.cpp

+        {
+            // TODO: Should we re-try launching thread for this FRU?
+            EventLogger::createSyncPel(
+                types::ErrorType::InvalidVpdMessage, types::SeverityType::Alert,


what does InvalidVpdMessage means?

PriyangaRamasamy · 2025-01-17T15:48:39Z

vpd-manager/src/worker.cpp

+        {
+            // TODO: Should we re-try launching thread for this FRU?
+            EventLogger::createSyncPel(
+                types::ErrorType::InvalidVpdMessage, types::SeverityType::Alert,


little bit of brainstorming is required for types::SeverityType::Alert
Alert refers to - > should be corrected immediately https://github.com/ibm-openbmc/phosphor-dbus-interfaces/blob/1110/yaml/xyz/openbmc_project/Logging/Entry.interface.yaml#L78

-> why Alert is better here and why not Critical or error condition?
who we are alerting here? how will they correct the issue ?

@PriyangaRamasamy , we discussed this post scrum today and decided upon the following design:

Vpd-manager thread creation fails for a FRU:

Create a list of FRUs for which thread failed

Post collection of all FRUs, before setting state == COLLECTED,

Trigger single FRU collection for all FRUs in failed list.

Single FRU collection will log PEL if it again fails.

Need to update Single FRU collection API to handle this case.

Or create a separate API?

As part of this PR, we shall just create a list of FRUs for which thread creation failed, and create a Story for the remaining implementation.

sounds like a plan 👍

This commit removes the redundant std:async call in the detached thread launched for parsing and publishing the VPD for an individual FRU. Since we have a dedicated detached thread for each FRU, we can do VPD parse and publish in a synchronous manner from the detached thread itself, instead of launching a asynchronous task which adds unnecessary performance cost. This commit also handles any exception thrown while launching the detached thread for a FRU. Incase launching detached thread for a FRU fails, we log a PEL and continue launching threads for other FRUs. This commit addresses issue ibm-openbmc#558. Test: ``` 1. Install bitbaked image on Everest (ever6bmc) 2. After initial boot, check: - BMC State Ready - vpd-manager's CollectionStatus property should be "Completed" busctl get-property com.ibm.VPD.Manager /com/ibm/VPD/Manager com.ibm.VPD.Manager CollectionStatus s "Completed" - vpd-manager status should be running with no restarts - vpd-manager should have only 1 thread running: check "ls -la /proc/<vpd-manager PID>/task" 3. Reboot the BMC several times and repeat Step 2. ``` Change-Id: I603c64dc9b5057429a2288f0edfde6086755b851 Signed-off-by: Souvik Roy <[email protected]>

This commit implements a list in Worker which holds the list of VPD file paths(EEPROM paths) for which collection thread creation has failed. After collection of all FRUs and before setting CollectionStatus as Completed, Manager needs to process this list. Note: Processing the list is a TODO. Test: ``` - Install bitbaked image on Everest system. - Check vpd-manager service status: root@p10bmc:~# systemctl show vpd-manager -p NRestarts NRestarts=0 - Check BMC reaches ready state: root@p10bmc:~# obmcutil state CurrentBMCState : xyz.openbmc_project.State.BMC.BMCState.Ready CurrentPowerState : xyz.openbmc_project.State.Chassis.PowerState.On CurrentHostState : xyz.openbmc_project.State.Host.HostState.Running BootProgress : xyz.openbmc_project.State.Boot.Progress.ProgressStages.OSRunning OperatingSystemState: xyz.openbmc_project.State.OperatingSystem.Status.OSStatus.Inactive -Check CollectionStatus property of vpd-manager D-Bus service: root@p10bmc:~# busctl get-property com.ibm.VPD.Manager /com/ibm/VPD/Manager com.ibm.VPD.Manager CollectionStatus s "Completed" ``` Change-Id: I472c54861f590167d8ec767ed6ddb3992c085b0f Signed-off-by: Souvik Roy <[email protected]>

This commit removes the redundant std:async call in the detached thread launched for parsing and publishing the VPD for an individual FRU. Since we have a dedicated detached thread for each FRU, we can do VPD parse and publish in a synchronous manner from the detached thread itself, instead of launching a asynchronous task which adds unnecessary performance cost. This commit also handles any exception thrown while launching the detached thread for a FRU. Incase launching detached thread for a FRU fails, we log a PEL and continue launching threads for other FRUs. This commit addresses issue ibm-openbmc#558. Test: ``` 1. Install bitbaked image on Everest (ever6bmc) 2. After initial boot, check: - BMC State Ready - vpd-manager's CollectionStatus property should be "Completed" busctl get-property com.ibm.VPD.Manager /com/ibm/VPD/Manager com.ibm.VPD.Manager CollectionStatus s "Completed" - vpd-manager status should be running with no restarts - vpd-manager should have only 1 thread running: check "ls -la /proc/<vpd-manager PID>/task" 3. Reboot the BMC several times and repeat Step 2. ``` Change-Id: I603c64dc9b5057429a2288f0edfde6086755b851 Signed-off-by: Souvik Roy <[email protected]>

This commit implements a list in Worker which holds the list of VPD file paths(EEPROM paths) for which collection thread creation has failed. After collection of all FRUs and before setting CollectionStatus as Completed, Manager needs to process this list. Note: Processing the list is a TODO. Test: ``` - Install bitbaked image on Everest system. - Check vpd-manager service status: root@p10bmc:~# systemctl show vpd-manager -p NRestarts NRestarts=0 - Check BMC reaches ready state: root@p10bmc:~# obmcutil state CurrentBMCState : xyz.openbmc_project.State.BMC.BMCState.Ready CurrentPowerState : xyz.openbmc_project.State.Chassis.PowerState.On CurrentHostState : xyz.openbmc_project.State.Host.HostState.Running BootProgress : xyz.openbmc_project.State.Boot.Progress.ProgressStages.OSRunning OperatingSystemState: xyz.openbmc_project.State.OperatingSystem.Status.OSStatus.Inactive -Check CollectionStatus property of vpd-manager D-Bus service: root@p10bmc:~# busctl get-property com.ibm.VPD.Manager /com/ibm/VPD/Manager com.ibm.VPD.Manager CollectionStatus s "Completed" ``` Change-Id: I472c54861f590167d8ec767ed6ddb3992c085b0f Signed-off-by: Souvik Roy <[email protected]>

souvik1914581 force-pushed the sr_vpdManagerPerformanceEnhancement branch from 1b81cd1 to 6c2ea1a Compare January 15, 2025 07:34

souvik1914581 force-pushed the sr_vpdManagerPerformanceEnhancement branch from 6c2ea1a to c7cc559 Compare January 15, 2025 07:36

souvik1914581 force-pushed the sr_vpdManagerPerformanceEnhancement branch from c7cc559 to 6a5fc3f Compare January 15, 2025 07:37

souvik1914581 requested review from SunnySrivastava1984, jinuthomas, branupama, PriyangaRamasamy and RekhaAparna01 January 15, 2025 07:39

PriyangaRamasamy reviewed Jan 16, 2025

View reviewed changes

vpd-manager/src/worker.cpp Outdated Show resolved Hide resolved

vpd-manager/src/worker.cpp Outdated Show resolved Hide resolved

souvik1914581 force-pushed the sr_vpdManagerPerformanceEnhancement branch from 6a5fc3f to bf9dbbf Compare January 17, 2025 07:55

souvik1914581 requested a review from PriyangaRamasamy January 17, 2025 08:00

souvik1914581 force-pushed the sr_vpdManagerPerformanceEnhancement branch from bf9dbbf to 76c6b03 Compare January 17, 2025 11:30

PriyangaRamasamy reviewed Jan 17, 2025

View reviewed changes

souvik1914581 force-pushed the sr_vpdManagerPerformanceEnhancement branch from 76c6b03 to 3e7f583 Compare January 21, 2025 09:56

souvik1914581 marked this pull request as draft January 21, 2025 09:56

souvik1914581 force-pushed the sr_vpdManagerPerformanceEnhancement branch from 3e7f583 to bb9ac0a Compare January 21, 2025 10:56

souvik1914581 force-pushed the sr_vpdManagerPerformanceEnhancement branch from bb9ac0a to c130ba4 Compare January 22, 2025 04:43

souvik1914581 force-pushed the sr_vpdManagerPerformanceEnhancement branch from c130ba4 to d8dd7d9 Compare January 23, 2025 04:45

souvik1914581 force-pushed the sr_vpdManagerPerformanceEnhancement branch from d8dd7d9 to 21d578d Compare January 23, 2025 04:51

souvik1914581 force-pushed the sr_vpdManagerPerformanceEnhancement branch from 21d578d to 1e18b81 Compare January 23, 2025 04:57

souvik1914581 marked this pull request as ready for review January 23, 2025 05:02

souvik1914581 requested a review from PriyangaRamasamy January 23, 2025 05:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove redundant async call in FRU collection threads #588

Remove redundant async call in FRU collection threads #588

Uh oh!

souvik1914581 commented Jan 15, 2025 •

edited

Loading

Uh oh!

branupama commented Jan 16, 2025

Uh oh!

souvik1914581 commented Jan 16, 2025

Uh oh!

Uh oh!

Uh oh!

PriyangaRamasamy Jan 17, 2025

Uh oh!

PriyangaRamasamy Jan 17, 2025

Uh oh!

souvik1914581 Jan 21, 2025

Uh oh!

PriyangaRamasamy Jan 21, 2025

Uh oh!

Uh oh!

Remove redundant async call in FRU collection threads #588

Are you sure you want to change the base?

Remove redundant async call in FRU collection threads #588

Uh oh!

Conversation

souvik1914581 commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

branupama commented Jan 16, 2025

Uh oh!

souvik1914581 commented Jan 16, 2025

Uh oh!

Uh oh!

Uh oh!

PriyangaRamasamy Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

PriyangaRamasamy Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

souvik1914581 Jan 21, 2025

Choose a reason for hiding this comment

Uh oh!

PriyangaRamasamy Jan 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

souvik1914581 commented Jan 15, 2025 •

edited

Loading