sh: Use generic lowlevellock-futex.h.
diff mbox

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

Commit Message

Torvald Riegel Dec. 17, 2014, 10:41 p.m. UTC
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?

Thanks!


2014-12-17  Torvald Riegel  <triegel@redhat.com>

	* sysdeps/unix/sysv/linux/sh/lowlevellock.h: Include
	<lowlevellock-futex.h>.  Remove FUTEX_* constants defined there.
	(__lll_private_flag): Remove.
	(lll_futex_wait): Likewise.
	(lll_futex_timed_wait): Likewise.
	(lll_futex_wake): Likewise.
	* sysdeps/unix/sysv/linux/sh/sh4/lowlevellock.h: Remove file.

Comments

Roland McGrath Dec. 17, 2014, 10:43 p.m. UTC | #1
I think you should first propose just removing it and let Kaz tell us if
there is anything wrong with that.
Torvald Riegel Dec. 17, 2014, 10:47 p.m. UTC | #2
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.
Kaz Kojima Dec. 18, 2014, 12:28 a.m. UTC | #3
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
Torvald Riegel Dec. 18, 2014, 10:33 a.m. UTC | #4
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?
Kaz Kojima Dec. 18, 2014, 12:43 p.m. UTC | #5
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

Patch
diff mbox

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>