* [PATCH v2] count_stat_eventual: Remove unnecessary READ_ONCE() and smp_mb() @ 2017-05-16 15:44 Junchang Wang 2017-05-16 17:33 ` Paul E. McKenney 0 siblings, 1 reply; 5+ messages in thread From: Junchang Wang @ 2017-05-16 15:44 UTC (permalink / raw) To: perfbook, akiyks, paulmck; +Cc: Junchang Wang In function eventual(), if the value of stopflag become larger than zero (the ''if'' branch is taken), then only the "eventual( )" thread updates stopflag. So, the READ_ONCE() within the WRITE_ONCE() is not necessary. Remove it. In function count_cleanup(), there is no need to run smp_mb( ) in between statement writing to and statement reading from the same variable, stopflag. Remove smp_mb(). Suggested-by: Akira Yokosawa <akiyks@gmail.com> 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 464de30..1365168 100644 --- a/CodeSamples/count/count_stat_eventual.c +++ b/CodeSamples/count/count_stat_eventual.c @@ -40,16 +40,17 @@ void *eventual(void *arg) { int t; unsigned long sum; + int stopflag_l = 0; - while (READ_ONCE(stopflag) < 3) { + 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 (READ_ONCE(stopflag)) { + if ((stopflag_l = READ_ONCE(stopflag)) != 0) { smp_mb(); - WRITE_ONCE(stopflag, READ_ONCE(stopflag) + 1); + WRITE_ONCE(stopflag, ++stopflag_l); } } return NULL; @@ -68,7 +69,6 @@ void count_init(void) void count_cleanup(void) { WRITE_ONCE(stopflag, 1); - smp_mb(); while (READ_ONCE(stopflag) < 3) poll(NULL, 0, 1); smp_mb(); -- 2.7.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] count_stat_eventual: Remove unnecessary READ_ONCE() and smp_mb() 2017-05-16 15:44 [PATCH v2] count_stat_eventual: Remove unnecessary READ_ONCE() and smp_mb() Junchang Wang @ 2017-05-16 17:33 ` Paul E. McKenney 2017-05-17 3:39 ` Junchang Wang 0 siblings, 1 reply; 5+ messages in thread From: Paul E. McKenney @ 2017-05-16 17:33 UTC (permalink / raw) To: Junchang Wang; +Cc: perfbook, akiyks On Tue, May 16, 2017 at 11:44:12PM +0800, Junchang Wang wrote: > In function eventual(), if the value of stopflag become larger than zero (the > ''if'' branch is taken), then only the "eventual( )" thread updates stopflag. > So, the READ_ONCE() within the WRITE_ONCE() is not necessary. Remove it. > > In function count_cleanup(), there is no need to run smp_mb( ) in between > statement writing to and statement reading from the same variable, stopflag. > Remove smp_mb(). > > Suggested-by: Akira Yokosawa <akiyks@gmail.com> > Signed-off-by: Junchang Wang <junchangwang@gmail.com> Removing the memory barrier is good. Removing the stopflag_l local variable is presumably intended to remove one load instruction per pass through the outer loop, assuming that the stopflag_l variable is kept in a register. Is the performance benefit actually visible? My guess is "no". If the performance is not substantial, my thought would be to keep the code simpler, given that this is a textbook. And yes, there might be other performance-neutral simplifications possible, and I would welcome patches to that effect. Thanx, Paul > --- > 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 464de30..1365168 100644 > --- a/CodeSamples/count/count_stat_eventual.c > +++ b/CodeSamples/count/count_stat_eventual.c > @@ -40,16 +40,17 @@ void *eventual(void *arg) > { > int t; > unsigned long sum; > + int stopflag_l = 0; > > - while (READ_ONCE(stopflag) < 3) { > + 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 (READ_ONCE(stopflag)) { > + if ((stopflag_l = READ_ONCE(stopflag)) != 0) { > smp_mb(); > - WRITE_ONCE(stopflag, READ_ONCE(stopflag) + 1); > + WRITE_ONCE(stopflag, ++stopflag_l); > } > } > return NULL; > @@ -68,7 +69,6 @@ void count_init(void) > void count_cleanup(void) > { > WRITE_ONCE(stopflag, 1); > - smp_mb(); > while (READ_ONCE(stopflag) < 3) > poll(NULL, 0, 1); > smp_mb(); > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] count_stat_eventual: Remove unnecessary READ_ONCE() and smp_mb() 2017-05-16 17:33 ` Paul E. McKenney @ 2017-05-17 3:39 ` Junchang Wang 2017-05-17 22:44 ` Akira Yokosawa 0 siblings, 1 reply; 5+ messages in thread From: Junchang Wang @ 2017-05-17 3:39 UTC (permalink / raw) To: Paul E. McKenney; +Cc: perfbook, Akira Yokosawa On Wed, May 17, 2017 at 1:33 AM, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote: > On Tue, May 16, 2017 at 11:44:12PM +0800, Junchang Wang wrote: >> In function eventual(), if the value of stopflag become larger than zero (the >> ''if'' branch is taken), then only the "eventual( )" thread updates stopflag. >> So, the READ_ONCE() within the WRITE_ONCE() is not necessary. Remove it. >> >> In function count_cleanup(), there is no need to run smp_mb( ) in between >> statement writing to and statement reading from the same variable, stopflag. >> Remove smp_mb(). >> >> Suggested-by: Akira Yokosawa <akiyks@gmail.com> >> Signed-off-by: Junchang Wang <junchangwang@gmail.com> > > Removing the memory barrier is good. Removing the stopflag_l local > variable is presumably intended to remove one load instruction per pass > through the outer loop, assuming that the stopflag_l variable is kept > in a register. > > Is the performance benefit actually visible? My guess is "no". > If the performance is not substantial, my thought would be to keep > the code simpler, given that this is a textbook. > Hi Paul, Agree. Thanks for the note. Anyway, let's submit a new patch on smp_mb(), which makes the code correct. For simplifying stopflag, we can consider submitting a new patch later. How does that sound like? Thanks, --Jason > And yes, there might be other performance-neutral simplifications possible, > and I would welcome patches to that effect. > > Thanx, Paul > >> --- >> 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 464de30..1365168 100644 >> --- a/CodeSamples/count/count_stat_eventual.c >> +++ b/CodeSamples/count/count_stat_eventual.c >> @@ -40,16 +40,17 @@ void *eventual(void *arg) >> { >> int t; >> unsigned long sum; >> + int stopflag_l = 0; >> >> - while (READ_ONCE(stopflag) < 3) { >> + 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 (READ_ONCE(stopflag)) { >> + if ((stopflag_l = READ_ONCE(stopflag)) != 0) { >> smp_mb(); >> - WRITE_ONCE(stopflag, READ_ONCE(stopflag) + 1); >> + WRITE_ONCE(stopflag, ++stopflag_l); >> } >> } >> return NULL; >> @@ -68,7 +69,6 @@ void count_init(void) >> void count_cleanup(void) >> { >> WRITE_ONCE(stopflag, 1); >> - smp_mb(); >> while (READ_ONCE(stopflag) < 3) >> poll(NULL, 0, 1); >> smp_mb(); >> -- >> 2.7.4 >> > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] count_stat_eventual: Remove unnecessary READ_ONCE() and smp_mb() 2017-05-17 3:39 ` Junchang Wang @ 2017-05-17 22:44 ` Akira Yokosawa 2017-05-17 23:07 ` Paul E. McKenney 0 siblings, 1 reply; 5+ messages in thread From: Akira Yokosawa @ 2017-05-17 22:44 UTC (permalink / raw) To: Junchang Wang, Paul E. McKenney; +Cc: perfbook On 2017/05/17 11:39:41 +0800, Junchang Wang wrote: > On Wed, May 17, 2017 at 1:33 AM, Paul E. McKenney > <paulmck@linux.vnet.ibm.com> wrote: >> On Tue, May 16, 2017 at 11:44:12PM +0800, Junchang Wang wrote: >>> In function eventual(), if the value of stopflag become larger than zero (the >>> ''if'' branch is taken), then only the "eventual( )" thread updates stopflag. >>> So, the READ_ONCE() within the WRITE_ONCE() is not necessary. Remove it. >>> >>> In function count_cleanup(), there is no need to run smp_mb( ) in between >>> statement writing to and statement reading from the same variable, stopflag. >>> Remove smp_mb(). >>> >>> Suggested-by: Akira Yokosawa <akiyks@gmail.com> >>> Signed-off-by: Junchang Wang <junchangwang@gmail.com> >> >> Removing the memory barrier is good. Removing the stopflag_l local >> variable is presumably intended to remove one load instruction per pass >> through the outer loop, assuming that the stopflag_l variable is kept >> in a register. >> >> Is the performance benefit actually visible? My guess is "no". >> If the performance is not substantial, my thought would be to keep >> the code simpler, given that this is a textbook. >> > > Hi Paul, > > Agree. Thanks for the note. Anyway, let's submit a new patch on > smp_mb(), which makes the code correct. For simplifying stopflag, we > can consider submitting a new patch later. How does that sound like? Hi Jason & Paul, For a material in Chapter 5, I agree that removing excess READ_ONCE() is not necessary. Such optimizations could be discussed somewhere (maybe a Quick Quiz) in Chapter 14, but I'm not sure. So, the revised patch just removing a smp_mb() looks good to me. Thanks, Akira. > > > Thanks, > --Jason > > >> And yes, there might be other performance-neutral simplifications possible, >> and I would welcome patches to that effect. >> >> Thanx, Paul >> >>> --- >>> 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 464de30..1365168 100644 >>> --- a/CodeSamples/count/count_stat_eventual.c >>> +++ b/CodeSamples/count/count_stat_eventual.c >>> @@ -40,16 +40,17 @@ void *eventual(void *arg) >>> { >>> int t; >>> unsigned long sum; >>> + int stopflag_l = 0; >>> >>> - while (READ_ONCE(stopflag) < 3) { >>> + 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 (READ_ONCE(stopflag)) { >>> + if ((stopflag_l = READ_ONCE(stopflag)) != 0) { >>> smp_mb(); >>> - WRITE_ONCE(stopflag, READ_ONCE(stopflag) + 1); >>> + WRITE_ONCE(stopflag, ++stopflag_l); >>> } >>> } >>> return NULL; >>> @@ -68,7 +69,6 @@ void count_init(void) >>> void count_cleanup(void) >>> { >>> WRITE_ONCE(stopflag, 1); >>> - smp_mb(); >>> while (READ_ONCE(stopflag) < 3) >>> poll(NULL, 0, 1); >>> smp_mb(); >>> -- >>> 2.7.4 >>> >> > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] count_stat_eventual: Remove unnecessary READ_ONCE() and smp_mb() 2017-05-17 22:44 ` Akira Yokosawa @ 2017-05-17 23:07 ` Paul E. McKenney 0 siblings, 0 replies; 5+ messages in thread From: Paul E. McKenney @ 2017-05-17 23:07 UTC (permalink / raw) To: Akira Yokosawa; +Cc: Junchang Wang, perfbook On Thu, May 18, 2017 at 07:44:10AM +0900, Akira Yokosawa wrote: > On 2017/05/17 11:39:41 +0800, Junchang Wang wrote: > > On Wed, May 17, 2017 at 1:33 AM, Paul E. McKenney > > <paulmck@linux.vnet.ibm.com> wrote: > >> On Tue, May 16, 2017 at 11:44:12PM +0800, Junchang Wang wrote: > >>> In function eventual(), if the value of stopflag become larger than zero (the > >>> ''if'' branch is taken), then only the "eventual( )" thread updates stopflag. > >>> So, the READ_ONCE() within the WRITE_ONCE() is not necessary. Remove it. > >>> > >>> In function count_cleanup(), there is no need to run smp_mb( ) in between > >>> statement writing to and statement reading from the same variable, stopflag. > >>> Remove smp_mb(). > >>> > >>> Suggested-by: Akira Yokosawa <akiyks@gmail.com> > >>> Signed-off-by: Junchang Wang <junchangwang@gmail.com> > >> > >> Removing the memory barrier is good. Removing the stopflag_l local > >> variable is presumably intended to remove one load instruction per pass > >> through the outer loop, assuming that the stopflag_l variable is kept > >> in a register. > >> > >> Is the performance benefit actually visible? My guess is "no". > >> If the performance is not substantial, my thought would be to keep > >> the code simpler, given that this is a textbook. > >> > > > > Hi Paul, > > > > Agree. Thanks for the note. Anyway, let's submit a new patch on > > smp_mb(), which makes the code correct. For simplifying stopflag, we > > can consider submitting a new patch later. How does that sound like? > > Hi Jason & Paul, > > For a material in Chapter 5, I agree that removing excess READ_ONCE() > is not necessary. > > Such optimizations could be discussed somewhere (maybe a Quick Quiz) > in Chapter 14, but I'm not sure. > > So, the revised patch just removing a smp_mb() looks good to me. Very good, queued and pushed. Thank you both! Thanx, Paul > Thanks, Akira. > > > > > > > Thanks, > > --Jason > > > > > >> And yes, there might be other performance-neutral simplifications possible, > >> and I would welcome patches to that effect. > >> > >> Thanx, Paul > >> > >>> --- > >>> 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 464de30..1365168 100644 > >>> --- a/CodeSamples/count/count_stat_eventual.c > >>> +++ b/CodeSamples/count/count_stat_eventual.c > >>> @@ -40,16 +40,17 @@ void *eventual(void *arg) > >>> { > >>> int t; > >>> unsigned long sum; > >>> + int stopflag_l = 0; > >>> > >>> - while (READ_ONCE(stopflag) < 3) { > >>> + 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 (READ_ONCE(stopflag)) { > >>> + if ((stopflag_l = READ_ONCE(stopflag)) != 0) { > >>> smp_mb(); > >>> - WRITE_ONCE(stopflag, READ_ONCE(stopflag) + 1); > >>> + WRITE_ONCE(stopflag, ++stopflag_l); > >>> } > >>> } > >>> return NULL; > >>> @@ -68,7 +69,6 @@ void count_init(void) > >>> void count_cleanup(void) > >>> { > >>> WRITE_ONCE(stopflag, 1); > >>> - smp_mb(); > >>> while (READ_ONCE(stopflag) < 3) > >>> poll(NULL, 0, 1); > >>> smp_mb(); > >>> -- > >>> 2.7.4 > >>> > >> > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-05-17 23:07 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-05-16 15:44 [PATCH v2] count_stat_eventual: Remove unnecessary READ_ONCE() and smp_mb() Junchang Wang 2017-05-16 17:33 ` Paul E. McKenney 2017-05-17 3:39 ` Junchang Wang 2017-05-17 22:44 ` Akira Yokosawa 2017-05-17 23:07 ` Paul E. McKenney
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.