[2/3] pthread: Refactor semaphore code

Message ID 20210203183414.571813-2-adhemerval.zanella@linaro.org
State Committed
Commit da4aea0b5e60ec2351367b0facee24e6035a7129
Headers
Series [1/3] linux: Require /dev/shm as the shared memory file system [BZ #25383] |

Commit Message

Adhemerval Zanella Feb. 3, 2021, 6:34 p.m. UTC
  The internal semaphore list code is moved to a specific file,
sem_routine.c, and the internal usage is simplified to only two
functions (one to insert a new semaphore and one to remove it
from the internal list).  There is no need to expose the
internal locking, neither how the semaphore mapping is implemented.

No functional or semantic change is expected, tested on
x86_64-linux-gnu.
---
 htl/Makefile                   |   1 +
 htl/semaphoreP.h               |  21 ----
 nptl/Makefile                  |   2 +-
 nptl/semaphoreP.h              |  21 ----
 sysdeps/pthread/sem_close.c    |  56 +---------
 sysdeps/pthread/sem_open.c     | 116 +-------------------
 sysdeps/pthread/sem_routines.c | 187 +++++++++++++++++++++++++++++++++
 sysdeps/pthread/sem_routines.h |  27 +++++
 8 files changed, 223 insertions(+), 208 deletions(-)
 create mode 100644 sysdeps/pthread/sem_routines.c
 create mode 100644 sysdeps/pthread/sem_routines.h
  

Comments

Florian Weimer Feb. 7, 2021, 9:30 a.m. UTC | #1
* Adhemerval Zanella via Libc-alpha:

> The internal semaphore list code is moved to a specific file,
> sem_routine.c, and the internal usage is simplified to only two
> functions (one to insert a new semaphore and one to remove it
> from the internal list).  There is no need to expose the
> internal locking, neither how the semaphore mapping is implemented.
>
> No functional or semantic change is expected, tested on
> x86_64-linux-gnu.

Looks okay to me.

Thanks,
Florian
  

Patch

diff --git a/htl/Makefile b/htl/Makefile
index 9adc95e07f..c15c1b194e 100644
--- a/htl/Makefile
+++ b/htl/Makefile
@@ -131,6 +131,7 @@  libpthread-routines := pt-attr pt-attr-destroy pt-attr-getdetachstate	    \
 	sem_close sem-destroy sem-getvalue sem-init sem_open		    \
 	sem-post sem-timedwait sem-trywait sem_unlink			    \
 	sem-wait sem-waitfast						    \
+	sem_routines							    \
 									    \
 	cancellation							    \
 	cthreads-compat							    \
diff --git a/htl/semaphoreP.h b/htl/semaphoreP.h
index 3d6efcdec7..4b13f7b30d 100644
--- a/htl/semaphoreP.h
+++ b/htl/semaphoreP.h
@@ -21,27 +21,6 @@ 
 
 #define SEM_SHM_PREFIX  "sem."
 
-/* Keeping track of currently used mappings.  */
-struct inuse_sem
-{
-  dev_t dev;
-  ino_t ino;
-  int refcnt;
-  sem_t *sem;
-  char name[];
-};
-
-
-/* The search tree for existing mappings.  */
-extern void *__sem_mappings attribute_hidden;
-
-/* Lock to protect the search tree.  */
-extern int __sem_mappings_lock attribute_hidden;
-
-
-/* Comparison function for search in tree with existing mappings.  */
-extern int __sem_search (const void *a, const void *b) attribute_hidden;
-
 static inline void __new_sem_open_init (struct new_sem *sem, unsigned value)
 {
   /* This always is a shared semaphore.  */
diff --git a/nptl/Makefile b/nptl/Makefile
index c49134b936..8fb7fee6db 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -137,7 +137,7 @@  libpthread-routines = nptl-init nptlfreeres vars events version pt-interp \
 		      pthread_once \
 		      old_pthread_atfork \
 		      pthread_getcpuclockid \
-		      sem_init sem_destroy \
+		      sem_init sem_destroy sem_routines \
 		      sem_open sem_close sem_unlink \
 		      sem_getvalue \
 		      sem_wait sem_timedwait sem_clockwait sem_post \
diff --git a/nptl/semaphoreP.h b/nptl/semaphoreP.h
index 3585af09fc..1b786149f4 100644
--- a/nptl/semaphoreP.h
+++ b/nptl/semaphoreP.h
@@ -22,27 +22,6 @@ 
 
 #define SEM_SHM_PREFIX  "sem."
 
-/* Keeping track of currently used mappings.  */
-struct inuse_sem
-{
-  dev_t dev;
-  ino_t ino;
-  int refcnt;
-  sem_t *sem;
-  char name[];
-};
-
-
-/* The search tree for existing mappings.  */
-extern void *__sem_mappings attribute_hidden;
-
-/* Lock to protect the search tree.  */
-extern int __sem_mappings_lock attribute_hidden;
-
-
-/* Comparison function for search in tree with existing mappings.  */
-extern int __sem_search (const void *a, const void *b) attribute_hidden;
-
 static inline void __new_sem_open_init (struct new_sem *sem, unsigned value)
 {
 #if __HAVE_64B_ATOMICS
diff --git a/sysdeps/pthread/sem_close.c b/sysdeps/pthread/sem_close.c
index 8b6c9c6dc5..6649196cac 100644
--- a/sysdeps/pthread/sem_close.c
+++ b/sysdeps/pthread/sem_close.c
@@ -17,65 +17,17 @@ 
    <https://www.gnu.org/licenses/>.  */
 
 #include <errno.h>
-#include <search.h>
-#include <sys/mman.h>
 #include "semaphoreP.h"
-
-struct walk_closure
-{
-  sem_t *the_sem;
-  struct inuse_sem *rec;
-};
-
-static void
-walker (const void *inodep, VISIT which, void *closure0)
-{
-  struct walk_closure *closure = closure0;
-  struct inuse_sem *nodep = *(struct inuse_sem **) inodep;
-
-  if (nodep->sem == closure->the_sem)
-    closure->rec = nodep;
-}
-
+#include <sem_routines.h>
 
 int
 sem_close (sem_t *sem)
 {
-  int result = 0;
-
-  /* Get the lock.  */
-  lll_lock (__sem_mappings_lock, LLL_PRIVATE);
-
-  /* Locate the entry for the mapping the caller provided.  */
-  struct inuse_sem *rec;
-  {
-    struct walk_closure closure = { .the_sem = sem, .rec = NULL };
-    __twalk_r (__sem_mappings, walker, &closure);
-    rec = closure.rec;
-  }
-  if  (rec != NULL)
+  if (!__sem_remove_mapping (sem))
     {
-      /* Check the reference counter.  If it is going to be zero, free
-	 all the resources.  */
-      if (--rec->refcnt == 0)
-	{
-	  /* Remove the record from the tree.  */
-	  (void) __tdelete (rec, &__sem_mappings, __sem_search);
-
-	  result = munmap (rec->sem, sizeof (sem_t));
-
-	  free (rec);
-	}
-    }
-  else
-    {
-      /* This is no valid semaphore.  */
-      result = -1;
       __set_errno (EINVAL);
+      return -1;
     }
 
-  /* Release the lock.  */
-  lll_unlock (__sem_mappings_lock, LLL_PRIVATE);
-
-  return result;
+  return 0;
 }
diff --git a/sysdeps/pthread/sem_open.c b/sysdeps/pthread/sem_open.c
index d666414f32..028d76a685 100644
--- a/sysdeps/pthread/sem_open.c
+++ b/sysdeps/pthread/sem_open.c
@@ -16,127 +16,17 @@ 
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
 #include <fcntl.h>
-#include <pthread.h>
-#include <search.h>
 #include <semaphore.h>
 #include <stdarg.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
 #include <unistd.h>
 #include <sys/mman.h>
-#include <sys/stat.h>
 #include "semaphoreP.h"
 #include <shm-directory.h>
+#include <sem_routines.h>
 #include <futex-internal.h>
 #include <libc-lock.h>
 
-/* Comparison function for search of existing mapping.  */
-int
-attribute_hidden
-__sem_search (const void *a, const void *b)
-{
-  const struct inuse_sem *as = (const struct inuse_sem *) a;
-  const struct inuse_sem *bs = (const struct inuse_sem *) b;
-
-  if (as->ino != bs->ino)
-    /* Cannot return the difference the type is larger than int.  */
-    return as->ino < bs->ino ? -1 : (as->ino == bs->ino ? 0 : 1);
-
-  if (as->dev != bs->dev)
-    /* Cannot return the difference the type is larger than int.  */
-    return as->dev < bs->dev ? -1 : (as->dev == bs->dev ? 0 : 1);
-
-  return strcmp (as->name, bs->name);
-}
-
-
-/* The search tree for existing mappings.  */
-void *__sem_mappings attribute_hidden;
-
-/* Lock to protect the search tree.  */
-int __sem_mappings_lock attribute_hidden = LLL_LOCK_INITIALIZER;
-
-
-/* Search for existing mapping and if possible add the one provided.  */
-static sem_t *
-check_add_mapping (const char *name, int fd, sem_t *existing)
-{
-  size_t namelen = strlen (name);
-  sem_t *result = SEM_FAILED;
-
-  /* Get the information about the file.  */
-  struct stat64 st;
-  if (__fstat64 (fd, &st) == 0)
-    {
-      /* Get the lock.  */
-      lll_lock (__sem_mappings_lock, LLL_PRIVATE);
-
-      /* Search for an existing mapping given the information we have.  */
-      struct inuse_sem *fake;
-      fake = (struct inuse_sem *) alloca (sizeof (*fake) + namelen);
-      memcpy (fake->name, name, namelen);
-      fake->dev = st.st_dev;
-      fake->ino = st.st_ino;
-
-      struct inuse_sem **foundp = __tfind (fake, &__sem_mappings,
-					   __sem_search);
-      if (foundp != NULL)
-	{
-	  /* There is already a mapping.  Use it.  */
-	  result = (*foundp)->sem;
-	  ++(*foundp)->refcnt;
-	}
-      else
-	{
-	  /* We haven't found a mapping.  Install ione.  */
-	  struct inuse_sem *newp;
-
-	  newp = (struct inuse_sem *) malloc (sizeof (*newp) + namelen);
-	  if (newp != NULL)
-	    {
-	      /* If the caller hasn't provided any map it now.  */
-	      if (existing == SEM_FAILED)
-		existing = (sem_t *) mmap (NULL, sizeof (sem_t),
-					   PROT_READ | PROT_WRITE, MAP_SHARED,
-					   fd, 0);
-
-	      newp->dev = st.st_dev;
-	      newp->ino = st.st_ino;
-	      newp->refcnt = 1;
-	      newp->sem = existing;
-	      memcpy (newp->name, name, namelen);
-
-	      /* Insert the new value.  */
-	      if (existing != MAP_FAILED
-		  && __tsearch (newp, &__sem_mappings, __sem_search) != NULL)
-		/* Successful.  */
-		result = existing;
-	      else
-		/* Something went wrong while inserting the new
-		   value.  We fail completely.  */
-		free (newp);
-	    }
-	}
-
-      /* Release the lock.  */
-      lll_unlock (__sem_mappings_lock, LLL_PRIVATE);
-    }
-
-  if (result != existing && existing != SEM_FAILED && existing != MAP_FAILED)
-    {
-      /* Do not disturb errno.  */
-      int save = errno;
-      munmap (existing, sizeof (sem_t));
-      errno = save;
-    }
-
-  return result;
-}
-
-
 sem_t *
 sem_open (const char *name, int oflag, ...)
 {
@@ -183,7 +73,7 @@  sem_open (const char *name, int oflag, ...)
       else
 	/* Check whether we already have this semaphore mapped and
 	   create one if necessary.  */
-	result = check_add_mapping (name, fd, SEM_FAILED);
+	result = __sem_check_add_mapping (name, fd, SEM_FAILED);
     }
   else
     {
@@ -294,7 +184,7 @@  sem_open (const char *name, int oflag, ...)
 	    /* Insert the mapping into the search tree.  This also
 	       determines whether another thread sneaked by and already
 	       added such a mapping despite the fact that we created it.  */
-	    result = check_add_mapping (name, fd, result);
+	    result = __sem_check_add_mapping (name, fd, result);
 	}
 
       /* Now remove the temporary name.  This should never fail.  If
diff --git a/sysdeps/pthread/sem_routines.c b/sysdeps/pthread/sem_routines.c
new file mode 100644
index 0000000000..78d9364ebd
--- /dev/null
+++ b/sysdeps/pthread/sem_routines.c
@@ -0,0 +1,187 @@ 
+/* Helper code for POSIX semaphore implementation.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <search.h>
+#include <semaphoreP.h>
+#include <sys/mman.h>
+#include <sem_routines.h>
+
+/* Keeping track of currently used mappings.  */
+struct inuse_sem
+{
+  dev_t dev;
+  ino_t ino;
+  int refcnt;
+  sem_t *sem;
+  char name[];
+};
+
+/* Comparison function for search of existing mapping.  */
+static int
+sem_search (const void *a, const void *b)
+{
+  const struct inuse_sem *as = (const struct inuse_sem *) a;
+  const struct inuse_sem *bs = (const struct inuse_sem *) b;
+
+  if (as->ino != bs->ino)
+    /* Cannot return the difference the type is larger than int.  */
+    return as->ino < bs->ino ? -1 : (as->ino == bs->ino ? 0 : 1);
+
+  if (as->dev != bs->dev)
+    /* Cannot return the difference the type is larger than int.  */
+    return as->dev < bs->dev ? -1 : (as->dev == bs->dev ? 0 : 1);
+
+  return strcmp (as->name, bs->name);
+}
+
+/* The search tree for existing mappings.  */
+static void *sem_mappings;
+
+/* Lock to protect the search tree.  */
+static int sem_mappings_lock = LLL_LOCK_INITIALIZER;
+
+
+/* Search for existing mapping and if possible add the one provided.  */
+sem_t *
+__sem_check_add_mapping (const char *name, int fd, sem_t *existing)
+{
+  size_t namelen = strlen (name);
+  sem_t *result = SEM_FAILED;
+
+  /* Get the information about the file.  */
+  struct stat64 st;
+  if (__fstat64 (fd, &st) == 0)
+    {
+      /* Get the lock.  */
+      lll_lock (sem_mappings_lock, LLL_PRIVATE);
+
+      /* Search for an existing mapping given the information we have.  */
+      struct inuse_sem *fake;
+      fake = (struct inuse_sem *) alloca (sizeof (*fake) + namelen);
+      memcpy (fake->name, name, namelen);
+      fake->dev = st.st_dev;
+      fake->ino = st.st_ino;
+
+      struct inuse_sem **foundp = __tfind (fake, &sem_mappings, sem_search);
+      if (foundp != NULL)
+	{
+	  /* There is already a mapping.  Use it.  */
+	  result = (*foundp)->sem;
+	  ++(*foundp)->refcnt;
+	}
+      else
+	{
+	  /* We haven't found a mapping.  Install ione.  */
+	  struct inuse_sem *newp;
+
+	  newp = (struct inuse_sem *) malloc (sizeof (*newp) + namelen);
+	  if (newp != NULL)
+	    {
+	      /* If the caller hasn't provided any map it now.  */
+	      if (existing == SEM_FAILED)
+		existing = (sem_t *) mmap (NULL, sizeof (sem_t),
+					   PROT_READ | PROT_WRITE, MAP_SHARED,
+					   fd, 0);
+
+	      newp->dev = st.st_dev;
+	      newp->ino = st.st_ino;
+	      newp->refcnt = 1;
+	      newp->sem = existing;
+	      memcpy (newp->name, name, namelen);
+
+	      /* Insert the new value.  */
+	      if (existing != MAP_FAILED
+		  && __tsearch (newp, &sem_mappings, sem_search) != NULL)
+		/* Successful.  */
+		result = existing;
+	      else
+		/* Something went wrong while inserting the new
+		   value.  We fail completely.  */
+		free (newp);
+	    }
+	}
+
+      /* Release the lock.  */
+      lll_unlock (sem_mappings_lock, LLL_PRIVATE);
+    }
+
+  if (result != existing && existing != SEM_FAILED && existing != MAP_FAILED)
+    {
+      /* Do not disturb errno.  */
+      int save = errno;
+      munmap (existing, sizeof (sem_t));
+      errno = save;
+    }
+
+  return result;
+}
+
+struct walk_closure
+{
+  sem_t *the_sem;
+  struct inuse_sem *rec;
+};
+
+static void
+walker (const void *inodep, VISIT which, void *closure0)
+{
+  struct walk_closure *closure = closure0;
+  struct inuse_sem *nodep = *(struct inuse_sem **) inodep;
+
+  if (nodep->sem == closure->the_sem)
+    closure->rec = nodep;
+}
+
+bool
+__sem_remove_mapping (sem_t *sem)
+{
+  bool ret = true;
+
+  /* Get the lock.  */
+  lll_lock (sem_mappings_lock, LLL_PRIVATE);
+
+  /* Locate the entry for the mapping the caller provided.  */
+  struct inuse_sem *rec;
+  {
+    struct walk_closure closure = { .the_sem = sem, .rec = NULL };
+    __twalk_r (sem_mappings, walker, &closure);
+    rec = closure.rec;
+  }
+  if (rec != NULL)
+    {
+      /* Check the reference counter.  If it is going to be zero, free
+	 all the resources.  */
+      if (--rec->refcnt == 0)
+	{
+	  /* Remove the record from the tree.  */
+	  __tdelete (rec, &sem_mappings, sem_search);
+
+	  if (munmap (rec->sem, sizeof (sem_t)) == -1)
+	    ret = false;
+
+	  free (rec);
+	}
+    }
+  else
+    ret = false;
+
+  /* Release the lock.  */
+  lll_unlock (sem_mappings_lock, LLL_PRIVATE);
+
+  return ret;
+}
diff --git a/sysdeps/pthread/sem_routines.h b/sysdeps/pthread/sem_routines.h
new file mode 100644
index 0000000000..25d3e880ea
--- /dev/null
+++ b/sysdeps/pthread/sem_routines.h
@@ -0,0 +1,27 @@ 
+/* Helper code for POSIX semaphore implementation.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   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
+   <https://www.gnu.org/licenses/>.  */
+
+#ifndef _SEM_ROUTINES_H
+#define _SEM_ROUTINES_H
+
+sem_t * __sem_check_add_mapping (const char *name, int fd, sem_t *existing)
+  attribute_hidden;
+
+bool __sem_remove_mapping (sem_t *sem) attribute_hidden;
+
+#endif