* hwmon: trace event support? @ 2018-10-03 23:46 Nicolin Chen 2018-10-04 0:42 ` Guenter Roeck 0 siblings, 1 reply; 6+ messages in thread From: Nicolin Chen @ 2018-10-03 23:46 UTC (permalink / raw) To: linux; +Cc: linux-hwmon Hello Guenter, I am thinking about a possibility of adding a trace event support in hwmon subsystem. Ftrace is pretty useful and widely applied to different subsystems already while hwmon doesn't seem to have one yet. I know some power/perf folks who rely on the trace events to analyse relationships among cpufreq <-> thermal <-> power: usually what they do is enabling trace events, then letting the system run for a while, and finally collecting all data from Ftrace outputs. Both cpufreq and thermal have trace events; but for power data, it seems that they have to go through sysfs nodes, which is difficult for them to get data at the same timestamps corresponding to those 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 a good reason to apply this API. What do you think about it? Any concern or better solution? Thank you Nicolin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: hwmon: trace event support? 2018-10-03 23:46 hwmon: trace event support? Nicolin Chen @ 2018-10-04 0:42 ` Guenter Roeck 2018-10-04 5:08 ` Nicolin Chen 0 siblings, 1 reply; 6+ messages in thread From: Guenter Roeck @ 2018-10-04 0:42 UTC (permalink / raw) To: Nicolin Chen; +Cc: linux-hwmon Ni Nicolin, On 10/03/2018 04:46 PM, Nicolin Chen wrote: > Hello Guenter, > > I am thinking about a possibility of adding a trace event support > in hwmon subsystem. Ftrace is pretty useful and widely applied to > different subsystems already while hwmon doesn't seem to have one > yet. I know some power/perf folks who rely on the trace events to > analyse relationships among cpufreq <-> thermal <-> power: usually > what they do is enabling trace events, then letting the system run > for a while, and finally collecting all data from Ftrace outputs. > > Both cpufreq and thermal have trace events; but for power data, it > seems that they have to go through sysfs nodes, which is difficult > for them to get data at the same timestamps corresponding to those > 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. > 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. 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. Hope this helps, Guenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: hwmon: trace event support? 2018-10-04 0:42 ` Guenter Roeck @ 2018-10-04 5:08 ` Nicolin Chen 2018-10-04 16:48 ` Guenter Roeck 0 siblings, 1 reply; 6+ messages in thread From: Nicolin Chen @ 2018-10-04 5:08 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-hwmon 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." > 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. Thank you Nicolin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: hwmon: trace event support? 2018-10-04 5:08 ` Nicolin Chen @ 2018-10-04 16:48 ` Guenter Roeck 2018-10-04 18:58 ` Nicolin Chen 0 siblings, 1 reply; 6+ messages in thread From: Guenter Roeck @ 2018-10-04 16:48 UTC (permalink / raw) To: Nicolin Chen; +Cc: linux-hwmon 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: hwmon: trace event support? 2018-10-04 16:48 ` Guenter Roeck @ 2018-10-04 18:58 ` Nicolin Chen 2018-10-04 20:02 ` Guenter Roeck 0 siblings, 1 reply; 6+ messages in thread From: Nicolin Chen @ 2018-10-04 18:58 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-hwmon Hello, On Thu, Oct 04, 2018 at 09:48:02AM -0700, Guenter Roeck wrote: > > > 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. I ain't 100% convinced either. I think at this point we can just insert a trace event to the hwmon_attr_show(), unless there is a substantial polling queue in the hwmon core as thermal_core has, although I am not sure what would be a legit reason to add one. > while true; do > cat /sys/class/hwmon/hwmon1/temp1_input > sleep 1 > done The power/perf folks were asking about something hands-free, as neither thermal nor cpufreq requires extra readings or polling, but I feel this should work for them too, reluctantly though. > ... 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. Yea, that's beyond the topic yet it sounds more interesting for certain people I guess, considering the fact that there are two ina2xx drivers in both hwmon and iio subsystems. > > > 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. I see, though I can't foresee a conflict if we just add a trace event in the hwmon_attr_show(). And it seems, at least now, it passes a NULL chip pointer via the _info API. Thank you Nicolin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: hwmon: trace event support? 2018-10-04 18:58 ` Nicolin Chen @ 2018-10-04 20:02 ` Guenter Roeck 0 siblings, 0 replies; 6+ messages in thread From: Guenter Roeck @ 2018-10-04 20:02 UTC (permalink / raw) To: Nicolin Chen; +Cc: linux-hwmon On Thu, Oct 04, 2018 at 11:58:47AM -0700, Nicolin Chen wrote: > Hello, > > On Thu, Oct 04, 2018 at 09:48:02AM -0700, Guenter Roeck wrote: > > > > 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. > > I ain't 100% convinced either. I think at this point we can just > insert a trace event to the hwmon_attr_show(), unless there is a > substantial polling queue in the hwmon core as thermal_core has, > although I am not sure what would be a legit reason to add one. > > > while true; do > > cat /sys/class/hwmon/hwmon1/temp1_input > > sleep 1 > > done > > The power/perf folks were asking about something hands-free, as > neither thermal nor cpufreq requires extra readings or polling, > but I feel this should work for them too, reluctantly though. > ... the difference being that both are active kernel subsystems, meaning they do something on their own, while hwmon is by its nature passive unless triggered by userspace or, sometimes, interrupts. > > ... 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. > > Yea, that's beyond the topic yet it sounds more interesting for > certain people I guess, considering the fact that there are two > ina2xx drivers in both hwmon and iio subsystems. > This of course is quite undesirable, and a solution permitting support for both ABIs with a single driver would be quite useful. Guenter ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-05 2:56 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-03 23:46 hwmon: trace event support? Nicolin Chen 2018-10-04 0:42 ` Guenter Roeck 2018-10-04 5:08 ` Nicolin Chen 2018-10-04 16:48 ` Guenter Roeck 2018-10-04 18:58 ` Nicolin Chen 2018-10-04 20:02 ` Guenter Roeck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.