Add FUTEX_CMP_REQUEUE futex op

Hi!

FUTEX_REQUEUE operation has been added to the kernel mainly to improve pthread_cond_broadcast which previously used FUTEX_WAKE INT_MAX op. pthread_cond_broadcast releases internal condvar mutex before FUTEX_REQUEUE operation, as otherwise the woken up thread most likely immediately sleeps again on the internal condvar mutex until the broadcasting thread releases it.
Unfortunately this is racy and causes e.g.
http://sources.redhat.com/cgi-bin/cvsweb.cgi/libc/nptl/tst-cond16.c?rev=1.1&content-type=text/x-cvsweb-markup&cvsroot=glibc
to hang on SMP.
http://listman.redhat.com/archives/phil-list/2004-May/msg00023.html
contains analysis how the hang happens, the problem is if any thread does pthread_cond_*wait in between releasing of the internal condvar mutex and FUTEX_REQUEUE operation, a wrong thread might be awaken (and immediately go to sleep again because it doesn't satisfy conditions for returning from pthread_cond_*wait) while the right thread requeued on the associated mutex and there would be nobody to wake that thread up.

The patch below extends FUTEX_REQUEUE operation with something FUTEX_WAIT already uses:
FUTEX_CMP_REQUEUE is passed an additional argument which is the expected value of *futex. Kernel then while holding the futex locks checks if *futex != expected and returns -EAGAIN in that case, while if it is equal, continues with a normal FUTEX_REQUEUE operation.
If the syscall returns -EAGAIN, NPTL can fall back to FUTEX_WAKE INT_MAX operation which doesn't have this problem, but is less efficient, while in the likely case that nobody hit the (small) window the efficient FUTEX_REQUEUE operation is used.

@@ -8,9 +8,10 @@
 #define FUTEX_WAKE (1)
 #define FUTEX_FD (2)
 #define FUTEX_REQUEUE (3)
-
+#define FUTEX_CMP_REQUEUE (4)
 
 long do_futex(unsigned long uaddr, int op, int val,
-		unsigned long timeout, unsigned long uaddr2, int val2);
+		unsigned long timeout, unsigned long uaddr2, int val2,
+		int val3);
 
 #endif

@@ -208,7 +208,7 @@ asmlinkage long compat_sys_sigprocmask(i
 
 #ifdef CONFIG_FUTEX
 asmlinkage long compat_sys_futex(u32 *uaddr, int op, int val,
-		struct compat_timespec *utime, u32 *uaddr2)
+		struct compat_timespec *utime, u32 *uaddr2, int val3)
 {
 	struct timespec t;
 	unsigned long timeout = MAX_SCHEDULE_TIMEOUT;

@@ -219,11 +219,11 @@ asmlinkage long compat_sys_futex(u32 *ua
 			return -EFAULT;
 		timeout = timespec_to_jiffies(&t) + 1;
 	}
-	if (op == FUTEX_REQUEUE)
+	if (op >= FUTEX_REQUEUE)
 		val2 = (int) (long) utime;
 
 	return do_futex((unsigned long)uaddr, op, val, timeout,
-			(unsigned long)uaddr2, val2);
+			(unsigned long)uaddr2, val2, val3);
 }
 #endif
 

Jakub

Yes. This is a multiplexed syscall and the opcode decides which syscall parameter is used.

We add parameters at the end. This does not influence how previous values are passed. And especially for syscalls it makes no difference.

➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

Of course.

what we're effectively doing is:

void foo(int a, int b, int c)
{
}

and from another compilation unit:

	foo(a, b);

and we're expecting the a's and b's to line up across all architectures and compiler options. I thought that on some architectures that only works out if the function has a vararg declaration.

Does it do the right thing on stack-grows-up machines?

If the compiler passes the first few args via registers and the rest on the stack, are we sure that it won't at some level of complexity decide to pass _all_ the args on the stack? It's free to do so, I think.

I have a vague memory of getting bitten by this trick once...

I never heard that.

Would be only HP/PA and I don't see this to be a problem.

This is not how the calling conventions are designed. If registers are used they happens unconditional of the remainder of the parameter list.
The stack is used as an overflow.

I don't and, as Ingo mentioned, we already did it before.

➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

The kernel syscall ABI is on many arches different from the compiler ABI. Adding an argument this way certainly works on the architectures I'm familiar with (i386, x86_64, ia64, ppc, ppc64, s390, s390x, sparc, sparc64, alpha). I believe arm will work too, don't keep track of the rest of arches.

Well, for s390/s390x there is a problem that it doesn't allow (yet) 6 argument syscalls at all, so one possibility for s390* is adding a wrapper around sys_futex which will take the 5th and 6th arguments for FUTEX_CMP_REQUEUE from a structure pointed to by 5th argument and pass that to sys_futex.

If some weirdo arch has problems with this, it can certainly deal with it in its architecture wrapper.

Jakub

I would really prefer not re-inventing brain-damage like ipc_kludge. I have tried to do a special s390_futex_cmp_requeue() syscall, which is still somewhat ugly. At least it has the advantage that it does not break the de-facto calling conventions for s390 syscalls that either pass up to five arguments in r2 to r6 or everything in an array pointed to by r2.

Martin and Jakub, does the patch below look ok to you or should we do it in yet another way?

	Arnd <><

===== arch/s390/kernel/compat_linux.c 1.21 vs edited =====

@@ -58,6 +58,7 @@
 #include <linux/compat.h>
 #include <linux/vfs.h>
 #include <linux/ptrace.h>
+#include <linux/futex.h>
 
 #include <asm/types.h>
 #include <asm/ipc.h>

@@ -1262,4 +1263,16 @@
 		ret = put_user (ktimer_id, timer_id);
 
 	return ret;
+}
+
+extern asmlinkage long compat_sys_futex(u32 *uaddr, int op, int val,
+                struct compat_timespec *utime, u32 *uaddr2, int val3);
+
+asmlinkage long compat_s390_futex(u32 *uaddr, int op, int val,
+                struct compat_timespec *utime, u32 *uaddr2, int val3)
+{
+	if (op == FUTEX_CMP_REQUEUE)
+		return -EINVAL;
+
+	return compat_sys_futex(uaddr, op, val, utime, uaddr2, 0);
 }

===== arch/s390/kernel/compat_wrapper.S 1.13 vs edited =====

@@ -1090,14 +1090,14 @@
 	llgtr	%r3,%r3			# struct stat64 *
 	jg	sys32_fstat64		# branch to system call
 
-	.globl  compat_sys_futex_wrapper 
-compat_sys_futex_wrapper:
+	.globl  compat_s390_futex_wrapper
+compat_s390_futex_wrapper:
 	llgtr	%r2,%r2			# u32 *
 	lgfr	%r3,%r3			# int
 	lgfr	%r4,%r4			# int
 	llgtr	%r5,%r5			# struct compat_timespec *
 	llgtr	%r6,%r6			# u32 *
-	jg	compat_sys_futex	# branch to system call
+	jg	compat_s390_futex	# branch to system call
 
 	.globl	sys32_setxattr_wrapper
 sys32_setxattr_wrapper:

@@ -1404,3 +1404,12 @@
 	llgtr	%r3,%r3			# struct compat_mq_attr *
 	llgtr	%r4,%r4			# struct compat_mq_attr *
 	jg	compat_sys_mq_getsetattr
+
+	.globl	compat_s390_futex_cmp_requeue_wrapper
+compat_s390_futex_cmp_requeue_wrapper:
+	llgtr	%r2,%r2			# u32 *
+	llgtr	%r3,%r3			# u32 *
+	lgfr	%r4,%r4			# int
+	lgfr	%r5,%r5			# int
+	lgfr	%r6,%r6			# int
+	jg	s390_futex_cmp_requeue

===== arch/s390/kernel/sys_s390.c 1.14 vs edited =====

@@ -26,9 +26,8 @@
 #include <linux/mman.h>
 #include <linux/file.h>
 #include <linux/utsname.h>
-#ifdef CONFIG_ARCH_S390X
 #include <linux/personality.h>
-#endif /* CONFIG_ARCH_S390X */
+#include <linux/futex.h>
 
 #include <asm/uaccess.h>
 #include <asm/ipc.h>

@@ -265,3 +264,20 @@
 	return sys_fadvise64_64(a.fd, a.offset, a.len, a.advice);
 }
 
+asmlinkage long
+s390_futex(u32 __user *uaddr, int op, int val,
+             struct timespec __user *utime, u32 __user *uaddr2)
+{
+	if (op == FUTEX_CMP_REQUEUE)
+		return -EINVAL;
+
+	return sys_futex(uaddr, op, val, utime, uaddr2, 0);
+}
+
+asmlinkage long
+s390_futex_cmp_requeue(u32 __user *uaddr, u32 __user *uaddr2,
+		int nr_wake, int nr_requeue, int val)
+{
+	return do_futex((unsigned long) uaddr, FUTEX_CMP_REQUEUE,
+			nr_wake, 0, (unsigned long)uaddr2, nr_requeue, val);
+}

===== arch/s390/kernel/syscalls.S 1.12 vs edited =====

@@ -246,7 +246,7 @@
 SYSCALL(sys_fremovexattr,sys_fremovexattr,sys32_fremovexattr_wrapper)	/* 235 */
 SYSCALL(sys_gettid,sys_gettid,sys_gettid)
 SYSCALL(sys_tkill,sys_tkill,sys_tkill)
-SYSCALL(sys_futex,sys_futex,compat_sys_futex_wrapper)
+SYSCALL(s390_futex,s390_futex,compat_s390_futex_wrapper)
 SYSCALL(sys_sched_setaffinity,sys_sched_setaffinity,sys32_sched_setaffinity_wrapper)
 SYSCALL(sys_sched_getaffinity,sys_sched_getaffinity,sys32_sched_getaffinity_wrapper)	/* 240 */
 SYSCALL(sys_tgkill,sys_tgkill,sys_tgkill)

@@ -285,3 +285,4 @@
 SYSCALL(sys_mq_timedreceive,sys_mq_timedreceive,compat_sys_mq_timedreceive_wrapper)
 SYSCALL(sys_mq_notify,sys_mq_notify,compat_sys_mq_notify_wrapper)
 SYSCALL(sys_mq_getsetattr,sys_mq_getsetattr,compat_sys_mq_getsetattr_wrapper)
+SYSCALL(s390_futex_cmp_requeue,s390_futex_cmp_requeue,compat_s390_futex_cmp_requeue_wrapper)

===== include/asm-s390/unistd.h 1.26 vs edited =====

@@ -269,8 +269,9 @@
 #define __NR_mq_timedreceive	274
 #define __NR_mq_notify		275
 #define __NR_mq_getsetattr	276
+#define __NR_futex_cmp_requeue	277
 
-#define NR_syscalls 277
+#define NR_syscalls 278
 
 /* 
  * There are some system calls that are not present on 64 bit, some

===== include/linux/syscalls.h 1.6 vs edited =====

@@ -165,7 +165,8 @@
 asmlinkage long sys_waitpid(pid_t pid, unsigned int __user *stat_addr, int options);
 asmlinkage long sys_set_tid_address(int __user *tidptr);
 asmlinkage long sys_futex(u32 __user *uaddr, int op, int val,
-			struct timespec __user *utime, u32 __user *uaddr2);
+			struct timespec __user *utime, u32 __user *uaddr2,
+			int val3);
 
 asmlinkage long sys_init_module(void __user *umod, unsigned long len,
 				const char __user *uargs);

===== kernel/fork.c 1.169 vs edited =====

@@ -516,7 +516,7 @@
 		 * not set up a proper pointer then tough luck.
 		 */
 		put_user(0, tidptr);
-		sys_futex(tidptr, FUTEX_WAKE, 1, NULL, NULL);
+		sys_futex(tidptr, FUTEX_WAKE, 1, NULL, NULL, 0);
 	}
 }
 

My personal preference is:
1) change s390* entry*.S so that all syscalls can use up to 6 arguments, otherwise we'll have the same problem every now and then (last syscall before this one was fadvise64_64 if I remember well)
2) allow sys_futex to have 6 arguments (the 6th on the stack, get_user'ed from an s390 wrapper)
3) special structure passed in 5th argument for FUTEX_CMP_REQUEUE
4) your solution
(you have a special wrapper around futex anyway, so why not to handle it there already and create completely new syscall).

That said, NPTL can deal with any of these variants and the decision is up to Martin I think (assuming the base patch gets accepted, that is).
http://listman.redhat.com/archives/phil-list/2004-May/msg00031.html
is the latest version of the userland part of FUTEX_CMP_REQUEUE changes (against CVS glibc) and there futex_requeue () macro is defined in arch specific headers, so nptl/sysdeps/unix/sysv/linux/s390/lowlevellock.h can do there its own black magic.

Jakub

Well the race is real and does need a kernel fix, so I queued it up. I guess if the new argument to sys_futex() breaks some architecture they can independently add a new syscall for it, or send a fix. Mutter.

It's a bit of a shame that you need to be a rocket scientist to understand the futex syscall interface. Bert, are you still maintaining the manpage? If so, is there enough info here to update it?

FUTEX_REQUEUE operation has been added to the kernel mainly to improve pthread_cond_broadcast which previously used FUTEX_WAKE INT_MAX op. pthread_cond_broadcast releases internal condvar mutex before FUTEX_REQUEUE operation, as otherwise the woken up thread most likely immediately sleeps again on the internal condvar mutex until the broadcasting thread releases it.

Unfortunately this is racy and causes e.g.
http://sources.redhat.com/cgi-bin/cvsweb.cgi/libc/nptl/tst-cond16.c?rev=1.1&content-type=text/x-cvsweb-markup&cvsroot=glibc
to hang on SMP.

http://listman.redhat.com/archives/phil-list/2004-May/msg00023.html contains analysis how the hang happens, the problem is if any thread does pthread_cond_*wait in between releasing of the internal condvar mutex and FUTEX_REQUEUE operation, a wrong thread might be awaken (and immediately go to sleep again because it doesn't satisfy conditions for returning from pthread_cond_*wait) while the right thread requeued on the associated mutex and there would be nobody to wake that thread up.

The patch below extends FUTEX_REQUEUE operation with something FUTEX_WAIT already uses:

FUTEX_CMP_REQUEUE is passed an additional argument which is the expected value of *futex. Kernel then while holding the futex locks checks if *futex != expected and returns -EAGAIN in that case, while if it is equal, continues with a normal FUTEX_REQUEUE operation. If the syscall returns -EAGAIN, NPTL can fall back to FUTEX_WAKE INT_MAX operation which doesn't have this problem, but is less efficient, while in the likely case that nobody hit the (small) window the efficient FUTEX_REQUEUE operation is used.

---

 25-akpm/include/linux/futex.h |    5 ++--
 25-akpm/kernel/compat.c       |    6 ++---
 25-akpm/kernel/futex.c        |   49 ++++++++++++++++++++++++++++++++++++------
 3 files changed, 49 insertions(+), 11 deletions(-)

diff -puN include/linux/futex.h~add-futex_cmp_requeue-futex-op include/linux/futex.h
@@ -8,9 +8,10 @@
 #define FUTEX_WAKE (1)
 #define FUTEX_FD (2)
 #define FUTEX_REQUEUE (3)
-
+#define FUTEX_CMP_REQUEUE (4)
 
 long do_futex(unsigned long uaddr, int op, int val,
-		unsigned long timeout, unsigned long uaddr2, int val2);
+		unsigned long timeout, unsigned long uaddr2, int val2,
+		int val3);
 
 #endif

diff -puN kernel/compat.c~add-futex_cmp_requeue-futex-op kernel/compat.c
@@ -208,7 +208,7 @@ asmlinkage long compat_sys_sigprocmask(i
 
 #ifdef CONFIG_FUTEX
 asmlinkage long compat_sys_futex(u32 *uaddr, int op, int val,
-		struct compat_timespec *utime, u32 *uaddr2)
+		struct compat_timespec *utime, u32 *uaddr2, int val3)
 {
 	struct timespec t;
 	unsigned long timeout = MAX_SCHEDULE_TIMEOUT;

@@ -219,11 +219,11 @@ asmlinkage long compat_sys_futex(u32 *ua
 			return -EFAULT;
 		timeout = timespec_to_jiffies(&t) + 1;
 	}
-	if (op == FUTEX_REQUEUE)
+	if (op >= FUTEX_REQUEUE)
 		val2 = (int) (long) utime;
 
 	return do_futex((unsigned long)uaddr, op, val, timeout,
-			(unsigned long)uaddr2, val2);
+			(unsigned long)uaddr2, val2, val3);
 }
 #endif
 

_

The latest futex(2) or futex(4) manpage I saw doesn't mention FUTEX_REQUEUE at all.
Also, any futex man page should probably SEE ALSO Ulrich's futex paper:
http://people.redhat.com/drepper/futex.pdf
which helps understanding how to successfully use futexes, because it is certainly not trivial.

Jakub

Now fixed, please see http://ds9a.nl/futex-manpages - but please realise I'm somewhat out of my depth. Comments welcome.

Futexes have mutated into complicated things, I wonder if this was the last of the changes needed.

The big change in the manpages is the addition of FUTEX_REQUEUE and FUTEX_CMP_REQUEUE. Furthermore, I realised that the futex system call does not return EAGAIN etc, it returns -EAGAIN. I guesstimated that CMP_REQUEUE will be merged before 2.6.7.

To clarify the overloaded situation wrt the futex syscall, I split it up in three prototypes.

Ulrich, does/will glibc provide a futex(2) function? Or should people just call the syscall themselves?

There were also some complaints I did not address futexfs but as far as I can see, it is a kernel internal matter of no interest to userspace coders?

Referenced, thanks!

http://www.PowerDNS.com      Open source, database driven DNS Software 
http://lartc.org           Linux Advanced Routing & Traffic Control HOWTO

futex behaves like most of the other syscalls, i.e. on error returns -1 and sets errno to the error value, like EAGAIN, EFAULT etc. This is done in a userland wrapper around the syscall, how the kernel tells userland an error happened and what errno value should be set is an architecture specific implementation detail.
The man pages in the 2nd section document the behaviour of the userland wrappers (see e.g. read(2)), not what the kernel actually returns.

glibc doesn't provide a futex(2) function, so at least ATM people can use #include <sys/syscall.h>
syscall (SYS_futex, &futex, FUTEX_xyz, ...) or come up with their own assembly to invoke the syscall.

Jakub

Ok - but in the absence of futex(2), is it perhaps better to move this page to futex(9)? Or merge it into futex(4)?

If it will provide one, I'll change the page to be in line with how futex(2) will probably work, with errno etc.

Regarding the verbiage, I need input about the use of the word 'process', as I'm not sure if there is a generic word covering both processes and threads. Jamie suggests 'tasks', but that is kernel lingo and of little use for userspace developers. Suggestions?

Thanks.

bert

http://www.PowerDNS.com      Open source, database driven DNS Software 
http://lartc.org           Linux Advanced Routing & Traffic Control HOWTO

I'll spend some time with the manpage tomorrow. Some other comments have been queued too and I'll process them. By that time, I'll hand over the manpages to aeb.

Thanks for the heads up.

http://www.PowerDNS.com      Open source, database driven DNS Software 
http://lartc.org           Linux Advanced Routing & Traffic Control HOWTO

I have talked to Martin about it (he won't be online for one more week), and he proposed a variant of 2): sys_futex, and possibly further new syscalls that might get added, pass their sixth argument in %r7, which is easier to get by than the user stack. Unlike doing it in the standard system call path, this won't cause any overhead for the other syscalls, but we need some wrappers.

I haven't tested this, because I'm not too familiar with glibc changes. Jakub, if you are happy with this approach, can you add the respective glibc stuff and try if this is any good?

	Arnd <><

[PATCH] Fix FUTEX_CMP_REQUEUE on s390

From: Arnd Bergmann <[email protected]>

System calls normally can not have more than five arguments on s390 because of ABI restrictions, but the addition of the FUTEX_CMP_REQUEUE added a sixth argument to sys_futex. This patch extends the regular syscall conventions by an extra argument register for sys_futex.

The regular method of passing all arguments on the user stack for six argument syscalls on s390 does not work here because it would break the existing usage of futex.

I'm also fixing the sys_futex prototype in <linux/syscalls.h> to make it possible to call it from s390_futex.

===== arch/s390/kernel/compat_linux.c 1.21 vs edited =====

@@ -1263,3 +1263,16 @@
 
 	return ret;
 }
+
+asmlinkage long compat_sys_futex(u32 *uaddr, int op, int val,
+                struct compat_timespec *utime, u32 *uaddr2, int val3);
+
+asmlinkage long
+sys32_futex(u32 __user *uaddr, int op, int val,
+	struct compat_timespec __user *utime, u32 __user *uaddr2)
+{
+	struct pt_regs *regs;
+
+	regs = __KSTK_PTREGS(current);
+	return compat_sys_futex(uaddr, op, val, utime, uaddr2, regs->gprs[7]);
+}

===== arch/s390/kernel/compat_wrapper.S 1.13 vs edited =====

@@ -1090,14 +1090,14 @@
 	llgtr	%r3,%r3			# struct stat64 *
 	jg	sys32_fstat64		# branch to system call
 
-	.globl  compat_sys_futex_wrapper 
-compat_sys_futex_wrapper:
+	.globl  sys32_futex_wrapper 
+sys32_futex_wrapper:
 	llgtr	%r2,%r2			# u32 *
 	lgfr	%r3,%r3			# int
 	lgfr	%r4,%r4			# int
 	llgtr	%r5,%r5			# struct compat_timespec *
 	llgtr	%r6,%r6			# u32 *
-	jg	compat_sys_futex	# branch to system call
+	jg	sys32_futex		# branch to system call
 
 	.globl	sys32_setxattr_wrapper
 sys32_setxattr_wrapper:

===== arch/s390/kernel/sys_s390.c 1.14 vs edited =====

@@ -265,3 +265,12 @@
 	return sys_fadvise64_64(a.fd, a.offset, a.len, a.advice);
 }
 
+asmlinkage long
+s390_futex(u32 __user *uaddr, int op, int val,
+	struct timespec __user *utime, u32 __user *uaddr2)
+{
+	struct pt_regs *regs;
+
+	regs = __KSTK_PTREGS(current);
+	return sys_futex(uaddr, op, val, utime, uaddr2, regs->gprs[7]);
+}

===== arch/s390/kernel/syscalls.S 1.12 vs edited =====

@@ -246,7 +246,7 @@
 SYSCALL(sys_fremovexattr,sys_fremovexattr,sys32_fremovexattr_wrapper)	/* 235 */
 SYSCALL(sys_gettid,sys_gettid,sys_gettid)
 SYSCALL(sys_tkill,sys_tkill,sys_tkill)
-SYSCALL(sys_futex,sys_futex,compat_sys_futex_wrapper)
+SYSCALL(s390_futex,s390_futex,sys32_futex_wrapper)
 SYSCALL(sys_sched_setaffinity,sys_sched_setaffinity,sys32_sched_setaffinity_wrapper)
 SYSCALL(sys_sched_getaffinity,sys_sched_getaffinity,sys32_sched_getaffinity_wrapper)	/* 240 */
 SYSCALL(sys_tgkill,sys_tgkill,sys_tgkill)

===== include/linux/syscalls.h 1.6 vs edited =====

@@ -165,7 +165,7 @@
 asmlinkage long sys_waitpid(pid_t pid, unsigned int __user *stat_addr, int options);
 asmlinkage long sys_set_tid_address(int __user *tidptr);
 asmlinkage long sys_futex(u32 __user *uaddr, int op, int val,
-			struct timespec __user *utime, u32 __user *uaddr2);
+			struct timespec __user *utime, u32 __user *uaddr2, int val3);
 
 asmlinkage long sys_init_module(void __user *umod, unsigned long len,
 				const char __user *uargs);

===== kernel/fork.c 1.169 vs edited =====

@@ -516,7 +516,7 @@
 		 * not set up a proper pointer then tough luck.
 		 */
 		put_user(0, tidptr);
-		sys_futex(tidptr, FUTEX_WAKE, 1, NULL, NULL);
+		sys_futex(tidptr, FUTEX_WAKE, 1, NULL, NULL, 0);
 	}
 }
 

===== kernel/futex.c 1.40 vs edited =====

@@ -38,6 +38,7 @@
 #include <linux/futex.h>
 #include <linux/mount.h>
 #include <linux/pagemap.h>
+#include <linux/syscalls.h>
 
 #define FUTEX_HASHBITS 8
 

we added a new futex paramater once before (the original requeue patch) - so if it broke anything, it didnt break loud enough for anyone to notice :-|

Ingo

                                      ^^^^^^^^^^^^^^^^^^^^^^^

But in your patch the check is happening _prior_ to taking the futex locks.

You've added an smp_mb(). These things are becoming like lock_kernel() - hard for the reader to discern what it's protecting against.

Please always include a comment when adding a barrier like this.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

What about including this in the API?

sth. like

#define smp_mb(why) do { \
	static __debugdata__ char __lock_reason[] = why; \
	} while(0)

would be sufficient. __debugdata__ section
will then be stripped from compressed image but not vmlinux.

Is there another way to force developers to comment? ;-)

Regards

Ingo Oeser

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFArycyU56oYWuOrkARAljxAKDNE04H2/zr+rZcu1SAR3x19rUtHgCgwqKV +Mpszd42hQ7hDQjphDobNRk=
=gh0g
-----END PGP SIGNATURE-----

Sure, that's because I'm trying to avoid get_user while holding spinlock, as get_user may sleep.

Comment added below.

@@ -8,9 +8,10 @@
 #define FUTEX_WAKE (1)
 #define FUTEX_FD (2)
 #define FUTEX_REQUEUE (3)
-
+#define FUTEX_CMP_REQUEUE (4)
 
 long do_futex(unsigned long uaddr, int op, int val,
-		unsigned long timeout, unsigned long uaddr2, int val2);
+		unsigned long timeout, unsigned long uaddr2, int val2,
+		int val3);
 
 #endif

@@ -208,7 +208,7 @@ asmlinkage long compat_sys_sigprocmask(i
 
 #ifdef CONFIG_FUTEX
 asmlinkage long compat_sys_futex(u32 *uaddr, int op, int val,
-		struct compat_timespec *utime, u32 *uaddr2)
+		struct compat_timespec *utime, u32 *uaddr2, int val3)
 {
 	struct timespec t;
 	unsigned long timeout = MAX_SCHEDULE_TIMEOUT;

@@ -219,11 +219,11 @@ asmlinkage long compat_sys_futex(u32 *ua
 			return -EFAULT;
 		timeout = timespec_to_jiffies(&t) + 1;
 	}
-	if (op == FUTEX_REQUEUE)
+	if (op >= FUTEX_REQUEUE)
 		val2 = (int) (long) utime;
 
 	return do_futex((unsigned long)uaddr, op, val, timeout,
-			(unsigned long)uaddr2, val2);
+			(unsigned long)uaddr2, val2, val3);
 }
 #endif
 

Jakub

For the record, I wasn't happy about adding FUTEX_REQUEUE to optimize for suboptimal apps using the horrid pthreads interface, but I understand the benchmarking realities.

I'm certainly way less than thrilled to discover that it doesn't work, and we need to implement FUTEX_CMP_REQUEUE, and of course can't get rid of FUTEX_REQUEUE.

The base futex concept and interface is simple (although the implementation w/ the Linux mm subsystem proved interesting in the corner cases); it's increasingly becoming a horror with these kind of hacks.

8(
Rusty.

Anyone who quotes me in their signature is an idiot -- Rusty Russell