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

[Jakub Jelinek <jakub@redhat.com>]

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.

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