Re: [PATCH V4] thermal: Add cooling device's statistics in sysfs

Re: [PATCH V4] thermal: Add cooling device's statistics in sysfs

[Viresh Kumar]

On 16-01-18, 15:22, Viresh Kumar wrote:
> This extends the sysfs interface for thermal cooling devices and exposes
> some pretty useful statistics. These statistics have proven to be quite
> useful specially while doing benchmarks related to the task scheduler,
> where we want to make sure that nothing has disrupted the test,
> specially the cooling device which may have put constraints on the CPUs.
> The information exposed here tells us to what extent the CPUs were
> constrained by the thermal framework.

@Eduardo/Zhang: Are you going to merge this for 4.17 ?

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4] thermal: Add cooling device's statistics in sysfs

[Zhang Rui]

Hi, Viresh,

I will queue it for 4.17, with just one minor fix below.

On 二, 2018-01-16 at 15:22 +0530, Viresh Kumar wrote:
> This extends the sysfs interface for thermal cooling devices and
> exposes
> some pretty useful statistics. These statistics have proven to be
> quite
> useful specially while doing benchmarks related to the task
> scheduler,
> where we want to make sure that nothing has disrupted the test,
> specially the cooling device which may have put constraints on the
> CPUs.
> The information exposed here tells us to what extent the CPUs were
> constrained by the thermal framework.
> 
> The write-only "reset" file is used to reset the statistics.
> 
> The read-only "time_in_state" file shows the clock_t time spent by
> the
> device in the respective cooling states, and it prints one line per
> cooling state.
> 
> The read-only "total_trans" file shows single positive integer value
> showing the total number of cooling state transitions the device has
> gone through since the time the cooling device is registered or the
> time
> when statistics were reset last.
> 
> The read-only "trans_table" file shows a two dimensional matrix,
> where
> an entry <i,j> (row i, column j) represents the number of transitions
> from State_i to State_j.
> 
> This is how the directory structure looks like for a single cooling
> device:
> 
> $ 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  total_trans  trans_table
> 
> This is tested on ARM 64-bit Hisilicon hikey620 board running Ubuntu
> and
> ARM 64-bit Hisilicon hikey960 board running Android.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

[snip]

> +static void cooling_device_stats_setup(struct thermal_cooling_device
> *cdev)
> +{
> +	struct cooling_dev_stats *stats;
> +	unsigned long states;
> +	int var;
> +
> +	if (cdev->ops->get_max_state(cdev, &states))
> +		return;
> +
> +	states++; /* Total number of states is highest state + 1 */
> +
> +	var = sizeof(*stats);
> +	var += sizeof(*stats->time_in_state) * states;
> +	var += sizeof(*stats->trans_table) * states * states;
> +
> +	stats = kzalloc(var, GFP_KERNEL);
> +	if (!stats)
> +		return;
> +
> +	stats->time_in_state = (ktime_t *)(stats + 1);
> +	stats->trans_table = (unsigned int *)(stats->time_in_state +
> states);
> +	cdev->stats = stats;
> +	stats->last_time = ktime_get();
> +	stats->max_states = states;
> +	cdev->stats = stats;
> +

cdev->stats is set twice here, I will remove the first one.

thanks,
rui
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4] thermal: Add cooling device's statistics in sysfs

[Viresh Kumar]

On 13-03-18, 15:02, Zhang Rui wrote:
> Hi, Viresh,
> 
> I will queue it for 4.17, with just one minor fix below.
> 
> On 二, 2018-01-16 at 15:22 +0530, Viresh Kumar wrote:

> > +	cdev->stats = stats;
> > +	stats->last_time = ktime_get();
> > +	stats->max_states = states;
> > +	cdev->stats = stats;
> > +
> 
> cdev->stats is set twice here, I will remove the first one.

Thanks for catching that :)

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4] thermal: Add cooling device's statistics in sysfs

[Zhang Rui]

On 二, 2018-03-13 at 15:02 +0800, Zhang Rui wrote:
> Hi, Viresh,
> 
> I will queue it for 4.17, with just one minor fix below.
> 
I got the following warning from checkpatch.pl
---------------

WARNING: please write a paragraph that describes the config symbol
fully
#147: FILE: drivers/thermal/Kconfig:18:
+config THERMAL_STATISTICS

WARNING: Consider renaming function(s)
'thermal_cooling_device_total_trans_show' to 'total_trans_show'
#391: FILE: drivers/thermal/thermal_sysfs.c:901:
+}

WARNING: Consider renaming function(s)
'thermal_cooling_device_time_in_state_show' to 'time_in_state_show'
#395: FILE: drivers/thermal/thermal_sysfs.c:905:
+static DEVICE_ATTR(time_in_state, 0444,

WARNING: Consider renaming function(s)
'thermal_cooling_device_reset_store' to 'reset_store'
#397: FILE: drivers/thermal/thermal_sysfs.c:907:
+static DEVICE_ATTR(reset, 0200, NULL,
thermal_cooling_device_reset_store);

WARNING: Consider renaming function(s)
'thermal_cooling_device_trans_table_show' to 'trans_table_show'
#398: FILE: drivers/thermal/thermal_sysfs.c:908:
+static DEVICE_ATTR(trans_table, 0444,

total: 0 errors, 5 warnings, 366 lines checked


I'm okay with the first one because the description does not have to be
larger than 3 lines.
the last 4 warnings makes sense to me. I think we should rename the
function and use DEVICE_ATTR_RO() and DEVICE_ATTR_WO() instead.

what do you think?

thanks,
rui

> On 二, 2018-01-16 at 15:22 +0530, Viresh Kumar wrote:
> > 
> > This extends the sysfs interface for thermal cooling devices and
> > exposes
> > some pretty useful statistics. These statistics have proven to be
> > quite
> > useful specially while doing benchmarks related to the task
> > scheduler,
> > where we want to make sure that nothing has disrupted the test,
> > specially the cooling device which may have put constraints on the
> > CPUs.
> > The information exposed here tells us to what extent the CPUs were
> > constrained by the thermal framework.
> > 
> > The write-only "reset" file is used to reset the statistics.
> > 
> > The read-only "time_in_state" file shows the clock_t time spent by
> > the
> > device in the respective cooling states, and it prints one line per
> > cooling state.
> > 
> > The read-only "total_trans" file shows single positive integer
> > value
> > showing the total number of cooling state transitions the device
> > has
> > gone through since the time the cooling device is registered or the
> > time
> > when statistics were reset last.
> > 
> > The read-only "trans_table" file shows a two dimensional matrix,
> > where
> > an entry <i,j> (row i, column j) represents the number of
> > transitions
> > from State_i to State_j.
> > 
> > This is how the directory structure looks like for a single cooling
> > device:
> > 
> > $ 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  total_trans  trans_table
> > 
> > This is tested on ARM 64-bit Hisilicon hikey620 board running
> > Ubuntu
> > and
> > ARM 64-bit Hisilicon hikey960 board running Android.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> [snip]
> 
> > 
> > +static void cooling_device_stats_setup(struct
> > thermal_cooling_device
> > *cdev)
> > +{
> > +	struct cooling_dev_stats *stats;
> > +	unsigned long states;
> > +	int var;
> > +
> > +	if (cdev->ops->get_max_state(cdev, &states))
> > +		return;
> > +
> > +	states++; /* Total number of states is highest state + 1
> > */
> > +
> > +	var = sizeof(*stats);
> > +	var += sizeof(*stats->time_in_state) * states;
> > +	var += sizeof(*stats->trans_table) * states * states;
> > +
> > +	stats = kzalloc(var, GFP_KERNEL);
> > +	if (!stats)
> > +		return;
> > +
> > +	stats->time_in_state = (ktime_t *)(stats + 1);
> > +	stats->trans_table = (unsigned int *)(stats->time_in_state 
> > +
> > states);
> > +	cdev->stats = stats;
> > +	stats->last_time = ktime_get();
> > +	stats->max_states = states;
> > +	cdev->stats = stats;
> > +
> cdev->stats is set twice here, I will remove the first one.
> 
> thanks,
> rui
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4] thermal: Add cooling device's statistics in sysfs

[Viresh Kumar]

On 14-03-18, 16:01, Zhang Rui wrote:
> WARNING: please write a paragraph that describes the config symbol
> fully
> #147: FILE: drivers/thermal/Kconfig:18:
> +config THERMAL_STATISTICS
> 
> WARNING: Consider renaming function(s)
> 'thermal_cooling_device_total_trans_show' to 'total_trans_show'
> #391: FILE: drivers/thermal/thermal_sysfs.c:901:
> +}
> 
> WARNING: Consider renaming function(s)
> 'thermal_cooling_device_time_in_state_show' to 'time_in_state_show'
> #395: FILE: drivers/thermal/thermal_sysfs.c:905:
> +static DEVICE_ATTR(time_in_state, 0444,
> 
> WARNING: Consider renaming function(s)
> 'thermal_cooling_device_reset_store' to 'reset_store'
> #397: FILE: drivers/thermal/thermal_sysfs.c:907:
> +static DEVICE_ATTR(reset, 0200, NULL,
> thermal_cooling_device_reset_store);
> 
> WARNING: Consider renaming function(s)
> 'thermal_cooling_device_trans_table_show' to 'trans_table_show'
> #398: FILE: drivers/thermal/thermal_sysfs.c:908:
> +static DEVICE_ATTR(trans_table, 0444,
> 
> total: 0 errors, 5 warnings, 366 lines checked
> 
> 
> I'm okay with the first one because the description does not have to be
> larger than 3 lines.

Right.

> the last 4 warnings makes sense to me. I think we should rename the
> function and use DEVICE_ATTR_RO() and DEVICE_ATTR_WO() instead.
> 
> what do you think?

I got those warnings as well, and I quietly ignored them :)

I ignored the renaming part for the sake of consistency. The other existing
routines for similar purpose are named as:

thermal_cooling_device_type_show
thermal_cooling_device_max_state_show
thermal_cooling_device_cur_state_show
thermal_cooling_device_cur_state_store

for me it made more sense to follow that naming convention. And I didn't use the
_RO and _WO variants for the same reason.

Now here is what I propose now:

- You apply this patch as-is and ignore the warning.

- I will send few patches on top of that to do:
  - renaming of all such routines to shorter versions.
  - Use the _RO or _WO variants of the macro everywhere.

What do you say ?

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4] thermal: Add cooling device's statistics in sysfs

[Viresh Kumar]

On 14-03-18, 13:44, Viresh Kumar wrote:
> I got those warnings as well, and I quietly ignored them :)
> 
> I ignored the renaming part for the sake of consistency. The other existing
> routines for similar purpose are named as:
> 
> thermal_cooling_device_type_show
> thermal_cooling_device_max_state_show
> thermal_cooling_device_cur_state_show
> thermal_cooling_device_cur_state_store
> 
> for me it made more sense to follow that naming convention. And I didn't use the
> _RO and _WO variants for the same reason.
> 
> Now here is what I propose now:
> 
> - You apply this patch as-is and ignore the warning.
> 
> - I will send few patches on top of that to do:
>   - renaming of all such routines to shorter versions.
>   - Use the _RO or _WO variants of the macro everywhere.
> 
> What do you say ?

@Zhang: Can you please share the way you want this to get merged ? And if the
above is fine with you ?

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V4] thermal: Add cooling device's statistics in sysfs

[Zhang Rui]

On 二, 2018-03-20 at 09:38 +0800, Viresh Kumar wrote:
> On 14-03-18, 13:44, Viresh Kumar wrote:
> > 
> > I got those warnings as well, and I quietly ignored them :)
> > 
> > I ignored the renaming part for the sake of consistency. The other
> > existing
> > routines for similar purpose are named as:
> > 
> > thermal_cooling_device_type_show
> > thermal_cooling_device_max_state_show
> > thermal_cooling_device_cur_state_show
> > thermal_cooling_device_cur_state_store
> > 
> > for me it made more sense to follow that naming convention. And I
> > didn't use the
> > _RO and _WO variants for the same reason.
> > 
> > Now here is what I propose now:
> > 
> > - You apply this patch as-is and ignore the warning.
> > 
> > - I will send few patches on top of that to do:
> >   - renaming of all such routines to shorter versions.
> >   - Use the _RO or _WO variants of the macro everywhere.
> > 
> > What do you say ?
> @Zhang: Can you please share the way you want this to get merged ?
> And if the
> above is fine with you ?
> 
yes, it works for me.
I will apply the patch later today.

thanks,
rui
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html