| Message ID | 1418856067.20194.7.camel@triegel.csb |
|---|---|
| State | New |
| Headers | show |
I think you should first propose just removing it and let Kaz tell us if there is anything wrong with that.
On Wed, 2014-12-17 at 14:43 -0800, Roland McGrath wrote: > I think you should first propose just removing it and let Kaz tell us if > there is anything wrong with that. Yes, that would be best thing to do, maintenance-wise. See the third paragraph of what I wrote.
Torvald Riegel <triegel@redhat.com> wrote: > This completely untested patch removes the custom definitions of futex > functions in sh lowlevellock.h, using the generic lowlevellock-futex.h > instead. This is part of the clean-up efforts regarding the > glibc-internal futex API (e.g., adding proper error checking). > > This also removes the sh4 lowlevellock.h, which just requires more > padding for the syscalls; the same requirement is made by sh4 sysdep.h, > so INTERNAL_SYSCALL used in the generic lowlevellock-futex.h will honor > this too. > > This keeps the custom asm for the lock fast paths because I don't know > whether the generic C implementation would be fine. If it would be > (e.g., test with sh's lowlevellock.h removed), then removing the custom > asm altogether would be even better. > > Is this OK for sh? If not, do you have an alternative suggestion for > how to use the generic futex interfaces? The patch is OK. I definitely agree with your view about the custom asm. Switch to the generic C implementation would be simply fine and the way to go. Regards, kaz
On Thu, 2014-12-18 at 09:28 +0900, Kaz Kojima wrote: > Torvald Riegel <triegel@redhat.com> wrote: > > This completely untested patch removes the custom definitions of futex > > functions in sh lowlevellock.h, using the generic lowlevellock-futex.h > > instead. This is part of the clean-up efforts regarding the > > glibc-internal futex API (e.g., adding proper error checking). > > > > This also removes the sh4 lowlevellock.h, which just requires more > > padding for the syscalls; the same requirement is made by sh4 sysdep.h, > > so INTERNAL_SYSCALL used in the generic lowlevellock-futex.h will honor > > this too. > > > > This keeps the custom asm for the lock fast paths because I don't know > > whether the generic C implementation would be fine. If it would be > > (e.g., test with sh's lowlevellock.h removed), then removing the custom > > asm altogether would be even better. > > > > Is this OK for sh? If not, do you have an alternative suggestion for > > how to use the generic futex interfaces? > > The patch is OK. OK, thanks. > I definitely agree with your view about the custom asm. Switch to > the generic C implementation would be simply fine and the way to go. Does that mean I can simply commit a patch that removes the custom lowlevellock.h, or do you want to test this further before this is committed?
Torvald Riegel <triegel@redhat.com> wrote: >> I definitely agree with your view about the custom asm. Switch to >> the generic C implementation would be simply fine and the way to go. > > Does that mean I can simply commit a patch that removes the custom > lowlevellock.h, or do you want to test this further before this is > committed? There are sh custom asm files depends on the custom lowlevellock.h. The files below should be removed from sysdeps/unix/sysv/linux/sh/ at the same time. lowlevellock.h lowlevellock.S lowlevelrobustlock.S libc-lowlevellock.S pthread_barrier_wait.S pthread_cond_broadcast.S pthread_cond_signal.S pthread_cond_timedwait.S pthread_cond_wait.S pthread_rwlock_rdlock.S pthread_rwlock_timedrdlock.S pthread_rwlock_timedwrlock.S pthread_rwlock_unlock.S pthread_rwlock_wrlock.S sem_post.S sem_timedwait.S sem_trywait.S sem_wait.S I'd like to pre-approve the patch removing them all. Regards, kaz
commit 59b40989765ddce33f61999068219c3a28f30d8e
Author: Torvald Riegel <triegel@redhat.com>
Date: Wed Dec 17 23:28:38 2014 +0100
sh: Use generic lowlevellock-futex.h.
diff --git a/sysdeps/unix/sysv/linux/sh/lowlevellock.h b/sysdeps/unix/sysv/linux/sh/lowlevellock.h
index 8045846..bba0657 100644
--- a/sysdeps/unix/sysv/linux/sh/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/sh/lowlevellock.h
@@ -25,53 +25,10 @@
#include <kernel-features.h>
#endif
-#define SYS_futex 240
-#define FUTEX_WAIT 0
-#define FUTEX_WAKE 1
-#define FUTEX_CMP_REQUEUE 4
-#define FUTEX_WAKE_OP 5
-#define FUTEX_LOCK_PI 6
-#define FUTEX_UNLOCK_PI 7
-#define FUTEX_TRYLOCK_PI 8
-#define FUTEX_WAIT_BITSET 9
-#define FUTEX_WAKE_BITSET 10
-#define FUTEX_PRIVATE_FLAG 128
-#define FUTEX_CLOCK_REALTIME 256
-
-#define FUTEX_BITSET_MATCH_ANY 0xffffffff
-
-#define FUTEX_OP_CLEAR_WAKE_IF_GT_ONE ((4 << 24) | 1)
-
-/* Values for 'private' parameter of locking macros. Yes, the
- definition seems to be backwards. But it is not. The bit will be
- reversed before passing to the system call. */
-#define LLL_PRIVATE 0
-#define LLL_SHARED FUTEX_PRIVATE_FLAG
-
+#include <lowlevellock-futex.h>
-#if IS_IN (libc) || IS_IN (rtld)
-/* In libc.so or ld.so all futexes are private. */
-# ifdef __ASSUME_PRIVATE_FUTEX
-# define __lll_private_flag(fl, private) \
- ((fl) | FUTEX_PRIVATE_FLAG)
-# else
-# define __lll_private_flag(fl, private) \
- ((fl) | THREAD_GETMEM (THREAD_SELF, header.private_futex))
-# endif
-#else
-# ifdef __ASSUME_PRIVATE_FUTEX
-# define __lll_private_flag(fl, private) \
- (((fl) | FUTEX_PRIVATE_FLAG) ^ (private))
-# else
-# define __lll_private_flag(fl, private) \
- (__builtin_constant_p (private) \
- ? ((private) == 0 \
- ? ((fl) | THREAD_GETMEM (THREAD_SELF, header.private_futex)) \
- : (fl)) \
- : ((fl) | (((private) ^ FUTEX_PRIVATE_FLAG) \
- & THREAD_GETMEM (THREAD_SELF, header.private_futex))))
-# endif
-#endif
+/* XXX Remove when no assembler code uses futexes anymore. */
+#define SYS_futex 240
#ifndef __ASSEMBLER__
@@ -309,45 +266,6 @@ extern int __lll_unlock_wake (int *__futex, int private) attribute_hidden;
trapa #0x14"
# endif
-#define lll_futex_wait(futex, val, private) \
- lll_futex_timed_wait (futex, val, NULL, private)
-
-
-#define lll_futex_timed_wait(futex, val, timeout, private) \
- ({ \
- int __status; \
- register unsigned long __r3 asm ("r3") = SYS_futex; \
- register unsigned long __r4 asm ("r4") = (unsigned long) (futex); \
- register unsigned long __r5 asm ("r5") \
- = __lll_private_flag (FUTEX_WAIT, private); \
- register unsigned long __r6 asm ("r6") = (unsigned long) (val); \
- register unsigned long __r7 asm ("r7") = (unsigned long) (timeout); \
- __asm __volatile (SYSCALL_WITH_INST_PAD \
- : "=z" (__status) \
- : "r" (__r3), "r" (__r4), "r" (__r5), \
- "r" (__r6), "r" (__r7) \
- : "memory", "t"); \
- __status; \
- })
-
-
-#define lll_futex_wake(futex, nr, private) \
- do { \
- int __ignore; \
- register unsigned long __r3 asm ("r3") = SYS_futex; \
- register unsigned long __r4 asm ("r4") = (unsigned long) (futex); \
- register unsigned long __r5 asm ("r5") \
- = __lll_private_flag (FUTEX_WAKE, private); \
- register unsigned long __r6 asm ("r6") = (unsigned long) (nr); \
- register unsigned long __r7 asm ("r7") = 0; \
- __asm __volatile (SYSCALL_WITH_INST_PAD \
- : "=z" (__ignore) \
- : "r" (__r3), "r" (__r4), "r" (__r5), \
- "r" (__r6), "r" (__r7) \
- : "memory", "t"); \
- } while (0)
-
-
#define lll_islocked(futex) \
(futex != LLL_LOCK_INITIALIZER)
diff --git a/sysdeps/unix/sysv/linux/sh/sh4/lowlevellock.h b/sysdeps/unix/sysv/linux/sh/sh4/lowlevellock.h
deleted file mode 100644
index 90be7bd..0000000
--- a/sysdeps/unix/sysv/linux/sh/sh4/lowlevellock.h
+++ /dev/null
@@ -1,4 +0,0 @@
-/* 4 instruction cycles not accessing cache and TLB are needed after
- trapa instruction to avoid an SH-4 silicon bug. */
-#define NEED_SYSCALL_INST_PAD
-#include_next <lowlevellock.h>