lkml.org 
[lkml]   [2018]   [Jan]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V3] thermal: Add cooling device's statistics in sysfs
On Mon, Jan 15, 2018 at 10:16:16AM +0530, Viresh Kumar wrote:
> On 12-01-18, 09:46, Eduardo Valentin wrote:
> > On Thu, Jan 11, 2018 at 03:06:09PM +0530, Viresh Kumar wrote:
> > > $ ls -R /sys/class/thermal/cooling_device0/
> > > /sys/class/thermal/cooling_device0/:
> > > cur_state max_state power stats subsystem type uevent
> > >
> > > /sys/class/thermal/cooling_device0/power:
> > > autosuspend_delay_ms runtime_active_time runtime_suspended_time
> > > control runtime_status
> > >
> > > /sys/class/thermal/cooling_device0/stats:
> > > reset time_in_state_ms total_trans
> > >
> >
> > I guess the only missing node from the cpufreq design copy was the
> > transition table.
>
> I don't think it would be very useful here and so didn't add it.
>

It is actually quite useful, on both, thermal zones and cooling devices,
to detect misbehaving policies. Typically the transitions recorded
by a well behaving policy is very well defined, typically very smooth
from one state to the nearby states. When you see jumps, which is very
clear on a transition table, that is hint for misbehaving scenarios.

Or, when jumps are expected, they are typically rare situation in which
the system engineer wants to be aware/ account for frequency of
occurrence.

> > Also, I think the stats per thermal zone is also useful, sometimes
> > more than per cooling device, as I have been using in
> > the past, but that is just a comment, not really to block your patch.
>
> Sure, that can be added in a separate patch. What kind of stats are you
> expecting there ?

Same set you added for cooling devices should also be reflected on
thermal zones, but instead of cooling state, you want to do the account
on trips, at least for the context of this patch set.

>
> > > diff --git a/drivers/thermal/thermal_helpers.c b/drivers/thermal/thermal_helpers.c
> > > index 8cdf75adcce1..eb03d7e099bb 100644
> > > --- a/drivers/thermal/thermal_helpers.c
> > > +++ b/drivers/thermal/thermal_helpers.c
> > > @@ -187,7 +187,10 @@ void thermal_cdev_update(struct thermal_cooling_device *cdev)
> > > if (instance->target > target)
> > > target = instance->target;
> > > }
> > > - cdev->ops->set_cur_state(cdev, target);
> > > +
> > > + if (!cdev->ops->set_cur_state(cdev, target))
> > > + thermal_cooling_device_stats_update(cdev, target);
> > > +
> >
> > I might be wrong but, this will maybe double account for cases the same
> > cooling state is selected.
>
> No because I am exiting early for same sates in that routine.

Did I miss that hunk?

>
> > > void thermal_cooling_device_setup_sysfs(struct thermal_cooling_device *cdev)
> > > {
> > > + struct cooling_dev_stats *stats;
> > > + unsigned long states;
> > > + int ret, size;
> > > +
> > > + ret = cdev->ops->get_max_state(cdev, &states);
> > > + if (ret)
> > > + return;
> > > +
> > > + states++; /* Total number of states is highest state + 1 */
> > > + size = sizeof(*stats);
> > > + size += sizeof(*stats->time_in_state) * states;
> > > +
> > > + stats = kzalloc(size, GFP_KERNEL);
> > > + if (!stats)
> > > + return;
> > > +
> > > + stats->time_in_state = (ktime_t *)(stats + 1);
> > > + cdev->stats = stats;
> > > + stats->last_time = ktime_get();
> > > + stats->max_states = states;
> > > + cdev->stats = stats;
> > > +
> > > + spin_lock_init(&stats->lock);
> >
> > I would say, the cooling device stat initialization deserves its own
> > initialization function, don't you think?
>
> Hmm, I am not sure if we need it right away. This function is only for cooling
> device's sysfs setup and initializing the "stats" structure kind of falls in the
> setup category. Of course if we want this function to handle sysfs setup for
> thermal zones as well, then yes we better split it up.
>
> The other thing was with thermal_cooling_device_remove_sysfs() as that would
> also need to call a separate routine to do the cleanup and that would be the
> only thing it will end up doing. So it would more be like a dummy caller.
>
> > Also, I see nothing about sysfs on the lines added to
> > thermal_cooling_device_setup_sysfs().
>
> Well, it is allocating structure to keep the values showed while reading sysfs
> files, so it is kind of sysfs related to me :)

Ideally these stats should be behind a config entry, which can be used
also to remove the code out of the kernel that does not want it in.
So, yes, I still think they deserve their own setup/destroy pair, and
a config entry to put the code behind it.

>
> --
> viresh

\
 
 \ /
  Last update: 2018-01-15 17:32    [W:0.050 / U:6.004 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site