Messages in this thread | | | Subject | Re: [PATCH 5/7] devfreq: move transition statistics to devfreq profile structure | From | Kamil Konieczny <> | Date | Fri, 15 Nov 2019 15:39:42 +0100 |
| |
On 14.11.2019 03:02, Chanwoo Choi wrote: > Hi Kamil, > > The devfreq_dev_profile structure have to only contain the each devfreq device > callback function/data which are used in the devfreq core. > > The generated information after registered the devfreq device > with devfreq_add_device() should be stored in the 'struct device'. > > The devfreq core need to split out the data between user input > data (struct devfreq_dev_profile) and the initialized/generated data > by core (struct devfreq). It is not same with cpufreq. cpufreq > don't require the any structure like 'devfreq_dev_profile'. > > So, I can't agree.
What about putting stats structure inside devfreq ?
> On 11/13/19 6:13 PM, Kamil Konieczny wrote: >> Move transition statistics to devfreq profile structure. This is for >> preparation for moving transition statistics into separate struct. >> It is safe to do as frequency table and maximum state information are >> already present in devfreq profile structure and there are no devfreq >> drivers using more than one instance of devfreq structure per devfreq >> profile one. >> >> It also makes devfreq code more similar to cpufreq one. >> >> Signed-off-by: Kamil Konieczny <k.konieczny@samsung.com> >> --- >> drivers/devfreq/devfreq.c | 115 +++++++++++++++++++------------------- >> include/linux/devfreq.h | 25 ++++----- >> 2 files changed, 70 insertions(+), 70 deletions(-) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 6e5a17f4c92c..70533b787744 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -128,7 +128,7 @@ static int set_freq_table(struct devfreq *devfreq) >> >> profile->max_state = count; >> profile->freq_table = devm_kcalloc(devfreq->dev.parent, >> - profile->max_state, >> + count, >> sizeof(*profile->freq_table), >> GFP_KERNEL); >> if (!profile->freq_table) { >> @@ -157,29 +157,30 @@ static int set_freq_table(struct devfreq *devfreq) >> */ >> int devfreq_update_status(struct devfreq *devfreq, unsigned long freq) >> { >> - int lev, prev_lev, ret = 0; >> + struct devfreq_dev_profile *profile = devfreq->profile; >> unsigned long long cur_time; >> + int lev, prev_lev, ret = 0; >> >> cur_time = get_jiffies_64(); >> >> /* Immediately exit if previous_freq is not initialized yet. */ >> if (!devfreq->previous_freq) { >> - spin_lock(&devfreq->stats_lock); >> - devfreq->last_time = cur_time; >> - spin_unlock(&devfreq->stats_lock); >> + spin_lock(&profile->stats_lock); >> + profile->last_time = cur_time; >> + spin_unlock(&profile->stats_lock); >> return 0; >> } >> >> prev_lev = devfreq_get_freq_level(devfreq, devfreq->previous_freq); >> >> - spin_lock(&devfreq->stats_lock); >> + spin_lock(&profile->stats_lock); >> if (prev_lev < 0) { >> ret = prev_lev; >> goto out; >> } >> >> - devfreq->time_in_state[prev_lev] += >> - cur_time - devfreq->last_time; >> + profile->time_in_state[prev_lev] += >> + cur_time - profile->last_time; >> lev = devfreq_get_freq_level(devfreq, freq); >> if (lev < 0) { >> ret = lev; >> @@ -187,14 +188,14 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq) >> } >> >> if (lev != prev_lev) { >> - devfreq->trans_table[(prev_lev * >> - devfreq->profile->max_state) + lev]++; >> - devfreq->total_trans++; >> + profile->trans_table[(prev_lev * >> + profile->max_state) + lev]++; >> + profile->total_trans++; >> } >> >> out: >> - devfreq->last_time = cur_time; >> - spin_unlock(&devfreq->stats_lock); >> + profile->last_time = cur_time; >> + spin_unlock(&profile->stats_lock); >> return ret; >> } >> EXPORT_SYMBOL(devfreq_update_status); >> @@ -474,23 +475,23 @@ EXPORT_SYMBOL(devfreq_monitor_suspend); >> void devfreq_monitor_resume(struct devfreq *devfreq) >> { >> unsigned long freq; >> + struct devfreq_dev_profile *profile = devfreq->profile; >> >> mutex_lock(&devfreq->lock); >> if (!devfreq->stop_polling) >> goto out; >> >> - if (!delayed_work_pending(&devfreq->work) && >> - devfreq->profile->polling_ms) >> + if (!delayed_work_pending(&devfreq->work) && profile->polling_ms) >> queue_delayed_work(devfreq_wq, &devfreq->work, >> - msecs_to_jiffies(devfreq->profile->polling_ms)); >> + msecs_to_jiffies(profile->polling_ms)); >> >> - spin_lock(&devfreq->stats_lock); >> - devfreq->last_time = get_jiffies_64(); >> - spin_unlock(&devfreq->stats_lock); >> + spin_lock(&profile->stats_lock); >> + profile->last_time = get_jiffies_64(); >> + spin_unlock(&profile->stats_lock); >> devfreq->stop_polling = false; >> >> - if (devfreq->profile->get_cur_freq && >> - !devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq)) >> + if (profile->get_cur_freq && >> + !profile->get_cur_freq(devfreq->dev.parent, &freq)) >> devfreq->previous_freq = freq; >> >> out: >> @@ -657,7 +658,7 @@ struct devfreq *devfreq_add_device(struct device *dev, >> devfreq->data = data; >> devfreq->nb.notifier_call = devfreq_notifier_call; >> >> - if (!devfreq->profile->max_state && !devfreq->profile->freq_table) { >> + if (!profile->max_state && !profile->freq_table) { >> mutex_unlock(&devfreq->lock); >> err = set_freq_table(devfreq); >> if (err < 0) >> @@ -693,29 +694,29 @@ struct devfreq *devfreq_add_device(struct device *dev, >> goto err_out; >> } >> >> - devfreq->trans_table = devm_kzalloc(&devfreq->dev, >> - array3_size(sizeof(unsigned int), >> - devfreq->profile->max_state, >> - devfreq->profile->max_state), >> - GFP_KERNEL); >> - if (!devfreq->trans_table) { >> + profile->trans_table = devm_kzalloc(&devfreq->dev, >> + array3_size(sizeof(unsigned int), >> + profile->max_state, >> + profile->max_state), >> + GFP_KERNEL); >> + if (!profile->trans_table) { >> mutex_unlock(&devfreq->lock); >> err = -ENOMEM; >> goto err_devfreq; >> } >> >> - devfreq->time_in_state = devm_kcalloc(&devfreq->dev, >> - devfreq->profile->max_state, >> - sizeof(*devfreq->time_in_state), >> - GFP_KERNEL); >> - if (!devfreq->time_in_state) { >> + profile->time_in_state = devm_kcalloc(&devfreq->dev, >> + profile->max_state, >> + sizeof(*profile->time_in_state), >> + GFP_KERNEL); >> + if (!profile->time_in_state) { >> mutex_unlock(&devfreq->lock); >> err = -ENOMEM; >> goto err_devfreq; >> } >> >> - devfreq->last_time = get_jiffies_64(); >> - spin_lock_init(&devfreq->stats_lock); >> + profile->last_time = get_jiffies_64(); >> + spin_lock_init(&profile->stats_lock); >> >> srcu_init_notifier_head(&devfreq->transition_notifier_list); >> >> @@ -1402,9 +1403,10 @@ static ssize_t trans_stat_show(struct device *dev, >> struct device_attribute *attr, char *buf) >> { >> struct devfreq *devfreq = to_devfreq(dev); >> + struct devfreq_dev_profile *profile = devfreq->profile; >> ssize_t len; >> int i, j; >> - unsigned int max_state = devfreq->profile->max_state; >> + unsigned int max_state = profile->max_state; >> >> if (!devfreq->stop_polling && >> devfreq_update_status(devfreq, devfreq->previous_freq)) >> @@ -1415,46 +1417,45 @@ static ssize_t trans_stat_show(struct device *dev, >> len = sprintf(buf, " From : To\n"); >> len += sprintf(buf + len, " :"); >> >> - spin_lock(&devfreq->stats_lock); >> + spin_lock(&profile->stats_lock); >> for (i = 0; i < max_state; i++) >> len += sprintf(buf + len, "%10lu", >> - devfreq->profile->freq_table[i]); >> + profile->freq_table[i]); >> >> len += sprintf(buf + len, " time(ms)\n"); >> >> for (i = 0; i < max_state; i++) { >> - if (devfreq->profile->freq_table[i] >> - == devfreq->previous_freq) { >> + if (profile->freq_table[i] == devfreq->previous_freq) >> len += sprintf(buf + len, "*"); >> - } else { >> + else >> len += sprintf(buf + len, " "); >> - } >> + >> len += sprintf(buf + len, "%10lu:", >> - devfreq->profile->freq_table[i]); >> + profile->freq_table[i]); >> for (j = 0; j < max_state; j++) >> len += sprintf(buf + len, "%10u", >> - devfreq->trans_table[(i * max_state) + j]); >> + profile->trans_table[(i * max_state) + j]); >> len += sprintf(buf + len, "%10llu\n", (u64) >> - jiffies64_to_msecs(devfreq->time_in_state[i])); >> + jiffies64_to_msecs(profile->time_in_state[i])); >> } >> >> len += sprintf(buf + len, "Total transition : %u\n", >> - devfreq->total_trans); >> - spin_unlock(&devfreq->stats_lock); >> + profile->total_trans); >> + spin_unlock(&profile->stats_lock); >> return len; >> } >> static DEVICE_ATTR_RO(trans_stat); >> >> -static void defvreq_stats_clear_table(struct devfreq *devfreq) >> +static void defvreq_stats_clear_table(struct devfreq_dev_profile *profile) >> { >> - 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_time = get_jiffies_64(); >> - devfreq->total_trans = 0; >> - spin_unlock(&devfreq->stats_lock); >> + unsigned int count = profile->max_state; >> + >> + spin_lock(&profile->stats_lock); >> + memset(profile->time_in_state, 0, count * sizeof(u64)); >> + memset(profile->trans_table, 0, count * count * sizeof(int)); >> + profile->last_time = get_jiffies_64(); >> + profile->total_trans = 0; >> + spin_unlock(&profile->stats_lock); >> } >> >> static ssize_t trans_reset_store(struct device *dev, >> @@ -1464,7 +1465,7 @@ static ssize_t trans_reset_store(struct device *dev, >> { >> struct devfreq *devfreq = to_devfreq(dev); >> >> - defvreq_stats_clear_table(devfreq); >> + defvreq_stats_clear_table(devfreq->profile); >> >> return count; >> } >> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h >> index 2ddf25993f7d..4ceb2a517a9c 100644 >> --- a/include/linux/devfreq.h >> +++ b/include/linux/devfreq.h >> @@ -91,6 +91,12 @@ struct devfreq_dev_status { >> * @freq_table: Optional list of frequencies to support statistics >> * and freq_table must be generated in ascending order. >> * @max_state: The size of freq_table. >> + * @total_trans: Number of devfreq transitions >> + * @trans_table: Statistics of devfreq transitions >> + * @time_in_state: Statistics of devfreq states >> + * @last_time: The last time stats were updated >> + * @stats_lock: Lock protecting trans_table, time_in_state, >> + * last_time and total_trans used for statistics >> */ >> struct devfreq_dev_profile { >> unsigned long initial_freq; >> @@ -104,6 +110,12 @@ struct devfreq_dev_profile { >> >> unsigned long *freq_table; >> unsigned int max_state; >> + /* information for device frequency transition */ >> + unsigned int total_trans; >> + unsigned int *trans_table; >> + u64 *time_in_state; >> + unsigned long long last_time; >> + spinlock_t stats_lock; >> }; >> >> /** >> @@ -131,12 +143,6 @@ struct devfreq_dev_profile { >> * @suspend_freq: frequency of a device set during suspend phase. >> * @resume_freq: frequency of a device set in resume phase. >> * @suspend_count: suspend requests counter for a device. >> - * @total_trans: Number of devfreq transitions >> - * @trans_table: Statistics of devfreq transitions >> - * @time_in_state: Statistics of devfreq states >> - * @last_time: The last time stats were updated >> - * @stats_lock: Lock protecting trans_table, time_in_state, last_time >> - * and total_trans used for statistics >> * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier >> * >> * This structure stores the devfreq information for a give device. >> @@ -173,13 +179,6 @@ struct devfreq { >> unsigned long resume_freq; >> atomic_t suspend_count; >> >> - /* information for device frequency transition */ >> - unsigned int total_trans; >> - unsigned int *trans_table; >> - u64 *time_in_state; >> - unsigned long long last_time; >> - spinlock_t stats_lock; >> - >> struct srcu_notifier_head transition_notifier_list; >> }; >> >> > >
-- Best regards, Kamil Konieczny Samsung R&D Institute Poland
| |