* [PATCH 0/2] Reviewing CodeSamples/count/count_stat_eventual.c @ 2017-05-09 5:08 Junchang Wang 2017-05-09 5:08 ` [PATCH 1/2] Replace definition of variable sum from int to unsigned long Junchang Wang 2017-05-09 5:08 ` [PATCH 2/2] Remove unnecessary memory fence Junchang Wang 0 siblings, 2 replies; 8+ messages in thread From: Junchang Wang @ 2017-05-09 5:08 UTC (permalink / raw) To: perfbook; +Cc: Junchang Wang Hi Paul, Thanks for this excellent book. I enjoy reading the book very much(especially the Q&A part), and have learned a lot from the first a few chapters. I will post patches and questions while reading the book. Definitely, I'm not an expert here, so please help correct if I misunderstand anything. Thanks! Junchang Wang (2): Replace definition of variable sum from int to unsigned long Remove unnecessary memory fence CodeSamples/count/count_stat_eventual.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] Replace definition of variable sum from int to unsigned long 2017-05-09 5:08 [PATCH 0/2] Reviewing CodeSamples/count/count_stat_eventual.c Junchang Wang @ 2017-05-09 5:08 ` Junchang Wang 2017-05-09 14:47 ` Paul E. McKenney 2017-05-09 5:08 ` [PATCH 2/2] Remove unnecessary memory fence Junchang Wang 1 sibling, 1 reply; 8+ messages in thread From: Junchang Wang @ 2017-05-09 5:08 UTC (permalink / raw) To: perfbook; +Cc: Junchang Wang The maximum size of integer variable on 64bit machines is 2^31. Suppose the writer takes 5 nanosecond to increase the counter by one, the result wraps around in about 10 seconds. Signed-off-by: Junchang Wang <junchangwang@gmail.com> --- CodeSamples/count/count_stat_eventual.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CodeSamples/count/count_stat_eventual.c b/CodeSamples/count/count_stat_eventual.c index 75a0ca9..059ab8b 100644 --- a/CodeSamples/count/count_stat_eventual.c +++ b/CodeSamples/count/count_stat_eventual.c @@ -38,7 +38,7 @@ unsigned long read_count(void) void *eventual(void *arg) { int t; - int sum; + unsigned long sum; while (stopflag < 3) { sum = 0; -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] Replace definition of variable sum from int to unsigned long 2017-05-09 5:08 ` [PATCH 1/2] Replace definition of variable sum from int to unsigned long Junchang Wang @ 2017-05-09 14:47 ` Paul E. McKenney 0 siblings, 0 replies; 8+ messages in thread From: Paul E. McKenney @ 2017-05-09 14:47 UTC (permalink / raw) To: Junchang Wang; +Cc: perfbook On Tue, May 09, 2017 at 01:08:30PM +0800, Junchang Wang wrote: > The maximum size of integer variable on 64bit machines is 2^31. Suppose the > writer takes 5 nanosecond to increase the counter by one, the result wraps > around in about 10 seconds. > > Signed-off-by: Junchang Wang <junchangwang@gmail.com> Good catch, queued! Thanx, Paul > --- > CodeSamples/count/count_stat_eventual.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/CodeSamples/count/count_stat_eventual.c b/CodeSamples/count/count_stat_eventual.c > index 75a0ca9..059ab8b 100644 > --- a/CodeSamples/count/count_stat_eventual.c > +++ b/CodeSamples/count/count_stat_eventual.c > @@ -38,7 +38,7 @@ unsigned long read_count(void) > void *eventual(void *arg) > { > int t; > - int sum; > + unsigned long sum; > > while (stopflag < 3) { > sum = 0; > -- > 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] Remove unnecessary memory fence 2017-05-09 5:08 [PATCH 0/2] Reviewing CodeSamples/count/count_stat_eventual.c Junchang Wang 2017-05-09 5:08 ` [PATCH 1/2] Replace definition of variable sum from int to unsigned long Junchang Wang @ 2017-05-09 5:08 ` Junchang Wang 2017-05-09 14:47 ` Paul E. McKenney 1 sibling, 1 reply; 8+ messages in thread From: Junchang Wang @ 2017-05-09 5:08 UTC (permalink / raw) To: perfbook; +Cc: Junchang Wang 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. 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Remove unnecessary memory fence 2017-05-09 5:08 ` [PATCH 2/2] Remove unnecessary memory fence Junchang Wang @ 2017-05-09 14:47 ` Paul E. McKenney 2017-05-09 15:20 ` Junchang Wang 0 siblings, 1 reply; 8+ messages in thread From: Paul E. McKenney @ 2017-05-09 14:47 UTC (permalink / raw) To: Junchang Wang; +Cc: perfbook 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Remove unnecessary memory fence 2017-05-09 14:47 ` Paul E. McKenney @ 2017-05-09 15:20 ` Junchang Wang 2017-05-09 15:40 ` Paul E. McKenney 0 siblings, 1 reply; 8+ messages in thread From: Junchang Wang @ 2017-05-09 15:20 UTC (permalink / raw) To: Paul E. McKenney; +Cc: perfbook 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? 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 >> > ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Remove unnecessary memory fence 2017-05-09 15:20 ` Junchang Wang @ 2017-05-09 15:40 ` Paul E. McKenney 2017-05-09 23:11 ` Junchang Wang 0 siblings, 1 reply; 8+ messages in thread From: Paul E. McKenney @ 2017-05-09 15:40 UTC (permalink / raw) To: Junchang Wang; +Cc: perfbook 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 > >> > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Remove unnecessary memory fence 2017-05-09 15:40 ` Paul E. McKenney @ 2017-05-09 23:11 ` Junchang Wang 0 siblings, 0 replies; 8+ messages in thread From: Junchang Wang @ 2017-05-09 23:11 UTC (permalink / raw) To: Paul E. McKenney; +Cc: perfbook 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@linux.vnet.ibm.com> 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@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 >> >> >> > >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-05-09 23:11 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-05-09 5:08 [PATCH 0/2] Reviewing CodeSamples/count/count_stat_eventual.c Junchang Wang 2017-05-09 5:08 ` [PATCH 1/2] Replace definition of variable sum from int to unsigned long Junchang Wang 2017-05-09 14:47 ` Paul E. McKenney 2017-05-09 5:08 ` [PATCH 2/2] Remove unnecessary memory fence Junchang Wang 2017-05-09 14:47 ` Paul E. McKenney 2017-05-09 15:20 ` Junchang Wang 2017-05-09 15:40 ` Paul E. McKenney 2017-05-09 23:11 ` Junchang Wang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.