DBus ObjectManager Interface usages within the project

[Nan Zhou]

Thanks Ed for bringing this up and writing a great summary.

I can provide some background information about how this was brought up and
how requiring ObjectManager (OM) can improve performance.

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 https://gerrit.openbmc.org/c/openbmc/bmcweb/+/52418 which
integrates OM's GetManagedObjects
<https://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-objectmanager>
with Redfish Expand
<https://redfish.dmtf.org/schemas/DSP0266_1.15.0.html#the-expand-query-parameter>
query parameters. With it, we got 40+ times speed up when enumerating 200+
sensors.

```
Before this change,
time wget -qO-
'http://localhost/redfish/v1/Chassis/xxx/Sensors?$expand=.($levels=1)'
> /tmp/log_slow.json

real 0m33.786s
user 0m0.000s
sys 0m0.000s

After this change
time wget -qO-
'http://localhost/redfish/v1/Chassis/xxx/Sensors?$expand=.($levels=1)'
> /tmp/log_fast.json

real 0m0.769s
user 0m0.010s
sys 0m0.010s
```

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.

Ed brought up this issue that OM is optional as of today in inventories and
sensors during the code review of
https://gerrit.openbmc.org/c/openbmc/bmcweb/+/53824. The path of the OM is
also inconsistent.

Please let us know what you think.

Best
Nan

On Tue, Jul 12, 2022 at 11:48 AM Ed Tanous <edtanous at google.com> wrote:

> 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.
>
> The basics:
> ObjectManager DBus interface will now be required for any daemon
> implementing a Sensor.Value interface or Inventory.Item interface.
>
> Daemons producing sensors will be required to place their ObjectManager at
> /xyz/openbmc_project/sensors
> Daemons producing inventory items will be required to place their
> ObjectManager at /xyz/openbmc_project/inventory.
>
> 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.
>
> 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.
>
> 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.
> 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.
> 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.
>
> A starter list of daemons that need updating is:
> All daemons in dbus-sensors
> peci-pcie
> entity-manager
> libpeci
> phosphor-u-boot-env-manager
> pfr-manager
> smbios-mdr
> s2600-misc
> dbus-sensors
>
>
> 4. Change the defaults of sdbusplus::asio::object_server to not init an
> ObjectManager interface at the root path /.
>
> 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.
>
> 6. Get system owners to retest their platforms, with the tree changes in,
> and fix any bugs we may have accidentally caused.
>
> 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.
>
> Thoughts?
>
> -Ed
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20220712/ce591b9e/attachment.htm>