This is the mail archive of the
libc-ports@sources.redhat.com
mailing list for the libc-ports project.
Re: [Patch][RFC] ARM NPTL lll_futex_wake_unlock() issue
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); \
})