* [PATCH 2/5] nptl: Set sem_open as a non cancellation point (BZ #15765)
2016-08-22 14:27 [PATCH 1/5] Consolidate sem_open implementations Adhemerval Zanella
2016-08-22 14:27 ` [PATCH 4/5] nptl: Fix sem_wait and sem_timedwait cancellation Adhemerval Zanella
@ 2016-08-22 14:27 ` Adhemerval Zanella
2016-09-05 17:03 ` Torvald Riegel
2016-08-22 14:27 ` [PATCH 5/5] rt: Set shm_open as a non cancellation point (BZ #18243) Adhemerval Zanella
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Adhemerval Zanella @ 2016-08-22 14:27 UTC (permalink / raw)
To: libc-alpha
This patch changes sem_open to not act as a cancellation point.
Cancellation is disable at start and reenable in function exit.
It fixes BZ #15765.
Tested on x86_64 and i686.
[BZ #15765]
* nptl/Makefile (tests): Add tst-sem16.
* nptl/tst-sem16.c: New file.
* nptl/sem_open.c (sem_open): Disable asynchronous cancellation.
---
nptl/Makefile | 2 +-
nptl/sem_open.c | 25 ++++++++--
nptl/tst-sem16.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 162 insertions(+), 5 deletions(-)
create mode 100644 nptl/tst-sem16.c
diff --git a/nptl/Makefile b/nptl/Makefile
index 2ddcd2b..7c0e082 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -241,7 +241,7 @@ tests = tst-typesizes \
tst-key1 tst-key2 tst-key3 tst-key4 \
tst-sem1 tst-sem2 tst-sem3 tst-sem4 tst-sem5 tst-sem6 tst-sem7 \
tst-sem8 tst-sem9 tst-sem10 tst-sem11 tst-sem12 tst-sem13 tst-sem14 \
- tst-sem15 \
+ tst-sem15 tst-sem16 \
tst-barrier1 tst-barrier2 tst-barrier3 tst-barrier4 tst-barrier5 \
tst-align tst-align3 \
tst-basic1 tst-basic2 tst-basic3 tst-basic4 tst-basic5 tst-basic6 \
diff --git a/nptl/sem_open.c b/nptl/sem_open.c
index 974cff9..5a04df7 100644
--- a/nptl/sem_open.c
+++ b/nptl/sem_open.c
@@ -31,7 +31,7 @@
#include "semaphoreP.h"
#include <shm-directory.h>
#include <futex-internal.h>
-
+#include <libc-lock.h>
/* Comparison function for search of existing mapping. */
int
@@ -153,6 +153,13 @@ sem_open (const char *name, int oflag, ...)
/* Create the name of the final file in local variable SHM_NAME. */
SHM_GET_NAME (EINVAL, SEM_FAILED, SEM_SHM_PREFIX);
+ /* Disable asynchronous cancellation. */
+#ifdef __libc_ptf_call
+ int state;
+ __libc_ptf_call (__pthread_setcancelstate,
+ (PTHREAD_CANCEL_DISABLE, &state), 0);
+#endif
+
/* If the semaphore object has to exist simply open it. */
if ((oflag & O_CREAT) == 0 || (oflag & O_EXCL) == 0)
{
@@ -193,7 +200,8 @@ sem_open (const char *name, int oflag, ...)
if (value > SEM_VALUE_MAX)
{
__set_errno (EINVAL);
- return SEM_FAILED;
+ result = SEM_FAILED;
+ goto out;
}
/* Create the initial file content. */
@@ -232,7 +240,10 @@ sem_open (const char *name, int oflag, ...)
mode cannot later be set since then we cannot apply the
file create mask. */
if (__mktemp (tmpfname) == NULL)
- return SEM_FAILED;
+ {
+ result = SEM_FAILED;
+ goto out;
+ }
/* Open the file. Make sure we do not overwrite anything. */
fd = __libc_open (tmpfname, O_RDWR | O_CREAT | O_EXCL, mode);
@@ -246,7 +257,8 @@ sem_open (const char *name, int oflag, ...)
__set_errno (EAGAIN);
}
- return SEM_FAILED;
+ result = SEM_FAILED;
+ goto out;
}
/* We got a file. */
@@ -307,5 +319,10 @@ sem_open (const char *name, int oflag, ...)
errno = save;
}
+out:
+#ifdef __libc_ptf_call
+ __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);
+#endif
+
return result;
}
diff --git a/nptl/tst-sem16.c b/nptl/tst-sem16.c
new file mode 100644
index 0000000..f99571c
--- /dev/null
+++ b/nptl/tst-sem16.c
@@ -0,0 +1,136 @@
+/* Test for sem_open cancellation handling: BZ #15765.
+ Copyright (C) 2016 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
+ <http://www.gnu.org/licenses/>. */
+
+#include <pthread.h>
+#include <sys/mman.h>
+#include <semaphore.h>
+#include <stdio.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <errno.h>
+#include <stdlib.h>
+
+static sem_t sem; /* Use to sync with thread start. */
+volatile int thread_ret;
+static const char pipe_name[] = "/glibc-tst-sem16";
+
+static void
+remove_sem (int status, void *arg)
+{
+ sem_unlink (arg);
+}
+
+static void *
+tf (void *arg)
+{
+ thread_ret = 0;
+
+ pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, 0);
+
+ if (sem_wait (&sem) != 0)
+ {
+ printf ("error: sem_wait failed: %m");
+ thread_ret = 1;
+ return NULL;
+ }
+
+ if (pthread_setcancelstate (PTHREAD_CANCEL_ENABLE, 0) != 0)
+ {
+ printf ("error: pthread_setcancelstate failed: %m");
+ thread_ret = 1;
+ return NULL;
+ }
+
+ /* Neither sem_unlink or sem_open should act on thread cancellation. */
+ sem_unlink (pipe_name);
+ on_exit (remove_sem, (void *) pipe_name);
+
+ sem_t *s = sem_open (pipe_name, O_CREAT, 0600, 1);
+ if (s == SEM_FAILED)
+ {
+ if (errno == ENOSYS || errno == EACCES)
+ thread_ret = 77;
+ else
+ thread_ret = 1;
+ return NULL;
+ }
+
+ if (pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, 0) != 0)
+ {
+ printf ("error: pthread_setcancelstate failed: %m");
+ thread_ret = 1;
+ return NULL;
+ }
+
+ if (sem_close (s) != 0)
+ {
+ printf ("error: sem_close failed: %m");
+ thread_ret = 1;
+ return NULL;
+ }
+
+ return NULL;
+}
+
+static int
+do_test (void)
+{
+ pthread_t td;
+
+ if (sem_init (&sem, 0, 0))
+ {
+ printf ("error: sem_init failed: %m\n");
+ return 1;
+ }
+
+ if (pthread_create (&td, NULL, tf, NULL) != 0)
+ {
+ printf ("error: pthread_create failed: %m\n");
+ return 1;
+ }
+
+ if (pthread_cancel (td) != 0)
+ {
+ printf ("error: pthread_cancel failed: %m\n");
+ return 1;
+ }
+
+ if (sem_post (&sem) != 0)
+ {
+ printf ("error: sem_post failed: %m\n");
+ return 1;
+ }
+
+ void *r;
+ if (pthread_join (td, &r) != 0)
+ {
+ printf ("error: pthread_join failed: %m\n");
+ return 1;
+ }
+
+ if (r == PTHREAD_CANCELED)
+ {
+ puts ("error: pthread_join returned PTHREAD_CANCELED");
+ return 1;
+ }
+
+ return thread_ret;
+}
+
+#define TEST_FUNCTION do_test ()
+#include <test-skeleton.c>
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 5/5] rt: Set shm_open as a non cancellation point (BZ #18243)
2016-08-22 14:27 [PATCH 1/5] Consolidate sem_open implementations Adhemerval Zanella
2016-08-22 14:27 ` [PATCH 4/5] nptl: Fix sem_wait and sem_timedwait cancellation Adhemerval Zanella
2016-08-22 14:27 ` [PATCH 2/5] nptl: Set sem_open as a non cancellation point (BZ #15765) Adhemerval Zanella
@ 2016-08-22 14:27 ` Adhemerval Zanella
2016-09-05 18:08 ` Torvald Riegel
2016-08-22 14:27 ` [PATCH 3/5] Remove sparc sem_wait Adhemerval Zanella
2016-09-05 17:21 ` [PATCH 1/5] Consolidate sem_open implementations Torvald Riegel
4 siblings, 1 reply; 13+ messages in thread
From: Adhemerval Zanella @ 2016-08-22 14:27 UTC (permalink / raw)
To: libc-alpha
This patch changes shm_open to not act as a cancellation point.
Cancellation is disable at start and reenable in function exit.
It fixes BZ #18243.
Tested on x86_64 and i686.
[BZ #18243]
* rt/Makefile (test): Add tst-shm-cancel.
* rt/tst-shm-cancel.c: New file.
* sysdeps/posix/shm_open.c: Disable asynchronous cancellation.
---
rt/Makefile | 2 +-
rt/tst-shm-cancel.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++
sysdeps/posix/shm_open.c | 7 +++
3 files changed, 143 insertions(+), 1 deletion(-)
create mode 100644 rt/tst-shm-cancel.c
diff --git a/rt/Makefile b/rt/Makefile
index cfa6837..7593b11 100644
--- a/rt/Makefile
+++ b/rt/Makefile
@@ -53,7 +53,7 @@ tests := tst-shm tst-clock tst-clock_nanosleep tst-timer tst-timer2 \
tst-timer3 tst-timer4 tst-timer5 \
tst-cpuclock1 tst-cpuclock2 \
tst-cputimer1 tst-cputimer2 tst-cputimer3 \
- tst-clock2
+ tst-clock2 tst-shm-cancel
extra-libs := librt
extra-libs-others := $(extra-libs)
diff --git a/rt/tst-shm-cancel.c b/rt/tst-shm-cancel.c
new file mode 100644
index 0000000..de70c1a
--- /dev/null
+++ b/rt/tst-shm-cancel.c
@@ -0,0 +1,135 @@
+/* Test for shm_open cancellation handling: BZ #18243.
+ Copyright (C) 2016 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
+ <http://www.gnu.org/licenses/>. */
+
+#include <pthread.h>
+#include <sys/mman.h>
+#include <semaphore.h>
+#include <stdio.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <errno.h>
+#include <stdlib.h>
+
+static sem_t sem; /* Use to sync with thread start. */
+volatile int thread_ret;
+static const char shm_name[] = "/glibc-shm_open-cancel";
+
+static void
+remove_shm (int status, void *arg)
+{
+ shm_unlink (shm_name);
+}
+
+static void *
+tf (void *arg)
+{
+ thread_ret = 0;
+
+ pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, 0);
+
+ if (sem_wait (&sem) != 0)
+ {
+ printf ("error: sem_wait failed: %m");
+ thread_ret = 1;
+ return NULL;
+ }
+
+ if (pthread_setcancelstate (PTHREAD_CANCEL_ENABLE, 0) != 0)
+ {
+ printf ("error: pthread_setcancelstate failed: %m");
+ thread_ret = 1;
+ return NULL;
+ }
+
+ /* Neither sem_unlink or sem_open should act on thread cancellation. */
+ shm_unlink (shm_name);
+ on_exit (remove_shm, NULL);
+
+ int fd = shm_open (shm_name, O_CREAT, 0600);
+ if (fd == -1)
+ {
+ if (errno == ENOSYS || errno == EACCES)
+ thread_ret = 77;
+ else
+ thread_ret = 1;
+ return NULL;
+ }
+
+ if (pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, 0) != 0)
+ {
+ printf ("error: pthread_setcancelstate failed: %m");
+ thread_ret = 1;
+ return NULL;
+ }
+
+ if (close (fd) != 0)
+ {
+ printf ("error: pthread_setcancelstate failed: %m");
+ thread_ret = 1;
+ }
+
+ return NULL;
+}
+
+static int
+do_test (void)
+{
+ pthread_t td;
+
+ if (sem_init (&sem, 0, 0))
+ {
+ printf ("error: sem_init failed: %m\n");
+ return 1;
+ }
+
+ if (pthread_create (&td, NULL, tf, NULL) != 0)
+ {
+ printf ("error: pthread_create failed: %m\n");
+ return 1;
+ }
+
+ if (pthread_cancel (td) != 0)
+ {
+ printf ("error: pthread_cancel failed: %m\n");
+ return 1;
+ }
+
+ if (sem_post (&sem) != 0)
+ {
+ printf ("error: sem_post failed: %m\n");
+ return 1;
+ }
+
+ void *r;
+ if (pthread_join (td, &r) != 0)
+ {
+ printf ("error: pthread_join failed: %m\n");
+ return 1;
+ }
+
+ if (r == PTHREAD_CANCELED)
+ {
+ puts ("error: pthread_join returned PTHREAD_CANCELED");
+ return 1;
+ }
+
+ return thread_ret;
+}
+
+#define TEST_FUNCTION do_test ()
+#include <test-skeleton.c>
diff --git a/sysdeps/posix/shm_open.c b/sysdeps/posix/shm_open.c
index f296162..0182e7b 100644
--- a/sysdeps/posix/shm_open.c
+++ b/sysdeps/posix/shm_open.c
@@ -40,6 +40,11 @@ shm_open (const char *name, int oflag, mode_t mode)
# ifdef O_CLOEXEC
oflag |= O_CLOEXEC;
# endif
+
+ /* Disable asynchronous cancellation. */
+ int state;
+ pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, &state);
+
int fd = open (shm_name, oflag, mode);
if (fd == -1 && __glibc_unlikely (errno == EISDIR))
/* It might be better to fold this error with EINVAL since
@@ -70,6 +75,8 @@ shm_open (const char *name, int oflag, mode_t mode)
}
# endif
+ pthread_setcancelstate (state, NULL);
+
return fd;
}
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/5] Consolidate sem_open implementations
@ 2016-08-22 14:27 Adhemerval Zanella
2016-08-22 14:27 ` [PATCH 4/5] nptl: Fix sem_wait and sem_timedwait cancellation Adhemerval Zanella
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Adhemerval Zanella @ 2016-08-22 14:27 UTC (permalink / raw)
To: libc-alpha
Current sparc32 sem_open and default one only differ on:
1. Default one contains a 'futex_supports_pshared' check.
2. sem.newsem.pad is initialized to zero.
This patch removes sparc32 and sparc32v9 sem_open arch specific
implementation and instead set sparc32 to use nptl default one.
Using 1. is fine since it should always evaluate 0 for Linux
(an optimized away by the compiler). Adding 2. to default
implementation should be ok since 'pad' field is used mainly
on sparc32 code.
I checked on i686 and checked a sparc32v9 build.
* nptl/sem_open.c (sem_open): Init pad value to 0.
* sysdeps/sparc/sparc32/sem_open.c: Remove file.
* sysdeps/sparc/sparc32/sparcv9/sem_open.c: Likewise.
---
nptl/sem_open.c | 1 +
sysdeps/sparc/sparc32/sem_open.c | 300 -------------------------------
sysdeps/sparc/sparc32/sparcv9/sem_open.c | 1 -
4 files changed, 7 insertions(+), 301 deletions(-)
delete mode 100644 sysdeps/sparc/sparc32/sem_open.c
delete mode 100644 sysdeps/sparc/sparc32/sparcv9/sem_open.c
diff --git a/nptl/sem_open.c b/nptl/sem_open.c
index 911b1f3..974cff9 100644
--- a/nptl/sem_open.c
+++ b/nptl/sem_open.c
@@ -207,6 +207,7 @@ sem_open (const char *name, int oflag, ...)
sem.newsem.data = value;
#else
sem.newsem.value = value << SEM_VALUE_SHIFT;
+ sem.newsem.pad = 0;
sem.newsem.nwaiters = 0;
#endif
/* This always is a shared semaphore. */
diff --git a/sysdeps/sparc/sparc32/sem_open.c b/sysdeps/sparc/sparc32/sem_open.c
deleted file mode 100644
index 57796bd..0000000
--- a/sysdeps/sparc/sparc32/sem_open.c
+++ /dev/null
@@ -1,300 +0,0 @@
-/* Copyright (C) 2002-2016 Free Software Foundation, Inc.
- This file is part of the GNU C Library.
- Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
-
- 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
- <http://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 <futex-internal.h>
-#include <shm-directory.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, size_t namelen, int fd, sem_t *existing)
-{
- sem_t *result = SEM_FAILED;
-
- /* Get the information about the file. */
- struct stat64 st;
- if (__fxstat64 (_STAT_VER, 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, ...)
-{
- int fd;
- sem_t *result;
-
- /* Create the name of the final file in local variable SHM_NAME. */
- SHM_GET_NAME (EINVAL, SEM_FAILED, SEM_SHM_PREFIX);
-
- /* If the semaphore object has to exist simply open it. */
- if ((oflag & O_CREAT) == 0 || (oflag & O_EXCL) == 0)
- {
- try_again:
- fd = __libc_open (shm_name,
- (oflag & ~(O_CREAT|O_ACCMODE)) | O_NOFOLLOW | O_RDWR);
-
- if (fd == -1)
- {
- /* If we are supposed to create the file try this next. */
- if ((oflag & O_CREAT) != 0 && errno == ENOENT)
- goto try_create;
-
- /* Return. errno is already set. */
- }
- else
- /* Check whether we already have this semaphore mapped and
- create one if necessary. */
- result = check_add_mapping (name, namelen, fd, SEM_FAILED);
- }
- else
- {
- /* We have to open a temporary file first since it must have the
- correct form before we can start using it. */
- char *tmpfname;
- mode_t mode;
- unsigned int value;
- va_list ap;
-
- try_create:
- va_start (ap, oflag);
-
- mode = va_arg (ap, mode_t);
- value = va_arg (ap, unsigned int);
-
- va_end (ap);
-
- if (value > SEM_VALUE_MAX)
- {
- __set_errno (EINVAL);
- return SEM_FAILED;
- }
-
- /* Create the initial file content. */
- union
- {
- sem_t initsem;
- struct new_sem newsem;
- } sem;
-
- sem.newsem.value = value << SEM_VALUE_SHIFT;
- sem.newsem.pad = 0;
- sem.newsem.nwaiters = 0;
-
- /* This always is a shared semaphore. */
- sem.newsem.private = FUTEX_SHARED;
-
- /* Initialize the remaining bytes as well. */
- memset ((char *) &sem.initsem + sizeof (struct new_sem), '\0',
- sizeof (sem_t) - sizeof (struct new_sem));
-
- tmpfname = __alloca (shm_dirlen + sizeof SEM_SHM_PREFIX + 6);
- char *xxxxxx = __mempcpy (tmpfname, shm_dir, shm_dirlen);
-
- int retries = 0;
-#define NRETRIES 50
- while (1)
- {
- /* Add the suffix for mktemp. */
- strcpy (xxxxxx, "XXXXXX");
-
- /* We really want to use mktemp here. We cannot use mkstemp
- since the file must be opened with a specific mode. The
- mode cannot later be set since then we cannot apply the
- file create mask. */
- if (__mktemp (tmpfname) == NULL)
- return SEM_FAILED;
-
- /* Open the file. Make sure we do not overwrite anything. */
- fd = __libc_open (tmpfname, O_RDWR | O_CREAT | O_EXCL, mode);
- if (fd == -1)
- {
- if (errno == EEXIST)
- {
- if (++retries < NRETRIES)
- continue;
-
- __set_errno (EAGAIN);
- }
-
- return SEM_FAILED;
- }
-
- /* We got a file. */
- break;
- }
-
- if (TEMP_FAILURE_RETRY (__libc_write (fd, &sem.initsem, sizeof (sem_t)))
- == sizeof (sem_t)
- /* Map the sem_t structure from the file. */
- && (result = (sem_t *) mmap (NULL, sizeof (sem_t),
- PROT_READ | PROT_WRITE, MAP_SHARED,
- fd, 0)) != MAP_FAILED)
- {
- /* Create the file. Don't overwrite an existing file. */
- if (link (tmpfname, shm_name) != 0)
- {
- /* Undo the mapping. */
- (void) munmap (result, sizeof (sem_t));
-
- /* Reinitialize 'result'. */
- result = SEM_FAILED;
-
- /* This failed. If O_EXCL is not set and the problem was
- that the file exists, try again. */
- if ((oflag & O_EXCL) == 0 && errno == EEXIST)
- {
- /* Remove the file. */
- (void) unlink (tmpfname);
-
- /* Close the file. */
- (void) __libc_close (fd);
-
- goto try_again;
- }
- }
- else
- /* 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, namelen, fd, result);
- }
-
- /* Now remove the temporary name. This should never fail. If
- it fails we leak a file name. Better fix the kernel. */
- (void) unlink (tmpfname);
- }
-
- /* Map the mmap error to the error we need. */
- if (MAP_FAILED != (void *) SEM_FAILED && result == MAP_FAILED)
- result = SEM_FAILED;
-
- /* We don't need the file descriptor anymore. */
- if (fd != -1)
- {
- /* Do not disturb errno. */
- int save = errno;
- __libc_close (fd);
- errno = save;
- }
-
- return result;
-}
diff --git a/sysdeps/sparc/sparc32/sparcv9/sem_open.c b/sysdeps/sparc/sparc32/sparcv9/sem_open.c
deleted file mode 100644
index bff2d2d..0000000
--- a/sysdeps/sparc/sparc32/sparcv9/sem_open.c
+++ /dev/null
@@ -1 +0,0 @@
-#include <nptl/sem_open.c>
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/5] Remove sparc sem_wait
2016-08-22 14:27 [PATCH 1/5] Consolidate sem_open implementations Adhemerval Zanella
` (2 preceding siblings ...)
2016-08-22 14:27 ` [PATCH 5/5] rt: Set shm_open as a non cancellation point (BZ #18243) Adhemerval Zanella
@ 2016-08-22 14:27 ` Adhemerval Zanella
2016-09-05 17:24 ` Torvald Riegel
2016-09-05 17:21 ` [PATCH 1/5] Consolidate sem_open implementations Torvald Riegel
4 siblings, 1 reply; 13+ messages in thread
From: Adhemerval Zanella @ 2016-08-22 14:27 UTC (permalink / raw)
To: libc-alpha
This patch removes the sparc32 sem_wait.c implementation since it is
identical to default nptl one. The sparcv9 is no longer required with
the removal.
Checked with a sparcv9 build.
* sysdeps/sparc/sparc32/sem_wait.c: Remove file.
* sysdeps/sparc/sparc32/sparcv9/sem_wait.c: Likewise.
---
sysdeps/sparc/sparc32/sem_wait.c | 93 --------------------------------
sysdeps/sparc/sparc32/sparcv9/sem_wait.c | 1 -
3 files changed, 3 insertions(+), 94 deletions(-)
delete mode 100644 sysdeps/sparc/sparc32/sem_wait.c
delete mode 100644 sysdeps/sparc/sparc32/sparcv9/sem_wait.c
diff --git a/sysdeps/sparc/sparc32/sem_wait.c b/sysdeps/sparc/sparc32/sem_wait.c
deleted file mode 100644
index 84b523a..0000000
--- a/sysdeps/sparc/sparc32/sem_wait.c
+++ /dev/null
@@ -1,93 +0,0 @@
-/* sem_wait -- wait on a semaphore. Generic futex-using version.
- Copyright (C) 2003-2016 Free Software Foundation, Inc.
- This file is part of the GNU C Library.
- Contributed by Paul Mackerras <paulus@au.ibm.com>, 2003.
-
- 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
- <http://www.gnu.org/licenses/>. */
-
-#include <lowlevellock.h> /* lll_futex* used by the old code. */
-#include "sem_waitcommon.c"
-
-int
-__new_sem_wait (sem_t *sem)
-{
- if (__new_sem_wait_fast ((struct new_sem *) sem, 0) == 0)
- return 0;
- else
- return __new_sem_wait_slow((struct new_sem *) sem, NULL);
-}
-versioned_symbol (libpthread, __new_sem_wait, sem_wait, GLIBC_2_1);
-
-#if SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_1)
-int
-attribute_compat_text_section
-__old_sem_wait (sem_t *sem)
-{
- int *futex = (int *) sem;
- int err;
-
- do
- {
- if (atomic_decrement_if_positive (futex) > 0)
- return 0;
-
- /* Enable asynchronous cancellation. Required by the standard. */
- int oldtype = __pthread_enable_asynccancel ();
-
- /* Always assume the semaphore is shared. */
- err = lll_futex_wait (futex, 0, LLL_SHARED);
-
- /* Disable asynchronous cancellation. */
- __pthread_disable_asynccancel (oldtype);
- }
- while (err == 0 || err == -EWOULDBLOCK);
-
- __set_errno (-err);
- return -1;
-}
-
-compat_symbol (libpthread, __old_sem_wait, sem_wait, GLIBC_2_0);
-#endif
-
-int
-__new_sem_trywait (sem_t *sem)
-{
- /* We must not fail spuriously, so require a definitive result even if this
- may lead to a long execution time. */
- if (__new_sem_wait_fast ((struct new_sem *) sem, 1) == 0)
- return 0;
- __set_errno (EAGAIN);
- return -1;
-}
-versioned_symbol (libpthread, __new_sem_trywait, sem_trywait, GLIBC_2_1);
-#if SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_1)
-int
-__old_sem_trywait (sem_t *sem)
-{
- int *futex = (int *) sem;
- int val;
-
- if (*futex > 0)
- {
- val = atomic_decrement_if_positive (futex);
- if (val > 0)
- return 0;
- }
-
- __set_errno (EAGAIN);
- return -1;
-}
-compat_symbol (libpthread, __old_sem_trywait, sem_trywait, GLIBC_2_0);
-#endif
diff --git a/sysdeps/sparc/sparc32/sparcv9/sem_wait.c b/sysdeps/sparc/sparc32/sparcv9/sem_wait.c
deleted file mode 100644
index bccdaed..0000000
--- a/sysdeps/sparc/sparc32/sparcv9/sem_wait.c
+++ /dev/null
@@ -1 +0,0 @@
-#include <nptl/sem_wait.c>
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/5] nptl: Fix sem_wait and sem_timedwait cancellation
2016-08-22 14:27 [PATCH 1/5] Consolidate sem_open implementations Adhemerval Zanella
@ 2016-08-22 14:27 ` Adhemerval Zanella
2016-09-05 18:07 ` Torvald Riegel
2016-08-22 14:27 ` [PATCH 2/5] nptl: Set sem_open as a non cancellation point (BZ #15765) Adhemerval Zanella
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Adhemerval Zanella @ 2016-08-22 14:27 UTC (permalink / raw)
To: libc-alpha
This patch fixes both sem_wait and sem_timedwait cancellation point for
uncontended case. In this scenario only atomics are involved and thus
the futex cancellable call is not issue and a pending cancellation signal
is not handled.
The fix is straighforward by calling pthread_testcancel is both function
start. Although it would be simpler to call CANCELLATION_P directly, I
decided to add an internal pthread_testcancel alias and use it to export
less internal implementation on such function. A possible change on
how pthread_testcancel is internally implemented would lead to either
continue to force use CANCELLATION_P or to adjust its every use.
GLIBC testcases do have tests for uncontended cases, test-cancel12
and test-cancel14.c, however both are flawed by adding another
cancellation point just after thread pthread_cleanup_pop:
47 static void *
48 tf (void *arg)
49 {
50 pthread_cleanup_push (cleanup, NULL);
51
52 int e = pthread_barrier_wait (&bar);
53 if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
54 {
55 puts ("tf: 1st barrier_wait failed");
56 exit (1);
57 }
58
59 /* This call should block and be cancelable. */
60 sem_wait (&sem);
61
62 pthread_cleanup_pop (0);
63
64 puts ("sem_wait returned");
65
66 return NULL;
67 }
So sem_{timed}wait does not act on cancellation, pthread_cleanup_pop executes
'cleanup' and then 'puts' acts on cancellation. Since pthread_cleanup_pop
removed the clean-up handler, it will ran only once and thus it won't accuse
an error to indicate sem_wait has not acted on the cancellation signal.
This patch also fixes this behavior by removing the cancellation point 'puts'.
It also adds some cleanup on all sem_{timed}wait cancel tests, mostly to avoid
calling 'exit' and some error formatting.
It partially fixes BZ #18243. Checked on x86_64.
[BZ #18243]
* nptl/pthreadP.h (__pthread_testcancel): Add prototype and hidden_proto.
* nptl/pthread_testcancel.c (pthread_cancel): Add internal aliais
definition.
* nptl/sem_timedwait.c (sem_timedwait): Add cancellation check for
uncontended case.
* nptl/sem_wait.c (__new_sem_wait): Likewise.
* nptl/tst-cancel12.c (ncall): New global variable.
(thread_ret): Likewise.
(clean): Remove cancellation points.
(tf): Fix check for uncontended case and remove exit calls.
(do_test): Likewise.
* nptl/tst-cancel13.c (ncall): New global variable.
(thread_ret): Likewise.
(clean): Remove cancellation points.
(tf): Fix check for uncontended case and remove exit calls.
(do_test): Likewise.
* nptl/tst-cancel14.c (ncall): New global variable.
(thread_ret): Likewise.
(clean): Remove cancellation points.
(tf): Fix check for uncontended case and remove exit calls.
(do_test): Likewise.
* nptl/tst-cancel15.c (ncall): New global variable.
(thread_ret): Likewise.
(clean): Remove cancellation points.
(tf): Fix check for uncontended case and remove exit calls.
(do_test): Likewise.
---
nptl/pthreadP.h | 2 ++
nptl/pthread_testcancel.c | 4 ++-
nptl/sem_timedwait.c | 3 +++
nptl/sem_wait.c | 3 +++
nptl/tst-cancel12.c | 64 +++++++++++++++++++++++------------------------
nptl/tst-cancel13.c | 49 ++++++++++++++++++++----------------
nptl/tst-cancel14.c | 43 ++++++++++++++++---------------
nptl/tst-cancel15.c | 53 +++++++++++++++++++++------------------
9 files changed, 148 insertions(+), 101 deletions(-)
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 4edc74b..6e0dd09 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -491,6 +491,7 @@ extern int __pthread_setcanceltype (int type, int *oldtype);
extern int __pthread_enable_asynccancel (void) attribute_hidden;
extern void __pthread_disable_asynccancel (int oldtype)
internal_function attribute_hidden;
+extern void __pthread_testcancel (void);
#if IS_IN (libpthread)
hidden_proto (__pthread_mutex_init)
@@ -505,6 +506,7 @@ hidden_proto (__pthread_getspecific)
hidden_proto (__pthread_setspecific)
hidden_proto (__pthread_once)
hidden_proto (__pthread_setcancelstate)
+hidden_proto (__pthread_testcancel)
#endif
extern int __pthread_cond_broadcast_2_0 (pthread_cond_2_0_t *cond);
diff --git a/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c
index 51be09f..fdef699 100644
--- a/nptl/pthread_testcancel.c
+++ b/nptl/pthread_testcancel.c
@@ -21,7 +21,9 @@
void
-pthread_testcancel (void)
+__pthread_testcancel (void)
{
CANCELLATION_P (THREAD_SELF);
}
+strong_alias (__pthread_testcancel, pthread_testcancel)
+hidden_def (__pthread_testcancel)
diff --git a/nptl/sem_timedwait.c b/nptl/sem_timedwait.c
index 1aab25a..8253021 100644
--- a/nptl/sem_timedwait.c
+++ b/nptl/sem_timedwait.c
@@ -30,6 +30,9 @@ sem_timedwait (sem_t *sem, const struct timespec *abstime)
return -1;
}
+ /* Check cancellation for uncontended case. */
+ __pthread_testcancel ();
+
if (__new_sem_wait_fast ((struct new_sem *) sem, 0) == 0)
return 0;
else
diff --git a/nptl/sem_wait.c b/nptl/sem_wait.c
index 84b523a..4928c7d 100644
--- a/nptl/sem_wait.c
+++ b/nptl/sem_wait.c
@@ -23,6 +23,9 @@
int
__new_sem_wait (sem_t *sem)
{
+ /* Check cancellation for uncontended case. */
+ __pthread_testcancel ();
+
if (__new_sem_wait_fast ((struct new_sem *) sem, 0) == 0)
return 0;
else
diff --git a/nptl/tst-cancel12.c b/nptl/tst-cancel12.c
index ac8f5a0..9d93ddb 100644
--- a/nptl/tst-cancel12.c
+++ b/nptl/tst-cancel12.c
@@ -1,4 +1,5 @@
-/* Copyright (C) 2003-2016 Free Software Foundation, Inc.
+/* Test sem_wait cancellation for uncontended case.
+ Copyright (C) 2003-2016 Free Software Foundation, Inc.
This file is part of the GNU C Library.
Contributed by Ulrich Drepper <drepper@redhat.com>, 2003.
@@ -23,104 +24,101 @@
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
-
+#include <stdbool.h>
static pthread_barrier_t bar;
static sem_t sem;
-
+static int ncall;
+static volatile int thread_ret;
static void
cleanup (void *arg)
{
- static int ncall;
-
if (++ncall != 1)
{
puts ("second call to cleanup");
- exit (1);
+ thread_ret = 1;
}
-
- printf ("cleanup call #%d\n", ncall);
}
-
static void *
tf (void *arg)
{
+ thread_ret = 0;
+ ncall = 0;
+
pthread_cleanup_push (cleanup, NULL);
int e = pthread_barrier_wait (&bar);
if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
{
- puts ("tf: 1st barrier_wait failed");
- exit (1);
+ puts ("error: tf: 1st barrier_wait failed");
+ thread_ret = 1;
+ return NULL;
}
- /* This call should block and be cancelable. */
sem_wait (&sem);
pthread_cleanup_pop (0);
- puts ("sem_wait returned");
-
return NULL;
}
-
static int
do_test (void)
{
pthread_t th;
+ thread_ret = 0;
+
if (pthread_barrier_init (&bar, NULL, 2) != 0)
{
- puts ("barrier_init failed");
- exit (1);
+ puts ("error: barrier_init failed");
+ return 1;
}
+ /* A value higher than 0 will check for uncontended pthread cancellation,
+ where the sem_wait operation will return immediatelly. */
if (sem_init (&sem, 0, 1) != 0)
{
- puts ("sem_init failed");
- exit (1);
+ puts ("error: sem_init failed");
+ return 1;
}
if (pthread_create (&th, NULL, tf, NULL) != 0)
{
- puts ("create failed");
- exit (1);
+ puts ("error: create failed");
+ return 1;
}
- /* Check whether cancellation is honored even before sem_wait does
- anything. */
if (pthread_cancel (th) != 0)
{
- puts ("1st cancel failed");
- exit (1);
+ puts ("error: pthread_cancel failed");
+ return 1;
}
int e = pthread_barrier_wait (&bar);
if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
{
- puts ("1st barrier_wait failed");
- exit (1);
+ puts ("error: 1st barrier_wait failed");
+ return 1;
}
void *r;
if (pthread_join (th, &r) != 0)
{
- puts ("join failed");
- exit (1);
+ puts ("error: pthread_join failed");
+ return 1;
}
if (r != PTHREAD_CANCELED)
{
- puts ("thread not canceled");
- exit (1);
+ puts ("error: thread not canceled");
+ return 1;
}
- return 0;
+ return thread_ret;
}
-
#define TEST_FUNCTION do_test ()
#include "../test-skeleton.c"
diff --git a/nptl/tst-cancel13.c b/nptl/tst-cancel13.c
index c84780d..7c7fd6b 100644
--- a/nptl/tst-cancel13.c
+++ b/nptl/tst-cancel13.c
@@ -1,4 +1,5 @@
-/* Copyright (C) 2003-2016 Free Software Foundation, Inc.
+/* Test sem_wait cancellation for contended case.
+ Copyright (C) 2003-2016 Free Software Foundation, Inc.
This file is part of the GNU C Library.
Contributed by Ulrich Drepper <drepper@redhat.com>, 2003.
@@ -27,6 +28,8 @@
static pthread_barrier_t bar;
static sem_t sem;
+static int ncall;
+static volatile int thread_ret;
static void
@@ -37,23 +40,24 @@ cleanup (void *arg)
if (++ncall != 1)
{
puts ("second call to cleanup");
- exit (1);
+ thread_ret = 1;
}
-
- printf ("cleanup call #%d\n", ncall);
}
-
static void *
tf (void *arg)
{
+ thread_ret = 0;
+ ncall = 0;
+
pthread_cleanup_push (cleanup, NULL);
int e = pthread_barrier_wait (&bar);
if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
{
- puts ("tf: 1st barrier_wait failed");
- exit (1);
+ puts ("error: tf: 1st barrier_wait failed");
+ thread_ret = 1;
+ return NULL;
}
/* This call should block and be cancelable. */
@@ -61,8 +65,6 @@ tf (void *arg)
pthread_cleanup_pop (0);
- puts ("sem_wait returned");
-
return NULL;
}
@@ -71,30 +73,33 @@ static int
do_test (void)
{
pthread_t th;
+ thread_ret = 0;
if (pthread_barrier_init (&bar, NULL, 2) != 0)
{
- puts ("barrier_init failed");
- exit (1);
+ puts ("error: barrier_init failed");
+ return 1;
}
+ /* A value equal to 0 will check for contended pthread cancellation,
+ where the sem_wait operation will block. */
if (sem_init (&sem, 0, 0) != 0)
{
- puts ("sem_init failed");
- exit (1);
+ puts ("error: sem_init failed");
+ return 1;
}
if (pthread_create (&th, NULL, tf, NULL) != 0)
{
- puts ("create failed");
- exit (1);
+ puts ("error: create failed");
+ return 1;
}
int e = pthread_barrier_wait (&bar);
if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
{
- puts ("1st barrier_wait failed");
- exit (1);
+ puts ("error: 1st barrier_wait failed");
+ return 1;
}
/* Give the child a chance to go to sleep in sem_wait. */
@@ -103,15 +108,15 @@ do_test (void)
/* Check whether cancellation is honored when waiting in sem_wait. */
if (pthread_cancel (th) != 0)
{
- puts ("1st cancel failed");
- exit (1);
+ puts ("error: 1st cancel failed");
+ return 1;
}
void *r;
if (pthread_join (th, &r) != 0)
{
- puts ("join failed");
- exit (1);
+ puts ("error: join failed");
+ return 1;
}
if (r != PTHREAD_CANCELED)
@@ -120,7 +125,7 @@ do_test (void)
exit (1);
}
- return 0;
+ return thread_ret;
}
diff --git a/nptl/tst-cancel14.c b/nptl/tst-cancel14.c
index 3810d2b..dbd0394 100644
--- a/nptl/tst-cancel14.c
+++ b/nptl/tst-cancel14.c
@@ -1,4 +1,5 @@
-/* Copyright (C) 2003-2016 Free Software Foundation, Inc.
+/* Test sem_timedwait cancellation for uncontended case.
+ Copyright (C) 2003-2016 Free Software Foundation, Inc.
This file is part of the GNU C Library.
Contributed by Ulrich Drepper <drepper@redhat.com>, 2003.
@@ -28,33 +29,35 @@
static pthread_barrier_t bar;
static sem_t sem;
+static int ncall;
+static volatile int thread_ret;
static void
cleanup (void *arg)
{
- static int ncall;
-
if (++ncall != 1)
{
puts ("second call to cleanup");
- exit (1);
+ thread_ret = 1;
}
-
- printf ("cleanup call #%d\n", ncall);
}
static void *
tf (void *arg)
{
+ thread_ret = 0;
+ ncall = 0;
+
pthread_cleanup_push (cleanup, NULL);
int e = pthread_barrier_wait (&bar);
if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
{
puts ("tf: 1st barrier_wait failed");
- exit (1);
+ thread_ret = 1;
+ return NULL;
}
struct timeval tv;
@@ -71,8 +74,6 @@ tf (void *arg)
pthread_cleanup_pop (0);
- puts ("sem_timedwait returned");
-
return NULL;
}
@@ -82,44 +83,46 @@ do_test (void)
{
pthread_t th;
+ thread_ret = 0;
+
if (pthread_barrier_init (&bar, NULL, 2) != 0)
{
- puts ("barrier_init failed");
- exit (1);
+ puts ("error: barrier_init failed");
+ return 1;
}
if (sem_init (&sem, 0, 1) != 0)
{
- puts ("sem_init failed");
- exit (1);
+ puts ("error: sem_init failed");
+ return 1;
}
if (pthread_create (&th, NULL, tf, NULL) != 0)
{
- puts ("create failed");
- exit (1);
+ puts ("error: create failed");
+ return 1;
}
/* Check whether cancellation is honored even before sem_timedwait does
anything. */
if (pthread_cancel (th) != 0)
{
- puts ("1st cancel failed");
- exit (1);
+ puts ("error: 1st cancel failed");
+ return 1;
}
int e = pthread_barrier_wait (&bar);
if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
{
puts ("1st barrier_wait failed");
- exit (1);
+ return 1;
}
void *r;
if (pthread_join (th, &r) != 0)
{
puts ("join failed");
- exit (1);
+ return 1;
}
if (r != PTHREAD_CANCELED)
@@ -128,7 +131,7 @@ do_test (void)
exit (1);
}
- return 0;
+ return thread_ret;
}
diff --git a/nptl/tst-cancel15.c b/nptl/tst-cancel15.c
index f413839..c86164e 100644
--- a/nptl/tst-cancel15.c
+++ b/nptl/tst-cancel15.c
@@ -1,4 +1,5 @@
-/* Copyright (C) 2003-2016 Free Software Foundation, Inc.
+/* Test sem_timedwait cancellation for contended case.
+ Copyright (C) 2003-2016 Free Software Foundation, Inc.
This file is part of the GNU C Library.
Contributed by Ulrich Drepper <drepper@redhat.com>, 2003.
@@ -28,26 +29,27 @@
static pthread_barrier_t bar;
static sem_t sem;
+static int ncall;
+static volatile int thread_ret;
static void
cleanup (void *arg)
{
- static int ncall;
-
if (++ncall != 1)
{
puts ("second call to cleanup");
- exit (1);
+ thread_ret = 1;
}
-
- printf ("cleanup call #%d\n", ncall);
}
static void *
tf (void *arg)
{
+ thread_ret = 0;
+ ncall = 0;
+
int e;
pthread_cleanup_push (cleanup, NULL);
@@ -55,8 +57,9 @@ tf (void *arg)
e = pthread_barrier_wait (&bar);
if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
{
- puts ("tf: 1st barrier_wait failed");
- exit (1);
+ puts ("error: tf: 1st barrier_wait failed");
+ thread_ret = 1;
+ return NULL;
}
struct timeval tv;
@@ -74,8 +77,6 @@ tf (void *arg)
pthread_cleanup_pop (0);
- printf ("sem_timedwait returned, e = %d, errno = %d\n", e, errno);
-
return NULL;
}
@@ -85,29 +86,31 @@ do_test (void)
{
pthread_t th;
+ thread_ret = 0;
+
if (pthread_barrier_init (&bar, NULL, 2) != 0)
{
- puts ("barrier_init failed");
- exit (1);
+ puts ("error: barrier_init failed");
+ return 1;
}
if (sem_init (&sem, 0, 0) != 0)
{
- puts ("sem_init failed");
- exit (1);
+ puts ("error: sem_init failed");
+ return 1;
}
if (pthread_create (&th, NULL, tf, NULL) != 0)
{
- puts ("create failed");
- exit (1);
+ puts ("error: create failed");
+ return 1;
}
int e = pthread_barrier_wait (&bar);
if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
{
- puts ("1st barrier_wait failed");
- exit (1);
+ puts ("error: 1st barrier_wait failed");
+ return 1;
}
/* Give the child a chance to go to sleep in sem_wait. */
@@ -116,24 +119,24 @@ do_test (void)
/* Check whether cancellation is honored when waiting in sem_timedwait. */
if (pthread_cancel (th) != 0)
{
- puts ("1st cancel failed");
- exit (1);
+ puts ("error: 1st cancel failed");
+ return 1;
}
void *r;
if (pthread_join (th, &r) != 0)
{
- puts ("join failed");
- exit (1);
+ puts ("error: join failed");
+ return 1;
}
if (r != PTHREAD_CANCELED)
{
- puts ("thread not canceled");
- exit (1);
+ puts ("error: thread not canceled");
+ return 1;
}
- return 0;
+ return thread_ret;
}
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/5] nptl: Set sem_open as a non cancellation point (BZ #15765)
2016-08-22 14:27 ` [PATCH 2/5] nptl: Set sem_open as a non cancellation point (BZ #15765) Adhemerval Zanella
@ 2016-09-05 17:03 ` Torvald Riegel
0 siblings, 0 replies; 13+ messages in thread
From: Torvald Riegel @ 2016-09-05 17:03 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
On Mon, 2016-08-22 at 11:27 -0300, Adhemerval Zanella wrote:
> This patch changes sem_open to not act as a cancellation point.
> Cancellation is disable at start and reenable in function exit.
> It fixes BZ #15765.
LGTM.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] Consolidate sem_open implementations
2016-08-22 14:27 [PATCH 1/5] Consolidate sem_open implementations Adhemerval Zanella
` (3 preceding siblings ...)
2016-08-22 14:27 ` [PATCH 3/5] Remove sparc sem_wait Adhemerval Zanella
@ 2016-09-05 17:21 ` Torvald Riegel
2016-09-14 21:39 ` Adhemerval Zanella
4 siblings, 1 reply; 13+ messages in thread
From: Torvald Riegel @ 2016-09-05 17:21 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
On Mon, 2016-08-22 at 11:27 -0300, Adhemerval Zanella wrote:
> Current sparc32 sem_open and default one only differ on:
>
> 1. Default one contains a 'futex_supports_pshared' check.
> 2. sem.newsem.pad is initialized to zero.
>
> This patch removes sparc32 and sparc32v9 sem_open arch specific
> implementation and instead set sparc32 to use nptl default one.
> Using 1. is fine since it should always evaluate 0 for Linux
> (an optimized away by the compiler). Adding 2. to default
> implementation should be ok since 'pad' field is used mainly
> on sparc32 code.
>
> I checked on i686 and checked a sparc32v9 build.
>
> * nptl/sem_open.c (sem_open): Init pad value to 0.
> * sysdeps/sparc/sparc32/sem_open.c: Remove file.
> * sysdeps/sparc/sparc32/sparcv9/sem_open.c: Likewise.
Can you do something similar for sem_init please?
> ---
> nptl/sem_open.c | 1 +
> sysdeps/sparc/sparc32/sem_open.c | 300 -------------------------------
> sysdeps/sparc/sparc32/sparcv9/sem_open.c | 1 -
> 4 files changed, 7 insertions(+), 301 deletions(-)
> delete mode 100644 sysdeps/sparc/sparc32/sem_open.c
> delete mode 100644 sysdeps/sparc/sparc32/sparcv9/sem_open.c
>
> diff --git a/nptl/sem_open.c b/nptl/sem_open.c
> index 911b1f3..974cff9 100644
> --- a/nptl/sem_open.c
> +++ b/nptl/sem_open.c
> @@ -207,6 +207,7 @@ sem_open (const char *name, int oflag, ...)
> sem.newsem.data = value;
> #else
> sem.newsem.value = value << SEM_VALUE_SHIFT;
> + sem.newsem.pad = 0;
> sem.newsem.nwaiters = 0;
> #endif
> /* This always is a shared semaphore. */
I think we should add a comment there, pointing to the use of .pad as a
mutex on pre-v9 sparc (same in sem_init when you consolidate this too).
Otherwise, this is OK.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/5] Remove sparc sem_wait
2016-08-22 14:27 ` [PATCH 3/5] Remove sparc sem_wait Adhemerval Zanella
@ 2016-09-05 17:24 ` Torvald Riegel
0 siblings, 0 replies; 13+ messages in thread
From: Torvald Riegel @ 2016-09-05 17:24 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
On Mon, 2016-08-22 at 11:27 -0300, Adhemerval Zanella wrote:
> This patch removes the sparc32 sem_wait.c implementation since it is
> identical to default nptl one. The sparcv9 is no longer required with
> the removal.
OK.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/5] nptl: Fix sem_wait and sem_timedwait cancellation
2016-08-22 14:27 ` [PATCH 4/5] nptl: Fix sem_wait and sem_timedwait cancellation Adhemerval Zanella
@ 2016-09-05 18:07 ` Torvald Riegel
2016-09-09 16:32 ` Torvald Riegel
0 siblings, 1 reply; 13+ messages in thread
From: Torvald Riegel @ 2016-09-05 18:07 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
On Mon, 2016-08-22 at 11:27 -0300, Adhemerval Zanella wrote:
> This patch fixes both sem_wait and sem_timedwait cancellation point for
> uncontended case. In this scenario only atomics are involved and thus
> the futex cancellable call is not issue and a pending cancellation signal
> is not handled.
I have added a comment on the BZ explaining why I think this is NOTABUG.
Essentially, if we don't need to suspend or block, there's no suspension
or blocking we need to be able to cancel. Adding a one-time check is
not reliable anyway because the cancellation request could be incoming
right after this check.
Also, if we don't want to make assumptions about timing, there is no way
to send a cancellation request reliably after sem_wait has reached it's
cancellation point because sem_wait doesn't communicate when it started
(or reached the cancellation point).
I don't see much value to allow a user to cancel something that would
never wait, because the user already is able to never call sem_wait at
all. Furthermore, relying on cancellation to take effect when sem_wait
would never wait would break if the user code called a sem_trywait right
before the sem_wait, which arguably would be quite inconsistent
semantics.
Another point to consider is that even if glibc's implementation added
bounded(!) spinning (without checking for cancellation) before
eventually using futexes with cancellation enabled, we would still
fulfill the cancellation requirements because we can argue that the
cancellation point just happens a little later.
> GLIBC testcases do have tests for uncontended cases, test-cancel12
> and test-cancel14.c, however both are flawed by adding another
> cancellation point just after thread pthread_cleanup_pop:
I think both tests should just be removed because they check for
behavior we do not need to implement. Sorry for not realizing this when
I updated the semaphore implementation.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 5/5] rt: Set shm_open as a non cancellation point (BZ #18243)
2016-08-22 14:27 ` [PATCH 5/5] rt: Set shm_open as a non cancellation point (BZ #18243) Adhemerval Zanella
@ 2016-09-05 18:08 ` Torvald Riegel
0 siblings, 0 replies; 13+ messages in thread
From: Torvald Riegel @ 2016-09-05 18:08 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
On Mon, 2016-08-22 at 11:27 -0300, Adhemerval Zanella wrote:
> This patch changes shm_open to not act as a cancellation point.
> Cancellation is disable at start and reenable in function exit.
> It fixes BZ #18243.
The patch looks good to me. If we agree that the sem_wait part of this
BZ is NOTABUG, then this patch indeed fixes the BZ.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/5] nptl: Fix sem_wait and sem_timedwait cancellation
2016-09-05 18:07 ` Torvald Riegel
@ 2016-09-09 16:32 ` Torvald Riegel
2016-09-14 20:51 ` Adhemerval Zanella
0 siblings, 1 reply; 13+ messages in thread
From: Torvald Riegel @ 2016-09-09 16:32 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
On Mon, 2016-09-05 at 20:07 +0200, Torvald Riegel wrote:
> On Mon, 2016-08-22 at 11:27 -0300, Adhemerval Zanella wrote:
> > This patch fixes both sem_wait and sem_timedwait cancellation point for
> > uncontended case. In this scenario only atomics are involved and thus
> > the futex cancellable call is not issue and a pending cancellation signal
> > is not handled.
>
> I have added a comment on the BZ explaining why I think this is NOTABUG.
I've looked at the POSIX rationale again, and it seems I was wrong in my
interpretation of what the POSIX spec requires. I now think that we
need to add the pthread_testcancel call or equivalent. The patch looks
okay, but please also add a comment to the added pthread_testcancel call
explaining why we need it. Something like this may be useful:
We need to check whether we need to act upon a cancellation request here
because POSIX specifies that cancellation points "shall occur" in
sem_wait and sem_timedwait, which also means that they need to check
this regardless whether they block or not (unlike "may occur"
functions). See the POSIX Rationale for this requirement: Section
"Thread Cancellation Overview" in
http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html
See http://austingroupbugs.net/view.php?id=1076 for thoughts on why this
may be a suboptimal design.
Thanks.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/5] nptl: Fix sem_wait and sem_timedwait cancellation
2016-09-09 16:32 ` Torvald Riegel
@ 2016-09-14 20:51 ` Adhemerval Zanella
0 siblings, 0 replies; 13+ messages in thread
From: Adhemerval Zanella @ 2016-09-14 20:51 UTC (permalink / raw)
To: Torvald Riegel; +Cc: libc-alpha
On 09/09/2016 13:32, Torvald Riegel wrote:
> On Mon, 2016-09-05 at 20:07 +0200, Torvald Riegel wrote:
>> On Mon, 2016-08-22 at 11:27 -0300, Adhemerval Zanella wrote:
>>> This patch fixes both sem_wait and sem_timedwait cancellation point for
>>> uncontended case. In this scenario only atomics are involved and thus
>>> the futex cancellable call is not issue and a pending cancellation signal
>>> is not handled.
>>
>> I have added a comment on the BZ explaining why I think this is NOTABUG.
>
> I've looked at the POSIX rationale again, and it seems I was wrong in my
> interpretation of what the POSIX spec requires. I now think that we
> need to add the pthread_testcancel call or equivalent. The patch looks
> okay, but please also add a comment to the added pthread_testcancel call
> explaining why we need it. Something like this may be useful:
>
> We need to check whether we need to act upon a cancellation request here
> because POSIX specifies that cancellation points "shall occur" in
> sem_wait and sem_timedwait, which also means that they need to check
> this regardless whether they block or not (unlike "may occur"
> functions). See the POSIX Rationale for this requirement: Section
> "Thread Cancellation Overview" in
> http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html
> See http://austingroupbugs.net/view.php?id=1076 for thoughts on why this
> may be a suboptimal design.
>
Thanks for check out this, my initial understanding followed your #4 comment
in bug report. I will add your comment in the patch.
>
> Thanks.
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] Consolidate sem_open implementations
2016-09-05 17:21 ` [PATCH 1/5] Consolidate sem_open implementations Torvald Riegel
@ 2016-09-14 21:39 ` Adhemerval Zanella
0 siblings, 0 replies; 13+ messages in thread
From: Adhemerval Zanella @ 2016-09-14 21:39 UTC (permalink / raw)
To: Torvald Riegel; +Cc: libc-alpha
On 05/09/2016 14:21, Torvald Riegel wrote:
> On Mon, 2016-08-22 at 11:27 -0300, Adhemerval Zanella wrote:
>> Current sparc32 sem_open and default one only differ on:
>>
>> 1. Default one contains a 'futex_supports_pshared' check.
>> 2. sem.newsem.pad is initialized to zero.
>>
>> This patch removes sparc32 and sparc32v9 sem_open arch specific
>> implementation and instead set sparc32 to use nptl default one.
>> Using 1. is fine since it should always evaluate 0 for Linux
>> (an optimized away by the compiler). Adding 2. to default
>> implementation should be ok since 'pad' field is used mainly
>> on sparc32 code.
>>
>> I checked on i686 and checked a sparc32v9 build.
>>
>> * nptl/sem_open.c (sem_open): Init pad value to 0.
>> * sysdeps/sparc/sparc32/sem_open.c: Remove file.
>> * sysdeps/sparc/sparc32/sparcv9/sem_open.c: Likewise.
>
> Can you do something similar for sem_init please?
I will send a consolidation patch to sem_init.
>
>> ---
>> nptl/sem_open.c | 1 +
>> sysdeps/sparc/sparc32/sem_open.c | 300 -------------------------------
>> sysdeps/sparc/sparc32/sparcv9/sem_open.c | 1 -
>> 4 files changed, 7 insertions(+), 301 deletions(-)
>> delete mode 100644 sysdeps/sparc/sparc32/sem_open.c
>> delete mode 100644 sysdeps/sparc/sparc32/sparcv9/sem_open.c
>>
>> diff --git a/nptl/sem_open.c b/nptl/sem_open.c
>> index 911b1f3..974cff9 100644
>> --- a/nptl/sem_open.c
>> +++ b/nptl/sem_open.c
>> @@ -207,6 +207,7 @@ sem_open (const char *name, int oflag, ...)
>> sem.newsem.data = value;
>> #else
>> sem.newsem.value = value << SEM_VALUE_SHIFT;
>> + sem.newsem.pad = 0;
>> sem.newsem.nwaiters = 0;
>> #endif
>> /* This always is a shared semaphore. */
>
> I think we should add a comment there, pointing to the use of .pad as a
> mutex on pre-v9 sparc (same in sem_init when you consolidate this too).
>
> Otherwise, this is OK.
>
I will add a comment.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-09-14 21:39 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-22 14:27 [PATCH 1/5] Consolidate sem_open implementations Adhemerval Zanella
2016-08-22 14:27 ` [PATCH 4/5] nptl: Fix sem_wait and sem_timedwait cancellation Adhemerval Zanella
2016-09-05 18:07 ` Torvald Riegel
2016-09-09 16:32 ` Torvald Riegel
2016-09-14 20:51 ` Adhemerval Zanella
2016-08-22 14:27 ` [PATCH 2/5] nptl: Set sem_open as a non cancellation point (BZ #15765) Adhemerval Zanella
2016-09-05 17:03 ` Torvald Riegel
2016-08-22 14:27 ` [PATCH 5/5] rt: Set shm_open as a non cancellation point (BZ #18243) Adhemerval Zanella
2016-09-05 18:08 ` Torvald Riegel
2016-08-22 14:27 ` [PATCH 3/5] Remove sparc sem_wait Adhemerval Zanella
2016-09-05 17:24 ` Torvald Riegel
2016-09-05 17:21 ` [PATCH 1/5] Consolidate sem_open implementations Torvald Riegel
2016-09-14 21:39 ` Adhemerval Zanella
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).