RE: [PATCH v6] libgfortran: Replace mutex with rwlock

["Zhu, Lipeng" <lipeng.zhu@intel.com>]

On 2023/12/8 18:19, Jakub Jelinek wrote:
> On Fri, Aug 18, 2023 at 11:18:19AM +0800, Zhu, Lipeng wrote:
> > From: Lipeng Zhu <lipeng.zhu@intel.com>
> >
> > This patch try to introduce the rwlock and split the read/write to
> > unit_root tree and unit_cache with rwlock instead of the mutex to
> > increase CPU efficiency. In the get_gfc_unit function, the percentage
> > to step into the insert_unit function is around 30%, in most
> > instances, we can get the unit in the phase of reading the unit_cache
> > or unit_root tree. So split the read/write phase by rwlock would be an
> > approach to make it more parallel.
> >
> > BTW, the IPC metrics can gain around 9x in our test server with 220
> > cores. The benchmark we used is https://github.com/rwesson/NEAT
> >
> > libgcc/ChangeLog:
> >
> >     * gthr-posix.h (__GTHREAD_RWLOCK_INIT): New macro
> >     (__gthrw): New function
> >     (__gthread_rwlock_rdlock): New function
> >     (__gthread_rwlock_tryrdlock): New function
> >     (__gthread_rwlock_wrlock): New function
> >     (__gthread_rwlock_trywrlock): New function
> >     (__gthread_rwlock_unlock): New function
> >
> > libgfortran/ChangeLog:
> >
> >     * io/async.c (DEBUG_LINE): New
> >     * io/async.h (RWLOCK_DEBUG_ADD): New macro
> >     (CHECK_RDLOCK): New macro
> >     (CHECK_WRLOCK): New macro
> >     (TAIL_RWLOCK_DEBUG_QUEUE): New macro
> >     (IN_RWLOCK_DEBUG_QUEUE): New macro
> >     (RDLOCK): New macro
> >     (WRLOCK): New macro
> >     (RWUNLOCK): New macro
> >     (RD_TO_WRLOCK): New macro
> >     (INTERN_RDLOCK): New macro
> >     (INTERN_WRLOCK): New macro
> >     (INTERN_RWUNLOCK): New macro
> >     * io/io.h (internal_proto): Define unit_rwlock
> >     * io/transfer.c (st_read_done_worker): Relace unit_lock with unit_rwlock
> >     (st_write_done_worker): Relace unit_lock with unit_rwlock
> >     * io/unit.c (get_gfc_unit): Relace unit_lock with unit_rwlock
> >     (if): Relace unit_lock with unit_rwlock
> >     (close_unit_1): Relace unit_lock with unit_rwlock
> >     (close_units): Relace unit_lock with unit_rwlock
> >     (newunit_alloc): Relace unit_lock with unit_rwlock
> >     * io/unix.c (flush_all_units): Relace unit_lock with unit_rwlock
> 
> The changeLog entries are all incorrect.
> 1) they should be indented by a tab, not 4 spaces, and should end with
>    a dot
> 2) when several consecutive descriptions have the same text, especially
>    when it is long, it should use Likewise. for the 2nd and following
> 3) (internal_proto) is certainly not what you've changed, from what I can
>    see in io.h you've done:
> 	* io/io.h (struct gfc_unit): Change UNIT_LOCK to UNIT_RWLOCK in
> 	a comment.
> 	(unit_lock): Remove including associated internal_proto.
> 	(unit_rwlock): New declarations including associated internal_proto.
> 	(dec_waiting_unlocked): Use WRLOCK and RWUNLOCK on unit_rwlock
> 	instead of __gthread_mutex_lock and __gthread_mutex_unlock on
> 	unit_lock.
>    (if) is certainly not what you've changed either, always find what
>    function or macro the change was in, or if you remove something, state
>    it, if you add something, state it.
> 4) all the
>    Replace unit_lock with unit_rwlock. descriptions only partially match
>    reality, you've also changed the operations on those variables.
> 

Hi Jakub, 
Thanks for your help, very appreciated! I just updated the patch according to
your comments. A new version [PATCH V7] is sent out for your review which 
update the change log and formatting code according to your following 
comments.

Lipeng Zhu

> > --- a/libgfortran/io/async.h
> > +++ b/libgfortran/io/async.h
> > @@ -207,9 +207,132 @@
> >      INTERN_LOCK (&debug_queue_lock);
> 	\
> >      MUTEX_DEBUG_ADD (mutex);
> 	\
> >      INTERN_UNLOCK (&debug_queue_lock);
> 	\
> > -    DEBUG_PRINTF ("%s" DEBUG_RED "ACQ:" DEBUG_NORM " %-
> 30s %78p\n", aio_prefix, #mutex, mutex); \
> > +    DEBUG_PRINTF ("%s" DEBUG_RED "ACQ:" DEBUG_NORM " %-
> 30s %78p\n", aio_prefix, #mutex, \
> > +		 mutex); \
> 
> Why are you changing this at all?
> 
> > +#define RD_TO_WRLOCK(rwlock) \
> > +  RWUNLOCK (rwlock);\
> 
> At least a space before \ (or better tab
> 
> > +#define RD_TO_WRLOCK(rwlock) \
> > +  RWUNLOCK (rwlock);\
> 
> Likewise.
> 
> > +  WRLOCK (rwlock);
> > +#endif
> > +#endif
> > +
> > +#ifndef __GTHREAD_RWLOCK_INIT
> > +#define RDLOCK(rwlock) LOCK (rwlock)
> > +#define WRLOCK(rwlock) LOCK (rwlock)
> > +#define RWUNLOCK(rwlock) UNLOCK (rwlock) #define
> RD_TO_WRLOCK(rwlock)
> > +{}
> 
> do {} while (0)
> instead of {}
> ?
> 
> >  #endif
> >
> >  #define INTERN_LOCK(mutex) T_ERROR (__gthread_mutex_lock, mutex);
> >
> >  #define INTERN_UNLOCK(mutex) T_ERROR (__gthread_mutex_unlock,
> mutex);
> >
> > +#define INTERN_RDLOCK(rwlock) T_ERROR (__gthread_rwlock_rdlock,
> > +rwlock); #define INTERN_WRLOCK(rwlock) T_ERROR
> > +(__gthread_rwlock_wrlock, rwlock); #define INTERN_RWUNLOCK(rwlock)
> > +T_ERROR (__gthread_rwlock_unlock, rwlock);
> 
> Admittedly preexisting issue, but I wonder why the ; at the end, all the uses of
> RDLOCK etc. I've seen were followed by ;
> 
> > --- a/libgfortran/io/unit.c
> > +++ b/libgfortran/io/unit.c
> > @@ -33,34 +33,36 @@ see the files COPYING3 and COPYING.RUNTIME
> > respectively.  If not, see
> >
> >
> >  /* IO locking rules:
> > -   UNIT_LOCK is a master lock, protecting UNIT_ROOT tree and UNIT_CACHE.
> > +   UNIT_RWLOCK is a master lock, protecting UNIT_ROOT tree and
> UNIT_CACHE.
> 
> master rw lock
> ?
> 
> > +/* get_gfc_unit_from_root()-- Given an integer, return a pointer
> > +  to the unit structure. Returns NULL if the unit does not exist,
> > +   otherwise returns a locked unit. */
> 
> Formatting, to is indented differently from otherwise, both should be 3 spaces,
> and there should be 2 spaces after ., rather than just one (in both spots).
> 
> > +
> > +static inline gfc_unit * get_gfc_unit_from_unit_root (int n)
> 
> Formatting, there shouldn't be space after *, but also function name should be
> at the beginning of line, so in this case it should be static inline gfc_unit *
> get_gfc_unit_from_unit_root (int n)
> 
> > +{
> > +    gfc_unit *p;
> > +    int c = 0;
> > +    p = unit_root;
> 
> This should be indented by 2 spaces rather than 4.
> 
> > +  while (p != NULL)
> > +    {
> > +      c = compare (n, p->unit_number);
> > +      if (c < 0)
> > +	p = p->left;
> > +      if (c > 0)
> > +	p = p->right;
> > +      if (c == 0)
> > +	break;
> > +    }
> > +    return p;
> 
> And the above return p; line as well.
> 
> 	Jakub