2019-11-13 09:15:05

by Kamil Konieczny

Subject: [PATCH 5/7] devfreq: move transition statistics to devfreq profile structure

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 <[email protected]>
---
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;
};

--
2.24.0

2019-11-14 01:58:53

by Chanwoo Choi

Subject: Re: [PATCH 5/7] devfreq: move transition statistics to devfreq profile structure

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.

Thanks.
Chanwoo Choi


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 <[email protected]>
> ---
> 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;
> };
>
>

2019-11-14 18:15:43

by Bartlomiej Zolnierkiewicz

Subject: Re: [PATCH 5/7] devfreq: move transition statistics to devfreq profile structure

Hi Chanwoo,

On 11/14/19 3:02 AM, 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

How's about doing it the other way around -> moving 'freq_table' and
'max_state' from 'struct devfreq_dev_profile' to 'struct devfreq'?

They are both initialized/generated by the core during
devfreq_add_device()->set_freq_table() for all in-kernel drivers.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> don't require the any structure like 'devfreq_dev_profile'.
>
> So, I can't agree.
>
> Thanks.
> Chanwoo Choi
>
>
> 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 <[email protected]>
>> ---
>> 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;
>> };
>>
>>

2019-11-15 05:11:11

by Chanwoo Choi

Subject: Re: [PATCH 5/7] devfreq: move transition statistics to devfreq profile structure

Hi Bartlomiej,

On 11/15/19 3:10 AM, Bartlomiej Zolnierkiewicz wrote:
>
> Hi Chanwoo,
>
> On 11/14/19 3:02 AM, 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
>
> How's about doing it the other way around -> moving 'freq_table' and
> 'max_state' from 'struct devfreq_dev_profile' to 'struct devfreq'?

I agree to move 'freq_table' and 'max_state' to 'struct devfreq
as I replied on patch7 from your reply. But, it have to be touched
with the ordering issue of freq_table on drivers/thermal/devfreq_cooling.c.

Regards,
Chanwoo Choi

>
> They are both initialized/generated by the core during
> devfreq_add_device()->set_freq_table() for all in-kernel drivers.
>
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
>
>> don't require the any structure like 'devfreq_dev_profile'.
>>
>> So, I can't agree.
>>
>> Thanks.
>> Chanwoo Choi
>>
>>
>> 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 <[email protected]>
>>> ---
>>> 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;
>>> };
>>>
>>>
>
>
>

2019-11-15 14:43:52

by Kamil Konieczny

Subject: Re: [PATCH 5/7] devfreq: move transition statistics to devfreq profile structure

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 <[email protected]>
>> ---
>> 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