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

Kamil Konieczny
2019-12-05T09:59:04Z

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