Re: [PATCH 2/2] Remove unnecessary memory fence

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

On Tue, May 09, 2017 at 11:20:01PM +0800, Junchang Wang wrote:
> Hi Paul,
> 
> Thanks a lot for the correction. Thanks's right; we need to care about
> other achitectures :-)
> 
> So barrier() is basically empty: define barrier() __asm__
> __volatile__("": : :"memory") . I'm curious about what on the earth it
> does? We do really use it in counttorture.h. Any reference to
> resources is welcome!
> 
> Even if we agree with smp_mb(), count_stat_eventual.c seems not 100%
> correct for me. Please check the following patch. It seems to me the
> barrier instruction (smp_mb) in function count_cleanup is to make RAW
> correct. So it should be inserted between the write and read
> instructions. Is that right?

Well, part of your change is in the right direction.  Either stopflag
needs to be volatile or the accesses to stopflag need to use READ_ONCE()
or WRITE_ONCE(), with the latter preferred.

I suggest looking at this Linux Weekly News (LWN) series:

	https://lwn.net/Articles/718628/
	https://lwn.net/Articles/720550/

This LWN article talks about use of ACCESS_ONCE(), which is the
predecessor to READ_ONCE() and WRITE_ONCE():

	http://lwn.net/Articles/508991/

So I would welcome a patch that applied READ_ONCE() and WRITE_ONCE()
to the stopflag variable.  Given that some of this code was written
before ACCESS_ONCE(), let alone READ_ONCE() and WRITE_ONCE(), there
might be more code needing this change, so please feel free to take
a look around.

							Thanx, Paul

> index 75a0ca9..32f7e80 100644
> --- a/CodeSamples/count/count_stat_eventual.c
> +++ b/CodeSamples/count/count_stat_eventual.c
> @@ -67,9 +67,9 @@ void count_init(void)
>  void count_cleanup(void)
>  {
>         stopflag = 1;
> +       smp_mb();
>         while (stopflag < 3)
>                 poll(NULL, 0, 1);
> -       smp_mb();
>  }
> 
>  #include "counttorture.h"
> -- 
> 2.7.4
> 
> 
> Thanks,
> --Jason
> 
> On Tue, May 9, 2017 at 10:47 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Tue, May 09, 2017 at 01:08:31PM +0800, Junchang Wang wrote:
> >> count_stat_eventual.c uses a single global variable (stopflag) to synchronize
> >> two separate threads (main and eventual) probably running on two different CPU
> >> cores. My understanding is that for this model, memory fence instruction
> >> (mfence) which typically are expensive can be avoided, only if we make sure (1)
> >> that compiler neither reorder the code around nor cache the variable, and (2)
> >> that the CPU core running code precedes in program order.
> >>
> >> We use keyword ``volatile'' to prevent compilers from reordering the code and
> >> caching the variable, and instruction barrier() to force CPU core running the
> >> code to obey program order.
> >
> > Please take another look at the barrier() macro.  It does not emit any
> > instructions, so cannot force the CPU to do much of anything.  Keep in
> > mind that this code needs to run on weakly ordered systems, not just x86.
> >
> >                                                         Thanx, Paul
> >
> >> Signed-off-by: Junchang Wang <junchangwang@gmail.com>
> >> ---
> >>  CodeSamples/count/count_stat_eventual.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/CodeSamples/count/count_stat_eventual.c b/CodeSamples/count/count_stat_eventual.c
> >> index 059ab8b..5ffc4aa 100644
> >> --- a/CodeSamples/count/count_stat_eventual.c
> >> +++ b/CodeSamples/count/count_stat_eventual.c
> >> @@ -23,7 +23,7 @@
> >>
> >>  DEFINE_PER_THREAD(unsigned long, counter);
> >>  unsigned long global_count;
> >> -int stopflag;
> >> +volatile int stopflag;
> >>
> >>  void inc_count(void)
> >>  {
> >> @@ -47,7 +47,7 @@ void *eventual(void *arg)
> >>               ACCESS_ONCE(global_count) = sum;
> >>               poll(NULL, 0, 1);
> >>               if (stopflag) {
> >> -                     smp_mb();
> >> +                     barrier();
> >>                       stopflag++;
> >>               }
> >>       }
> >> @@ -67,9 +67,9 @@ void count_init(void)
> >>  void count_cleanup(void)
> >>  {
> >>       stopflag = 1;
> >> +     barrier();
> >>       while (stopflag < 3)
> >>               poll(NULL, 0, 1);
> >> -     smp_mb();
> >>  }
> >>
> >>  #include "counttorture.h"
> >> --
> >> 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
> >>
> >
>