Sensor Value PropertiesChanged Events

[Ed Tanous]

On Mon, Feb 1, 2021 at 4:44 PM Bills, Jason M
<jason.m.bills at linux.intel.com> wrote:
>
> Hi All,
>
> There is an issue and idea that James Feist and I chatted about to maybe
> relieve some of our D-Bus traffic.
>
> A major contributor to our D-Bus traffic (as seen in dbus-monitor) is
> the polling sensors updating the xyz.openbmc_project.Sensor.Value.Value
> property on each polling loop, which generates a PropertiesChanged
> signal for every sensor on every polling loop (once per second?).
>
> The concern is that more important D-Bus messages could be getting
> delayed as D-Bus processes these Sensor Value signals.
>
> The idea to fix this is to change the sensors with a custom getter on
> the Value property, so the last read can be pulled from D-Bus using a
> get-property call, but it would no longer signal a PropertiesChanged event.

Doesn't this break..... like... everything that relies on sensor
values changing over time?

>
> I pushed a proposed change here:
> https://gerrit.openbmc-project.xyz/c/openbmc/dbus-sensors/+/40199.
>
> Our original assumption was that nobody was matching on this
> PropertiesChanged signal for the Value property; however, it was pointed
> out to me today, that PID control has a match for it and may be using it.

Pid control, telemetry, redfish event service are the ones that
immediately come to mind.  It should be noted, dbus-sensors even uses
that message internally for things like CFM sensor, which has to base
its output on a transform of other sensor values, so there'd be a lot
of stuff to fix if we did this, we'd have to make sure it's worth it.

>
> So, I wanted to start a broader community discussion about this issue:
>
> 1. Is this a real concern or are PropertiesChanged signals so
> lightweight that removing them won't help with D-Bus load?

It's a valid concern IMO.  It's arguably the current limit on how fast
we can scan sensors on large-sensor-count BMCs.  In terms of
dbus-sensors architecture stuff, it's next in line on my priority list
after the whole "reading sensors from hwmon blocks the thread"
problem.

>
> 2. Does anyone need to match on sensor Value property updates or is
> reading them with get-property enough?

See above.  Lots of stuff needs property value updates, and moving all
that stuff back to polling doesn't really seem like an option, or a
good thing.

>
> 3. Does PID control use the Value match?  If so and there are benefits
> to removing these signals, could PID control manage without them?

Phosphor-pid-control and friends could theoretically move to polling,
but that seems much much worse, and increases the dbus traffic
overall, given that every poll would now have to go round trip through
both processes, instead of one way.

>
>
> As a side note, I still have two remaining services that publish
> PropertiesChanged events on sensor Value properties:
>
> PWM Sensors.  I have a proposed (and untested) change here:
> gerrit.openbmc-project.xyz/c/openbmc/dbus-sensors/+/40200.
>
> A Power sensor, that I will track down based on this discussion.


One thought I've had in this area was to rearrange/redefine the dbus
interfaces such that a single properties changed (or equivalent) could
batch multiple sensor values together, thus avoiding the per-message
cost, while still keeping the eventing available.

The three basic strawman ideas I've had for this in
phosphor-dbus-interfaces yaml would be something like

NewSensorValueValueAPI
- name: Names
type: array<string>
- name: Values
type: array<double>
description: >
The sensor reading.
- name: MaxValues
type: array<double>
description: >
The Maximum supported sensor reading.
- name: MinValues
type: array<double>
description: >
The Minimum supported sensor reading.
- name: Units
type: array<enum[self.Unit]>


Because all properties are vector<double>, and Names are specified in
a single property, the PropertiesChanged events could batch together
10s of sensors in a single message, and use a tombstone value for
"haven't updated since the last update"

The other thought was we could completely delete the Value property
from Sensor.Value, and replace it with a SensorManager that would live
at /xyz/openbmc_project/sensors, which would publish a
SensorValuesUpdated signal with a dict of name and value, again,
allowing sensor producers to batch the sensor reads, but still keeping
it reasonably close to the existing API.

In both cases, the implementation in dbus-sensors or phosphor-hwmon
would be something like "read as many sensors as I can in 250ms, then
batch it up and send out one event" the 250ms timer would help with
stuck sensors, and avoid a lot of latency if the system is overloaded.

The third one is a little more out there.  We could change the
sensor.Value.Value property into an FD type, and point to a memmapped
area of memory into which we write the current sensor value.  That
way, the "sensor value" on dbus rarely changes, but you can always
read the current state of the sensor with zero overhead or context
switching to the sensor processes.  If this works, it has the
potential to speed up most sensor polling operations by an order of
magnitude, but seems hard to do, and has a lot of questions.  Does
inotify work on mmaped files?  How do FD permissions work when sent
over dbus?  How do you "lock" the memory region to write it (or do you
not have to)?



With all of the above said, I think we really need to take a look and
profile why individual dbus messages are so slow, and if it's fixable
at a lower layer than the interfaces.  I know there were some efforts
to put the broker into the kernel that kinda petered out, but I was
never clear as to why.  I wanted to try grabbing those patches to see
what performance benefit they gave at some point.

>
> Thanks!
> -Jason