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....@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
