Re: [PATCH 1/2] count_stat_eventual: Switch from ACCESS_ONCE() to READ_ONCE()/WRITE_ONCE()

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Sat, May 13, 2017 at 09:04:38PM +0900, Akira Yokosawa wrote:
> Hi Jason & Paul,
> 
> although this has already been applied, I have a comment.
> 
> On 2017/05/11 23:03:41 +0800, Junchang Wang wrote:
> > Signed-off-by: Junchang Wang <junchangwang@gmail.com>
> > ---
> >  CodeSamples/count/count_stat_eventual.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/CodeSamples/count/count_stat_eventual.c b/CodeSamples/count/count_stat_eventual.c
> > index 059ab8b..cbde4aa 100644
> > --- a/CodeSamples/count/count_stat_eventual.c
> > +++ b/CodeSamples/count/count_stat_eventual.c
> > @@ -27,12 +27,12 @@ int stopflag;
> >  
> >  void inc_count(void)
> >  {
> > -	ACCESS_ONCE(__get_thread_var(counter))++;
> > +	READ_ONCE(__get_thread_var(counter))++;
> 
> This is OK because READ_ONCE() is defined as the same as ACCESS_ONCE()
> in CodeSamples. However, the definition in the current Linux kernel
> would not permit this.
> 
> A read-modify-write access would need both READ_ONCE() and WRITE_ONCE().
> However, since "counter" is thread local and updated only by its owner,
> we don't need READ_ONCE() here. So:
> 
> +	WRITE_ONCE(__get_thread_var(counter), __get_thread_var(counter) + 1);
> 
> should have been sufficient.
> 
> Problem with this change is that the line gets too wide when applied to
> the code snippet in 2-column layout.

Good point -- though renumbering the code is not all -that- hard.

I clearly should have made a better READ_ONCE() that enforced the same
constraints as does the Linux kernel, perhaps something like this:

	#define READ_ONCE(x) ({ ACCESS_ONCE(x) })

Thoughts?

							Thanx, Paul

> Hmm...
> 
>                                  Thanks, Akira
> 
> >  }
> >  
> >  unsigned long read_count(void)
> >  {
> > -	return ACCESS_ONCE(global_count);
> > +	return READ_ONCE(global_count);
> >  }
> >  
> >  void *eventual(void *arg)
> > @@ -43,8 +43,8 @@ void *eventual(void *arg)
> >  	while (stopflag < 3) {
> >  		sum = 0;
> >  		for_each_thread(t)
> > -			sum += ACCESS_ONCE(per_thread(counter, t));
> > -		ACCESS_ONCE(global_count) = sum;
> > +			sum += READ_ONCE(per_thread(counter, t));
> > +		WRITE_ONCE(global_count, sum);
> >  		poll(NULL, 0, 1);
> >  		if (stopflag) {
> >  			smp_mb();
> > 
>