diff mbox

[4/5] nptl: Fix sem_wait and sem_timedwait cancellation

Message ID 1471876053-780-4-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show

Commit Message

Adhemerval Zanella Aug. 22, 2016, 2:27 p.m. UTC 
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(-)

-- 
2.7.4

Comments

Adhemerval Zanella Sept. 14, 2016, 8:50 p.m. UTC | #1 
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.

>
diff mbox

Patch

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