Re: [PATCH v2][BZ #13065] New pthread_barrier algorithm to fulfill barrier destruction requirements.

[Torvald Riegel <triegel at redhat dot com>]

On Thu, 2016-01-14 at 18:05 -0200, Adhemerval Zanella wrote:
> On 13-01-2016 20:12, Torvald Riegel wrote:
> > diff --git a/nptl/pthread_barrier_init.c b/nptl/pthread_barrier_init.c
> > index ef14ed3..c9c8ccd 100644
> > --- a/nptl/pthread_barrier_init.c
> > +++ b/nptl/pthread_barrier_init.c
> > @@ -18,7 +18,7 @@
> >  
> >  #include <errno.h>
> >  #include "pthreadP.h"
> > -#include <lowlevellock.h>
> > +#include <futex-internal.h>
> >  #include <kernel-features.h>
> >  
> >  
> > @@ -34,8 +34,10 @@ __pthread_barrier_init (pthread_barrier_t *barrier,
> >  {
> >    struct pthread_barrier *ibarrier;
> 
> I wonder if it is worth since to also fix BZ#18868 [1]
> 
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=18868

I'll do this as a follow-up.

> >  
> > -  /* XXX EINVAL is not specified by POSIX as a possible error code.  */
> > -  if (__glibc_unlikely (count == 0))
> > +  /* XXX EINVAL is not specified by POSIX as a possible error code.  See
> > +     pthread_barrier_wait for the reason for the comparison with
> > +     BARRIER_IN_THRESHOLD.  */
> > +  if (__glibc_unlikely (count == 0 || count >= BARRIER_IN_THRESHOLD))
> >      return EINVAL;
> >  
> 
> Accordoing to POSIX 2013 [1] EINVAL is indeed allowed to be returned when
> could is 0, it does not specify is the value is too high.
> 
> [1] http://pubs.opengroup.org/onlinepubs/9699919799/

I have corrected the comment.

> > diff --git a/nptl/tst-barrier4.c b/nptl/tst-barrier4.c
> > index 4eef5aa..d3d3209 100644
> > --- a/nptl/tst-barrier4.c
> > +++ b/nptl/tst-barrier4.c
> > @@ -16,7 +16,7 @@
> >     License along with the GNU C Library; if not, see
> >     <http://www.gnu.org/licenses/>.  */
> >  
> > -/* This is a test for behavior not guaranteed by POSIX.  */
> > +/* This tests destruction of a barrier right after waiting on it.  */
> >  #include <errno.h>
> >  #include <pthread.h>
> >  #include <stdio.h>
> > diff --git a/nptl/tst-barrier5.c b/nptl/tst-barrier5.c
> > new file mode 100644
> > index 0000000..b99bd00
> > --- /dev/null
> > +++ b/nptl/tst-barrier5.c
> > @@ -0,0 +1,145 @@
> > +/* Copyright (C) 2004-2015 Free Software Foundation, Inc.
> > +   This file is part of the GNU C Library.
> 
> I think first line of new files should be its description.

I sticked to the same formatting as in tst-barrier3.c and
tst-barrier4.c, which put the one-line description after the header
comment, so ...

> > +
> > +   The GNU C Library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   The GNU C Library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with the GNU C Library; if not, see
> > +   <http://www.gnu.org/licenses/>.  */
> > +
> > +/* This tests the barrier reset mechanism.  */

... here.  I've kept this for now -- do you want me to give all the
barrier tests a one-line description in the first line (in a follow-up
patch)?

> > diff --git a/sysdeps/nptl/internaltypes.h b/sysdeps/nptl/internaltypes.h
> > index 74e848f..cf9a474 100644
> > --- a/sysdeps/nptl/internaltypes.h
> > +++ b/sysdeps/nptl/internaltypes.h
> > @@ -95,12 +95,13 @@ struct pthread_rwlockattr
> >  /* Barrier data structure.  */
> >  struct pthread_barrier
> >  {
> > -  unsigned int curr_event;
> > -  int lock;
> > -  unsigned int left;
> > -  unsigned int init_count;
> > -  int private;
> > +  unsigned int in;
> > +  unsigned int current_round;
> > +  unsigned int count;
> > +  int shared;
> > +  unsigned int out;
> >  };
> > +#define BARRIER_IN_THRESHOLD (UINT_MAX/2)
> >  
> 
> Could you add a comments about which field in the same way you did at 
> pthread_barrier_wait?

Sorry, I don't understand this sentence.  I guess you meant a
description of the fields similar in detail to the description of the
algorithm before pthread_barrier_wait?
I added this, given that the algorithm description already describes how
these fields are used, and gives the background on the choice of value
of BARRIER_IN_THRESHOLD:

/* Barrier data structure.  See pthread_barrier_wait for a description
   of how these fields are used.  */

If you were looking for something else, please let me know and I'll
address it in a follow-up patch.

I have committed the patch with changes mentioned above.  Thanks for
your review and testing!