* [PATCH 1/2] cpufreq: stats: Declare freq-attr right after their callbacks
@ 2019-02-01 6:15 Viresh Kumar
2019-02-01 6:15 ` [PATCH 2/2] cpufreq: stats: Fix concurrency issues while resetting stats Viresh Kumar
2019-02-06 10:29 ` [PATCH 1/2] cpufreq: stats: Declare freq-attr right after their callbacks Rafael J. Wysocki
0 siblings, 2 replies; 4+ messages in thread
From: Viresh Kumar @ 2019-02-01 6:15 UTC (permalink / raw)
To: Rafael Wysocki; +Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel
Freq attribute for "trans_table" is defined right after its callback
(without any blank line between them), but the others are defined
separately later on. Keep this consistent and define all attributes
right after their callbacks.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq_stats.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 1572129844a5..941e63e3e652 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -51,6 +51,7 @@ static ssize_t show_total_trans(struct cpufreq_policy *policy, char *buf)
{
return sprintf(buf, "%d\n", policy->stats->total_trans);
}
+cpufreq_freq_attr_ro(total_trans);
static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
{
@@ -69,6 +70,7 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
}
return len;
}
+cpufreq_freq_attr_ro(time_in_state);
static ssize_t store_reset(struct cpufreq_policy *policy, const char *buf,
size_t count)
@@ -77,6 +79,7 @@ static ssize_t store_reset(struct cpufreq_policy *policy, const char *buf,
cpufreq_stats_clear_table(policy->stats);
return count;
}
+cpufreq_freq_attr_wo(reset);
static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
{
@@ -126,10 +129,6 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
}
cpufreq_freq_attr_ro(trans_table);
-cpufreq_freq_attr_ro(total_trans);
-cpufreq_freq_attr_ro(time_in_state);
-cpufreq_freq_attr_wo(reset);
-
static struct attribute *default_attrs[] = {
&total_trans.attr,
&time_in_state.attr,
--
2.20.1.321.g9e740568ce00
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 2/2] cpufreq: stats: Fix concurrency issues while resetting stats
2019-02-01 6:15 [PATCH 1/2] cpufreq: stats: Declare freq-attr right after their callbacks Viresh Kumar
@ 2019-02-01 6:15 ` Viresh Kumar
2019-02-04 22:31 ` Matthias Kaehlcke
2019-02-06 10:29 ` [PATCH 1/2] cpufreq: stats: Declare freq-attr right after their callbacks Rafael J. Wysocki
1 sibling, 1 reply; 4+ messages in thread
From: Viresh Kumar @ 2019-02-01 6:15 UTC (permalink / raw)
To: Rafael Wysocki; +Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel
It is possible for cpufreq_stats_clear_table() and
cpufreq_stats_record_transition() to get called concurrently and they
will try to update same variables simultaneously and may lead to
corruption of data.
Prevent that with the help of existing spinlock.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq_stats.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 941e63e3e652..e2db5581489a 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -31,20 +31,20 @@ static void cpufreq_stats_update(struct cpufreq_stats *stats)
{
unsigned long long cur_time = get_jiffies_64();
- spin_lock(&cpufreq_stats_lock);
stats->time_in_state[stats->last_index] += cur_time - stats->last_time;
stats->last_time = cur_time;
- spin_unlock(&cpufreq_stats_lock);
}
static void cpufreq_stats_clear_table(struct cpufreq_stats *stats)
{
unsigned int count = stats->max_state;
+ spin_lock(&cpufreq_stats_lock);
memset(stats->time_in_state, 0, count * sizeof(u64));
memset(stats->trans_table, 0, count * count * sizeof(int));
stats->last_time = get_jiffies_64();
stats->total_trans = 0;
+ spin_unlock(&cpufreq_stats_lock);
}
static ssize_t show_total_trans(struct cpufreq_policy *policy, char *buf)
@@ -62,7 +62,10 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
if (policy->fast_switch_enabled)
return 0;
+ spin_lock(&cpufreq_stats_lock);
cpufreq_stats_update(stats);
+ spin_unlock(&cpufreq_stats_lock);
+
for (i = 0; i < stats->state_num; i++) {
len += sprintf(buf + len, "%u %llu\n", stats->freq_table[i],
(unsigned long long)
@@ -239,9 +242,11 @@ void cpufreq_stats_record_transition(struct cpufreq_policy *policy,
if (old_index == -1 || new_index == -1 || old_index == new_index)
return;
+ spin_lock(&cpufreq_stats_lock);
cpufreq_stats_update(stats);
stats->last_index = new_index;
stats->trans_table[old_index * stats->max_state + new_index]++;
stats->total_trans++;
+ spin_unlock(&cpufreq_stats_lock);
}
--
2.20.1.321.g9e740568ce00
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 2/2] cpufreq: stats: Fix concurrency issues while resetting stats
2019-02-01 6:15 ` [PATCH 2/2] cpufreq: stats: Fix concurrency issues while resetting stats Viresh Kumar
@ 2019-02-04 22:31 ` Matthias Kaehlcke
0 siblings, 0 replies; 4+ messages in thread
From: Matthias Kaehlcke @ 2019-02-04 22:31 UTC (permalink / raw)
To: Viresh Kumar; +Cc: Rafael Wysocki, linux-pm, Vincent Guittot, linux-kernel
On Fri, Feb 01, 2019 at 11:45:45AM +0530, Viresh Kumar wrote:
> It is possible for cpufreq_stats_clear_table() and
> cpufreq_stats_record_transition() to get called concurrently and they
> will try to update same variables simultaneously and may lead to
> corruption of data.
>
> Prevent that with the help of existing spinlock.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> drivers/cpufreq/cpufreq_stats.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index 941e63e3e652..e2db5581489a 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -31,20 +31,20 @@ static void cpufreq_stats_update(struct cpufreq_stats *stats)
> {
> unsigned long long cur_time = get_jiffies_64();
>
> - spin_lock(&cpufreq_stats_lock);
> stats->time_in_state[stats->last_index] += cur_time - stats->last_time;
> stats->last_time = cur_time;
> - spin_unlock(&cpufreq_stats_lock);
> }
>
> static void cpufreq_stats_clear_table(struct cpufreq_stats *stats)
> {
> unsigned int count = stats->max_state;
>
> + spin_lock(&cpufreq_stats_lock);
> memset(stats->time_in_state, 0, count * sizeof(u64));
> memset(stats->trans_table, 0, count * count * sizeof(int));
> stats->last_time = get_jiffies_64();
> stats->total_trans = 0;
> + spin_unlock(&cpufreq_stats_lock);
> }
>
> static ssize_t show_total_trans(struct cpufreq_policy *policy, char *buf)
> @@ -62,7 +62,10 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
> if (policy->fast_switch_enabled)
> return 0;
>
> + spin_lock(&cpufreq_stats_lock);
> cpufreq_stats_update(stats);
> + spin_unlock(&cpufreq_stats_lock);
> +
> for (i = 0; i < stats->state_num; i++) {
> len += sprintf(buf + len, "%u %llu\n", stats->freq_table[i],
> (unsigned long long)
> @@ -239,9 +242,11 @@ void cpufreq_stats_record_transition(struct cpufreq_policy *policy,
> if (old_index == -1 || new_index == -1 || old_index == new_index)
> return;
>
> + spin_lock(&cpufreq_stats_lock);
> cpufreq_stats_update(stats);
>
> stats->last_index = new_index;
> stats->trans_table[old_index * stats->max_state + new_index]++;
> stats->total_trans++;
> + spin_unlock(&cpufreq_stats_lock);
> }
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] cpufreq: stats: Declare freq-attr right after their callbacks
2019-02-01 6:15 [PATCH 1/2] cpufreq: stats: Declare freq-attr right after their callbacks Viresh Kumar
2019-02-01 6:15 ` [PATCH 2/2] cpufreq: stats: Fix concurrency issues while resetting stats Viresh Kumar
@ 2019-02-06 10:29 ` Rafael J. Wysocki
1 sibling, 0 replies; 4+ messages in thread
From: Rafael J. Wysocki @ 2019-02-06 10:29 UTC (permalink / raw)
To: Viresh Kumar; +Cc: linux-pm, Vincent Guittot, linux-kernel
On Friday, February 1, 2019 7:15:44 AM CET Viresh Kumar wrote:
> Freq attribute for "trans_table" is defined right after its callback
> (without any blank line between them), but the others are defined
> separately later on. Keep this consistent and define all attributes
> right after their callbacks.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> drivers/cpufreq/cpufreq_stats.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index 1572129844a5..941e63e3e652 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -51,6 +51,7 @@ static ssize_t show_total_trans(struct cpufreq_policy *policy, char *buf)
> {
> return sprintf(buf, "%d\n", policy->stats->total_trans);
> }
> +cpufreq_freq_attr_ro(total_trans);
>
> static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
> {
> @@ -69,6 +70,7 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
> }
> return len;
> }
> +cpufreq_freq_attr_ro(time_in_state);
>
> static ssize_t store_reset(struct cpufreq_policy *policy, const char *buf,
> size_t count)
> @@ -77,6 +79,7 @@ static ssize_t store_reset(struct cpufreq_policy *policy, const char *buf,
> cpufreq_stats_clear_table(policy->stats);
> return count;
> }
> +cpufreq_freq_attr_wo(reset);
>
> static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
> {
> @@ -126,10 +129,6 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
> }
> cpufreq_freq_attr_ro(trans_table);
>
> -cpufreq_freq_attr_ro(total_trans);
> -cpufreq_freq_attr_ro(time_in_state);
> -cpufreq_freq_attr_wo(reset);
> -
> static struct attribute *default_attrs[] = {
> &total_trans.attr,
> &time_in_state.attr,
>
Applied, along with the [2/2], thanks!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-02-06 10:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-01 6:15 [PATCH 1/2] cpufreq: stats: Declare freq-attr right after their callbacks Viresh Kumar
2019-02-01 6:15 ` [PATCH 2/2] cpufreq: stats: Fix concurrency issues while resetting stats Viresh Kumar
2019-02-04 22:31 ` Matthias Kaehlcke
2019-02-06 10:29 ` [PATCH 1/2] cpufreq: stats: Declare freq-attr right after their callbacks Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).