<div dir="ltr">Thanks Ed for bringing this up and writing a great summary.<div><br></div><div>I can provide some background information about how this was brought up and how requiring ObjectManager (OM) can improve performance.</div><div><br></div><div>On Google hardware with more than 200 sensors, we found that fetching a single sensor in the Redfish route is extremely slow (~0.6 seconds). On the other hand ipmi sdr list gives a fair performance (returns all sensors in seconds). We then root caused it and found that the BMCWeb sensor code is inefficient. We introduced the query parameter into BMCWeb, and submitted <a href="https://gerrit.openbmc.org/c/openbmc/bmcweb/+/52418">https://gerrit.openbmc.org/c/openbmc/bmcweb/+/52418</a> which integrates OM's <a href="https://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-objectmanager">GetManagedObjects</a> with <a href="https://redfish.dmtf.org/schemas/DSP0266_1.15.0.html#the-expand-query-parameter">Redfish Expand</a> query parameters. With it, we got 40+ times speed up when enumerating 200+ sensors.</div><div><br></div><div>```</div><div>Before this change,<br>time wget -qO-<br>'<a href="http://localhost/redfish/v1/Chassis/xxx/Sensors?$expand=.($levels=1)">http://localhost/redfish/v1/Chassis/xxx/Sensors?$expand=.($levels=1)</a>'<br>> /tmp/log_slow.json<br><br>real    0m33.786s<br>user 0m0.000s<br>sys   0m0.000s<br><br>After this change<br>time wget -qO-<br>'<a href="http://localhost/redfish/v1/Chassis/xxx/Sensors?$expand=.($levels=1)">http://localhost/redfish/v1/Chassis/xxx/Sensors?$expand=.($levels=1)</a>'<br>> /tmp/log_fast.json<br><br>real 0m0.769s<br>user  0m0.010s<br>sys   0m0.010s<br></div><div>```</div><div><br></div><div>We found the same pattern (use OM for bulk query) can be applied to most inventory resources (Processors, DIMMS, etc), where similar performance improvements can be achieved.</div><div><br></div><div>Ed brought up this issue that OM is optional as of today in inventories and sensors during the code review of <a href="https://gerrit.openbmc.org/c/openbmc/bmcweb/+/53824">https://gerrit.openbmc.org/c/openbmc/bmcweb/+/53824</a>. The path of the OM is also inconsistent.</div><div><br></div><div>Please let us know what you think.</div><div><br></div><div>Best</div><div>Nan</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jul 12, 2022 at 11:48 AM Ed Tanous <<a href="mailto:edtanous@google.com">edtanous@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">We've had a couple cases in the project where patches have needed to be slowed down because of inconsistencies in our use of object manager, so IMO, the time has come to make our usage of ObjectManager consistent, documented, and drop support for the (not upstream) daemons that don't follow the rules.  As part of this, we will port forward all the upstream daemons to be consistent, and do our best to avoid bugs, but this email is intended to inform those that have non-upstream daemons about the change so they can resolve their implementations.<br><br>The basics:<br>ObjectManager DBus interface will now be required for any daemon implementing a Sensor.Value interface or Inventory.Item interface.<br><br>Daemons producing sensors will be required to place their ObjectManager at /xyz/openbmc_project/sensors<br>Daemons producing inventory items will be required to place their ObjectManager at /xyz/openbmc_project/inventory.<br><br>Both of these interfaces will be required to be published before claiming a well known name on dbus, to allow for the possibility of caching implementations at some point in future.<br><br>Functionally, this gets a little complicated because the sdbusplus::asio::object_manager bindings in their default invocation create an ObjectManager at the root object /, so we need to execute this in a few steps. <br><br>0. Send this email out, outlining the problem, and warn the community that if this is an assumption you make in your downstream daemons, those assumptions will need to change.  Get consensus from maintainers.<br>1. Update the phosphor-dbus-interfaces documentation for both Sensor.Value and Inventory.Item to call out the required-ness of ObjectManager, and the explicit dbus path where it's required to be placed.<br>2. Identify all the asio daemons that need changes, and publish changes that move the object_manager to the appropriate path.  This is mostly going to be a tree-wide grep for sdbusplus::asio::object_server, and look for daemonst that don't make use of the add_manager() API.  Anyone directly calling the Sensor or Invertory ObjectManager interfaces will need to port to the new paths, but luckily, this isn't a very common operation, and I beleive bmcweb and phosphor-ipmid-host might be the only direct users.  In bmcweb, there is actually a convoluted piece of code that uses the mapper to sort out the location that the ObjectManager exists at so Redfish sensors should remain consistent, and we don't yet have code that relies on ObjectManager for Inventory items.  I believe phosphor-ipmi-host has a similar piece of code that should protect us in this case, but I defer to those maintainers.<br><br>A starter list of daemons that need updating is:<br>All daemons in dbus-sensors<br>peci-pcie<br>entity-manager<br>libpeci<div>phosphor-u-boot-env-manager</div><div>pfr-manager</div><div>smbios-mdr<br>s2600-misc<br>dbus-sensors</div><div><br><br>4. Change the defaults of sdbusplus::asio::object_server to not init an ObjectManager interface at the root path /.<br><br>5. Unwind the various complexities of searching for ObjectManager interfaces, and rely on the assumptions above that should net us some speedups in at least the sensors API, completely avoiding an extra mapper call.<br><br>6. Get system owners to retest their platforms, with the tree changes in, and fix any bugs we may have accidentally caused.<br><br>This was discussed pretty heavily on discord in the #development topic, and some of the nitty gritty details on why this is needed is available there, as well as I'm happy to discuss here.  This is one of those nasty spaghetti-code things that we've lived with for a while;  Hopefully if we can get this behind us, we can get some good real world performance improvements.<br></div><div><br></div><div>Thoughts?</div><div><br></div><div>-Ed</div></div>
</blockquote></div>