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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> >>> Signed-off-by: Junchang Wang <junchangwang@xxxxxxxxx>
> >>
> >> 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
> >>>
> >>
> >
>
--
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
