Re: hwmon: trace event support?

Guenter Roeck
2018-10-04T23:42:11Z

On Wed, Oct 03, 2018 at 10:08:39PM -0700, Nicolin Chen wrote:
> Thanks for the quick response.
>
> On Wed, Oct 03, 2018 at 05:42:23PM -0700, Guenter Roeck wrote:
> > > Ftrace data. Similar to tz->poll_queue in thermal_core, hwmon core
> > > could also have a work queue polling the registered sensor inputs
> > > (by default disabled; enabled only if users configure poll_delay)
> > > so that the power data can be generated to Ftrace outputs as well.
> > >
> > > To add this, the hwmon core needs an interface that can get sensor
> > > inputs as drivers like ina3221 don't report any values back to the
> > > core but directly expose them via sysfs ABI nodes.
> > >
> > > I noticed the hwmon_device_register_with_info() could be actually
> > > a good API to use since it has defined different sensor types and
> > > more importantly the ops->read() interface, but the ina3221 driver
> > > is very compact that there would be very little gain from this API
> > > at this moment. However, maybe having trace event support would be
> >
> > "very little gain" is a false assumption. Its size would most likely
> > be reduced by 20% or more, mostly because all the sysfs handling
> > is done by the core if one uses the _info API.
>
> Didn't intend to deny the improvement; probably should have used a
> more positive word instead of "little" :)
>
> > > a good reason to apply this API.
> > >
> > > What do you think about it? Any concern or better solution?
> >
> > I would not object to adding trace support into the hwmon subsystem.
> > However, it should be tied to the new API. I would resist patches
> > adding trace support to individual hwmon drivers unless the new API
> > is used and additional driver specific trace support is warranted.
>
> Yes, my idea is to implement it with the _info API inside the hwmon
> core. What do you think about the mentioned solution? Would you be
> in favor of a polling work queue?
>
> "----------. Similar to tz->poll_queue in thermal_core, hwmon core
> could also have a work queue polling the registered sensor inputs
> (by default disabled; enabled only if users configure poll_delay)
> so that the power data can be generated to Ftrace outputs as well."
>

I am not really in favor of it. This goes well beyond tracing. Tracing
by its nature should be non-invasive and impact the system as little as
possible. Adding a thread which polls thermal sensors, which are often
connected with a slow i2c interface or even blocking, is quite invasive.

I don't mind adding tracing support to trace sensor access. Adding code
to poll thermal sensors on a regular basis is a completely different
beast. I am not convinced that this should really be done in the kernel.
The same could be accomplished with a simple loop from userspace.

while true; do
cat /sys/class/hwmon/hwmon1/temp1_input
sleep 1
done

... and you could actually trace those accesses in the kernel.

Now, if the problem is added overhead due to requiring a sysfs access
for each sensor read, we can discuss introducing a different and more
efficient user-space ABI (such as adding a hwmon->iio bridge).
That would however be a different discussion.

> > Note that this also applies to hwmon drivers registering through
> > thermal. The thermal subsystem calls the _info API but misuses it
> > to avoid a warning generated when using the old API. Of course,
> > I have no influence over the hwmon code in the thermal subsystem,
> > so whatever is done there is essentially wild-wild-west.
>
> I saw they have some obvious code in the hwmon core. If you want,
> we can keep the polling work queue and trace events away from it,
> which sounds plausible to me considering that thermal subsystem
> has its own polling work queue and trace events for sensor data.

The code in the hwmon core is different. I am referring to hwmon code
in the thermal core.

Guenter