Re: [PATCH 3/7] devfreq: add clearing transitions stats in sysfs

[Kamil Konieczny]

Hi Chanwoo,

On 15.11.2019 03:47, Chanwoo Choi wrote:
> Hi Bartlomiej,
> 
> On 11/15/19 3:23 AM, Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi Chanwoo,
>>
>> On 11/13/19 10:41 AM, Chanwoo Choi wrote:
>>> Hi,
>>>
>>> If user only want to use the transitions stats information
>>> from now, the user just read the sysfs twice and then
>>> can calculate them between the read data. It is enough
>>
>> IOW you are suggesting that user should invest his valuable time
>> into actually doing such calculations (or implementing some extra
>> script to do the data parsing and calculations automatically)
>> instead of doing simple write to special sysfs file?
> 
> Actually, I wouldn't like to add the new node for the debugging.
> If there are some requirement for the debugging, we better to
> add the debugfs for devfreq (devfreq has not yet developed
> the debugfs. I'll do that). If some node is necessary for the devfreq
> core/device operation, Surely, I'll agree.
> 
> On the initial version of devfreq, it contained the 'trans_stat'.
> In order to keep the compatibility, just could not move 'trans_stat'
> to under debugfs path.
> 
> If there are any requirement for resetting for transition stats,
> I prefer to use the following style without any extra debugging node.
> 
> 	For resetting for transition stats as following,
> 	echo 0 > /sys/class/devfreq/devfreq0/trans_stat

ok, I will change the stats reset to this.

>> Also similar patch for cpufreq has been applied not so long ago:
>>
>> commit ee7930ee27fe5240398cc302fa8eb4454725f188
>> Author: Markus Mayer <mmayer@xxxxxxxxxxxx>
>> Date:   Mon Nov 7 10:02:23 2016 -0800
>>
>>     cpufreq: stats: New sysfs attribute for clearing statistics
>>     
>>     Allow CPUfreq statistics to be cleared by writing anything to
>>     /sys/.../cpufreq/stats/reset.
>>     
>>     Signed-off-by: Markus Mayer <mmayer@xxxxxxxxxxxx>
>>     Acked-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
>>     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>> ...
>>
>> Best regards,
>> --
>> Bartlomiej Zolnierkiewicz
>> Samsung R&D Institute Poland
>> Samsung Electronics
>>
>>> to get the existing sysfs information. 
>>>
>>> And I don't know the any other reason. So, I can't agree this patch.
>>> So, Not ack.
>>>
>>> Regards,
>>> Chanwoo Choi
>>>
>>>
>>> On 11/13/19 6:13 PM, Kamil Konieczny wrote:
>>>> Add new function trans_reset in sysfs for clearing transition
>>>> table and time in states devfreq statistics.> 
>>>> Signed-off-by: Kamil Konieczny <k.konieczny@xxxxxxxxxxx>
>>>> ---
>>>>  drivers/devfreq/devfreq.c | 26 ++++++++++++++++++++++++++
>>>>  1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index ac04b5baef70..0a88055d1362 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -1445,6 +1445,31 @@ static ssize_t trans_stat_show(struct device *dev,
>>>>  }
>>>>  static DEVICE_ATTR_RO(trans_stat);
>>>>  
>>>> +static void defvreq_stats_clear_table(struct devfreq *devfreq)
>>>> +{
>>>> +	unsigned int count = devfreq->profile->max_state;
>>>> +
>>>> +	spin_lock(&devfreq->stats_lock);
>>>> +	memset(devfreq->time_in_state, 0, count * sizeof(u64));
>>>> +	memset(devfreq->trans_table, 0, count * count * sizeof(int));
>>>> +	devfreq->last_stat_updated = get_jiffies_64();
>>>> +	devfreq->total_trans = 0;
>>>> +	spin_unlock(&devfreq->stats_lock);
>>>> +}
>>>> +
>>>> +static ssize_t trans_reset_store(struct device *dev,
>>>> +				 struct device_attribute *attr,
>>>> +				 const char *buf,
>>>> +				 size_t count)
>>>> +{
>>>> +	struct devfreq *devfreq = to_devfreq(dev);
>>>> +
>>>> +	defvreq_stats_clear_table(devfreq);
>>>> +
>>>> +	return count;
>>>> +}
>>>> +static DEVICE_ATTR_WO(trans_reset);
>>>> +
>>>>  static struct attribute *devfreq_attrs[] = {
>>>>  	&dev_attr_governor.attr,
>>>>  	&dev_attr_available_governors.attr,
>>>> @@ -1455,6 +1480,7 @@ static struct attribute *devfreq_attrs[] = {
>>>>  	&dev_attr_min_freq.attr,
>>>>  	&dev_attr_max_freq.attr,
>>>>  	&dev_attr_trans_stat.attr,
>>>> +	&dev_attr_trans_reset.attr,
>>>>  	NULL,
>>>>  };
>>>>  ATTRIBUTE_GROUPS(devfreq);

-- 
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland