[PATCH 0/2] Use READ_ONCE() and WRITE_ONCE in count_stat_eventual.c

[PATCH 0/2] Use READ_ONCE() and WRITE_ONCE in count_stat_eventual.c

[Junchang Wang]

Hi Paul,

Please check the updated patch. The first patch is to replace existing
ACCESS_ONCE to new READ/WRITE_ONCE primitives, and the second to protect global
share variable stopflag.

BTW, I didn't see any performance differences between versions using volatile
and READ/WRITE_ONCE primitives on my 16 cores Intel machine. I guess the major
reason is that the sample code is simple such that registers are enough even if
keyword volatile is used.


Junchang Wang (2):
  count_stat_eventual: Switch from ACCESS_ONCE() to
    READ_ONCE()/WRITE_ONCE()
  count_stat_eventual: Add READ_ONCE() to protect global shared variable
    stopflag

 CodeSamples/count/count_stat_eventual.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

-- 
2.7.4

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

[Junchang Wang]

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))++;
 }

 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();
-- 
2.7.4

[PATCH 2/2] count_stat_eventual: Add READ_ONCE() to protect global shared variable stopflag

[Junchang Wang]

Signed-off-by: Junchang Wang <junchangwang@gmail.com>
---
 CodeSamples/count/count_stat_eventual.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/CodeSamples/count/count_stat_eventual.c b/CodeSamples/count/count_stat_eventual.c
index cbde4aa..2caebfd 100644
--- a/CodeSamples/count/count_stat_eventual.c
+++ b/CodeSamples/count/count_stat_eventual.c
@@ -40,15 +40,15 @@ void *eventual(void *arg)
 	int t;
 	unsigned long sum;

-	while (stopflag < 3) {
+	while (READ_ONCE(stopflag) < 3) {
 		sum = 0;
 		for_each_thread(t)
 			sum += READ_ONCE(per_thread(counter, t));
 		WRITE_ONCE(global_count, sum);
 		poll(NULL, 0, 1);
-		if (stopflag) {
+		if (READ_ONCE(stopflag)) {
 			smp_mb();
-			stopflag++;
+			WRITE_ONCE(stopflag, READ_ONCE(stopflag) + 1);
 		}
 	}
 	return NULL;
@@ -66,8 +66,9 @@ void count_init(void)

 void count_cleanup(void)
 {
-	stopflag = 1;
-	while (stopflag < 3)
+	WRITE_ONCE(stopflag, 1);
+	smp_mb();
+	while (READ_ONCE(stopflag) < 3)
 		poll(NULL, 0, 1);
 	smp_mb();
 }
-- 
2.7.4

Re: [PATCH 0/2] Use READ_ONCE() and WRITE_ONCE in count_stat_eventual.c

[Paul E. McKenney]

On Thu, May 11, 2017 at 11:03:40PM +0800, Junchang Wang wrote:
> Hi Paul,
> 
> Please check the updated patch. The first patch is to replace existing
> ACCESS_ONCE to new READ/WRITE_ONCE primitives, and the second to protect global
> share variable stopflag.
> 
> BTW, I didn't see any performance differences between versions using volatile
> and READ/WRITE_ONCE primitives on my 16 cores Intel machine. I guess the major
> reason is that the sample code is simple such that registers are enough even if
> keyword volatile is used.

Very nice, sometimes we get lucky.  ;-)

Queue and pushed, thank you!

							Thanx, Paul

> Junchang Wang (2):
>   count_stat_eventual: Switch from ACCESS_ONCE() to
>     READ_ONCE()/WRITE_ONCE()
>   count_stat_eventual: Add READ_ONCE() to protect global shared variable
>     stopflag
> 
>  CodeSamples/count/count_stat_eventual.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe perfbook" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Akira Yokosawa]

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.

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();
> 

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

[Paul E. McKenney]

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();
> > 
> 

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

[Junchang Wang]

On Sat, May 13, 2017 at 8:45 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> 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?

Hi Akira and Paul,

When I was working on the code, my idea was that ACCESS_ONCE() brings
the following benefits:
    1) Clearly underlines the code programmers need to take care.
    2) Prevent compiler from arranging the code.
    3) Remove register/reference hassle.
So, functionally READ_ONCE alone is enough. And it makes the code
'easy' to read :-)

But I agree WRITE_ONCE is logically right here. And it would be better
if the definition in CodeSample is idential to the kernel.


Cheers!
--Jason



>
>
>> 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();
>> >
>>
>

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

[Akira Yokosawa]

On 2017/05/13 05:45:56 -0700, Paul E. McKenney wrote:
> 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?

I assume you meant:

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

I'm afraid this still permits uses such as:

	READ_ONCE(y)++;

Looks like we need a complex definition which resembles that of
include/linux/compiler.h.  Hmm???

And I have another pending question regarding 2/2 of this patch set.
That might result in other addition of line to the code. I think
I can send it tomorrow.

                     Thanks, Akira

> 
> 							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();
>>>
>>
> 
> 

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

[Paul E. McKenney]

On Sat, May 13, 2017 at 11:37:06PM +0900, Akira Yokosawa wrote:
> On 2017/05/13 05:45:56 -0700, Paul E. McKenney wrote:
> > 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?
> 
> I assume you meant:
> 
> 	#define READ_ONCE(x) ({ ACCESS_ONCE(x); })

Indeed!  Good catch!!!

> I'm afraid this still permits uses such as:
> 
> 	READ_ONCE(y)++;
> 
> Looks like we need a complex definition which resembles that of
> include/linux/compiler.h.  Hmm???

OK, how about this?

#define READ_ONCE(x) ({ typeof(x) ___x = ACCESS_ONCE(x); ___x; })

> And I have another pending question regarding 2/2 of this patch set.
> That might result in other addition of line to the code. I think
> I can send it tomorrow.

Looking forward to seeing it!  ;-)

							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();
> >>>
> >>
> > 
> > 
> 

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

[Paul E. McKenney]

On Sat, May 13, 2017 at 09:31:52PM +0800, Junchang Wang wrote:
> On Sat, May 13, 2017 at 8:45 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > 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?
> 
> Hi Akira and Paul,
> 
> When I was working on the code, my idea was that ACCESS_ONCE() brings
> the following benefits:
>     1) Clearly underlines the code programmers need to take care.
>     2) Prevent compiler from arranging the code.
>     3) Remove register/reference hassle.
> So, functionally READ_ONCE alone is enough. And it makes the code
> 'easy' to read :-)
> 
> But I agree WRITE_ONCE is logically right here. And it would be better
> if the definition in CodeSample is idential to the kernel.

Agreed, my initial take on perfbook READ_ONCE() isn't good enough.
I hope we don't need to be as complicated as the kernel, but who knows?

							Thanx, Paul

> Cheers!
> --Jason
> 
> 
> 
> >
> >
> >> 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();
> >> >
> >>
> >
> 

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

[Akira Yokosawa]

On 2017/05/14 7:56, Paul E. McKenney wrote:
> On Sat, May 13, 2017 at 11:37:06PM +0900, Akira Yokosawa wrote:
>> On 2017/05/13 05:45:56 -0700, Paul E. McKenney wrote:
>>> 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?
>>
>> I assume you meant:
>>
>> 	#define READ_ONCE(x) ({ ACCESS_ONCE(x); })
> 
> Indeed!  Good catch!!!
> 
>> I'm afraid this still permits uses such as:
>>
>> 	READ_ONCE(y)++;
>>
>> Looks like we need a complex definition which resembles that of
>> include/linux/compiler.h.  Hmm???
> 
> OK, how about this?
> 
> #define READ_ONCE(x) ({ typeof(x) ___x = ACCESS_ONCE(x); ___x; })

Ah, this resulted in an error as expected!

	main.c: In function ‘inc_test’:
	main.c:11:18: error: lvalue required as increment operand
	     READ_ONCE(*p)++;

Glad to know we can avoid the complexity of the kernel.

                        Thanks, Akira

> 
>> And I have another pending question regarding 2/2 of this patch set.
>> That might result in other addition of line to the code. I think
>> I can send it tomorrow.
> 
> Looking forward to seeing it!  ;-)
> 
> 							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();
>>>>>
>>>>
>>>
>>>
>>
> 
> .
> 

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

[Akira Yokosawa]

On 2017/05/14 9:58, Akira Yokosawa wrote:
> On 2017/05/14 7:56, Paul E. McKenney wrote:
>> On Sat, May 13, 2017 at 11:37:06PM +0900, Akira Yokosawa wrote:
>>> On 2017/05/13 05:45:56 -0700, Paul E. McKenney wrote:
>>>> 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?
>>>
>>> I assume you meant:
>>>
>>> 	#define READ_ONCE(x) ({ ACCESS_ONCE(x); })
>>
>> Indeed!  Good catch!!!
>>
>>> I'm afraid this still permits uses such as:
>>>
>>> 	READ_ONCE(y)++;
>>>
>>> Looks like we need a complex definition which resembles that of
>>> include/linux/compiler.h.  Hmm???
>>
>> OK, how about this?
>>
>> #define READ_ONCE(x) ({ typeof(x) ___x = ACCESS_ONCE(x); ___x; })
> 
> Ah, this resulted in an error as expected!
> 
> 	main.c: In function ‘inc_test’:
> 	main.c:11:18: error: lvalue required as increment operand
> 	     READ_ONCE(*p)++;
>

But the definition above will conflict with the argument "___x":

	y = READ_ONCE(___x);

This won't work as expected.
For CodeSamples, I guess we can safely reserve the name "___x".

Thoughts?

                   Thanks, Akira

> Glad to know we can avoid the complexity of the kernel.
> 
>                         Thanks, Akira
> 
>>
>>> And I have another pending question regarding 2/2 of this patch set.
>>> That might result in other addition of line to the code. I think
>>> I can send it tomorrow.
>>
>> Looking forward to seeing it!  ;-)
>>
>> 							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();
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>> .
>>
> 
> 

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

[Paul E. McKenney]

On Sun, May 14, 2017 at 10:31:51AM +0900, Akira Yokosawa wrote:
> On 2017/05/14 9:58, Akira Yokosawa wrote:
> > On 2017/05/14 7:56, Paul E. McKenney wrote:
> >> On Sat, May 13, 2017 at 11:37:06PM +0900, Akira Yokosawa wrote:
> >>> On 2017/05/13 05:45:56 -0700, Paul E. McKenney wrote:
> >>>> 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?
> >>>
> >>> I assume you meant:
> >>>
> >>> 	#define READ_ONCE(x) ({ ACCESS_ONCE(x); })
> >>
> >> Indeed!  Good catch!!!
> >>
> >>> I'm afraid this still permits uses such as:
> >>>
> >>> 	READ_ONCE(y)++;
> >>>
> >>> Looks like we need a complex definition which resembles that of
> >>> include/linux/compiler.h.  Hmm???
> >>
> >> OK, how about this?
> >>
> >> #define READ_ONCE(x) ({ typeof(x) ___x = ACCESS_ONCE(x); ___x; })
> > 
> > Ah, this resulted in an error as expected!
> > 
> > 	main.c: In function ‘inc_test’:
> > 	main.c:11:18: error: lvalue required as increment operand
> > 	     READ_ONCE(*p)++;
> >
> 
> But the definition above will conflict with the argument "___x":
> 
> 	y = READ_ONCE(___x);
> 
> This won't work as expected.
> For CodeSamples, I guess we can safely reserve the name "___x".
> 
> Thoughts?

It is already reserved.  User programs are not supposed to start
variables with "__".  Which means that the "___x" name is of dubious
legality as far as the C standard is concerned, but so it goes.
If the compiler starts complaining about it, it can be changed.  ;-)

							Thanx, Paul

>                    Thanks, Akira
> 
> > Glad to know we can avoid the complexity of the kernel.
> > 
> >                         Thanks, Akira
> > 
> >>
> >>> And I have another pending question regarding 2/2 of this patch set.
> >>> That might result in other addition of line to the code. I think
> >>> I can send it tomorrow.
> >>
> >> Looking forward to seeing it!  ;-)
> >>
> >> 							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();
> >>>>>>
> >>>>>
> >>>>
> >>>>
> >>>
> >>
> >> .
> >>
> > 
> > 
> 

Re: [PATCH 2/2] count_stat_eventual: Add READ_ONCE() to protect global shared variable stopflag

[Akira Yokosawa]

Hi Jason & Paul,

So, here is a comment regarding 2/2 of this patch set.
I'm aware that this patch only touches slow path, but anyway...

On 2017/05/11 23:03:42 +0800, Junchang Wang wrote:
> Signed-off-by: Junchang Wang <junchangwang@gmail.com>
> ---
>  CodeSamples/count/count_stat_eventual.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/CodeSamples/count/count_stat_eventual.c b/CodeSamples/count/count_stat_eventual.c
> index cbde4aa..2caebfd 100644
> --- a/CodeSamples/count/count_stat_eventual.c
> +++ b/CodeSamples/count/count_stat_eventual.c
> @@ -40,15 +40,15 @@ void *eventual(void *arg)
>  	int t;
>  	unsigned long sum;
>  
> -	while (stopflag < 3) {
> +	while (READ_ONCE(stopflag) < 3) {
>  		sum = 0;
>  		for_each_thread(t)
>  			sum += READ_ONCE(per_thread(counter, t));
>  		WRITE_ONCE(global_count, sum);
>  		poll(NULL, 0, 1);
> -		if (stopflag) {
> +		if (READ_ONCE(stopflag)) {
>  			smp_mb();
> -			stopflag++;
> +			WRITE_ONCE(stopflag, READ_ONCE(stopflag) + 1);

If I'm reading this correctly, the smp_mb() is to ensure the ordering of write
to global_count and write to stopflag.  This "if" branch is taken after
count_cleanup()'s update of stopflag is observed.  After the update,
only the "eventual()" thread updates stopflag. So, the READ_ONCE() within
the WRITE_ONCE() is not necessary.
To make the locality obvious, it might be better to hold the value in an auto
variable.
Here is my version of the entire eventual() function. Note that read of stopflag
is not necessary at the beginning of the while loop.

void *eventual(void *arg)
{
	int t;
	unsigned long sum;
	int stopflag_l = 0;

	while (stopflag_l < 3) {
		sum = 0;
		for_each_thread(t)
			sum += READ_ONCE(per_thread(counter, t));
		WRITE_ONCE(global_count, sum);
		poll(NULL, 0, 1);
		if ((stopflag_l = READ_ONCE(stopflag)) != 0) {
			smp_mb();
			WRITE_ONCE(stopflag, ++stopflag_l);
		}
	}
	return NULL;
}

>  		}
>  	}
>  	return NULL;
> @@ -66,8 +66,9 @@ void count_init(void)
>  
>  void count_cleanup(void)
>  {
> -	stopflag = 1;
> -	while (stopflag < 3)
> +	WRITE_ONCE(stopflag, 1);
> +	smp_mb();

Is this extra smp_mb() really necessary?
We'll loop until stopflag becomes 3 anyway, we can rely on memory coherency
for the updated value to propagate.

> +	while (READ_ONCE(stopflag) < 3)
>  		poll(NULL, 0, 1);
>  	smp_mb();

This smp_mb() pairs with the smp_mb() in eventual() and ensures the value of
global_count to be read in read_count() is the one written preceding the
update of stopflag.

Am I reading right?

                       Thanks, Akira

>  }
> 

Re: [PATCH 2/2] count_stat_eventual: Add READ_ONCE() to protect global shared variable stopflag

[Junchang Wang]

Hi Akira,

Good comments! Please see below:

On Sun, May 14, 2017 at 6:57 PM, Akira Yokosawa <akiyks@gmail.com> wrote:
> Hi Jason & Paul,
>
> So, here is a comment regarding 2/2 of this patch set.
> I'm aware that this patch only touches slow path, but anyway...
>
> On 2017/05/11 23:03:42 +0800, Junchang Wang wrote:
>> Signed-off-by: Junchang Wang <junchangwang@gmail.com>
>> ---
>>  CodeSamples/count/count_stat_eventual.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/CodeSamples/count/count_stat_eventual.c b/CodeSamples/count/count_stat_eventual.c
>> index cbde4aa..2caebfd 100644
>> --- a/CodeSamples/count/count_stat_eventual.c
>> +++ b/CodeSamples/count/count_stat_eventual.c
>> @@ -40,15 +40,15 @@ void *eventual(void *arg)
>>       int t;
>>       unsigned long sum;
>>
>> -     while (stopflag < 3) {
>> +     while (READ_ONCE(stopflag) < 3) {
>>               sum = 0;
>>               for_each_thread(t)
>>                       sum += READ_ONCE(per_thread(counter, t));
>>               WRITE_ONCE(global_count, sum);
>>               poll(NULL, 0, 1);
>> -             if (stopflag) {
>> +             if (READ_ONCE(stopflag)) {
>>                       smp_mb();
>> -                     stopflag++;
>> +                     WRITE_ONCE(stopflag, READ_ONCE(stopflag) + 1);
>
> If I'm reading this correctly, the smp_mb() is to ensure the ordering of write
> to global_count and write to stopflag.

Besides that, smp_mb() is to ensure the correctness of WAR of
stopflag, because stopflag works as a shared variable to synchronize
two threads. I know it's not necessary for x86/gcc, but as Paul has
pointed out, other architectures/Compilers may need it :-).

>This "if" branch is taken after
> count_cleanup()'s update of stopflag is observed.  After the update,
> only the "eventual()" thread updates stopflag. So, the READ_ONCE() within
> the WRITE_ONCE() is not necessary.
> To make the locality obvious, it might be better to hold the value in an auto
> variable.
> Here is my version of the entire eventual() function. Note that read of stopflag
> is not necessary at the beginning of the while loop.
>
> void *eventual(void *arg)
> {
>         int t;
>         unsigned long sum;
>         int stopflag_l = 0;
>
>         while (stopflag_l < 3) {
>                 sum = 0;
>                 for_each_thread(t)
>                         sum += READ_ONCE(per_thread(counter, t));
>                 WRITE_ONCE(global_count, sum);
>                 poll(NULL, 0, 1);
>                 if ((stopflag_l = READ_ONCE(stopflag)) != 0) {
>                         smp_mb();
>                         WRITE_ONCE(stopflag, ++stopflag_l);
>                 }
>         }
>         return NULL;
> }
>

Personally, I don't like to add too many implications in the code such
as excessive optimizations unless we have a strong reason (performance
gain).  Excessive optimizations may confuse compilers and other
programmers. But again, I agree your code might be slightly better in
performance :-).


>>               }
>>       }
>>       return NULL;
>> @@ -66,8 +66,9 @@ void count_init(void)
>>
>>  void count_cleanup(void)
>>  {
>> -     stopflag = 1;
>> -     while (stopflag < 3)
>> +     WRITE_ONCE(stopflag, 1);
>> +     smp_mb();
>
> Is this extra smp_mb() really necessary?
> We'll loop until stopflag becomes 3 anyway, we can rely on memory coherency
> for the updated value to propagate.
>

It's necessary to protect RAW of stopflag on weak consistency machines :-).

Cheers!
--Jason

>> +     while (READ_ONCE(stopflag) < 3)
>>               poll(NULL, 0, 1);
>>       smp_mb();
>
> This smp_mb() pairs with the smp_mb() in eventual() and ensures the value of
> global_count to be read in read_count() is the one written preceding the
> update of stopflag.
>
> Am I reading right?
>
>                        Thanks, Akira
>
>>  }
>>

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

[Junchang Wang]

On Sun, May 14, 2017 at 12:20 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Sun, May 14, 2017 at 10:31:51AM +0900, Akira Yokosawa wrote:
>> On 2017/05/14 9:58, Akira Yokosawa wrote:
>> > On 2017/05/14 7:56, Paul E. McKenney wrote:
>> >> On Sat, May 13, 2017 at 11:37:06PM +0900, Akira Yokosawa wrote:
>> >>> On 2017/05/13 05:45:56 -0700, Paul E. McKenney wrote:
>> >>>> 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?
>> >>>
>> >>> I assume you meant:
>> >>>
>> >>>   #define READ_ONCE(x) ({ ACCESS_ONCE(x); })
>> >>
>> >> Indeed!  Good catch!!!
>> >>
>> >>> I'm afraid this still permits uses such as:
>> >>>
>> >>>   READ_ONCE(y)++;
>> >>>
>> >>> Looks like we need a complex definition which resembles that of
>> >>> include/linux/compiler.h.  Hmm???
>> >>
>> >> OK, how about this?
>> >>
>> >> #define READ_ONCE(x) ({ typeof(x) ___x = ACCESS_ONCE(x); ___x; })
>> >
>> > Ah, this resulted in an error as expected!
>> >
>> >     main.c: In function ‘inc_test’:
>> >     main.c:11:18: error: lvalue required as increment operand
>> >          READ_ONCE(*p)++;
>> >
>>
>> But the definition above will conflict with the argument "___x":
>>
>>       y = READ_ONCE(___x);
>>
>> This won't work as expected.
>> For CodeSamples, I guess we can safely reserve the name "___x".
>>
>> Thoughts?
>
> It is already reserved.  User programs are not supposed to start
> variables with "__".  Which means that the "___x" name is of dubious
> legality as far as the C standard is concerned, but so it goes.
> If the compiler starts complaining about it, it can be changed.  ;-)
>
>                                                         Thanx, Paul
>

Hi Paul and Akira,

It looks good to me :-). I can help look around and update use cases
of READ/WRITE_ONCE accordingly after the definition has been pushed.


Cheers!
--Jason

>>                    Thanks, Akira
>>
>> > Glad to know we can avoid the complexity of the kernel.
>> >
>> >                         Thanks, Akira
>> >
>> >>
>> >>> And I have another pending question regarding 2/2 of this patch set.
>> >>> That might result in other addition of line to the code. I think
>> >>> I can send it tomorrow.
>> >>
>> >> Looking forward to seeing it!  ;-)
>> >>
>> >>                                                    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();
>> >>>>>>
>> >>>>>
>> >>>>
>> >>>>
>> >>>
>> >>
>> >> .
>> >>
>> >
>> >
>>
>

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

[Akira Yokosawa]

On 2017/05/13 21:20:23 -0700, Paul E. McKenney wrote:
> On Sun, May 14, 2017 at 10:31:51AM +0900, Akira Yokosawa wrote:
>> On 2017/05/14 9:58, Akira Yokosawa wrote:
>>> On 2017/05/14 7:56, Paul E. McKenney wrote:
>>>> On Sat, May 13, 2017 at 11:37:06PM +0900, Akira Yokosawa wrote:
>>>>> On 2017/05/13 05:45:56 -0700, Paul E. McKenney wrote:
>>>>>> 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.

So, I measured the performance after this change.
Unfortunately, this change doubles the overhead of updater.

By inspecting the generated code, it looks like GCC (v5.4 for x86_64 with -O3 option)
can't figure out two "__get_thread_var(counter)"s are identical, and uses two
registers as pointers to access it. __get_thread_var() seems complex enough
to obfuscate the compiler.

To avoid this performance regression, we can use a temporary pointer as a hint for
optimization:

	void inc_count(void)
	{
		unsigned long *p_cnt = &__get_thread_var(counter);

		WRITE_ONCE(*p_cnt, *p_cnt + 1);
	}

Another idea is to restore ACCESS_ONCE().
As long as the argument is properly aligned scalar type, ACCESS_ONCE() should be OK.

	ACCESS_ONCE(__get_thread_var(counter))++;

will emit both load and store instructions, but in this short function,
the compiler has no way to optimize away either access even if we don't use
ACCESS_ONCE().

Thoughts?

                      Thanks, Akira

>>>>>>>
>>>>>>> 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?
>>>>>
>>>>> I assume you meant:
>>>>>
>>>>> 	#define READ_ONCE(x) ({ ACCESS_ONCE(x); })
>>>>
>>>> Indeed!  Good catch!!!
>>>>
>>>>> I'm afraid this still permits uses such as:
>>>>>
>>>>> 	READ_ONCE(y)++;
>>>>>
>>>>> Looks like we need a complex definition which resembles that of
>>>>> include/linux/compiler.h.  Hmm???
>>>>
>>>> OK, how about this?
>>>>
>>>> #define READ_ONCE(x) ({ typeof(x) ___x = ACCESS_ONCE(x); ___x; })
>>>
>>> Ah, this resulted in an error as expected!
>>>
>>> 	main.c: In function ‘inc_test’:
>>> 	main.c:11:18: error: lvalue required as increment operand
>>> 	     READ_ONCE(*p)++;
>>>
>>
>> But the definition above will conflict with the argument "___x":
>>
>> 	y = READ_ONCE(___x);
>>
>> This won't work as expected.
>> For CodeSamples, I guess we can safely reserve the name "___x".
>>
>> Thoughts?
> 
> It is already reserved.  User programs are not supposed to start
> variables with "__".  Which means that the "___x" name is of dubious
> legality as far as the C standard is concerned, but so it goes.
> If the compiler starts complaining about it, it can be changed.  ;-)
> 
> 							Thanx, Paul
> 
>>                    Thanks, Akira
>>
>>> Glad to know we can avoid the complexity of the kernel.
>>>
>>>                         Thanks, Akira
>>>
>>>>
>>>>> And I have another pending question regarding 2/2 of this patch set.
>>>>> That might result in other addition of line to the code. I think
>>>>> I can send it tomorrow.
>>>>
>>>> Looking forward to seeing it!  ;-)
>>>>
>>>> 							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();
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>
> 
> 

Re: [PATCH 2/2] count_stat_eventual: Add READ_ONCE() to protect global shared variable stopflag

[Akira Yokosawa]

On 2017/05/14 21:28:56 +0800, Junchang Wang wrote:
> Hi Akira,
> 
> Good comments! Please see below:
> 
> On Sun, May 14, 2017 at 6:57 PM, Akira Yokosawa <akiyks@gmail.com> wrote:
>> Hi Jason & Paul,
>>
>> So, here is a comment regarding 2/2 of this patch set.
>> I'm aware that this patch only touches slow path, but anyway...
>>
>> On 2017/05/11 23:03:42 +0800, Junchang Wang wrote:
>>> Signed-off-by: Junchang Wang <junchangwang@gmail.com>
>>> ---
>>>  CodeSamples/count/count_stat_eventual.c | 11 ++++++-----
>>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/CodeSamples/count/count_stat_eventual.c b/CodeSamples/count/count_stat_eventual.c
>>> index cbde4aa..2caebfd 100644
>>> --- a/CodeSamples/count/count_stat_eventual.c
>>> +++ b/CodeSamples/count/count_stat_eventual.c
>>> @@ -40,15 +40,15 @@ void *eventual(void *arg)
>>>       int t;
>>>       unsigned long sum;
>>>
>>> -     while (stopflag < 3) {
>>> +     while (READ_ONCE(stopflag) < 3) {
>>>               sum = 0;
>>>               for_each_thread(t)
>>>                       sum += READ_ONCE(per_thread(counter, t));
>>>               WRITE_ONCE(global_count, sum);
>>>               poll(NULL, 0, 1);
>>> -             if (stopflag) {
>>> +             if (READ_ONCE(stopflag)) {
>>>                       smp_mb();
>>> -                     stopflag++;
>>> +                     WRITE_ONCE(stopflag, READ_ONCE(stopflag) + 1);
>>
>> If I'm reading this correctly, the smp_mb() is to ensure the ordering of write
>> to global_count and write to stopflag.
> 
> Besides that, smp_mb() is to ensure the correctness of WAR of
> stopflag, because stopflag works as a shared variable to synchronize
> two threads. I know it's not necessary for x86/gcc, but as Paul has
> pointed out, other architectures/Compilers may need it :-).

Well, Section 14.2.9 says:

    3.  A series of stores to a single variable will appear to all CPUs
        to have occurred in a single order, though this order might not
        be predictable from the code, and in fact the order might vary
        from one run to another.

This means that for the value of stopflag, we don't need any memory barrier.

> 
>> This "if" branch is taken after
>> count_cleanup()'s update of stopflag is observed.  After the update,
>> only the "eventual()" thread updates stopflag. So, the READ_ONCE() within
>> the WRITE_ONCE() is not necessary.
>> To make the locality obvious, it might be better to hold the value in an auto
>> variable.
>> Here is my version of the entire eventual() function. Note that read of stopflag
>> is not necessary at the beginning of the while loop.
>>
>> void *eventual(void *arg)
>> {
>>         int t;
>>         unsigned long sum;
>>         int stopflag_l = 0;
>>
>>         while (stopflag_l < 3) {
>>                 sum = 0;
>>                 for_each_thread(t)
>>                         sum += READ_ONCE(per_thread(counter, t));
>>                 WRITE_ONCE(global_count, sum);
>>                 poll(NULL, 0, 1);
>>                 if ((stopflag_l = READ_ONCE(stopflag)) != 0) {
>>                         smp_mb();
>>                         WRITE_ONCE(stopflag, ++stopflag_l);
>>                 }
>>         }
>>         return NULL;
>> }
>>
> 
> Personally, I don't like to add too many implications in the code such
> as excessive optimizations unless we have a strong reason (performance
> gain).  Excessive optimizations may confuse compilers and other
> programmers. But again, I agree your code might be slightly better in
> performance :-)
It might be easier to see the purpose of the memory barrier in my version.
Performance wise, eventual() does not matter much.

> 
>>>               }
>>>       }
>>>       return NULL;
>>> @@ -66,8 +66,9 @@ void count_init(void)
>>>
>>>  void count_cleanup(void)
>>>  {
>>> -     stopflag = 1;
>>> -     while (stopflag < 3)
>>> +     WRITE_ONCE(stopflag, 1);
>>> +     smp_mb();
>>
>> Is this extra smp_mb() really necessary?
>> We'll loop until stopflag becomes 3 anyway, we can rely on memory coherency
>> for the updated value to propagate.
>>
> 
> It's necessary to protect RAW of stopflag on weak consistency machines :-).

Ditto.

> 
> Cheers!
> --Jason
> 
>>> +     while (READ_ONCE(stopflag) < 3)
>>>               poll(NULL, 0, 1);
>>>       smp_mb();
>>
>> This smp_mb() pairs with the smp_mb() in eventual() and ensures the value of
>> global_count to be read in read_count() is the one written preceding the
>> update of stopflag.

This barrier is necessary because we read both global_count and stopflag.
Have I made my point clear enough?

                       Thanks, Akira

>>
>> Am I reading right?
>>
>>                        Thanks, Akira
>>
>>>  }
>>>
> 

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

[Paul E. McKenney]

On Mon, May 15, 2017 at 12:15:34AM +0900, Akira Yokosawa wrote:
> On 2017/05/13 21:20:23 -0700, Paul E. McKenney wrote:
> > On Sun, May 14, 2017 at 10:31:51AM +0900, Akira Yokosawa wrote:
> >> On 2017/05/14 9:58, Akira Yokosawa wrote:
> >>> On 2017/05/14 7:56, Paul E. McKenney wrote:
> >>>> On Sat, May 13, 2017 at 11:37:06PM +0900, Akira Yokosawa wrote:
> >>>>> On 2017/05/13 05:45:56 -0700, Paul E. McKenney wrote:
> >>>>>> 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.
> 
> So, I measured the performance after this change.
> Unfortunately, this change doubles the overhead of updater.
> 
> By inspecting the generated code, it looks like GCC (v5.4 for x86_64 with -O3 option)
> can't figure out two "__get_thread_var(counter)"s are identical, and uses two
> registers as pointers to access it. __get_thread_var() seems complex enough
> to obfuscate the compiler.
> 
> To avoid this performance regression, we can use a temporary pointer as a hint for
> optimization:
> 
> 	void inc_count(void)
> 	{
> 		unsigned long *p_cnt = &__get_thread_var(counter);
> 
> 		WRITE_ONCE(*p_cnt, *p_cnt + 1);
> 	}
> 
> Another idea is to restore ACCESS_ONCE().
> As long as the argument is properly aligned scalar type, ACCESS_ONCE() should be OK.
> 
> 	ACCESS_ONCE(__get_thread_var(counter))++;
> 
> will emit both load and store instructions, but in this short function,
> the compiler has no way to optimize away either access even if we don't use
> ACCESS_ONCE().
> 
> Thoughts?

I am tempted to deprecate ACCESS_ONCE(), but yes, it is hard for the
compiler to optimize.  Unless, that is, you allow the compiler to
inline the function.  Which is the case by default.

Getting back to READ_ONCE(), how does the following patch look?

							Thanx, Paul

------------------------------------------------------------------------

commit 85747990ec809f57d61d30cd27bdc18b58163681
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Sun May 14 17:26:41 2017 -0700

    count: Don't in-place increment a READ_ONCE()
    
    This commit prohibits READ_ONCE(x)++ and fixes the occurrence of this
    in count_state_eventual.c.
    
    Reported-by: Akira Yokosawa <akiyks@gmail.com>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/CodeSamples/api-pthreads/api-pthreads.h b/CodeSamples/api-pthreads/api-pthreads.h
index b12c3e14911c..c6031f7d59b7 100644
--- a/CodeSamples/api-pthreads/api-pthreads.h
+++ b/CodeSamples/api-pthreads/api-pthreads.h
@@ -127,7 +127,7 @@ static __inline__ int spin_is_locked(spinlock_t *sp)
 #define spin_unlock_irqrestore(l, f) do { f = 0; spin_unlock(l); } while (0)

 #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
-#define READ_ONCE(x) ACCESS_ONCE(x)
+#define READ_ONCE(x) ({ typeof(x) ___x = ACCESS_ONCE(x); ___x; })
 #define WRITE_ONCE(x, val) ({ ACCESS_ONCE(x) = (val); })
 #ifndef unlikely
 #define unlikely(x) x
diff --git a/CodeSamples/api.h b/CodeSamples/api.h
index a18c70f2346c..3b4bc1629c7a 100644
--- a/CodeSamples/api.h
+++ b/CodeSamples/api.h
@@ -243,7 +243,7 @@ static __inline__ int spin_is_locked(spinlock_t *sp)
 #define spin_unlock_irqrestore(l, f) do { f = 0; spin_unlock(l); } while (0)

 #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
-#define READ_ONCE(x) ACCESS_ONCE(x)
+#define READ_ONCE(x) ({ typeof(x) ___x = ACCESS_ONCE(x); ___x; })
 #define WRITE_ONCE(x, val) ({ ACCESS_ONCE(x) = (val); })
 #ifndef unlikely
 #define unlikely(x) x
diff --git a/CodeSamples/count/count_stat_eventual.c b/CodeSamples/count/count_stat_eventual.c
index 2caebfd1fb26..464de30d7aa8 100644
--- a/CodeSamples/count/count_stat_eventual.c
+++ b/CodeSamples/count/count_stat_eventual.c
@@ -27,7 +27,8 @@ int stopflag;

 void inc_count(void)
 {
-	READ_ONCE(__get_thread_var(counter))++;
+	WRITE_ONCE(__get_thread_var(counter),
+		   READ_ONCE(__get_thread_var(counter)) + 1);
 }

 unsigned long read_count(void)
diff --git a/count/count.tex b/count/count.tex
index 096b53d1d2bb..ae624c650c11 100644
--- a/count/count.tex
+++ b/count/count.tex
@@ -746,56 +746,58 @@ eventually consistent.
 \begin{figure}[tbp]
 { \scriptsize
 \begin{verbbox}
-  1 DEFINE_PER_THREAD(unsigned long, counter);
-  2 unsigned long global_count;
-  3 int stopflag;
-  4 
-  5 void inc_count(void)
-  6 {
-  7   ACCESS_ONCE(__get_thread_var(counter))++;
-  8 }
-  9 
- 10 unsigned long read_count(void)
- 11 {
- 12   return ACCESS_ONCE(global_count);
- 13 }
- 14 
- 15 void *eventual(void *arg)
- 16 {
- 17   int t;
- 18   int sum;
- 19 
- 20   while (stopflag < 3) {
- 21     sum = 0;
- 22     for_each_thread(t)
- 23       sum += ACCESS_ONCE(per_thread(counter, t));
- 24     ACCESS_ONCE(global_count) = sum;
- 25     poll(NULL, 0, 1);
- 26     if (stopflag) {
- 27       smp_mb();
- 28       stopflag++;
- 29     }
- 30   }
- 31   return NULL;
- 32 }
- 33 
- 34 void count_init(void)
- 35 {
- 36   thread_id_t tid;
- 37 
- 38   if (pthread_create(&tid, NULL, eventual, NULL)) {
- 39     perror("count_init:pthread_create");
- 40     exit(-1);
- 41   }
- 42 }
- 43 
- 44 void count_cleanup(void)
- 45 {
- 46   stopflag = 1;
- 47   while (stopflag < 3)
- 48     poll(NULL, 0, 1);
- 49   smp_mb();
- 50 }
+ 1 DEFINE_PER_THREAD(unsigned long, counter);
+ 2 unsigned long global_count;
+ 3 int stopflag;
+ 4
+ 5 void inc_count(void)
+ 6 {
+ 7   WRITE_ONCE(__get_thread_var(counter),
+ 8              READ_ONCE(__get_thread_var(counter)) + 1);
+ 9 }
+10
+11 unsigned long read_count(void)
+12 {
+13   return READ_ONCE(global_count);
+14 }
+15
+16 void *eventual(void *arg)
+17 {
+18   int t;
+19   unsigned long sum;
+20
+21   while (READ_ONCE(stopflag) < 3) {
+22     sum = 0;
+23     for_each_thread(t)
+24       sum += READ_ONCE(per_thread(counter, t));
+25     WRITE_ONCE(global_count, sum);
+26     poll(NULL, 0, 1);
+27     if (READ_ONCE(stopflag)) {
+28       smp_mb();
+29       WRITE_ONCE(stopflag, READ_ONCE(stopflag) + 1);
+30     }
+31   }
+32   return NULL;
+33 }
+34
+35 void count_init(void)
+36 {
+37   thread_id_t tid;
+38
+39   if (pthread_create(&tid, NULL, eventual, NULL) != 0) {
+40     perror("count_init:pthread_create");
+41     exit(-1);
+42   }
+43 }
+44
+45 void count_cleanup(void)
+46 {
+47   WRITE_ONCE(stopflag, 1);
+48   smp_mb();
+49   while (READ_ONCE(stopflag) < 3)
+50     poll(NULL, 0, 1);
+51   smp_mb();
+52 }
 \end{verbbox}
 }
 \centering
@@ -811,20 +813,20 @@ Lines~1-2 show the per-thread variable and the global variable that
 track the counter's value, and line three shows \co{stopflag}
 which is used to coordinate termination (for the case where we want
 to terminate the program with an accurate counter value).
-The \co{inc_count()} function shown on lines~5-8 is similar to its
+The \co{inc_count()} function shown on lines~5-9 is similar to its
 counterpart in
 Figure~\ref{fig:count:Array-Based Per-Thread Statistical Counters}.
-The \co{read_count()} function shown on lines~10-13 simply returns the
+The \co{read_count()} function shown on lines~11-14 simply returns the
 value of the \co{global_count} variable.

-However, the \co{count_init()} function on lines~34-42
-creates the \co{eventual()} thread shown on lines~15-32, which
+However, the \co{count_init()} function on lines~35-43
+creates the \co{eventual()} thread shown on lines~16-33, which
 cycles through all the threads,
 summing the per-thread local \co{counter} and storing the
 sum to the \co{global_count} variable.
 The \co{eventual()} thread waits an arbitrarily chosen one millisecond
 between passes.
-The \co{count_cleanup()} function on lines~44-50 coordinates termination.
+The \co{count_cleanup()} function on lines~45-52 coordinates termination.

 This approach gives extremely fast counter read-out while still
 supporting linear counter-update performance.

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

[Akira Yokosawa]

On 2017/05/14 17:31:04 -0700, Paul E. McKenney wrote:
> On Mon, May 15, 2017 at 12:15:34AM +0900, Akira Yokosawa wrote:
>> On 2017/05/13 21:20:23 -0700, Paul E. McKenney wrote:
>>> On Sun, May 14, 2017 at 10:31:51AM +0900, Akira Yokosawa wrote:
>>>> On 2017/05/14 9:58, Akira Yokosawa wrote:
>>>>> On 2017/05/14 7:56, Paul E. McKenney wrote:
>>>>>> On Sat, May 13, 2017 at 11:37:06PM +0900, Akira Yokosawa wrote:
>>>>>>> On 2017/05/13 05:45:56 -0700, Paul E. McKenney wrote:
>>>>>>>> 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.
>>
>> So, I measured the performance after this change.
>> Unfortunately, this change doubles the overhead of updater.
>>
>> By inspecting the generated code, it looks like GCC (v5.4 for x86_64 with -O3 option)
>> can't figure out two "__get_thread_var(counter)"s are identical, and uses two
>> registers as pointers to access it. __get_thread_var() seems complex enough
>> to obfuscate the compiler.
>>
>> To avoid this performance regression, we can use a temporary pointer as a hint for
>> optimization:
>>
>> 	void inc_count(void)
>> 	{
>> 		unsigned long *p_cnt = &__get_thread_var(counter);
>>
>> 		WRITE_ONCE(*p_cnt, *p_cnt + 1);
>> 	}
>>
>> Another idea is to restore ACCESS_ONCE().
>> As long as the argument is properly aligned scalar type, ACCESS_ONCE() should be OK.
>>
>> 	ACCESS_ONCE(__get_thread_var(counter))++;
>>
>> will emit both load and store instructions, but in this short function,
>> the compiler has no way to optimize away either access even if we don't use
>> ACCESS_ONCE().
>>
>> Thoughts?
> 
> I am tempted to deprecate ACCESS_ONCE(), but yes, it is hard for the
> compiler to optimize.  Unless, that is, you allow the compiler to
> inline the function.  Which is the case by default.
> 
> Getting back to READ_ONCE(), how does the following patch look?

So, you've already pushed it.

As for the definition of READ_ONCE(), it looks good.

You might also want to update the definition of READ_ONCE() at the end of
Section 4.2.5.

As for the code snippet, I'm anticipating your take on the discussion
on Patch 2/2 in another thread.

Optimization of inc_count() can wait for the time being.

                 Thanks, Akira

> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit 85747990ec809f57d61d30cd27bdc18b58163681
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Date:   Sun May 14 17:26:41 2017 -0700
> 
>     count: Don't in-place increment a READ_ONCE()
>     
>     This commit prohibits READ_ONCE(x)++ and fixes the occurrence of this
>     in count_state_eventual.c.
>     
>     Reported-by: Akira Yokosawa <akiyks@gmail.com>
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> diff --git a/CodeSamples/api-pthreads/api-pthreads.h b/CodeSamples/api-pthreads/api-pthreads.h
> index b12c3e14911c..c6031f7d59b7 100644
> --- a/CodeSamples/api-pthreads/api-pthreads.h
> +++ b/CodeSamples/api-pthreads/api-pthreads.h
> @@ -127,7 +127,7 @@ static __inline__ int spin_is_locked(spinlock_t *sp)
>  #define spin_unlock_irqrestore(l, f) do { f = 0; spin_unlock(l); } while (0)
>  
>  #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> -#define READ_ONCE(x) ACCESS_ONCE(x)
> +#define READ_ONCE(x) ({ typeof(x) ___x = ACCESS_ONCE(x); ___x; })
>  #define WRITE_ONCE(x, val) ({ ACCESS_ONCE(x) = (val); })
>  #ifndef unlikely
>  #define unlikely(x) x
> diff --git a/CodeSamples/api.h b/CodeSamples/api.h
> index a18c70f2346c..3b4bc1629c7a 100644
> --- a/CodeSamples/api.h
> +++ b/CodeSamples/api.h
> @@ -243,7 +243,7 @@ static __inline__ int spin_is_locked(spinlock_t *sp)
>  #define spin_unlock_irqrestore(l, f) do { f = 0; spin_unlock(l); } while (0)
>  
>  #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> -#define READ_ONCE(x) ACCESS_ONCE(x)
> +#define READ_ONCE(x) ({ typeof(x) ___x = ACCESS_ONCE(x); ___x; })
>  #define WRITE_ONCE(x, val) ({ ACCESS_ONCE(x) = (val); })
>  #ifndef unlikely
>  #define unlikely(x) x
> diff --git a/CodeSamples/count/count_stat_eventual.c b/CodeSamples/count/count_stat_eventual.c
> index 2caebfd1fb26..464de30d7aa8 100644
> --- a/CodeSamples/count/count_stat_eventual.c
> +++ b/CodeSamples/count/count_stat_eventual.c
> @@ -27,7 +27,8 @@ int stopflag;
>  
>  void inc_count(void)
>  {
> -	READ_ONCE(__get_thread_var(counter))++;
> +	WRITE_ONCE(__get_thread_var(counter),
> +		   READ_ONCE(__get_thread_var(counter)) + 1);
>  }
>  
>  unsigned long read_count(void)
> diff --git a/count/count.tex b/count/count.tex
> index 096b53d1d2bb..ae624c650c11 100644
> --- a/count/count.tex
> +++ b/count/count.tex
> @@ -746,56 +746,58 @@ eventually consistent.
>  \begin{figure}[tbp]
>  { \scriptsize
>  \begin{verbbox}
> -  1 DEFINE_PER_THREAD(unsigned long, counter);
> -  2 unsigned long global_count;
> -  3 int stopflag;
> -  4 
> -  5 void inc_count(void)
> -  6 {
> -  7   ACCESS_ONCE(__get_thread_var(counter))++;
> -  8 }
> -  9 
> - 10 unsigned long read_count(void)
> - 11 {
> - 12   return ACCESS_ONCE(global_count);
> - 13 }
> - 14 
> - 15 void *eventual(void *arg)
> - 16 {
> - 17   int t;
> - 18   int sum;
> - 19 
> - 20   while (stopflag < 3) {
> - 21     sum = 0;
> - 22     for_each_thread(t)
> - 23       sum += ACCESS_ONCE(per_thread(counter, t));
> - 24     ACCESS_ONCE(global_count) = sum;
> - 25     poll(NULL, 0, 1);
> - 26     if (stopflag) {
> - 27       smp_mb();
> - 28       stopflag++;
> - 29     }
> - 30   }
> - 31   return NULL;
> - 32 }
> - 33 
> - 34 void count_init(void)
> - 35 {
> - 36   thread_id_t tid;
> - 37 
> - 38   if (pthread_create(&tid, NULL, eventual, NULL)) {
> - 39     perror("count_init:pthread_create");
> - 40     exit(-1);
> - 41   }
> - 42 }
> - 43 
> - 44 void count_cleanup(void)
> - 45 {
> - 46   stopflag = 1;
> - 47   while (stopflag < 3)
> - 48     poll(NULL, 0, 1);
> - 49   smp_mb();
> - 50 }
> + 1 DEFINE_PER_THREAD(unsigned long, counter);
> + 2 unsigned long global_count;
> + 3 int stopflag;
> + 4
> + 5 void inc_count(void)
> + 6 {
> + 7   WRITE_ONCE(__get_thread_var(counter),
> + 8              READ_ONCE(__get_thread_var(counter)) + 1);
> + 9 }
> +10
> +11 unsigned long read_count(void)
> +12 {
> +13   return READ_ONCE(global_count);
> +14 }
> +15
> +16 void *eventual(void *arg)
> +17 {
> +18   int t;
> +19   unsigned long sum;
> +20
> +21   while (READ_ONCE(stopflag) < 3) {
> +22     sum = 0;
> +23     for_each_thread(t)
> +24       sum += READ_ONCE(per_thread(counter, t));
> +25     WRITE_ONCE(global_count, sum);
> +26     poll(NULL, 0, 1);
> +27     if (READ_ONCE(stopflag)) {
> +28       smp_mb();
> +29       WRITE_ONCE(stopflag, READ_ONCE(stopflag) + 1);
> +30     }
> +31   }
> +32   return NULL;
> +33 }
> +34
> +35 void count_init(void)
> +36 {
> +37   thread_id_t tid;
> +38
> +39   if (pthread_create(&tid, NULL, eventual, NULL) != 0) {
> +40     perror("count_init:pthread_create");
> +41     exit(-1);
> +42   }
> +43 }
> +44
> +45 void count_cleanup(void)
> +46 {
> +47   WRITE_ONCE(stopflag, 1);
> +48   smp_mb();
> +49   while (READ_ONCE(stopflag) < 3)
> +50     poll(NULL, 0, 1);
> +51   smp_mb();
> +52 }
>  \end{verbbox}
>  }
>  \centering
> @@ -811,20 +813,20 @@ Lines~1-2 show the per-thread variable and the global variable that
>  track the counter's value, and line three shows \co{stopflag}
>  which is used to coordinate termination (for the case where we want
>  to terminate the program with an accurate counter value).
> -The \co{inc_count()} function shown on lines~5-8 is similar to its
> +The \co{inc_count()} function shown on lines~5-9 is similar to its
>  counterpart in
>  Figure~\ref{fig:count:Array-Based Per-Thread Statistical Counters}.
> -The \co{read_count()} function shown on lines~10-13 simply returns the
> +The \co{read_count()} function shown on lines~11-14 simply returns the
>  value of the \co{global_count} variable.
>  
> -However, the \co{count_init()} function on lines~34-42
> -creates the \co{eventual()} thread shown on lines~15-32, which
> +However, the \co{count_init()} function on lines~35-43
> +creates the \co{eventual()} thread shown on lines~16-33, which
>  cycles through all the threads,
>  summing the per-thread local \co{counter} and storing the
>  sum to the \co{global_count} variable.
>  The \co{eventual()} thread waits an arbitrarily chosen one millisecond
>  between passes.
> -The \co{count_cleanup()} function on lines~44-50 coordinates termination.
> +The \co{count_cleanup()} function on lines~45-52 coordinates termination.
>  
>  This approach gives extremely fast counter read-out while still
>  supporting linear counter-update performance.
> 
> 

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

[Paul E. McKenney]

On Mon, May 15, 2017 at 11:35:51PM +0900, Akira Yokosawa wrote:
> On 2017/05/14 17:31:04 -0700, Paul E. McKenney wrote:
> > On Mon, May 15, 2017 at 12:15:34AM +0900, Akira Yokosawa wrote:
> >> On 2017/05/13 21:20:23 -0700, Paul E. McKenney wrote:
> >>> On Sun, May 14, 2017 at 10:31:51AM +0900, Akira Yokosawa wrote:
> >>>> On 2017/05/14 9:58, Akira Yokosawa wrote:
> >>>>> On 2017/05/14 7:56, Paul E. McKenney wrote:
> >>>>>> On Sat, May 13, 2017 at 11:37:06PM +0900, Akira Yokosawa wrote:
> >>>>>>> On 2017/05/13 05:45:56 -0700, Paul E. McKenney wrote:
> >>>>>>>> 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.
> >>
> >> So, I measured the performance after this change.
> >> Unfortunately, this change doubles the overhead of updater.
> >>
> >> By inspecting the generated code, it looks like GCC (v5.4 for x86_64 with -O3 option)
> >> can't figure out two "__get_thread_var(counter)"s are identical, and uses two
> >> registers as pointers to access it. __get_thread_var() seems complex enough
> >> to obfuscate the compiler.
> >>
> >> To avoid this performance regression, we can use a temporary pointer as a hint for
> >> optimization:
> >>
> >> 	void inc_count(void)
> >> 	{
> >> 		unsigned long *p_cnt = &__get_thread_var(counter);
> >>
> >> 		WRITE_ONCE(*p_cnt, *p_cnt + 1);
> >> 	}
> >>
> >> Another idea is to restore ACCESS_ONCE().
> >> As long as the argument is properly aligned scalar type, ACCESS_ONCE() should be OK.
> >>
> >> 	ACCESS_ONCE(__get_thread_var(counter))++;
> >>
> >> will emit both load and store instructions, but in this short function,
> >> the compiler has no way to optimize away either access even if we don't use
> >> ACCESS_ONCE().
> >>
> >> Thoughts?
> > 
> > I am tempted to deprecate ACCESS_ONCE(), but yes, it is hard for the
> > compiler to optimize.  Unless, that is, you allow the compiler to
> > inline the function.  Which is the case by default.
> > 
> > Getting back to READ_ONCE(), how does the following patch look?
> 
> So, you've already pushed it.

Good point -- I should have held it back for review.  One of those days.

Still, changes can be made as needed.

> As for the definition of READ_ONCE(), it looks good.
> 
> You might also want to update the definition of READ_ONCE() at the end of
> Section 4.2.5.
> 
> As for the code snippet, I'm anticipating your take on the discussion
> on Patch 2/2 in another thread.

Will do!

							Thanx, Paul

> Optimization of inc_count() can wait for the time being.
> 
>                  Thanks, Akira
> 
> > 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > commit 85747990ec809f57d61d30cd27bdc18b58163681
> > Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Date:   Sun May 14 17:26:41 2017 -0700
> > 
> >     count: Don't in-place increment a READ_ONCE()
> >     
> >     This commit prohibits READ_ONCE(x)++ and fixes the occurrence of this
> >     in count_state_eventual.c.
> >     
> >     Reported-by: Akira Yokosawa <akiyks@gmail.com>
> >     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> > diff --git a/CodeSamples/api-pthreads/api-pthreads.h b/CodeSamples/api-pthreads/api-pthreads.h
> > index b12c3e14911c..c6031f7d59b7 100644
> > --- a/CodeSamples/api-pthreads/api-pthreads.h
> > +++ b/CodeSamples/api-pthreads/api-pthreads.h
> > @@ -127,7 +127,7 @@ static __inline__ int spin_is_locked(spinlock_t *sp)
> >  #define spin_unlock_irqrestore(l, f) do { f = 0; spin_unlock(l); } while (0)
> >  
> >  #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> > -#define READ_ONCE(x) ACCESS_ONCE(x)
> > +#define READ_ONCE(x) ({ typeof(x) ___x = ACCESS_ONCE(x); ___x; })
> >  #define WRITE_ONCE(x, val) ({ ACCESS_ONCE(x) = (val); })
> >  #ifndef unlikely
> >  #define unlikely(x) x
> > diff --git a/CodeSamples/api.h b/CodeSamples/api.h
> > index a18c70f2346c..3b4bc1629c7a 100644
> > --- a/CodeSamples/api.h
> > +++ b/CodeSamples/api.h
> > @@ -243,7 +243,7 @@ static __inline__ int spin_is_locked(spinlock_t *sp)
> >  #define spin_unlock_irqrestore(l, f) do { f = 0; spin_unlock(l); } while (0)
> >  
> >  #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> > -#define READ_ONCE(x) ACCESS_ONCE(x)
> > +#define READ_ONCE(x) ({ typeof(x) ___x = ACCESS_ONCE(x); ___x; })
> >  #define WRITE_ONCE(x, val) ({ ACCESS_ONCE(x) = (val); })
> >  #ifndef unlikely
> >  #define unlikely(x) x
> > diff --git a/CodeSamples/count/count_stat_eventual.c b/CodeSamples/count/count_stat_eventual.c
> > index 2caebfd1fb26..464de30d7aa8 100644
> > --- a/CodeSamples/count/count_stat_eventual.c
> > +++ b/CodeSamples/count/count_stat_eventual.c
> > @@ -27,7 +27,8 @@ int stopflag;
> >  
> >  void inc_count(void)
> >  {
> > -	READ_ONCE(__get_thread_var(counter))++;
> > +	WRITE_ONCE(__get_thread_var(counter),
> > +		   READ_ONCE(__get_thread_var(counter)) + 1);
> >  }
> >  
> >  unsigned long read_count(void)
> > diff --git a/count/count.tex b/count/count.tex
> > index 096b53d1d2bb..ae624c650c11 100644
> > --- a/count/count.tex
> > +++ b/count/count.tex
> > @@ -746,56 +746,58 @@ eventually consistent.
> >  \begin{figure}[tbp]
> >  { \scriptsize
> >  \begin{verbbox}
> > -  1 DEFINE_PER_THREAD(unsigned long, counter);
> > -  2 unsigned long global_count;
> > -  3 int stopflag;
> > -  4 
> > -  5 void inc_count(void)
> > -  6 {
> > -  7   ACCESS_ONCE(__get_thread_var(counter))++;
> > -  8 }
> > -  9 
> > - 10 unsigned long read_count(void)
> > - 11 {
> > - 12   return ACCESS_ONCE(global_count);
> > - 13 }
> > - 14 
> > - 15 void *eventual(void *arg)
> > - 16 {
> > - 17   int t;
> > - 18   int sum;
> > - 19 
> > - 20   while (stopflag < 3) {
> > - 21     sum = 0;
> > - 22     for_each_thread(t)
> > - 23       sum += ACCESS_ONCE(per_thread(counter, t));
> > - 24     ACCESS_ONCE(global_count) = sum;
> > - 25     poll(NULL, 0, 1);
> > - 26     if (stopflag) {
> > - 27       smp_mb();
> > - 28       stopflag++;
> > - 29     }
> > - 30   }
> > - 31   return NULL;
> > - 32 }
> > - 33 
> > - 34 void count_init(void)
> > - 35 {
> > - 36   thread_id_t tid;
> > - 37 
> > - 38   if (pthread_create(&tid, NULL, eventual, NULL)) {
> > - 39     perror("count_init:pthread_create");
> > - 40     exit(-1);
> > - 41   }
> > - 42 }
> > - 43 
> > - 44 void count_cleanup(void)
> > - 45 {
> > - 46   stopflag = 1;
> > - 47   while (stopflag < 3)
> > - 48     poll(NULL, 0, 1);
> > - 49   smp_mb();
> > - 50 }
> > + 1 DEFINE_PER_THREAD(unsigned long, counter);
> > + 2 unsigned long global_count;
> > + 3 int stopflag;
> > + 4
> > + 5 void inc_count(void)
> > + 6 {
> > + 7   WRITE_ONCE(__get_thread_var(counter),
> > + 8              READ_ONCE(__get_thread_var(counter)) + 1);
> > + 9 }
> > +10
> > +11 unsigned long read_count(void)
> > +12 {
> > +13   return READ_ONCE(global_count);
> > +14 }
> > +15
> > +16 void *eventual(void *arg)
> > +17 {
> > +18   int t;
> > +19   unsigned long sum;
> > +20
> > +21   while (READ_ONCE(stopflag) < 3) {
> > +22     sum = 0;
> > +23     for_each_thread(t)
> > +24       sum += READ_ONCE(per_thread(counter, t));
> > +25     WRITE_ONCE(global_count, sum);
> > +26     poll(NULL, 0, 1);
> > +27     if (READ_ONCE(stopflag)) {
> > +28       smp_mb();
> > +29       WRITE_ONCE(stopflag, READ_ONCE(stopflag) + 1);
> > +30     }
> > +31   }
> > +32   return NULL;
> > +33 }
> > +34
> > +35 void count_init(void)
> > +36 {
> > +37   thread_id_t tid;
> > +38
> > +39   if (pthread_create(&tid, NULL, eventual, NULL) != 0) {
> > +40     perror("count_init:pthread_create");
> > +41     exit(-1);
> > +42   }
> > +43 }
> > +44
> > +45 void count_cleanup(void)
> > +46 {
> > +47   WRITE_ONCE(stopflag, 1);
> > +48   smp_mb();
> > +49   while (READ_ONCE(stopflag) < 3)
> > +50     poll(NULL, 0, 1);
> > +51   smp_mb();
> > +52 }
> >  \end{verbbox}
> >  }
> >  \centering
> > @@ -811,20 +813,20 @@ Lines~1-2 show the per-thread variable and the global variable that
> >  track the counter's value, and line three shows \co{stopflag}
> >  which is used to coordinate termination (for the case where we want
> >  to terminate the program with an accurate counter value).
> > -The \co{inc_count()} function shown on lines~5-8 is similar to its
> > +The \co{inc_count()} function shown on lines~5-9 is similar to its
> >  counterpart in
> >  Figure~\ref{fig:count:Array-Based Per-Thread Statistical Counters}.
> > -The \co{read_count()} function shown on lines~10-13 simply returns the
> > +The \co{read_count()} function shown on lines~11-14 simply returns the
> >  value of the \co{global_count} variable.
> >  
> > -However, the \co{count_init()} function on lines~34-42
> > -creates the \co{eventual()} thread shown on lines~15-32, which
> > +However, the \co{count_init()} function on lines~35-43
> > +creates the \co{eventual()} thread shown on lines~16-33, which
> >  cycles through all the threads,
> >  summing the per-thread local \co{counter} and storing the
> >  sum to the \co{global_count} variable.
> >  The \co{eventual()} thread waits an arbitrarily chosen one millisecond
> >  between passes.
> > -The \co{count_cleanup()} function on lines~44-50 coordinates termination.
> > +The \co{count_cleanup()} function on lines~45-52 coordinates termination.
> >  
> >  This approach gives extremely fast counter read-out while still
> >  supporting linear counter-update performance.
> > 
> > 
> 

Re: [PATCH 2/2] count_stat_eventual: Add READ_ONCE() to protect global shared variable stopflag

[Junchang Wang]

Hi Akira,

Thanks a lot for the comments. Please see below:

On Mon, May 15, 2017 at 6:57 AM, Akira Yokosawa <akiyks@gmail.com> wrote:
> On 2017/05/14 21:28:56 +0800, Junchang Wang wrote:
>> Hi Akira,
>>
>> Good comments! Please see below:
>>
>> On Sun, May 14, 2017 at 6:57 PM, Akira Yokosawa <akiyks@gmail.com> wrote:
>>> Hi Jason & Paul,
>>>
>>> So, here is a comment regarding 2/2 of this patch set.
>>> I'm aware that this patch only touches slow path, but anyway...
>>>
>>> On 2017/05/11 23:03:42 +0800, Junchang Wang wrote:
>>>> Signed-off-by: Junchang Wang <junchangwang@gmail.com>
>>>> ---
>>>>  CodeSamples/count/count_stat_eventual.c | 11 ++++++-----
>>>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/CodeSamples/count/count_stat_eventual.c b/CodeSamples/count/count_stat_eventual.c
>>>> index cbde4aa..2caebfd 100644
>>>> --- a/CodeSamples/count/count_stat_eventual.c
>>>> +++ b/CodeSamples/count/count_stat_eventual.c
>>>> @@ -40,15 +40,15 @@ void *eventual(void *arg)
>>>>       int t;
>>>>       unsigned long sum;
>>>>
>>>> -     while (stopflag < 3) {
>>>> +     while (READ_ONCE(stopflag) < 3) {
>>>>               sum = 0;
>>>>               for_each_thread(t)
>>>>                       sum += READ_ONCE(per_thread(counter, t));
>>>>               WRITE_ONCE(global_count, sum);
>>>>               poll(NULL, 0, 1);
>>>> -             if (stopflag) {
>>>> +             if (READ_ONCE(stopflag)) {
>>>>                       smp_mb();
>>>> -                     stopflag++;
>>>> +                     WRITE_ONCE(stopflag, READ_ONCE(stopflag) + 1);
>>>
>>> If I'm reading this correctly, the smp_mb() is to ensure the ordering of write
>>> to global_count and write to stopflag.
>>
>> Besides that, smp_mb() is to ensure the correctness of WAR of
>> stopflag, because stopflag works as a shared variable to synchronize
>> two threads. I know it's not necessary for x86/gcc, but as Paul has
>> pointed out, other architectures/Compilers may need it :-).
>
> Well, Section 14.2.9 says:
>
>     3.  A series of stores to a single variable will appear to all CPUs
>         to have occurred in a single order, though this order might not
>         be predictable from the code, and in fact the order might vary
>         from one run to another.
>
> This means that for the value of stopflag, we don't need any memory barrier.
>

You are right. I Need another cup of tea :-)

>>
>>> This "if" branch is taken after
>>> count_cleanup()'s update of stopflag is observed.  After the update,
>>> only the "eventual()" thread updates stopflag. So, the READ_ONCE() within
>>> the WRITE_ONCE() is not necessary.
>>> To make the locality obvious, it might be better to hold the value in an auto
>>> variable.
>>> Here is my version of the entire eventual() function. Note that read of stopflag
>>> is not necessary at the beginning of the while loop.
>>>
>>> void *eventual(void *arg)
>>> {
>>>         int t;
>>>         unsigned long sum;
>>>         int stopflag_l = 0;
>>>
>>>         while (stopflag_l < 3) {
>>>                 sum = 0;
>>>                 for_each_thread(t)
>>>                         sum += READ_ONCE(per_thread(counter, t));
>>>                 WRITE_ONCE(global_count, sum);
>>>                 poll(NULL, 0, 1);
>>>                 if ((stopflag_l = READ_ONCE(stopflag)) != 0) {
>>>                         smp_mb();
>>>                         WRITE_ONCE(stopflag, ++stopflag_l);
>>>                 }
>>>         }
>>>         return NULL;
>>> }
>>>
>>
>> Personally, I don't like to add too many implications in the code such
>> as excessive optimizations unless we have a strong reason (performance
>> gain).  Excessive optimizations may confuse compilers and other
>> programmers. But again, I agree your code might be slightly better in
>> performance :-)
> It might be easier to see the purpose of the memory barrier in my version.
> Performance wise, eventual() does not matter much.
>

Sounds reasonable. Your version is easier to get the idea of why
READ_ONCE is here.

>>
>>>>               }
>>>>       }
>>>>       return NULL;
>>>> @@ -66,8 +66,9 @@ void count_init(void)
>>>>
>>>>  void count_cleanup(void)
>>>>  {
>>>> -     stopflag = 1;
>>>> -     while (stopflag < 3)
>>>> +     WRITE_ONCE(stopflag, 1);
>>>> +     smp_mb();
>>>
>>> Is this extra smp_mb() really necessary?
>>> We'll loop until stopflag becomes 3 anyway, we can rely on memory coherency
>>> for the updated value to propagate.
>>>
>>
>> It's necessary to protect RAW of stopflag on weak consistency machines :-).
>
> Ditto.
>

Agree after search. Will submit a new patch to remove the unnecessary
smp_mb() soon. Thanks a lot!

>>
>> Cheers!
>> --Jason
>>
>>>> +     while (READ_ONCE(stopflag) < 3)
>>>>               poll(NULL, 0, 1);
>>>>       smp_mb();
>>>
>>> This smp_mb() pairs with the smp_mb() in eventual() and ensures the value of
>>> global_count to be read in read_count() is the one written preceding the
>>> update of stopflag.
>
> This barrier is necessary because we read both global_count and stopflag.
> Have I made my point clear enough?
>

Agree!

Thanks,
--Jason

>                        Thanks, Akira
>
>>>
>>> Am I reading right?
>>>
>>>                        Thanks, Akira
>>>
>>>>  }
>>>>
>>
>