Re: RE: [PATCH v7] libgfortran: Replace mutex with rwlock

[Lipeng Zhu]

On 2023/12/21 19:42, Thomas Schwinge wrote:

Hi!

On 2023-12-13T21:52:29+0100, I wrote:

On 2023-12-12T02:05:26+, "Zhu, Lipeng"  wrote:

On 2023/12/12 1:45, H.J. Lu wrote:

On Sat, Dec 9, 2023 at 7:25 PM Zhu, Lipeng  wrote:

On 2023/12/9 23:23, Jakub Jelinek wrote:

On Sat, Dec 09, 2023 at 10:39:45AM -0500, Lipeng Zhu wrote:

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



Ok for trunk, thanks.



Thanks! Looking forward to landing to trunk.



Pushed for you.



I've just filed 
"'libgomp.fortran/rwlock_1.f90', 'libgomp.fortran/rwlock_3.f90' execution test 
timeouts".
Would you be able to look into that?


See my update in there.


Grüße
  Thomas
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955



Updated in https://gcc.gnu.org/PR113005. Could you help to verify if the 
draft patch would fix the execution test timeout issue on your side?


diff --git a/libgomp/testsuite/libgomp.fortran/rwlock_1.f90 
b/libgomp/testsuite/libgomp.fortran/rwlock_1.f90

index f90ecbeb00f..1c271ae031d 100644
--- a/libgomp/testsuite/libgomp.fortran/rwlock_1.f90
+++ b/libgomp/testsuite/libgomp.fortran/rwlock_1.f90
@@ -7,13 +7,12 @@ program main
   use omp_lib
   implicit none
   integer:: unit_number, v1, v2, i
-  character(11) :: file_name
+  character(16) :: file_name
   character(3) :: async = "no"
   !$omp parallel private (unit_number, v1, v2, file_name, async, i)
 do i = 0, 100
   unit_number = 10 + omp_get_thread_num ()
-  write (file_name, "(I3, A)") unit_number, "_tst.dat"
-  file_name = adjustl(file_name)
+  write (file_name, "(I5.5, A)") unit_number, "_tst.dat"
   open (unit_number, file=file_name, asynchronous="yes")
   ! call inquire with file parameter to test find_file in unix.c
   inquire (file=file_name, asynchronous=async)

Lipeng Zhu

Re: [PATCH] libgfortran: Bugfix if not define HAVE_ATOMIC_FETCH_ADD

[Tobias Burnus]

On 22.12.23 03:36, Lipeng Zhu wrote:

This patch try to fix the bug when HAVE_ATOMIC_FETCH_ADD is
not defined in dec_waiting_unlocked function.

libgfortran/ChangeLog:

  * io/io.h (dec_waiting_unlocked): Use
  __gthread_rwlock_wrlock/__gthread_rwlock_unlock or
  __gthread_mutex_lock/__gthread_mutex_unlock functions
  to replace WRLOCK and RWUNLOCK macros.

Signed-off-by: Lipeng Zhu 


The change looks good to me + I assume it will work, but have not tested
it myself.

Downside is that it slightly breaks with the abstraction done with all
the macros, but it seems to be the simplest solution.

What is really missing - and should be included in the commit message
(before the ChangeLog block) - is the following information:

   As io.h does not include async.h, the WRLOCK and RWUNLOCK macros are
undefined.

(Or something similar in other words.)

I think that helps others when looking at "git log" and wondering *why*
that change was needed.

Thanks,

Tobias


  libgfortran/io/io.h | 10 --
  1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h
index 15daa0995b1..c7f0f7d7d9e 100644
--- a/libgfortran/io/io.h
+++ b/libgfortran/io/io.h
@@ -1020,9 +1020,15 @@ dec_waiting_unlocked (gfc_unit *u)
  #ifdef HAVE_ATOMIC_FETCH_ADD
(void) __atomic_fetch_add (&u->waiting, -1, __ATOMIC_RELAXED);
  #else
-  WRLOCK (&unit_rwlock);
+#ifdef __GTHREAD_RWLOCK_INIT
+  __gthread_rwlock_wrlock (&unit_rwlock);
+  u->waiting--;
+  __gthread_rwlock_unlock (&unit_rwlock);
+#else
+  __gthread_mutex_lock (&unit_rwlock);
u->waiting--;
-  RWUNLOCK (&unit_rwlock);
+  __gthread_mutex_unlock (&unit_rwlock);
+#endif
  #endif
  }

-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955

[PATCH, committed] Fortran: fix FE memleak

[Harald Anlauf]

Dear all,

I've committed the attached, simple & obvious patch for a
gmp memory leak in gfc_get_nodesc_array_type that shows
up when running f951 under valgrind e.g. on testcase
gfortran.dg/class_optional_2.f90, after regtesting on
x86_64-pc-linux-gnu.

(Note that this does not address the underlying issues
of pr55978).

Thanks,
Harald

From 93c96e3ad0024a397115aa17bf29c7efc6b535a1 Mon Sep 17 00:00:00 2001
From: Harald Anlauf 
Date: Wed, 3 Jan 2024 20:21:00 +0100
Subject: [PATCH] Fortran: fix FE memleak

gcc/fortran/ChangeLog:

	* trans-types.cc (gfc_get_nodesc_array_type): Clear used gmp
	variables.
---
 gcc/fortran/trans-types.cc | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/gcc/fortran/trans-types.cc b/gcc/fortran/trans-types.cc
index e6db1c95450..676014e9b98 100644
--- a/gcc/fortran/trans-types.cc
+++ b/gcc/fortran/trans-types.cc
@@ -1795,7 +1795,7 @@ gfc_get_nodesc_array_type (tree etype, gfc_array_spec * as, gfc_packed packed,
 	  TYPE_LANG_SPECIFIC (type) = TYPE_LANG_SPECIFIC (TREE_TYPE (type));
 	}

-  return type;
+  goto array_type_done;
 }

   if (known_stride)
@@ -1814,10 +1814,6 @@ gfc_get_nodesc_array_type (tree etype, gfc_array_spec * as, gfc_packed packed,

   layout_type (type);

-  mpz_clear (offset);
-  mpz_clear (stride);
-  mpz_clear (delta);
-
   /* Represent packed arrays as multi-dimensional if they have rank >
  1 and with proper bounds, instead of flat arrays.  This makes for
  better debug info.  */
@@ -1848,6 +1844,12 @@ gfc_get_nodesc_array_type (tree etype, gfc_array_spec * as, gfc_packed packed,
   GFC_ARRAY_TYPE_P (type) = 1;
   TYPE_LANG_SPECIFIC (type) = TYPE_LANG_SPECIFIC (TREE_TYPE (type));
 }
+
+array_type_done:
+  mpz_clear (offset);
+  mpz_clear (stride);
+  mpz_clear (delta);
+
   return type;
 }

--
2.35.3

Re: [PATCH] libgfortran: Bugfix if not define HAVE_ATOMIC_FETCH_ADD

[Jerry D]

On 1/3/24 3:12 AM, Tobias Burnus wrote:

On 22.12.23 03:36, Lipeng Zhu wrote:

This patch try to fix the bug when HAVE_ATOMIC_FETCH_ADD is
not defined in dec_waiting_unlocked function.

libgfortran/ChangeLog:

  * io/io.h (dec_waiting_unlocked): Use
  __gthread_rwlock_wrlock/__gthread_rwlock_unlock or
  __gthread_mutex_lock/__gthread_mutex_unlock functions
  to replace WRLOCK and RWUNLOCK macros.

Signed-off-by: Lipeng Zhu 


The change looks good to me + I assume it will work, but have not tested
it myself.

Downside is that it slightly breaks with the abstraction done with all
the macros, but it seems to be the simplest solution.

What is really missing - and should be included in the commit message
(before the ChangeLog block) - is the following information:

    As io.h does not include async.h, the WRLOCK and RWUNLOCK macros are
undefined.


 snip 

Would it make sense to include or merge async.h into io.h?

Jerry

Re: [PATCH] libgfortran: Bugfix if not define HAVE_ATOMIC_FETCH_ADD

[Lipeng Zhu]

On 2024/1/3 19:12, Tobias Burnus wrote:

On 22.12.23 03:36, Lipeng Zhu wrote:

This patch try to fix the bug when HAVE_ATOMIC_FETCH_ADD is
not defined in dec_waiting_unlocked function.

libgfortran/ChangeLog:

  * io/io.h (dec_waiting_unlocked): Use
  __gthread_rwlock_wrlock/__gthread_rwlock_unlock or
  __gthread_mutex_lock/__gthread_mutex_unlock functions
  to replace WRLOCK and RWUNLOCK macros.

Signed-off-by: Lipeng Zhu 


The change looks good to me + I assume it will work, but have not tested
it myself.

Downside is that it slightly breaks with the abstraction done with all
the macros, but it seems to be the simplest solution.



Hi Tobias,

Thanks for your review, the reason I changed like this is because I 
found when LOCK macro was first introduced in this patch: 
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=2b4c90656132abb8b8ad155d345c7d4fbf1687c9, 
it replaced __gthread_mutex_lock method with LOCK macro in other files 
like io/unit.c, but remained __gthread_mutex_lock in io/io.h. I am not 
sure if this is intentional or not, to avoid potential risk, I used the 
most straightforward solution.




What is really missing - and should be included in the commit message
(before the ChangeLog block) - is the following information:

    As io.h does not include async.h, the WRLOCK and RWUNLOCK macros are
undefined.

(Or something similar in other words.)

I think that helps others when looking at "git log" and wondering *why*
that change was needed.

Thanks,

Tobias



Thanks, I will update the commit message as suggested.

Lipeng Zhu


  libgfortran/io/io.h | 10 --
  1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h
index 15daa0995b1..c7f0f7d7d9e 100644
--- a/libgfortran/io/io.h
+++ b/libgfortran/io/io.h
@@ -1020,9 +1020,15 @@ dec_waiting_unlocked (gfc_unit *u)
  #ifdef HAVE_ATOMIC_FETCH_ADD
    (void) __atomic_fetch_add (&u->waiting, -1, __ATOMIC_RELAXED);
  #else
-  WRLOCK (&unit_rwlock);
+#ifdef __GTHREAD_RWLOCK_INIT
+  __gthread_rwlock_wrlock (&unit_rwlock);
+  u->waiting--;
+  __gthread_rwlock_unlock (&unit_rwlock);
+#else
+  __gthread_mutex_lock (&unit_rwlock);
    u->waiting--;
-  RWUNLOCK (&unit_rwlock);
+  __gthread_mutex_unlock (&unit_rwlock);
+#endif
  #endif
  }

-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 
80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: 
Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; 
Registergericht München, HRB 106955