Hi Paul,
Thanks a LOT for the references. They are pretty much helpful :-). I
need a deeper look into it (and of course think about it) and will
submit new patches by tomorrow.
Thanks,
--Jason
On Tue, May 9, 2017 at 11:40 PM, Paul E. McKenney
<paulmck@xxxxxxxxxxxxxxxxxx> wrote:
> 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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>> >> ---
>> >> 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@xxxxxxxxxxxxxxx
>> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> >>
>> >
>>
>
--
To unsubscribe from this list: send the line "unsubscribe perfbook" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
