Hi,
this patch series improves devfreq statistics:
- do conversion to use 64-bit jiffies for storing elapsed time and prevent counters
overflow,
- add ability to reset statistics using sysfs,
- move statistics data to separate structure for improved code
readability and maintenance,
- make devfreq statistics code more similar to cpufreq statistics
code for improved long-term maintainability
The first four patches fix time stats to use 64-bits, add spinlock for protecting data
access, add new function in sysfs for clearing statistics counters and change var name
used in time counters. Remaining patches make steps to moving stats into separate
structure devfreq_stats.
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
Kamil Konieczny (7):
devfreq: change time stats to 64-bit
devfreq: protect devfreq stats data with spinlock
devfreq: add clearing transitions stats in sysfs
devfreq: change var name used in time statistics
devfreq: move transition statistics to devfreq profile structure
devfreq: move transition statistics allocations to set_freq_stats()
devfreq: move statistics to separate struct
drivers/devfreq/devfreq.c | 199 ++++++++++++++++++-----------
drivers/devfreq/exynos-bus.c | 6 +-
drivers/devfreq/governor_passive.c | 26 ++--
include/linux/devfreq.h | 41 +++---
4 files changed, 167 insertions(+), 105 deletions(-)
--
2.24.0
Protect access to devfreq transitions stats with spinlock.
Signed-off-by: Kamil Konieczny <[email protected]>
---
drivers/devfreq/devfreq.c | 18 +++++++++++++++---
include/linux/devfreq.h | 3 +++
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 1602cca20fc4..ac04b5baef70 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -163,10 +163,16 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
cur_time = get_jiffies_64();
/* Immediately exit if previous_freq is not initialized yet. */
- if (!devfreq->previous_freq)
- goto out;
+ if (!devfreq->previous_freq) {
+ spin_lock(&devfreq->stats_lock);
+ devfreq->last_stat_updated = cur_time;
+ spin_unlock(&devfreq->stats_lock);
+ return 0;
+ }
prev_lev = devfreq_get_freq_level(devfreq, devfreq->previous_freq);
+
+ spin_lock(&devfreq->stats_lock);
if (prev_lev < 0) {
ret = prev_lev;
goto out;
@@ -174,7 +180,6 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
devfreq->time_in_state[prev_lev] +=
cur_time - devfreq->last_stat_updated;
-
lev = devfreq_get_freq_level(devfreq, freq);
if (lev < 0) {
ret = lev;
@@ -189,6 +194,7 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
out:
devfreq->last_stat_updated = cur_time;
+ spin_unlock(&devfreq->stats_lock);
return ret;
}
EXPORT_SYMBOL(devfreq_update_status);
@@ -478,7 +484,9 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
queue_delayed_work(devfreq_wq, &devfreq->work,
msecs_to_jiffies(devfreq->profile->polling_ms));
+ spin_lock(&devfreq->stats_lock);
devfreq->last_stat_updated = get_jiffies_64();
+ spin_unlock(&devfreq->stats_lock);
devfreq->stop_polling = false;
if (devfreq->profile->get_cur_freq &&
@@ -707,6 +715,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
}
devfreq->last_stat_updated = get_jiffies_64();
+ spin_lock_init(&devfreq->stats_lock);
srcu_init_notifier_head(&devfreq->transition_notifier_list);
@@ -1405,6 +1414,8 @@ static ssize_t trans_stat_show(struct device *dev,
len = sprintf(buf, " From : To\n");
len += sprintf(buf + len, " :");
+
+ spin_lock(&devfreq->stats_lock);
for (i = 0; i < max_state; i++)
len += sprintf(buf + len, "%10lu",
devfreq->profile->freq_table[i]);
@@ -1429,6 +1440,7 @@ static ssize_t trans_stat_show(struct device *dev,
len += sprintf(buf + len, "Total transition : %u\n",
devfreq->total_trans);
+ spin_unlock(&devfreq->stats_lock);
return len;
}
static DEVICE_ATTR_RO(trans_stat);
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index b81a86e47fb9..a344e0be99f3 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -135,6 +135,8 @@ struct devfreq_dev_profile {
* @trans_table: Statistics of devfreq transitions
* @time_in_state: Statistics of devfreq states
* @last_stat_updated: The last time stat 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.
@@ -176,6 +178,7 @@ struct devfreq {
unsigned int *trans_table;
u64 *time_in_state;
unsigned long long last_stat_updated;
+ spinlock_t stats_lock;
struct srcu_notifier_head transition_notifier_list;
};
--
2.24.0
Hi,
On 11/13/19 6:13 PM, Kamil Konieczny wrote:
> Protect access to devfreq transitions stats with spinlock.
You have to add the more detailed reason why spinlock is necessary.
And are there any issue without spinlock?
In my case, I used the devfreq sysfs entries on Tizen Platfrom
for a long time, there are no any issue without spinlock.
Regards,
Chanwoo Choi
>
> Signed-off-by: Kamil Konieczny <[email protected]>
> ---
> drivers/devfreq/devfreq.c | 18 +++++++++++++++---
> include/linux/devfreq.h | 3 +++
> 2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 1602cca20fc4..ac04b5baef70 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -163,10 +163,16 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
> cur_time = get_jiffies_64();
>
> /* Immediately exit if previous_freq is not initialized yet. */
> - if (!devfreq->previous_freq)
> - goto out;
> + if (!devfreq->previous_freq) {
> + spin_lock(&devfreq->stats_lock);
> + devfreq->last_stat_updated = cur_time;
> + spin_unlock(&devfreq->stats_lock);
> + return 0;
> + }
>
> prev_lev = devfreq_get_freq_level(devfreq, devfreq->previous_freq);
> +
> + spin_lock(&devfreq->stats_lock);
> if (prev_lev < 0) {
> ret = prev_lev;
> goto out;
> @@ -174,7 +180,6 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>
> devfreq->time_in_state[prev_lev] +=
> cur_time - devfreq->last_stat_updated;
> -
> lev = devfreq_get_freq_level(devfreq, freq);
> if (lev < 0) {
> ret = lev;
> @@ -189,6 +194,7 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>
> out:
> devfreq->last_stat_updated = cur_time;
> + spin_unlock(&devfreq->stats_lock);
> return ret;
> }
> EXPORT_SYMBOL(devfreq_update_status);
> @@ -478,7 +484,9 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
> queue_delayed_work(devfreq_wq, &devfreq->work,
> msecs_to_jiffies(devfreq->profile->polling_ms));
>
> + spin_lock(&devfreq->stats_lock);
> devfreq->last_stat_updated = get_jiffies_64();
> + spin_unlock(&devfreq->stats_lock);
> devfreq->stop_polling = false;
>
> if (devfreq->profile->get_cur_freq &&
> @@ -707,6 +715,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
> }
>
> devfreq->last_stat_updated = get_jiffies_64();
> + spin_lock_init(&devfreq->stats_lock);
>
> srcu_init_notifier_head(&devfreq->transition_notifier_list);
>
> @@ -1405,6 +1414,8 @@ static ssize_t trans_stat_show(struct device *dev,
>
> len = sprintf(buf, " From : To\n");
> len += sprintf(buf + len, " :");
> +
> + spin_lock(&devfreq->stats_lock);
> for (i = 0; i < max_state; i++)
> len += sprintf(buf + len, "%10lu",
> devfreq->profile->freq_table[i]);
> @@ -1429,6 +1440,7 @@ static ssize_t trans_stat_show(struct device *dev,
>
> len += sprintf(buf + len, "Total transition : %u\n",
> devfreq->total_trans);
> + spin_unlock(&devfreq->stats_lock);
> return len;
> }
> static DEVICE_ATTR_RO(trans_stat);
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index b81a86e47fb9..a344e0be99f3 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -135,6 +135,8 @@ struct devfreq_dev_profile {
> * @trans_table: Statistics of devfreq transitions
> * @time_in_state: Statistics of devfreq states
> * @last_stat_updated: The last time stat 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.
> @@ -176,6 +178,7 @@ struct devfreq {
> unsigned int *trans_table;
> u64 *time_in_state;
> unsigned long long last_stat_updated;
> + spinlock_t stats_lock;
>
> struct srcu_notifier_head transition_notifier_list;
> };
>