From b74977c19f5ad227559ddd22256f3156f2be6da1 Mon Sep 17 00:00:00 2001 From: Souvik Roy Date: Wed, 8 Jan 2025 00:04:17 -0600 Subject: [PATCH 1/2] Remove redundant async call in FRU collection threads (#588) 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 #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//task" 3. Reboot the BMC several times and repeat Step 2. ``` Change-Id: I603c64dc9b5057429a2288f0edfde6086755b851 Signed-off-by: Souvik Roy --- vpd-manager/include/worker.hpp | 1 + vpd-manager/src/worker.cpp | 37 +++++++++++++++++++++------------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/vpd-manager/include/worker.hpp b/vpd-manager/include/worker.hpp index a0da85d9a..3384b5e9f 100644 --- a/vpd-manager/include/worker.hpp +++ b/vpd-manager/include/worker.hpp @@ -80,6 +80,7 @@ class Worker * Note: Config JSON file path should be passed to worker class constructor * to make use of this API. * + * @throw std::runtime_error */ void collectFrusFromJson(); diff --git a/vpd-manager/src/worker.cpp b/vpd-manager/src/worker.cpp index 531dc6e59..1214ad070 100644 --- a/vpd-manager/src/worker.cpp +++ b/vpd-manager/src/worker.cpp @@ -1508,22 +1508,31 @@ void Worker::collectFrusFromJson() continue; } - std::thread{[vpdFilePath, this]() { - auto l_futureObject = std::async(&Worker::parseAndPublishVPD, this, - vpdFilePath); - - std::tuple l_threadInfo = l_futureObject.get(); + try + { + std::thread{[vpdFilePath, this]() { + const auto& l_parseResult = parseAndPublishVPD(vpdFilePath); - // thread returned. - m_mutex.lock(); - m_activeCollectionThreadCount--; - m_mutex.unlock(); + m_mutex.lock(); + m_activeCollectionThreadCount--; + m_mutex.unlock(); - if (!m_activeCollectionThreadCount) - { - m_isAllFruCollected = true; - } - }}.detach(); + if (!m_activeCollectionThreadCount) + { + m_isAllFruCollected = true; + } + }}.detach(); + } + catch (const std::exception& l_ex) + { + // TODO: Should we re-try launching thread for this FRU? + EventLogger::createSyncPel( + types::ErrorType::InvalidVpdMessage, types::SeverityType::Alert, + __FILE__, __FUNCTION__, 0, + std::string("Failed to start collection thread for FRU :[" + + vpdFilePath + "]. Error: " + l_ex.what()), + std::nullopt, std::nullopt, std::nullopt, std::nullopt); + } } } From 1e18b817d882f78ef047cf0c9fa7d6d4d55ed844 Mon Sep 17 00:00:00 2001 From: Souvik Roy Date: Tue, 21 Jan 2025 03:53:12 -0600 Subject: [PATCH 2/2] Add list of EEPROMs for which collection thread failed (#588) 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 --- vpd-manager/include/manager.hpp | 6 ++++++ vpd-manager/include/worker.hpp | 20 +++++++++++++++++++- vpd-manager/src/manager.cpp | 15 +++++++++++++++ vpd-manager/src/worker.cpp | 21 +++++++++++++-------- 4 files changed, 53 insertions(+), 9 deletions(-) diff --git a/vpd-manager/include/manager.hpp b/vpd-manager/include/manager.hpp index cb64123b4..fd5ca8c63 100644 --- a/vpd-manager/include/manager.hpp +++ b/vpd-manager/include/manager.hpp @@ -255,6 +255,12 @@ class Manager * @param[in] i_msg - The callback message. */ void processAssetTagChangeCallback(sdbusplus::message_t& i_msg); + + /** + * @brief API to process list of EEPROMs for which VPD collection thread + * creation has failed. + */ + void ProcessFailedEeproms(); #endif /** diff --git a/vpd-manager/include/worker.hpp b/vpd-manager/include/worker.hpp index 3384b5e9f..660ac76fa 100644 --- a/vpd-manager/include/worker.hpp +++ b/vpd-manager/include/worker.hpp @@ -80,7 +80,6 @@ class Worker * Note: Config JSON file path should be passed to worker class constructor * to make use of this API. * - * @throw std::runtime_error */ void collectFrusFromJson(); @@ -147,6 +146,22 @@ class Worker return m_activeCollectionThreadCount; } + /** + * @brief API to get the list of EEPROMs for which VPD collection thread + * creation has failed. + * + * This API returns the list of EEPROMs for which VPD collection thread + * creation has failed by reference. Manager needs to process this list of + * EEPROMs and take appropriate action. + * + * @return reference to list of EEPROM paths for which VPD collection thread + * creation has failed + */ + std::forward_list& getListOfEepromPathsThreadFailed() noexcept + { + return m_failedEepromPaths; + } + private: /** * @brief An API to parse and publish a FRU VPD over D-Bus. @@ -534,5 +549,8 @@ class Worker // Counting semaphore to limit the number of threads. std::counting_semaphore m_semaphore{ constants::MAX_THREADS}; + + // List of EEPROM paths for which VPD collection thread creation has failed. + std::forward_list m_failedEepromPaths; }; } // namespace vpd diff --git a/vpd-manager/src/manager.cpp b/vpd-manager/src/manager.cpp index 7f9efd487..550955403 100644 --- a/vpd-manager/src/manager.cpp +++ b/vpd-manager/src/manager.cpp @@ -236,6 +236,8 @@ void Manager::SetTimerToDetectSVPDOnDbus() m_interface->set_property("CollectionStatus", std::string("InProgress")); m_worker->collectFrusFromJson(); + + ProcessFailedEeproms(); } }); } @@ -910,4 +912,17 @@ void Manager::performVpdRecollection() std::string(l_ex.what())); } } + +void Manager::ProcessFailedEeproms() +{ + if (m_worker.get() != nullptr) + { + // TODO: + // - iterate through list of EEPROMs for which thread creation has + // failed + // - get list of FRUs under the EEPROM + // - For each FRU, extract the object path and do collect single FRU + m_worker->getListOfEepromPathsThreadFailed().clear(); + } +} } // namespace vpd diff --git a/vpd-manager/src/worker.cpp b/vpd-manager/src/worker.cpp index 1214ad070..6711d76a0 100644 --- a/vpd-manager/src/worker.cpp +++ b/vpd-manager/src/worker.cpp @@ -1517,7 +1517,8 @@ void Worker::collectFrusFromJson() m_activeCollectionThreadCount--; m_mutex.unlock(); - if (!m_activeCollectionThreadCount) + if (!m_activeCollectionThreadCount && + m_failedEepromPaths.empty()) { m_isAllFruCollected = true; } @@ -1525,13 +1526,17 @@ void Worker::collectFrusFromJson() } catch (const std::exception& l_ex) { - // TODO: Should we re-try launching thread for this FRU? - EventLogger::createSyncPel( - types::ErrorType::InvalidVpdMessage, types::SeverityType::Alert, - __FILE__, __FUNCTION__, 0, - std::string("Failed to start collection thread for FRU :[" + - vpdFilePath + "]. Error: " + l_ex.what()), - std::nullopt, std::nullopt, std::nullopt, std::nullopt); + try + { + // add vpdFilePath(EEPROM path) to failed list + m_failedEepromPaths.push_front(vpdFilePath); + } + catch (const std::exception& l_ex) + { + logging::logMessage( + "Failed to add [" + vpdFilePath + + "] to failed EEPROM list. Error: " + l_ex.what()); + } } } }