[2/2] pthread_once: Use futex wrappers with error checking.
diff mbox

Message ID 1417804623.22797.106.camel@triegel.csb
State New
Headers show

Commit Message

Torvald Riegel Dec. 5, 2014, 6:37 p.m. UTC
On Thu, 2014-12-04 at 16:36 -0800, Roland McGrath wrote:
> > -	      /* Same generation, some other thread was faster. Wait.  */
> > -	      lll_futex_wait (once_control, newval, LLL_PRIVATE);
> > +	      /* Same generation, some other thread was faster. Wait and
> > +		 retry.  Ignore the return value because all possible
> > +		 values (0, -EWOULDBLOCK, -EINTR) need to be handled the
> > +		 same.  */
> 
> Two spaces between sentences, even when it was wrong before.
> Always mention EAGAIN rather than EWOULDBLOCK.
> 
> Use ignore_value rather than only a comment, to be more explicit about a
> correct case of ignoring errors.  Then we can use warn_unused_result on all
> the new inlines, and turn up cases where failing to check for errors is
> dubious (we have many today).
> 
> Aside from those nits, this change is fine and good once we've settled on
> the new internal API details.

Updated patch attached.  If there's no objection, and once we have
agreed on a Patch 1/2 that can be committed, I will commit this one as
well.

Comments

Roland McGrath Dec. 10, 2014, 12:02 a.m. UTC | #1
> -  lll_futex_wake (once_control, INT_MAX, LLL_PRIVATE);
> +  futex_wake ((unsigned int *)once_control, INT_MAX, FUTEX_PRIVATE);

Always put a space after a cast.

> +	      ignore_value (futex_wait ((unsigned int *)once_control,
> +					(unsigned)newval, FUTEX_PRIVATE));

Ditto, and always 'unsigned int', never 'unsigned'.

> -      lll_futex_wake (once_control, INT_MAX, LLL_PRIVATE);
> +      futex_wake ((unsigned int *)once_control, INT_MAX, FUTEX_PRIVATE);

Space here too.

Patch
diff mbox

commit 5858b0ec69ff188310c049ec27b102303c38b7b3
Author: Torvald Riegel <triegel@redhat.com>
Date:   Thu Dec 4 14:29:27 2014 +0100

    pthread_once: Use futex wrappers with error checking.

diff --git a/nptl/pthread_once.c b/nptl/pthread_once.c
index 9bb82e9..b5d3963 100644
--- a/nptl/pthread_once.c
+++ b/nptl/pthread_once.c
@@ -17,8 +17,9 @@ 
    <http://www.gnu.org/licenses/>.  */
 
 #include "pthreadP.h"
-#include <lowlevellock.h>
+#include <futex-internal.h>
 #include <atomic.h>
+#include <libc-internal.h>
 
 
 unsigned long int __fork_generation attribute_hidden;
@@ -35,7 +36,7 @@  clear_once_control (void *arg)
      get interrupted (see __pthread_once), so all we need to relay to other
      threads is the state being reset again.  */
   atomic_store_relaxed (once_control, 0);
-  lll_futex_wake (once_control, INT_MAX, LLL_PRIVATE);
+  futex_wake ((unsigned int *)once_control, INT_MAX, FUTEX_PRIVATE);
 }
 
 
@@ -100,8 +101,11 @@  __pthread_once_slow (pthread_once_t *once_control, void (*init_routine) (void))
 	     is set and __PTHREAD_ONCE_DONE is not.  */
 	  if (val == newval)
 	    {
-	      /* Same generation, some other thread was faster. Wait.  */
-	      lll_futex_wait (once_control, newval, LLL_PRIVATE);
+	      /* Same generation, some other thread was faster.  Wait and
+		 retry.  Ignore the return value because all possible
+		 values (0, EAGAIN, EINTR) need to be handled the same.  */
+	      ignore_value (futex_wait ((unsigned int *)once_control,
+					(unsigned)newval, FUTEX_PRIVATE));
 	      continue;
 	    }
 	}
@@ -122,7 +126,7 @@  __pthread_once_slow (pthread_once_t *once_control, void (*init_routine) (void))
       atomic_store_release (once_control, __PTHREAD_ONCE_DONE);
 
       /* Wake up all other threads.  */
-      lll_futex_wake (once_control, INT_MAX, LLL_PRIVATE);
+      futex_wake ((unsigned int *)once_control, INT_MAX, FUTEX_PRIVATE);
       break;
     }