dbus-sdr: Remove excessive `Failed to GetAll` error message
The `Failed to GetAll` message will happen if any of the sensor doesn't
have `Inventory.Decorator.Ipmi` dbus interface. Instead of making a
`GetAll` call to all sensors, we will check if the sensor have the
interface first before requesting it. This will remove the need to
attempt sigificantly amount of unnecessary dbus calls.
Tested:
ipmitool sdr return the same sensor as before, but with no excessive
error message.
There is also some performance gain after removing the extra dbus calls.
Before,
```
$ time ipmitool sdr | wc -l
346
real 0m13.786s
user 0m0.315s
sys 0m0.115s
$ time ipmitool sdr | wc -l
346
real 0m14.692s
user 0m0.274s
sys 0m0.196s
```
After,
```
$ time ipmitool sdr | wc -l
346
real 0m11.765s
user 0m0.205s
sys 0m0.155s
$ time ipmitool sdr | wc -l
346
real 0m11.403s
user 0m0.269s
sys 0m0.141s
```
Change-Id: Ieab19fbae9e1fb40c2cfac9cd584cf40dba3f500
Signed-off-by: Willy Tu <wltu@google.com>
diff --git a/dbus-sdr/sdrutils.cpp b/dbus-sdr/sdrutils.cpp
index abc8671..e818ef3 100644
--- a/dbus-sdr/sdrutils.cpp
+++ b/dbus-sdr/sdrutils.cpp
@@ -16,6 +16,9 @@
#include "dbus-sdr/sdrutils.hpp"
+#include <optional>
+#include <unordered_set>
+
#ifdef FEATURE_HYBRID_SENSORS
#include <ipmid/utils.hpp>
@@ -365,6 +368,37 @@
return properties;
}
+// Fetch the ipmiDecoratorPaths to get the list of dbus objects that
+// have ipmi decorator to prevent unnessary dbus call to fetch the info
+std::optional<std::unordered_set<std::string>>&
+ getIpmiDecoratorPaths(const std::optional<ipmi::Context::ptr>& ctx)
+{
+ static std::optional<std::unordered_set<std::string>> ipmiDecoratorPaths;
+
+ if (!ctx.has_value() || ipmiDecoratorPaths != std::nullopt)
+ {
+ return ipmiDecoratorPaths;
+ }
+
+ boost::system::error_code ec;
+ std::vector<std::string> paths =
+ (*ctx)->bus->yield_method_call<std::vector<std::string>>(
+ (*ctx)->yield, ec, "xyz.openbmc_project.ObjectMapper",
+ "/xyz/openbmc_project/object_mapper",
+ "xyz.openbmc_project.ObjectMapper", "GetSubTreePaths", "/",
+ int32_t(0),
+ std::array<const char*, 1>{
+ "xyz.openbmc_project.Inventory.Decorator.Ipmi"});
+ if (ec)
+ {
+ return ipmiDecoratorPaths;
+ }
+
+ ipmiDecoratorPaths =
+ std::unordered_set<std::string>(paths.begin(), paths.end());
+ return ipmiDecoratorPaths;
+}
+
const std::string* getSensorConfigurationInterface(
const std::map<std::string, std::vector<std::string>>&
sensorInterfacesResponse)
@@ -402,9 +436,11 @@
// Follow Association properties for Sensor back to the Board dbus object to
// check for an EntityId and EntityInstance property.
-void updateIpmiFromAssociation(const std::string& path,
- const DbusInterfaceMap& sensorMap,
- uint8_t& entityId, uint8_t& entityInstance)
+void updateIpmiFromAssociation(
+ const std::string& path,
+ const std::unordered_set<std::string>& ipmiDecoratorPaths,
+ const DbusInterfaceMap& sensorMap, uint8_t& entityId,
+ uint8_t& entityInstance)
{
namespace fs = std::filesystem;
@@ -456,22 +492,27 @@
// the right interface.
// just try grabbing the properties first.
- std::map<std::string, Value> ipmiProperties =
- getEntityManagerProperties(
- endpoint.c_str(),
- "xyz.openbmc_project.Inventory.Decorator.Ipmi");
+ ipmi::PropertyMap::iterator entityIdProp;
+ ipmi::PropertyMap::iterator entityInstanceProp;
+ if (ipmiDecoratorPaths.contains(endpoint))
+ {
+ std::map<std::string, Value> ipmiProperties =
+ getEntityManagerProperties(
+ endpoint.c_str(),
+ "xyz.openbmc_project.Inventory.Decorator.Ipmi");
- auto entityIdProp = ipmiProperties.find("EntityId");
- auto entityInstanceProp = ipmiProperties.find("EntityInstance");
- if (entityIdProp != ipmiProperties.end())
- {
- entityId =
- static_cast<uint8_t>(std::get<uint64_t>(entityIdProp->second));
- }
- if (entityInstanceProp != ipmiProperties.end())
- {
- entityInstance = static_cast<uint8_t>(
- std::get<uint64_t>(entityInstanceProp->second));
+ entityIdProp = ipmiProperties.find("EntityId");
+ entityInstanceProp = ipmiProperties.find("EntityInstance");
+ if (entityIdProp != ipmiProperties.end())
+ {
+ entityId = static_cast<uint8_t>(
+ std::get<uint64_t>(entityIdProp->second));
+ }
+ if (entityInstanceProp != ipmiProperties.end())
+ {
+ entityInstance = static_cast<uint8_t>(
+ std::get<uint64_t>(entityInstanceProp->second));
+ }
}
// Now check the entity-manager entry for this sensor to see
diff --git a/dbus-sdr/sensorcommands.cpp b/dbus-sdr/sensorcommands.cpp
index 522cbb8..b356f93 100644
--- a/dbus-sdr/sensorcommands.cpp
+++ b/dbus-sdr/sensorcommands.cpp
@@ -117,6 +117,7 @@
"sensors/'",
[](sdbusplus::message_t&) {
getSensorTree().clear();
+ getIpmiDecoratorPaths(/*ctx=*/std::nullopt).reset();
sdrLastAdd = std::chrono::duration_cast<std::chrono::seconds>(
std::chrono::system_clock::now().time_since_epoch())
.count();
@@ -128,6 +129,7 @@
"sensors/'",
[](sdbusplus::message_t&) {
getSensorTree().clear();
+ getIpmiDecoratorPaths(/*ctx=*/std::nullopt).reset();
sdrLastRemove = std::chrono::duration_cast<std::chrono::seconds>(
std::chrono::system_clock::now().time_since_epoch())
.count();
@@ -1591,10 +1593,11 @@
record.key.owner_lun = lun;
record.key.sensor_number = sensornumber;
}
-bool constructSensorSdr(ipmi::Context::ptr ctx, uint16_t sensorNum,
- uint16_t recordID, const std::string& service,
- const std::string& path,
- get_sdr::SensorDataFullRecord& record)
+bool constructSensorSdr(
+ ipmi::Context::ptr ctx,
+ const std::unordered_set<std::string>& ipmiDecoratorPaths,
+ uint16_t sensorNum, uint16_t recordID, const std::string& service,
+ const std::string& path, get_sdr::SensorDataFullRecord& record)
{
constructSensorSdrHeaderKey(sensorNum, recordID, record);
@@ -1634,7 +1637,8 @@
// follow the association chain to get the parent board's entityid and
// entityInstance
- updateIpmiFromAssociation(path, sensorMap, entityId, entityInstance);
+ updateIpmiFromAssociation(path, ipmiDecoratorPaths, sensorMap, entityId,
+ entityInstance);
record.body.entity_id = entityId;
record.body.entity_instance = entityInstance;
@@ -1858,9 +1862,10 @@
}
// Construct a type 3 SDR for VR typed sensor(daemon).
-bool constructVrSdr(ipmi::Context::ptr ctx, uint16_t sensorNum,
- uint16_t recordID, const std::string& service,
- const std::string& path,
+bool constructVrSdr(ipmi::Context::ptr ctx,
+ const std::unordered_set<std::string>& ipmiDecoratorPaths,
+ uint16_t sensorNum, uint16_t recordID,
+ const std::string& service, const std::string& path,
get_sdr::SensorDataEventRecord& record)
{
constructEventSdrHeaderKey(sensorNum, recordID, record);
@@ -1876,7 +1881,8 @@
}
// follow the association chain to get the parent board's entityid and
// entityInstance
- updateIpmiFromAssociation(path, sensorMap, record.body.entity_id,
+ updateIpmiFromAssociation(path, ipmiDecoratorPaths, sensorMap,
+ record.body.entity_id,
record.body.entity_instance);
// Sensor type is hardcoded as a module/board type instead of parsing from
@@ -1908,10 +1914,11 @@
return std::min(getSensorTree().size(), maxIPMISensors);
}
-static int
- getSensorDataRecord(ipmi::Context::ptr ctx,
- std::vector<uint8_t>& recordData, uint16_t recordID,
- uint8_t readBytes = std::numeric_limits<uint8_t>::max())
+static int getSensorDataRecord(
+ ipmi::Context::ptr ctx,
+ const std::unordered_set<std::string>& ipmiDecoratorPaths,
+ std::vector<uint8_t>& recordData, uint16_t recordID,
+ uint8_t readBytes = std::numeric_limits<uint8_t>::max())
{
size_t fruCount = 0;
ipmi::Cc ret = ipmi::storage::getFruSdrCount(ctx, fruCount);
@@ -2053,8 +2060,8 @@
{
constructSensorSdrHeaderKey(sensorNum, recordID, record);
}
- else if (!constructSensorSdr(ctx, sensorNum, recordID, connection, path,
- record))
+ else if (!constructSensorSdr(ctx, ipmiDecoratorPaths, sensorNum,
+ recordID, connection, path, record))
{
return GENERAL_ERROR;
}
@@ -2103,8 +2110,8 @@
{
constructEventSdrHeaderKey(sensorNum, recordID, record);
}
- else if (!constructVrSdr(ctx, sensorNum, recordID, connection, path,
- record))
+ else if (!constructVrSdr(ctx, ipmiDecoratorPaths, sensorNum, recordID,
+ connection, path, record))
{
return GENERAL_ERROR;
}
@@ -2145,8 +2152,12 @@
uint16_t numSensors = getNumberOfSensors();
if (count.value_or(0) == getSdrCount)
{
+ auto& ipmiDecoratorPaths = getIpmiDecoratorPaths(ctx);
+
// Count the number of Type 1 SDR entries assigned to the LUN
- while (!getSensorDataRecord(ctx, record, recordID++))
+ while (!getSensorDataRecord(
+ ctx, ipmiDecoratorPaths.value_or(std::unordered_set<std::string>()),
+ record, recordID++))
{
get_sdr::SensorDataRecordHeader* hdr =
reinterpret_cast<get_sdr::SensorDataRecordHeader*>(
@@ -2346,8 +2357,12 @@
return ipmi::responseResponseError();
}
+ auto& ipmiDecoratorPaths = getIpmiDecoratorPaths(ctx);
+
std::vector<uint8_t> record;
- if (getSensorDataRecord(ctx, record, recordID, offset + bytesToRead))
+ if (getSensorDataRecord(
+ ctx, ipmiDecoratorPaths.value_or(std::unordered_set<std::string>()),
+ record, recordID, offset + bytesToRead))
{
phosphor::logging::log<phosphor::logging::level::ERR>(
"ipmiStorageGetSDR: fail to get SDR");
diff --git a/include/dbus-sdr/sdrutils.hpp b/include/dbus-sdr/sdrutils.hpp
index c4b7cfa..c129876 100644
--- a/include/dbus-sdr/sdrutils.hpp
+++ b/include/dbus-sdr/sdrutils.hpp
@@ -24,9 +24,11 @@
#include <ipmid/api.hpp>
#include <ipmid/types.hpp>
#include <map>
+#include <optional>
#include <phosphor-logging/log.hpp>
#include <sdbusplus/bus/match.hpp>
#include <string>
+#include <unordered_set>
#include <vector>
#pragma once
@@ -370,11 +372,16 @@
std::map<std::string, Value> getEntityManagerProperties(const char* path,
const char* interface);
+std::optional<std::unordered_set<std::string>>&
+ getIpmiDecoratorPaths(const std::optional<ipmi::Context::ptr>& ctx);
+
const std::string* getSensorConfigurationInterface(
const std::map<std::string, std::vector<std::string>>&
sensorInterfacesResponse);
-void updateIpmiFromAssociation(const std::string& path,
- const DbusInterfaceMap& sensorMap,
- uint8_t& entityId, uint8_t& entityInstance);
+void updateIpmiFromAssociation(
+ const std::string& path,
+ const std::unordered_set<std::string>& ipmiDecoratorPaths,
+ const DbusInterfaceMap& sensorMap, uint8_t& entityId,
+ uint8_t& entityInstance);
} // namespace ipmi