Re: [PATCH v2 1/3] devfreq: change time stats to 64-bit

From: Kamil Konieczny <k.konieczny@samsung.com>
To: Chanwoo Choi <cw00.choi@samsung.com>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	MyungJoo Ham <myungjoo.ham@samsung.com>
Subject: Re: [PATCH v2 1/3] devfreq: change time stats to 64-bit
Date: Thu, 5 Dec 2019 10:58:58 +0100	[thread overview]
Message-ID: <b6e26205-7aa0-7502-c936-76a031b3b1d6@samsung.com> (raw)
In-Reply-To: <0fd243ee-001e-a93a-3421-3e3c82947f11@samsung.com>

Hi,

On 05.12.2019 01:27, Chanwoo Choi wrote:
> On 12/5/19 12:00 AM, Kamil Konieczny wrote:
>> Change time stats counting to bigger type by using 64-bit jiffies.
>> This will make devfreq stats code look similar to cpufreq stats and
>> prevents overflow (for HZ = 1000 after 49.7 days).
>>
>> Signed-off-by: Kamil Konieczny <k.konieczny@samsung.com>
>> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>> Changes in v2:
>>  added Acked-by, rebased on linux-next
>>
>>  drivers/devfreq/devfreq.c | 14 +++++++-------
>>  include/linux/devfreq.h   |  4 ++--
>>  2 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index bdeb4189c978..0e2030403e4a 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -199,10 +199,10 @@ static int set_freq_table(struct devfreq *devfreq)
>>  int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>  {
>>  	int lev, prev_lev, ret = 0;
>> -	unsigned long cur_time;
>> +	unsigned long long cur_time;
> 
> It looks better to use 'u64' instead of 'unsigned long long'.
> Because get_jiffies_u64 has 'u64' return type.

You are right, I will change this and send v3.

>>  
>>  	lockdep_assert_held(&devfreq->lock);
>> -	cur_time = jiffies;
>> +	cur_time = get_jiffies_64();
>>  
>>  	/* Immediately exit if previous_freq is not initialized yet. */
>>  	if (!devfreq->previous_freq)
>> @@ -525,7 +525,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
>>  			msecs_to_jiffies(devfreq->profile->polling_ms));
>>  
>>  out_update:
>> -	devfreq->last_stat_updated = jiffies;
>> +	devfreq->last_stat_updated = get_jiffies_64();
>>  	devfreq->stop_polling = false;
>>  
>>  	if (devfreq->profile->get_cur_freq &&
>> @@ -748,7 +748,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>  
>>  	devfreq->time_in_state = devm_kcalloc(&devfreq->dev,
>>  			devfreq->profile->max_state,
>> -			sizeof(unsigned long),
>> +			sizeof(*devfreq->time_in_state),
>>  			GFP_KERNEL);
>>  	if (!devfreq->time_in_state) {
>>  		mutex_unlock(&devfreq->lock);
>> @@ -756,7 +756,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>  		goto err_devfreq;
>>  	}
>>  
>> -	devfreq->last_stat_updated = jiffies;
>> +	devfreq->last_stat_updated = get_jiffies_64();
>>  
>>  	srcu_init_notifier_head(&devfreq->transition_notifier_list);
>>  
>> @@ -1470,8 +1470,8 @@ static ssize_t trans_stat_show(struct device *dev,
>>  		for (j = 0; j < max_state; j++)
>>  			len += sprintf(buf + len, "%10u",
>>  				devfreq->trans_table[(i * max_state) + j]);
>> -		len += sprintf(buf + len, "%10u\n",
>> -			jiffies_to_msecs(devfreq->time_in_state[i]));
>> +		len += sprintf(buf + len, "%10llu\n", (u64)
>> +			jiffies64_to_msecs(devfreq->time_in_state[i]));
>>  	}
>>  
>>  	len += sprintf(buf + len, "Total transition : %u\n",
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index 2bae9ed3c783..b81a86e47fb9 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -174,8 +174,8 @@ struct devfreq {
>>  	/* information for device frequency transition */
>>  	unsigned int total_trans;
>>  	unsigned int *trans_table;
>> -	unsigned long *time_in_state;
>> -	unsigned long last_stat_updated;
>> +	u64 *time_in_state;
>> +	unsigned long long last_stat_updated;
> 
> ditto. 'unsigned long long' -> 'u64'.

Yes, will change this too.

>>  	struct srcu_notifier_head transition_notifier_list;
>>  };
>>

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