Re: [PATCH 1/4] cpufreq: stats: Defer stats update to cpufreq_stats_record_transition()

2020-09-15 Thread Viresh Kumar 
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 
> > ---
> >   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

Re: [PATCH 1/4] cpufreq: stats: Defer stats update to cpufreq_stats_record_transition()

2020-09-15 Thread Lukasz Luba 

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 
---
  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

Re: [PATCH 1/4] cpufreq: stats: Defer stats update to cpufreq_stats_record_transition()

2020-09-11 Thread Rafael J. Wysocki 
On Fri, Sep 11, 2020 at 1:36 PM Viresh Kumar  wrote:
>
> On 11-09-20, 12:11, pet...@infradead.org wrote:
> > On Wed, Sep 02, 2020 at 12:54:41PM +0530, Viresh Kumar wrote:
> > > +   atomic_t reset_pending;
> >
> > > +   atomic_set(>reset_pending, 0);
> > > +   if (atomic_read(>reset_pending))
> > > +   bool pending = atomic_read(>reset_pending);
> > > +   atomic_set(>reset_pending, 1);
> > > +   bool pending = atomic_read(>reset_pending);
> > > +   if (atomic_read(>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!

Re: [PATCH 1/4] cpufreq: stats: Defer stats update to cpufreq_stats_record_transition()

2020-09-11 Thread Viresh Kumar 
On 11-09-20, 12:11, pet...@infradead.org wrote:
> On Wed, Sep 02, 2020 at 12:54:41PM +0530, Viresh Kumar wrote:
> > +   atomic_t reset_pending;
> 
> > +   atomic_set(>reset_pending, 0);
> > +   if (atomic_read(>reset_pending))
> > +   bool pending = atomic_read(>reset_pending);
> > +   atomic_set(>reset_pending, 1);
> > +   bool pending = atomic_read(>reset_pending);
> > +   if (atomic_read(>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

Re: [PATCH 1/4] cpufreq: stats: Defer stats update to cpufreq_stats_record_transition()

2020-09-11 Thread peterz 
On Wed, Sep 02, 2020 at 12:54:41PM +0530, Viresh Kumar wrote:
> + atomic_t reset_pending;

> + atomic_set(>reset_pending, 0);
> + if (atomic_read(>reset_pending))
> + bool pending = atomic_read(>reset_pending);
> + atomic_set(>reset_pending, 1);
> + bool pending = atomic_read(>reset_pending);
> + if (atomic_read(>reset_pending))

What do you think atomic_t is doing for you?

[PATCH 1/4] cpufreq: stats: Defer stats update to cpufreq_stats_record_transition()

2020-09-02 Thread Viresh Kumar 
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 
---
 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 {
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(>reset_pending, 0);
+   cpufreq_stats_update(stats, stats->reset_time);
spin_unlock(>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(>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(>reset_pending);
+   unsigned long long time;
ssize_t len = 0;
int i;
 
if (policy->fast_switch_enabled)
return 0;
 
-   spin_lock(>lock);
-   cpufreq_stats_update(stats);
-   spin_unlock(>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(>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(>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 =