| Message ID | 973bd0536c4957d03f36447398498cfacb2393d9.1599031227.git.viresh.kumar@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cpufreq: Record stats with fast-switching | expand |
On Wed, Sep 02, 2020 at 12:54:41PM +0530, Viresh Kumar wrote: > + atomic_t reset_pending; > + atomic_set(&stats->reset_pending, 0); > + if (atomic_read(&stats->reset_pending)) > + bool pending = atomic_read(&stats->reset_pending); > + atomic_set(&stats->reset_pending, 1); > + bool pending = atomic_read(&stats->reset_pending); > + if (atomic_read(&stats->reset_pending)) What do you think atomic_t is doing for you?
On 11-09-20, 12:11, peterz@infradead.org wrote: > On Wed, Sep 02, 2020 at 12:54:41PM +0530, Viresh Kumar wrote: > > + atomic_t reset_pending; > > > + atomic_set(&stats->reset_pending, 0); > > + if (atomic_read(&stats->reset_pending)) > > + bool pending = atomic_read(&stats->reset_pending); > > + atomic_set(&stats->reset_pending, 1); > > + bool pending = atomic_read(&stats->reset_pending); > > + if (atomic_read(&stats->reset_pending)) > > What do you think atomic_t is doing for you? I was trying to avoid races while two writes are going in parallel, but obviously as this isn't a RMW operation, it won't result in anything for me. Maybe what I should be doing is just READ_ONCE()/WRITE_ONCE()? So the other side doesn't see any intermediate value that was never meant to be set/read ? -- viresh
On Fri, Sep 11, 2020 at 1:36 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 11-09-20, 12:11, peterz@infradead.org wrote: > > On Wed, Sep 02, 2020 at 12:54:41PM +0530, Viresh Kumar wrote: > > > + atomic_t reset_pending; > > > > > + atomic_set(&stats->reset_pending, 0); > > > + if (atomic_read(&stats->reset_pending)) > > > + bool pending = atomic_read(&stats->reset_pending); > > > + atomic_set(&stats->reset_pending, 1); > > > + bool pending = atomic_read(&stats->reset_pending); > > > + if (atomic_read(&stats->reset_pending)) > > > > What do you think atomic_t is doing for you? > > I was trying to avoid races while two writes are going in parallel, > but obviously as this isn't a RMW operation, it won't result in > anything for me. > > Maybe what I should be doing is just READ_ONCE()/WRITE_ONCE()? So the > other side doesn't see any intermediate value that was never meant to > be set/read ? If the value in question is a pointer or an int (or equivalent), READ_ONCE()/WRITE_ONCE() should be sufficient, and should be used at least as a matter of annotation of the sensitive code IMO. IIRC, atomic_set() and atomic_read() are pretty much the same as WRITE_ONCE() and READ_ONCE(), respectively, anyway. Cheers!
Hi Viresh, On 9/2/20 8:24 AM, Viresh Kumar wrote: > In order to prepare for lock-less stats update, add support to defer any > updates to it until cpufreq_stats_record_transition() is called. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cpufreq_stats.c | 75 ++++++++++++++++++++++++--------- > 1 file changed, 56 insertions(+), 19 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c > index 94d959a8e954..fdf9e8556a49 100644 > --- a/drivers/cpufreq/cpufreq_stats.c > +++ b/drivers/cpufreq/cpufreq_stats.c > @@ -22,17 +22,22 @@ struct cpufreq_stats { Would it be possible to move this structure in the linux/cpufreq.h header? Any subsystem could have access to it, like to the cpuidle stats. Apart from that (and the comment regarding the 'atomic_t' field) I don't see any issues. Regards, Lukasz
On 15-09-20, 11:04, Lukasz Luba wrote: > Hi Viresh, > > On 9/2/20 8:24 AM, Viresh Kumar wrote: > > In order to prepare for lock-less stats update, add support to defer any > > updates to it until cpufreq_stats_record_transition() is called. > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > drivers/cpufreq/cpufreq_stats.c | 75 ++++++++++++++++++++++++--------- > > 1 file changed, 56 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c > > index 94d959a8e954..fdf9e8556a49 100644 > > --- a/drivers/cpufreq/cpufreq_stats.c > > +++ b/drivers/cpufreq/cpufreq_stats.c > > @@ -22,17 +22,22 @@ struct cpufreq_stats { > > Would it be possible to move this structure in the > linux/cpufreq.h header? Any subsystem could have access to it, > like to the cpuidle stats. Hmm, I am not sure why we should be doing it. In case of cpuidle many parts of the kernel are playing with cpuidle code, like drivers/idle/, drivers/cpuidle, etc. Something should land in include/ only if you want others to use it, but in case of cpufreq no one should be using cpufreq stats. So unless you have a real case where that might be beneficial, I am going to keep it as is. > Apart from that (and the comment regarding the 'atomic_t' field) > I don't see any issues. Thanks. -- viresh
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c index 94d959a8e954..fdf9e8556a49 100644 --- a/drivers/cpufreq/cpufreq_stats.c +++ b/drivers/cpufreq/cpufreq_stats.c @@ -22,17 +22,22 @@ struct cpufreq_stats { spinlock_t lock; unsigned int *freq_table; unsigned int *trans_table; + + /* Deferred reset */ + atomic_t reset_pending; + unsigned long long reset_time; }; -static void cpufreq_stats_update(struct cpufreq_stats *stats) +static void cpufreq_stats_update(struct cpufreq_stats *stats, + unsigned long long time) { unsigned long long cur_time = get_jiffies_64(); - stats->time_in_state[stats->last_index] += cur_time - stats->last_time; + stats->time_in_state[stats->last_index] += cur_time - time; stats->last_time = cur_time; } -static void cpufreq_stats_clear_table(struct cpufreq_stats *stats) +static void cpufreq_stats_reset_table(struct cpufreq_stats *stats) { unsigned int count = stats->max_state; @@ -41,42 +46,67 @@ static void cpufreq_stats_clear_table(struct cpufreq_stats *stats) memset(stats->trans_table, 0, count * count * sizeof(int)); stats->last_time = get_jiffies_64(); stats->total_trans = 0; + + /* Adjust for the time elapsed since reset was requested */ + atomic_set(&stats->reset_pending, 0); + cpufreq_stats_update(stats, stats->reset_time); spin_unlock(&stats->lock); } static ssize_t show_total_trans(struct cpufreq_policy *policy, char *buf) { - return sprintf(buf, "%d\n", policy->stats->total_trans); + struct cpufreq_stats *stats = policy->stats; + + if (atomic_read(&stats->reset_pending)) + return sprintf(buf, "%d\n", 0); + else + return sprintf(buf, "%d\n", stats->total_trans); } cpufreq_freq_attr_ro(total_trans); static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf) { struct cpufreq_stats *stats = policy->stats; + bool pending = atomic_read(&stats->reset_pending); + unsigned long long time; ssize_t len = 0; int i; if (policy->fast_switch_enabled) return 0; - spin_lock(&stats->lock); - cpufreq_stats_update(stats); - spin_unlock(&stats->lock); - for (i = 0; i < stats->state_num; i++) { + if (pending) { + if (i == stats->last_index) + time = get_jiffies_64() - stats->reset_time; + else + time = 0; + } else { + time = stats->time_in_state[i]; + if (i == stats->last_index) + time += get_jiffies_64() - stats->last_time; + } + len += sprintf(buf + len, "%u %llu\n", stats->freq_table[i], - (unsigned long long) - jiffies_64_to_clock_t(stats->time_in_state[i])); + jiffies_64_to_clock_t(time)); } return len; } cpufreq_freq_attr_ro(time_in_state); +/* We don't care what is written to the attribute */ static ssize_t store_reset(struct cpufreq_policy *policy, const char *buf, size_t count) { - /* We don't care what is written to the attribute. */ - cpufreq_stats_clear_table(policy->stats); + struct cpufreq_stats *stats = policy->stats; + + /* + * Defer resetting of stats to cpufreq_stats_record_transition() to + * avoid races. + */ + atomic_set(&stats->reset_pending, 1); + stats->reset_time = get_jiffies_64(); + return count; } cpufreq_freq_attr_wo(reset); @@ -84,8 +114,9 @@ cpufreq_freq_attr_wo(reset); static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf) { struct cpufreq_stats *stats = policy->stats; + bool pending = atomic_read(&stats->reset_pending); ssize_t len = 0; - int i, j; + int i, j, count; if (policy->fast_switch_enabled) return 0; @@ -113,8 +144,13 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf) for (j = 0; j < stats->state_num; j++) { if (len >= PAGE_SIZE) break; - len += scnprintf(buf + len, PAGE_SIZE - len, "%9u ", - stats->trans_table[i*stats->max_state+j]); + + if (pending) + count = 0; + else + count = stats->trans_table[i * stats->max_state + j]; + + len += scnprintf(buf + len, PAGE_SIZE - len, "%9u ", count); } if (len >= PAGE_SIZE) break; @@ -228,10 +264,11 @@ void cpufreq_stats_record_transition(struct cpufreq_policy *policy, struct cpufreq_stats *stats = policy->stats; int old_index, new_index; - if (!stats) { - pr_debug("%s: No stats found\n", __func__); + if (!stats) return; - } + + if (atomic_read(&stats->reset_pending)) + cpufreq_stats_reset_table(stats); old_index = stats->last_index; new_index = freq_table_get_index(stats, new_freq); @@ -241,7 +278,7 @@ void cpufreq_stats_record_transition(struct cpufreq_policy *policy, return; spin_lock(&stats->lock); - cpufreq_stats_update(stats); + cpufreq_stats_update(stats, stats->last_time); stats->last_index = new_index; stats->trans_table[old_index * stats->max_state + new_index]++;
In order to prepare for lock-less stats update, add support to defer any updates to it until cpufreq_stats_record_transition() is called. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/cpufreq_stats.c | 75 ++++++++++++++++++++++++--------- 1 file changed, 56 insertions(+), 19 deletions(-) -- 2.25.0.rc1.19.g042ed3e048af