Re: [Patch][RFC] ARM NPTL lll_futex_wake_unlock() issue

[ryosei at sm dot sony dot co dot jp]

From: "Carlos O'Donell" <carlos@systemhalted.org>
Subject: Re: [Patch][RFC] ARM NPTL lll_futex_wake_unlock() issue
Date: Fri, 12 Sep 2008 08:04:39 -0400

> On Fri, Sep 12, 2008 at 4:15 AM,  <ryosei@sm.sony.co.jp> wrote:
> > Thank you.
> >
> > I have executed glibc testsuite on ARM.
> > No regression was found.
> 
> Please review the contribution checklist here:
> http://sources.redhat.com/glibc/wiki/Contribution%20checklist

I have reviewed the above checklist.
And I rewrite my mail as follows.



At first, I executed glibc testsuite on 
the following environment.

CPU: ARM11 MPCore
kernel: linux-2.6.23
binutils: binutils-2.17
gcc: gcc-4.1.2
glibc: glibc-2.7


And the following test failed because of Timed out.

 [test code]
   glibc-2.7/nptl/tst-cond20.c

   This code tests pthread_cond_wait(), pthread_cond_signal(),
   pthread_cond_broadcast(), and other functions.

 [test log]
GCONV_PATH=/mnt/sda1/cmplrs-build/build/glibc-2.7/iconvdata LC_ALL=C  
(snip)
glibc-2.7/nptl/tst-cond20  > /mnt/sda1/cmplrs-build/build/glibc-2.7/nptl/tst-cond20.out
Timed out: killed the child process
make[2]: [/mnt/sda1/cmplrs-build/build/glibc-2.7/nptl/tst-cond20.out] Error 1 (ignored)

I have found that this test sometimes become wait-forever
and then Timed out on the above test environment.



I have investigated and found one issue regarding
ARM NPTL lll_futex_wake_unlock(), which is called by pthread_cond_signal().

ARM NPTL lll_futex_wake_unlock() source code is as follows.
This function should return zero if success

[glibc-ports-2.7/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h]

(snip)

   117  /* Returns non-zero if error happened, zero if success.  */
   118  #define lll_futex_wake_unlock(futexp, nr_wake, nr_wake2, futexp2, private) \
   119    ({                                                                          \
   120      INTERNAL_SYSCALL_DECL (__err);                                            \
   121      long int __ret;                                                           \
   122      __ret = INTERNAL_SYSCALL (futex, __err, 6, (futexp),                      \
   123                                __lll_private_flag (FUTEX_WAKE_OP, private),    \
   124                                (nr_wake), (nr_wake2), (futexp2),               \
   125                                FUTEX_OP_CLEAR_WAKE_IF_GT_ONE);                 \
   126      __ret;                                                                    \
   127    })
   128

But the above ARM implementation does *NOT* return zero if success, I suppose.
Because futex system call at line 122 returns the number of processes 
woken up.



This issue is not good for pthread_cond_signal().
For example, as a following scenario.

# pthread_cond_signal() source code is below.

[glibc-2.7/nptl/pthread_cond_wait.c]

(snip)

    31  int
    32  __pthread_cond_signal (cond)
    33       pthread_cond_t *cond;
    34  {
    35    int pshared = (cond->__data.__mutex == (void *) ~0l)
    36                  ? LLL_SHARED : LLL_PRIVATE;
    37
    38    /* Make sure we are alone.  */
    39    lll_lock (cond->__data.__lock, pshared);
    40
    41    /* Are there any waiters to be woken?  */
    42    if (cond->__data.__total_seq > cond->__data.__wakeup_seq)
    43      {
    44        /* Yes.  Mark one of them as woken.  */
    45        ++cond->__data.__wakeup_seq;
    46        ++cond->__data.__futex;
    47
    48        /* Wake one.  */
    49        if (! __builtin_expect (lll_futex_wake_unlock (&cond->__data.__futex, 1,
    50                                                       1, &cond->__data.__lock,
    51                                                       pshared), 0))
    52          return 0;
    53
    54        lll_futex_wake (&cond->__data.__futex, 1, pshared);  
    55      }
    56
    57    /* We are done.  */
    58    lll_unlock (cond->__data.__lock, pshared);
    59
    60    return 0;
    61  }


 1) lll_futex_wake_unlock() is called at line 49.
    If it succeeded, it would wake up a waiter and unlock.
    But it does *NOT* return zero because of the above reason.

 2) Then lll_unlock() at line 58 is called and unlock again.
    If other thread had the lock at that time, 
    it would be unlocked unexpectedly.

 I guess this is not good for 
 pthread_cond_signal() internal lock/unlock mechanism.



lll_futex_requeue(), which is called by pthread_cond_broadcast(), also
has the same issue.

I think these macros can be fixed as the following patch
like other architecture's source code.
ex) glibc-ports-2.7/sysdeps/unix/sysv/linux/mips/nptl/lowlevellock.h



After applying this patch to glibc-2.7, 
glibc-2.7/nptl/tst-cond20.c always pass on the above 
test environment.
And the other test results are the same.


Could you please give me some comments??

Best Regards,
Ryosei Takagi


2008-09-26  Ryosei Takagi  <ryosei@sm.sony.co.jp>

	* sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h:
	lll_futex_wake_unlock() and lll_futex_requeue() return
	zero if success.

diff -Naur ports.orig/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h ports/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h
--- ports.orig/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h	2008-09-26 07:26:01.000000000 +0000
+++ ports/sysdeps/unix/sysv/linux/arm/nptl/lowlevellock.h	2008-09-26 07:29:35.000000000 +0000
@@ -110,7 +110,7 @@
     __ret = INTERNAL_SYSCALL (futex, __err, 6, (futexp),		      \
 			      __lll_private_flag (FUTEX_CMP_REQUEUE, private),\
 			      (nr_wake), (nr_move), (mutex), (val));	      \
-    __ret;								      \
+    INTERNAL_SYSCALL_ERROR_P (__ret, __err);				      \
   })
 
 
@@ -123,7 +123,7 @@
 			      __lll_private_flag (FUTEX_WAKE_OP, private),    \
 			      (nr_wake), (nr_wake2), (futexp2),		      \
 			      FUTEX_OP_CLEAR_WAKE_IF_GT_ONE);		      \
-    __ret;								      \
+    INTERNAL_SYSCALL_ERROR_P (__ret, __err);				      \
   })