phosphor-hwmon bottleneck potential

phosphor-hwmon bottleneck potential

[Patrick Venture]

[-- Attachment #1: Type: text/plain, Size: 3647 bytes --]

The system upon which I'm presently running openbmc uses phosphor-hwmon to
expose reading of fans and thermal sensors.  And its works great for
precisely this function, however, when another program attempts to get the
sensor values over dbus from phosphor-hwmon I run into timing issues that
can likely be addressed.

The driver we're using for the fans requires a time period to measure its
RPMs.  I don't know if this is the only driver on earth that does so, but
if it is --then this is only a problem I'll run into.  The time period is
about 1 second.

As it stands now, there is one expected problem and one bizarre one that I
haven't traced down.

Firstly, if I try to read a specific fan speed it takes 8s to get a
response from phosphor-hwmon over dbus.  This may be that it's reading all
the fan speeds before returning; but I haven't verified it.  There are
eight fans on the system being handled by that instance of phosphor-hwmon,
but the 8 seconds could be a coincidence.  I'd need to really look at how
it handles dbus requests to determine that that's the issue.

To avoid this, I registered for listen for Sensor.Value updates over dbus,
however, from looking at the round robin sensor update loop, my hypothesis
is that I would receive an update from each fan once every 8 seconds or so
given its first time to run.  This was effectively what I saw, except of
course it was an update to the sensor every 9 seconds, because my
hypothesis forgot to account for the one second pause between each loop
iteration.

The solution that comes to mind would be to simply parallelize sensor
updates, such that phosphor-hwmon uses threads to update the sensor values
independently of each other.  There are certainly downsides to this
implementation and it won't be 100% optimized because there are switching
overheads with threads, but since my driver read appears to be a
bottleneck, each thread will take their 1 second reading different files
and update the information more rapidly.  This also prevents other sensors
under the same configuration from interfering with general updates to the
dbus sensor namespace.  My system runs four instances of phosphor-hwmon
because of the layout, but others may run different variations and
experience other problems that could be alleviated by multi-threading.

The other suggestion I have to improve involves adding a timestamp to
phosphos-dbus-interfaces for Sensor.Value to let the recipient of the
information know when it was last updated.  Certainly if someone listens
for property updates, they can produce this information themselves, but it
doesn't hurt to provide it to any reader regardless of mechanism and as it
stands now, it feels missing.  It's true that some drivers cache the
information, and phosphor-hwmon itself waits one second before looping
around to update, but a way to reduce some amount of ambiguity would be to
provide a timestamp on the information.  By adding a timestamp the
information would also come across to the reader as more of a time series,
which is more or less is.

Without multi-threading to remove the timing bottleneck on my platform, and
the inability to provide other more direct interfaces for controlling fans,
I'll be unable to use phosphor-hwmon as a primary source for
reading/writing sensor data and will be unable to upstream any effort i
make to write a pluggable thermal controller.

I felt it was important to thoroughly explain my reasoning as other
engineering teams attempting to use openbmc for their platforms could run
into similar problems and that the decision to develop independently was
data-driven and not arbitrary.

Patrick

[-- Attachment #2: Type: text/html, Size: 3951 bytes --]

Re: phosphor-hwmon bottleneck potential

[Brad Bishop]

Hi Patrick, thanks for the feedback!

On Thu, 2017-05-04 at 09:11 -0700, Patrick Venture wrote:
> The system upon which I'm presently running openbmc uses
> phosphor-hwmon to expose reading of fans and thermal sensors.  And its
> works great for precisely this function, however, when another program
> attempts to get the sensor values over dbus from phosphor-hwmon I run
> into timing issues that can likely be addressed.
> 
> 
> The driver we're using for the fans requires a time period to measure
>  its RPMs.  I don't know if this is the only driver on earth that does
> so, but if it is --then this is only a problem I'll run into.

I think this is a pretty critical point.  If this is one-off driver
behavior I'm not very excited about adding special code to
phosphor-hwmon to work around it.  But if its SOP for hwmon drivers, we
should definitely improve phosphor-hwmon to handle long read times (more
on long read times below).

>   The time period is about 1 second.

I'm translating this slightly and assuming it to mean the read system
call on the sysfs attribute blocks phosphor-hwmon for ~1 second.  If
that is the case it would explain the observed behavior.

I didn't consider such long blocking read operations on the sysfs entry
when implementing phosphor-hwmon.

> 
> 
> As it stands now, there is one expected problem and one bizarre one
> that I haven't traced down.
> 
> 
> Firstly, if I try to read a specific fan speed it takes 8s to get a
> response from phosphor-hwmon over dbus.  This may be that it's

This is what I would expect given the read delay.  The Dbus handler
doesn't make read system calls on files in sysfs, but there is a single
loop handling both the dbus socket and updating sensor values.  What is
happening is that the loop body is taking eight seconds so the call to
process dbus traffic is only occurring every eight seconds.

>  reading all the fan speeds before returning; but I haven't verified
> it.  There are eight fans on the system being handled by that instance
> of phosphor-hwmon, but the 8 seconds could be a coincidence.  I'd need
> to really look at how it handles dbus requests to determine that
> that's the issue.
> 
> 
> To avoid this, I registered for listen for Sensor.Value updates over
> dbus, however, from looking at the round robin sensor update loop, my
> hypothesis is that I would receive an update from each fan once every
> 8 seconds or so given its first time to run.  This was effectively
> what I saw, except of course it was an update to the sensor every 9
> seconds, because my hypothesis forgot to account for the one second
> pause between each loop iteration.

Right, each sensor is only being polled once every 8+1 seconds, so
events won't be triggered any faster than that.

> 
> 
> The solution that comes to mind would be to simply parallelize sensor
> updates, such that phosphor-hwmon uses threads to update the sensor

I think this is a great idea.  But I would vote for some kind of
non-blocking or async io rather than threads.  I don't know what support
for that sort of thing is available in the hwmon subsystem so I'm not
sure if its even possible, but it seems worth a look anyway.

>  values independently of each other.  There are certainly downsides to
> this implementation and it won't be 100% optimized because there are
> switching overheads with threads, but since my driver read

non-blocking IO would address these concerns.

>  appears to be a bottleneck, each thread will take their 1 second
> reading different files and update the information more rapidly.  This
> also prevents other sensors under the same configuration from
> interfering with general updates to the dbus sensor namespace.  My
> system runs four instances of phosphor-hwmon because of the layout,
> but others may run different variations and experience other problems
> that could be alleviated by multi-threading.
> 
> 
> The other suggestion I have to improve involves adding a timestamp to
> phosphos-dbus-interfaces for Sensor.Value to let the recipient of the
> information know when it was last updated.  Certainly if someone

When a dbus signal is emitted for a property changed event, the time
associated with the signal is "right now".  When invoking Get on the
sensor value property, again the timestamp of the data provided by the
method call is "right now".

If an application wants to grab a timestamp when it gets a signal or
makes a Get method call and associate it with the sensor value I think
thats fine but I'm not seeing the value in requiring that every
implementation of a sensor do that.

>  listens for property updates, they can produce this information
> themselves, but it doesn't hurt to provide it to any reader

Honestly I'm not able to come up with a reason that it doesn't hurt.
But it hurts even less for the application using the Dbus interface to
collect the timestamp itself does it not?  Is it not preferable to have
a reduced API requirement for applications implementing sensors?

>  regardless of mechanism and as it stands now, it feels missing.  It's
> true that some drivers cache the information, and phosphor-hwmon
> itself waits one second before looping around to update, but a way to
> reduce some amount of ambiguity would be to provide a timestamp on the
> information.  By adding a timestamp the information would also come
> across to the reader as more of a time series, which is more or less
> is.
> 
> 
> Without multi-threading to remove the timing bottleneck on my
> platform, and the inability to provide other more direct interfaces
> for controlling fans, I'll be unable to use phosphor-hwmon as a
> primary source for reading/writing sensor data and will be unable to
> upstream any effort i make to write a pluggable thermal controller.
> 
> 
> I felt it was important to thoroughly explain my reasoning as other
> engineering teams attempting to use openbmc for their platforms could
> run into similar problems and that the decision to develop
> independently was data-driven and not arbitrary.
> 
> 
> Patrick

Re: phosphor-hwmon bottleneck potential

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 1598 bytes --]

On Fri, May 05, 2017 at 01:07:45AM -0400, Brad Bishop wrote:
> > The solution that comes to mind would be to simply parallelize sensor
> > updates, such that phosphor-hwmon uses threads to update the sensor
> 
> I think this is a great idea.  But I would vote for some kind of
> non-blocking or async io rather than threads.  I don't know what support
> for that sort of thing is available in the hwmon subsystem so I'm not
> sure if its even possible, but it seems worth a look anyway.

If the IO operation within the hwmon kernel driver is taking 1 second, I
don't think multi-threading does anything to improve this, except
perhaps if you have two threads: 1 for dbus and 1 for hwmon polling.
Going to N threads or N processes for the hwmon polling would not be
beneficial since there would only be single driver queueing up the N
threads anyhow.  Two threads just improves the dbus get-property
response time for returning the cached value.

Hopefully we can use sd-event with a non-blocking read on the hwmon
sysfs entry to avoid having to resort to multi-threading.

I don't know which device you are interacting with that is taking so
long or how the driver was written, but a very common optimization in
other hwmon drivers is to read all N hwmon registers from the device
when the user touches any of the N sysfs entries and then cache them for
a polling interval.  This would hopefully take your 8 seconds down to 1
second for 8 devices.  Still pretty horrible if you are having to take 1
second of kernel time for IO every <2 seconds.

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: phosphor-hwmon bottleneck potential

[Rick Altherr]

[-- Attachment #1: Type: text/plain, Size: 1945 bytes --]

I've chatted with Patrick V. separately about the driver.  AST2400/2500 fan
tach hardware measures only one fan at a time.  I think we can adjust the
driver settings to reduce the measurement time but it will scale with # of
tachs being read.

On Fri, May 5, 2017 at 9:34 AM, Patrick Williams <patrick@stwcx.xyz> wrote:

> On Fri, May 05, 2017 at 01:07:45AM -0400, Brad Bishop wrote:
> > > The solution that comes to mind would be to simply parallelize sensor
> > > updates, such that phosphor-hwmon uses threads to update the sensor
> >
> > I think this is a great idea.  But I would vote for some kind of
> > non-blocking or async io rather than threads.  I don't know what support
> > for that sort of thing is available in the hwmon subsystem so I'm not
> > sure if its even possible, but it seems worth a look anyway.
>
> If the IO operation within the hwmon kernel driver is taking 1 second, I
> don't think multi-threading does anything to improve this, except
> perhaps if you have two threads: 1 for dbus and 1 for hwmon polling.
> Going to N threads or N processes for the hwmon polling would not be
> beneficial since there would only be single driver queueing up the N
> threads anyhow.  Two threads just improves the dbus get-property
> response time for returning the cached value.
>
> Hopefully we can use sd-event with a non-blocking read on the hwmon
> sysfs entry to avoid having to resort to multi-threading.
>
> I don't know which device you are interacting with that is taking so
> long or how the driver was written, but a very common optimization in
> other hwmon drivers is to read all N hwmon registers from the device
> when the user touches any of the N sysfs entries and then cache them for
> a polling interval.  This would hopefully take your 8 seconds down to 1
> second for 8 devices.  Still pretty horrible if you are having to take 1
> second of kernel time for IO every <2 seconds.
>
> --
> Patrick Williams
>

[-- Attachment #2: Type: text/html, Size: 2465 bytes --]

Re: phosphor-hwmon bottleneck potential

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 1123 bytes --]

Rick,

On Fri, May 05, 2017 at 09:48:01AM -0700, Rick Altherr wrote:
> I've chatted with Patrick V. separately about the driver.  AST2400/2500 fan
> tach hardware measures only one fan at a time.  I think we can adjust the
> driver settings to reduce the measurement time but it will scale with # of
> tachs being read.

I never looked at this driver before but it looks like it is doing an
'msleep' in the hwmon read path after resetting a counter register and
then counting rotations?  Two comments:

1. As it stands, it doesn't appear that this driver is actually
multi-reader safe (either thread or process).  There is no locking or
queueing to prevent one reader from resetting the result register while
another is performing the msleep loop.  Multi-threading might "go
faster" but it will give entirely wrong results by my reading.

2. It seems bad to do a long-running msleep in the hwmon read path to
begin with.  Should this driver be restructured to have a kthread read
the channels in the background on a polling interval instead of
initiating by userspace action?

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: phosphor-hwmon bottleneck potential

[Rick Altherr]

[-- Attachment #1: Type: text/plain, Size: 1393 bytes --]

I can't comment on the driver implementation choices.  They have been
reviewed and accepted by upstream.  I'm not sure what version of the driver
you are looking at.

On Fri, May 5, 2017 at 10:43 AM, Patrick Williams <patrick@stwcx.xyz> wrote:

> Rick,
>
> On Fri, May 05, 2017 at 09:48:01AM -0700, Rick Altherr wrote:
> > I've chatted with Patrick V. separately about the driver.  AST2400/2500
> fan
> > tach hardware measures only one fan at a time.  I think we can adjust the
> > driver settings to reduce the measurement time but it will scale with #
> of
> > tachs being read.
>
> I never looked at this driver before but it looks like it is doing an
> 'msleep' in the hwmon read path after resetting a counter register and
> then counting rotations?  Two comments:
>
> 1. As it stands, it doesn't appear that this driver is actually
> multi-reader safe (either thread or process).  There is no locking or
> queueing to prevent one reader from resetting the result register while
> another is performing the msleep loop.  Multi-threading might "go
> faster" but it will give entirely wrong results by my reading.
>
> 2. It seems bad to do a long-running msleep in the hwmon read path to
> begin with.  Should this driver be restructured to have a kthread read
> the channels in the background on a polling interval instead of
> initiating by userspace action?
>
> --
> Patrick Williams
>

[-- Attachment #2: Type: text/html, Size: 1892 bytes --]

Re: phosphor-hwmon bottleneck potential

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]

Jaghu,

Can you comment?  I'm looking at the version that was merged into Linus'
master for 4.12:

commit 2d7a548a3eff382da5cd743670693b7657327714
Author: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
Date:   Tue Apr 4 17:52:41 2017 -0700

    drivers: hwmon: Support for ASPEED PWM/Fan tach


On Fri, May 05, 2017 at 10:48:39AM -0700, Rick Altherr wrote:
> I can't comment on the driver implementation choices.  They have been
> reviewed and accepted by upstream.  I'm not sure what version of the driver
> you are looking at.
> 
> On Fri, May 5, 2017 at 10:43 AM, Patrick Williams <patrick@stwcx.xyz> wrote:
> 
> > Rick,
> >
> > On Fri, May 05, 2017 at 09:48:01AM -0700, Rick Altherr wrote:
> > > I've chatted with Patrick V. separately about the driver.  AST2400/2500
> > fan
> > > tach hardware measures only one fan at a time.  I think we can adjust the
> > > driver settings to reduce the measurement time but it will scale with #
> > of
> > > tachs being read.
> >
> > I never looked at this driver before but it looks like it is doing an
> > 'msleep' in the hwmon read path after resetting a counter register and
> > then counting rotations?  Two comments:
> >
> > 1. As it stands, it doesn't appear that this driver is actually
> > multi-reader safe (either thread or process).  There is no locking or
> > queueing to prevent one reader from resetting the result register while
> > another is performing the msleep loop.  Multi-threading might "go
> > faster" but it will give entirely wrong results by my reading.
> >
> > 2. It seems bad to do a long-running msleep in the hwmon read path to
> > begin with.  Should this driver be restructured to have a kthread read
> > the channels in the background on a polling interval instead of
> > initiating by userspace action?
> >
> > --
> > Patrick Williams
> >

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: phosphor-hwmon bottleneck potential

[Patrick Venture]

[-- Attachment #1: Type: text/plain, Size: 2140 bytes --]

For what it's worth, I'm experimenting with shorter measurement periods for
this specific driver.

Patrick

On Fri, May 5, 2017 at 11:02 AM, Patrick Williams <patrick@stwcx.xyz> wrote:

> Jaghu,
>
> Can you comment?  I'm looking at the version that was merged into Linus'
> master for 4.12:
>
> commit 2d7a548a3eff382da5cd743670693b7657327714
> Author: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
> Date:   Tue Apr 4 17:52:41 2017 -0700
>
>     drivers: hwmon: Support for ASPEED PWM/Fan tach
>
>
> On Fri, May 05, 2017 at 10:48:39AM -0700, Rick Altherr wrote:
> > I can't comment on the driver implementation choices.  They have been
> > reviewed and accepted by upstream.  I'm not sure what version of the
> driver
> > you are looking at.
> >
> > On Fri, May 5, 2017 at 10:43 AM, Patrick Williams <patrick@stwcx.xyz>
> wrote:
> >
> > > Rick,
> > >
> > > On Fri, May 05, 2017 at 09:48:01AM -0700, Rick Altherr wrote:
> > > > I've chatted with Patrick V. separately about the driver.
> AST2400/2500
> > > fan
> > > > tach hardware measures only one fan at a time.  I think we can
> adjust the
> > > > driver settings to reduce the measurement time but it will scale
> with #
> > > of
> > > > tachs being read.
> > >
> > > I never looked at this driver before but it looks like it is doing an
> > > 'msleep' in the hwmon read path after resetting a counter register and
> > > then counting rotations?  Two comments:
> > >
> > > 1. As it stands, it doesn't appear that this driver is actually
> > > multi-reader safe (either thread or process).  There is no locking or
> > > queueing to prevent one reader from resetting the result register while
> > > another is performing the msleep loop.  Multi-threading might "go
> > > faster" but it will give entirely wrong results by my reading.
> > >
> > > 2. It seems bad to do a long-running msleep in the hwmon read path to
> > > begin with.  Should this driver be restructured to have a kthread read
> > > the channels in the background on a polling interval instead of
> > > initiating by userspace action?
> > >
> > > --
> > > Patrick Williams
> > >
>
> --
> Patrick Williams
>

[-- Attachment #2: Type: text/html, Size: 3061 bytes --]

Re: phosphor-hwmon bottleneck potential

[Rick Altherr]

[-- Attachment #1: Type: text/plain, Size: 2062 bytes --]

Jagha is currently on leave.

On Fri, May 5, 2017 at 11:02 AM, Patrick Williams <patrick@stwcx.xyz> wrote:

> Jaghu,
>
> Can you comment?  I'm looking at the version that was merged into Linus'
> master for 4.12:
>
> commit 2d7a548a3eff382da5cd743670693b7657327714
> Author: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
> Date:   Tue Apr 4 17:52:41 2017 -0700
>
>     drivers: hwmon: Support for ASPEED PWM/Fan tach
>
>
> On Fri, May 05, 2017 at 10:48:39AM -0700, Rick Altherr wrote:
> > I can't comment on the driver implementation choices.  They have been
> > reviewed and accepted by upstream.  I'm not sure what version of the
> driver
> > you are looking at.
> >
> > On Fri, May 5, 2017 at 10:43 AM, Patrick Williams <patrick@stwcx.xyz>
> wrote:
> >
> > > Rick,
> > >
> > > On Fri, May 05, 2017 at 09:48:01AM -0700, Rick Altherr wrote:
> > > > I've chatted with Patrick V. separately about the driver.
> AST2400/2500
> > > fan
> > > > tach hardware measures only one fan at a time.  I think we can
> adjust the
> > > > driver settings to reduce the measurement time but it will scale
> with #
> > > of
> > > > tachs being read.
> > >
> > > I never looked at this driver before but it looks like it is doing an
> > > 'msleep' in the hwmon read path after resetting a counter register and
> > > then counting rotations?  Two comments:
> > >
> > > 1. As it stands, it doesn't appear that this driver is actually
> > > multi-reader safe (either thread or process).  There is no locking or
> > > queueing to prevent one reader from resetting the result register while
> > > another is performing the msleep loop.  Multi-threading might "go
> > > faster" but it will give entirely wrong results by my reading.
> > >
> > > 2. It seems bad to do a long-running msleep in the hwmon read path to
> > > begin with.  Should this driver be restructured to have a kthread read
> > > the channels in the background on a polling interval instead of
> > > initiating by userspace action?
> > >
> > > --
> > > Patrick Williams
> > >
>
> --
> Patrick Williams
>

[-- Attachment #2: Type: text/html, Size: 2951 bytes --]

Re: phosphor-hwmon bottleneck potential

[Nancy Yuen]

[-- Attachment #1: Type: text/plain, Size: 2719 bytes --]

1. General design issue if time between reads of a sensor is dependent on
the number of sensors in the system.  And drivers can fail, due to miss
behaving code or hardware.  In this design, sensor report could be
significantly delayed if one sensor/driver were bad or misbehaving.

2. So putting the onus on each application to implement timestamps is
better design than having timestamps recorded by a central source, the
source that actually is reading from the driver?

----------
Nancy

On Fri, May 5, 2017 at 11:05 AM, Rick Altherr <raltherr@google.com> wrote:

> Jagha is currently on leave.
>
> On Fri, May 5, 2017 at 11:02 AM, Patrick Williams <patrick@stwcx.xyz>
> wrote:
>
>> Jaghu,
>>
>> Can you comment?  I'm looking at the version that was merged into Linus'
>> master for 4.12:
>>
>> commit 2d7a548a3eff382da5cd743670693b7657327714 <(765)%20732-7714>
>> Author: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
>> Date:   Tue Apr 4 17:52:41 2017 -0700
>>
>>     drivers: hwmon: Support for ASPEED PWM/Fan tach
>>
>>
>> On Fri, May 05, 2017 at 10:48:39AM -0700, Rick Altherr wrote:
>> > I can't comment on the driver implementation choices.  They have been
>> > reviewed and accepted by upstream.  I'm not sure what version of the
>> driver
>> > you are looking at.
>> >
>> > On Fri, May 5, 2017 at 10:43 AM, Patrick Williams <patrick@stwcx.xyz>
>> wrote:
>> >
>> > > Rick,
>> > >
>> > > On Fri, May 05, 2017 at 09:48:01AM -0700, Rick Altherr wrote:
>> > > > I've chatted with Patrick V. separately about the driver.
>> AST2400/2500
>> > > fan
>> > > > tach hardware measures only one fan at a time.  I think we can
>> adjust the
>> > > > driver settings to reduce the measurement time but it will scale
>> with #
>> > > of
>> > > > tachs being read.
>> > >
>> > > I never looked at this driver before but it looks like it is doing an
>> > > 'msleep' in the hwmon read path after resetting a counter register and
>> > > then counting rotations?  Two comments:
>> > >
>> > > 1. As it stands, it doesn't appear that this driver is actually
>> > > multi-reader safe (either thread or process).  There is no locking or
>> > > queueing to prevent one reader from resetting the result register
>> while
>> > > another is performing the msleep loop.  Multi-threading might "go
>> > > faster" but it will give entirely wrong results by my reading.
>> > >
>> > > 2. It seems bad to do a long-running msleep in the hwmon read path to
>> > > begin with.  Should this driver be restructured to have a kthread read
>> > > the channels in the background on a polling interval instead of
>> > > initiating by userspace action?
>> > >
>> > > --
>> > > Patrick Williams
>> > >
>>
>> --
>> Patrick Williams
>>
>
>

[-- Attachment #2: Type: text/html, Size: 4154 bytes --]

Re: phosphor-hwmon bottleneck potential

[Robi Buranyi]

[-- Attachment #1: Type: text/plain, Size: 3262 bytes --]

my 50 cents on this.

We could change the driver(s) to periodically poll the sensors like every
second, and store the latest value. Then the sysfs read shall always return
the last completed measurement data. We should do this for both the
tachometers reading and the temperature sensors. There shall be some logic
implemented for aging (when sensor read constantly fails)

On Fri, May 5, 2017 at 11:37 AM, Nancy Yuen <yuenn@google.com> wrote:

> 1. General design issue if time between reads of a sensor is dependent on
> the number of sensors in the system.  And drivers can fail, due to miss
> behaving code or hardware.  In this design, sensor report could be
> significantly delayed if one sensor/driver were bad or misbehaving.
>
> 2. So putting the onus on each application to implement timestamps is
> better design than having timestamps recorded by a central source, the
> source that actually is reading from the driver?
>
> ----------
> Nancy
>
> On Fri, May 5, 2017 at 11:05 AM, Rick Altherr <raltherr@google.com> wrote:
>
>> Jagha is currently on leave.
>>
>> On Fri, May 5, 2017 at 11:02 AM, Patrick Williams <patrick@stwcx.xyz>
>> wrote:
>>
>>> Jaghu,
>>>
>>> Can you comment?  I'm looking at the version that was merged into Linus'
>>> master for 4.12:
>>>
>>> commit 2d7a548a3eff382da5cd743670693b7657327714 <(765)%20732-7714>
>>> Author: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
>>> Date:   Tue Apr 4 17:52:41 2017 -0700
>>>
>>>     drivers: hwmon: Support for ASPEED PWM/Fan tach
>>>
>>>
>>> On Fri, May 05, 2017 at 10:48:39AM -0700, Rick Altherr wrote:
>>> > I can't comment on the driver implementation choices.  They have been
>>> > reviewed and accepted by upstream.  I'm not sure what version of the
>>> driver
>>> > you are looking at.
>>> >
>>> > On Fri, May 5, 2017 at 10:43 AM, Patrick Williams <patrick@stwcx.xyz>
>>> wrote:
>>> >
>>> > > Rick,
>>> > >
>>> > > On Fri, May 05, 2017 at 09:48:01AM -0700, Rick Altherr wrote:
>>> > > > I've chatted with Patrick V. separately about the driver.
>>> AST2400/2500
>>> > > fan
>>> > > > tach hardware measures only one fan at a time.  I think we can
>>> adjust the
>>> > > > driver settings to reduce the measurement time but it will scale
>>> with #
>>> > > of
>>> > > > tachs being read.
>>> > >
>>> > > I never looked at this driver before but it looks like it is doing an
>>> > > 'msleep' in the hwmon read path after resetting a counter register
>>> and
>>> > > then counting rotations?  Two comments:
>>> > >
>>> > > 1. As it stands, it doesn't appear that this driver is actually
>>> > > multi-reader safe (either thread or process).  There is no locking or
>>> > > queueing to prevent one reader from resetting the result register
>>> while
>>> > > another is performing the msleep loop.  Multi-threading might "go
>>> > > faster" but it will give entirely wrong results by my reading.
>>> > >
>>> > > 2. It seems bad to do a long-running msleep in the hwmon read path to
>>> > > begin with.  Should this driver be restructured to have a kthread
>>> read
>>> > > the channels in the background on a polling interval instead of
>>> > > initiating by userspace action?
>>> > >
>>> > > --
>>> > > Patrick Williams
>>> > >
>>>
>>> --
>>> Patrick Williams
>>>
>>
>>
>

[-- Attachment #2: Type: text/html, Size: 5073 bytes --]

Re: phosphor-hwmon bottleneck potential

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]

On Fri, May 05, 2017 at 11:50:52AM -0700, Robi Buranyi wrote:
> We could change the driver(s) to periodically poll the sensors like every
> second, and store the latest value. Then the sysfs read shall always return
> the last completed measurement data. We should do this for both the
> tachometers reading and the temperature sensors. There shall be some logic
> implemented for aging (when sensor read constantly fails)

This could certainly be done for the Aspeed PWM driver in question and
by my reading this might be the best approach.  Due to the way that
hardware works it seems like you'd an interval between samples of at
least 1 second in order to be accurate to roughly 100 RPMs.  I don't
know what accuracy you are striving for, but blocking an application on
a syscall for 1 second doesn't sound like a great implementation.

We could check with Guenter Roeck, the HWMON maintainer, to see if he
has any recommendations on the best way to handle this driver.

Changing HWMON on the whole to do this (the '(s)' part of your 'driver(s)'
statement) seems pretty out of scope and not likely to be accepted by
upstream.  Most HWMON drivers do simple IO in the read syscall, like a
few i2c operations to read a register out in a device so there isn't
problem with a long occurring block.

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: phosphor-hwmon bottleneck potential

[Rick Altherr]

[-- Attachment #1: Type: text/plain, Size: 3095 bytes --]

Re #2: timestamps lose value as they shift away from the time of the actual
measurement.  Ideally, the hardware would timestamp the sample, hwmon would
preserve and provide that timestamp to the userspace.

On Fri, May 5, 2017 at 11:37 AM, Nancy Yuen <yuenn@google.com> wrote:

> 1. General design issue if time between reads of a sensor is dependent on
> the number of sensors in the system.  And drivers can fail, due to miss
> behaving code or hardware.  In this design, sensor report could be
> significantly delayed if one sensor/driver were bad or misbehaving.
>
> 2. So putting the onus on each application to implement timestamps is
> better design than having timestamps recorded by a central source, the
> source that actually is reading from the driver?
>
> ----------
> Nancy
>
> On Fri, May 5, 2017 at 11:05 AM, Rick Altherr <raltherr@google.com> wrote:
>
>> Jagha is currently on leave.
>>
>> On Fri, May 5, 2017 at 11:02 AM, Patrick Williams <patrick@stwcx.xyz>
>> wrote:
>>
>>> Jaghu,
>>>
>>> Can you comment?  I'm looking at the version that was merged into Linus'
>>> master for 4.12:
>>>
>>> commit 2d7a548a3eff382da5cd743670693b7657327714 <(765)%20732-7714>
>>> Author: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
>>> Date:   Tue Apr 4 17:52:41 2017 -0700
>>>
>>>     drivers: hwmon: Support for ASPEED PWM/Fan tach
>>>
>>>
>>> On Fri, May 05, 2017 at 10:48:39AM -0700, Rick Altherr wrote:
>>> > I can't comment on the driver implementation choices.  They have been
>>> > reviewed and accepted by upstream.  I'm not sure what version of the
>>> driver
>>> > you are looking at.
>>> >
>>> > On Fri, May 5, 2017 at 10:43 AM, Patrick Williams <patrick@stwcx.xyz>
>>> wrote:
>>> >
>>> > > Rick,
>>> > >
>>> > > On Fri, May 05, 2017 at 09:48:01AM -0700, Rick Altherr wrote:
>>> > > > I've chatted with Patrick V. separately about the driver.
>>> AST2400/2500
>>> > > fan
>>> > > > tach hardware measures only one fan at a time.  I think we can
>>> adjust the
>>> > > > driver settings to reduce the measurement time but it will scale
>>> with #
>>> > > of
>>> > > > tachs being read.
>>> > >
>>> > > I never looked at this driver before but it looks like it is doing an
>>> > > 'msleep' in the hwmon read path after resetting a counter register
>>> and
>>> > > then counting rotations?  Two comments:
>>> > >
>>> > > 1. As it stands, it doesn't appear that this driver is actually
>>> > > multi-reader safe (either thread or process).  There is no locking or
>>> > > queueing to prevent one reader from resetting the result register
>>> while
>>> > > another is performing the msleep loop.  Multi-threading might "go
>>> > > faster" but it will give entirely wrong results by my reading.
>>> > >
>>> > > 2. It seems bad to do a long-running msleep in the hwmon read path to
>>> > > begin with.  Should this driver be restructured to have a kthread
>>> read
>>> > > the channels in the background on a polling interval instead of
>>> > > initiating by userspace action?
>>> > >
>>> > > --
>>> > > Patrick Williams
>>> > >
>>>
>>> --
>>> Patrick Williams
>>>
>>
>>
>

[-- Attachment #2: Type: text/html, Size: 4883 bytes --]

Re: phosphor-hwmon bottleneck potential

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 2562 bytes --]

On Fri, May 05, 2017 at 11:52:15AM -0700, Rick Altherr wrote:
> On Fri, May 5, 2017 at 11:37 AM, Nancy Yuen <yuenn@google.com> wrote:
> > 2. So putting the onus on each application to implement timestamps is
> > better design than having timestamps recorded by a central source, the
> > source that actually is reading from the driver?

> Re #2: timestamps lose value as they shift away from the time of the actual
> measurement.  Ideally, the hardware would timestamp the sample, hwmon would
> preserve and provide that timestamp to the userspace.

I'm not seeing the utility, specifically in the context of a fan control
algorithm, for a timestamp to know when the reading occurred having been
described.  I do see a need for it in the context of a "repository of
historical sensor values" such as what we will need to implement the
DCMI interfaces for IPMI.

I don't have any problem with a timestamp being added to the
dbus-objects being presented by phosphor-hwmon.  We do have Timestamp
property on xyz.openbmc_project.Logging.Entry already.  I would suggest
we make a xyz.openbmc_project.Common.Timestamp interface instead and
transition the logging entry server to including it.  The reason being
that I've recently had some questions about the date-time format we are
presenting across REST (ms since 1970 as uint64) as being difficult for
humans; having a Common.Timestamp interface makes it much easier for the
REST server (and later Redfish) to identify "things representing time
that should be converted to date-time format".

There is an issue with having two properties in a single dbus object
being changed at the same time and generating a single signal for it.
The org.freedesktop.DBus.Properties.PropertiesChanged signal only allows
a single interface, which might be an argument against what I wrote in
the previous paragraph.  Also, the sdbusplus-generated bindings do not
currently have a mechanism to indicate multiple properties changing as a
transaction even on a single interface.

If we do add a timestamp we need to be clear on what the expected
behavior of it is.  As we are seeing with this issue there can be some
time between sysfs read-start and read-end.  Also, if we are reading an
external device, like the fan controller on Witherspoon, the value
obtained isn't an instantaneous fan speed but an average over some
hardware-defined interval.  So I think we would need some language such
that "time immediately after sysfs read-end" is a valid representation.

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: phosphor-hwmon bottleneck potential

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 2256 bytes --]

On Fri, May 05, 2017 at 11:37:06AM -0700, Nancy Yuen wrote:
> 1. General design issue if time between reads of a sensor is dependent on
> the number of sensors in the system.

I don't see any general design issue with phosphor-hwmon or dbus being
talked about here.  There is one specific driver that has a pretty large
scaling factor no matter how you read sensor values out of it.  We've
talked elsewhere in the thread about a potential driver change to
improve it.  Most hwmon drivers do not have any issue.

> And drivers can fail, due to miss behaving code or hardware.

hwmon drivers should never block userspace forever.  If they do that is
a serious driver bug.  We could be defensive against it by enhancing
phosphor-hwmon to use non-blocking IO, assuming the kernel supports it,
but that seems like a lot of code for a non-existent problem.

Hardware failures are already handled by the hwmon drivers, reported
back to phosphor-hwmon as errnos on read, and dealt with.

> In this design, sensor report could be
> significantly delayed if one sensor/driver were bad or misbehaving.

You have no difference in the problem of one sensor reading not working
in any of these three potential designs:
    1. One big loop that reads each hwmon sysfs entry for the whole
       system in sequence.
    2. One big program with N threads that, with a stampeding herd,
       attempt to read all hwmon sysfs at the same instant.
    3. M processes with N hwmon sysfs reads in sequence.

In any of these designs, if the driver delays your read for 8 seconds
your data is delayed and stale.  I think our expectation is that a
fan-control algorithm is using dbus signals to keep track of the most
recent value and if it doesn't have up-to-date data by the time it wants
to make a decision it would deal with it in whatever way it sees fit.
Likely, either using old data or treating that sensor as in-error.

If you chose design #2 and then expanded on it by adding a thermal
control loop in yet another thread, you'd still have the exact same
problems to deal with.  It just is now all in one process using shm to
communicate the cached sensor values instead of using dbus between
processes.

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: phosphor-hwmon bottleneck potential

[Patrick Venture]

[-- Attachment #1: Type: text/plain, Size: 5892 bytes --]

Just FYI, I've been working on the fan driver to improve its speed.  The
driver can be sped up a bit to provide an answer sooner.  Interestingly,
and this may need some level of accounting even in your systems, I'm seeing
erroneous values periodically.  So, 10 readings of fan A in a row and
1/10th will be something super low.  So any controller you implement should
consider that the data may be invalid,.., while the fan is still present
and functioning.  I plan to check and have a super tight loop if necessary
if I'm seeing bad numbers, and if two in a row, maybe flag the fan as bad
or something along those lines.

To avoid read conflicts or issues, i'll be using phosphor-hwmon to read,
however, to avoid "waiting" I'll be listening for the property updates in a
background thread.  -- and having phosphor-hwmon not wait 1 second.  The
algorithm I'm using wasn't written by me but rather by someone who
understands thermal stuff, and it's written to be a very tight loop on
controlling the fans.

i haven't tried to make the wait time in phosphor-hwmon configurable yet.
It's just not been a priority.  But I'd rather not maintain a hacked
version.  So, hopefully sometime in a couple weeks I can submit it for
review.

Since phosphor-hwmon doesn't allow writing non-fan speeds and I don't want
to write a conversion calibration thing, I'll be writing directly to sysfs
as pwm.  The algorithm I"m using outputs in pwm, so it's a bit of a
shortest-path.  However, I could include as a configuration option in the
fan control itself whether to update over dbus or sysfs and so on, such
that it could be handled either way.  I don't want the configuration to be
too cumbersome though.

i think one of the biggest steps away from something we can easily share
will be the OEM commands I need. -- more or less.  I could put them in a
separate daemon and maybe I will later (once it's all working) and then it
is more shareable.  The host needs to be able to send down thermal data
that the BMC can't read as well as take over control from the algorithm.  I
mentioned the manual control, and y'all indicated it was something you
might require.  But being it's outside the scope of IPMI's preset of
commands, can OEM commands be shared?

I'm still designing my controller to use multiple threads, although no
longer to read, because yeah, it has one status register ... but, one
background thread can maintain the latest fan speed updates received over
dbus.  Because phosphor-hwmon only responds to dbus messages once per loop,
I need to make my information retrieval asynchronous -- hence the
background thread listening for updates.  My loop(s) will just assume the
information they are reading is fresh.

One thing I'm still working on designing is how to cleanly handle chassis
temperature differences.  We have requirements about the difference in
temperature between incoming and exiting air.  All the PID loops will run
and compute their goal PWM and basically maximize that and then check
against the specially listed thermal margin, and then try to drive the fans
there...  ...  I'm not convinced I like it yet.

I hope to have something mostly done these next two weeks.  It isn't built
from the fan-presence, but I'd like to show you guys what it looks like if
it's at that stage and see if I can save you guys time even though maybe it
can't all be used -- or maybe we can come up with ways of splitting it
apart cleanly such that it's a matter of configuration or providing X, Y,
or Z.

Regards,
Patrick

On Fri, May 5, 2017 at 1:35 PM, Patrick Williams <patrick@stwcx.xyz> wrote:

> On Fri, May 05, 2017 at 11:37:06AM -0700, Nancy Yuen wrote:
> > 1. General design issue if time between reads of a sensor is dependent on
> > the number of sensors in the system.
>
> I don't see any general design issue with phosphor-hwmon or dbus being
> talked about here.  There is one specific driver that has a pretty large
> scaling factor no matter how you read sensor values out of it.  We've
> talked elsewhere in the thread about a potential driver change to
> improve it.  Most hwmon drivers do not have any issue.
>
> > And drivers can fail, due to miss behaving code or hardware.
>
> hwmon drivers should never block userspace forever.  If they do that is
> a serious driver bug.  We could be defensive against it by enhancing
> phosphor-hwmon to use non-blocking IO, assuming the kernel supports it,
> but that seems like a lot of code for a non-existent problem.
>
> Hardware failures are already handled by the hwmon drivers, reported
> back to phosphor-hwmon as errnos on read, and dealt with.
>
> > In this design, sensor report could be
> > significantly delayed if one sensor/driver were bad or misbehaving.
>
> You have no difference in the problem of one sensor reading not working
> in any of these three potential designs:
>     1. One big loop that reads each hwmon sysfs entry for the whole
>        system in sequence.
>     2. One big program with N threads that, with a stampeding herd,
>        attempt to read all hwmon sysfs at the same instant.
>     3. M processes with N hwmon sysfs reads in sequence.
>
> In any of these designs, if the driver delays your read for 8 seconds
> your data is delayed and stale.  I think our expectation is that a
> fan-control algorithm is using dbus signals to keep track of the most
> recent value and if it doesn't have up-to-date data by the time it wants
> to make a decision it would deal with it in whatever way it sees fit.
> Likely, either using old data or treating that sensor as in-error.
>
> If you chose design #2 and then expanded on it by adding a thermal
> control loop in yet another thread, you'd still have the exact same
> problems to deal with.  It just is now all in one process using shm to
> communicate the cached sensor values instead of using dbus between
> processes.
>
> --
> Patrick Williams
>

[-- Attachment #2: Type: text/html, Size: 6877 bytes --]

Re: phosphor-hwmon bottleneck potential

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 799 bytes --]

On Thu, May 04, 2017 at 09:11:49AM -0700, Patrick Venture wrote:
> Without multi-threading to remove the timing bottleneck on my platform, and
> the inability to provide other more direct interfaces for controlling fans,
> I'll be unable to use phosphor-hwmon as a primary source for
> reading/writing sensor data and will be unable to upstream any effort i
> make to write a pluggable thermal controller.

From what you wrote earlier, it seems like the constraint has nothing to
do with phosphor-hwmon but with the hwmon subsystem for your particular
driver?  How does a "direct interface" avoid the hwmon latency?  I'm 99%
certain that it isn't dbus itself contributing to the 8 second latency
you are seeing here.  Are you arriving at a different conclusion?

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]