[PATCH] RFC: futex: make futex_lock_pi interruptible

[PATCH] RFC: futex: make futex_lock_pi interruptible

[Darren Hart]

>From 9ea67856951c87a03a5177e1382d836622642521 Mon Sep 17 00:00:00 2001
From: Darren Hart <dvhltc@us.ibm.com>
Date: Mon, 26 Oct 2009 17:15:54 -0700
Subject: [PATCH] RFC: futex: make futex_lock_pi interruptible

NOT FOR INCLUSION

Interruptible locking constructs can currently be implemented in
userspace using condvars and mutexes. However, if FIFO ordered wakeup
and Priority Inheritance are requirements, the sleeping kernel call must
be interruptible. The following patch implements this by allowing
futex_lock_pi() to be interrupted if the new FUTEX_INTERRUPTIBLE opcode
flag is set (rather than perform a syscall restart).

An alternative approach might be to implement a new FUTEX_CANCEL opcode
and return -ECANCELED. This is slightly more complicated to use from
userspace I believe, but not overly so.

A final patch would also need to add support to FUTEX_WAIT. Before I do,
I wanted to get some opinions on this approach.

Signed-off-by: Darren Hart <dvhltc@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@elte.hu>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Dinakar Guniguntala <dino@in.ibm.com>
CC: John Stultz <johnstul@us.ibm.com>
---
 include/linux/futex.h |    3 ++-
 kernel/futex.c        |   20 ++++++++++++++------
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/include/linux/futex.h b/include/linux/futex.h
index 34956c8..f1a4f89 100644
--- a/include/linux/futex.h
+++ b/include/linux/futex.h
@@ -28,7 +28,8 @@ union ktime;
 
 #define FUTEX_PRIVATE_FLAG	128
 #define FUTEX_CLOCK_REALTIME	256
-#define FUTEX_CMD_MASK		~(FUTEX_PRIVATE_FLAG | FUTEX_CLOCK_REALTIME)
+#define FUTEX_INTERRUPTIBLE	512
+#define FUTEX_CMD_MASK		~(FUTEX_PRIVATE_FLAG | FUTEX_CLOCK_REALTIME | FUTEX_INTERRUPTIBLE)
 
 #define FUTEX_WAIT_PRIVATE	(FUTEX_WAIT | FUTEX_PRIVATE_FLAG)
 #define FUTEX_WAKE_PRIVATE	(FUTEX_WAKE | FUTEX_PRIVATE_FLAG)
diff --git a/kernel/futex.c b/kernel/futex.c
index 7c4a6ac..d41c55c 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1834,8 +1834,8 @@ static long futex_wait_restart(struct restart_block *restart)
  * if there are waiters then it will block, it does PI, etc. (Due to
  * races the kernel might see a 0 value of the futex too.)
  */
-static int futex_lock_pi(u32 __user *uaddr, int fshared,
-			 int detect, ktime_t *time, int trylock)
+static int futex_lock_pi(u32 __user *uaddr, int fshared, int detect,
+			 ktime_t *time, int trylock, int interruptible)
 {
 	struct hrtimer_sleeper timeout, *to = NULL;
 	struct futex_hash_bucket *hb;
@@ -1937,7 +1937,11 @@ out_put_key:
 out:
 	if (to)
 		destroy_hrtimer_on_stack(&to->timer);
-	return ret != -EINTR ? ret : -ERESTARTNOINTR;
+
+	if (interruptible)
+		return ret;
+	else
+		return ret != -EINTR ? ret : -ERESTARTNOINTR;
 
 uaddr_faulted:
 	queue_unlock(&q, hb);
@@ -2489,7 +2493,7 @@ void exit_robust_list(struct task_struct *curr)
 long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 		u32 __user *uaddr2, u32 val2, u32 val3)
 {
-	int clockrt, ret = -ENOSYS;
+	int clockrt, interruptible, ret = -ENOSYS;
 	int cmd = op & FUTEX_CMD_MASK;
 	int fshared = 0;
 
@@ -2500,6 +2504,8 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 	if (clockrt && cmd != FUTEX_WAIT_BITSET && cmd != FUTEX_WAIT_REQUEUE_PI)
 		return -ENOSYS;
 
+	interruptible = op & FUTEX_INTERRUPTIBLE;
+
 	switch (cmd) {
 	case FUTEX_WAIT:
 		val3 = FUTEX_BITSET_MATCH_ANY;
@@ -2523,7 +2529,8 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 		break;
 	case FUTEX_LOCK_PI:
 		if (futex_cmpxchg_enabled)
-			ret = futex_lock_pi(uaddr, fshared, val, timeout, 0);
+			ret = futex_lock_pi(uaddr, fshared, val, timeout, 0,
+					    interruptible);
 		break;
 	case FUTEX_UNLOCK_PI:
 		if (futex_cmpxchg_enabled)
@@ -2531,7 +2538,8 @@ long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
 		break;
 	case FUTEX_TRYLOCK_PI:
 		if (futex_cmpxchg_enabled)
-			ret = futex_lock_pi(uaddr, fshared, 0, timeout, 1);
+			ret = futex_lock_pi(uaddr, fshared, 0, timeout, 1,
+					    interruptible);
 		break;
 	case FUTEX_WAIT_REQUEUE_PI:
 		val3 = FUTEX_BITSET_MATCH_ANY;
-- 
1.6.0.4
-- 
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team

Re: [PATCH] RFC: futex: make futex_lock_pi interruptible

[Darren Hart]

Darren Hart wrote:
>  From 9ea67856951c87a03a5177e1382d836622642521 Mon Sep 17 00:00:00 2001
> From: Darren Hart <dvhltc@us.ibm.com>
> Date: Mon, 26 Oct 2009 17:15:54 -0700
> Subject: [PATCH] RFC: futex: make futex_lock_pi interruptible
> 

The following C test case demonstrates how this patch could be used to
implement interruptible locking. There is an awful lot of debug code and
some other relics of a hacked together test in there now, but if anyone
wanted to test the futex changes, this will do the trick.


/******************************************************************************
 *
 *   Copyright © International Business Machines  Corp., 2009
 *
 *   This program is free software;  you can redistribute it and/or modify
 *   it under the terms of the GNU General Public License as published by
 *   the Free Software Foundation; either version 2 of the License, or
 *   (at your option) any later version.
 *
 *   This program is distributed in the hope that it will be useful,
 *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
 *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
 *   the GNU General Public License for more details.
 *
 *   You should have received a copy of the GNU General Public License
 *   along with this program;  if not, write to the Free Software
 *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 *
 * NAME
 *      mutexint.c
 *
 * DESCRIPTION
 *      Provide an interruptible PI-aware locking construct.
 *
 * AUTHOR
 *      Darren Hart <dvhltc@us.ibm.com>
 *
 * HISTORY
 *      2009-Oct-26: Initial version by Darren Hart <dvhltc@us.ibm.com>
 *
 *****************************************************************************/

#include <pthread.h>
#include <stdio.h>
#include <signal.h>
#include <syscall.h>
#include <sys/types.h>
#include <linux/futex.h>
#include <errno.h>

/*
 * The futex() call has been removed from the include/futex.h header, implement
 * our own version.
 */
//#define SYS_futex 202
#define FUTEX_PRIVATE 128
#define FUTEX_INTERRUPTIBLE 512
#define futex(uaddr, op, val, timeout, uaddr2, val3) \
	syscall(SYS_futex, uaddr, op, val, timeout, uaddr2, val3)

#define futex_lock_pi(uaddr, val, timeout) \
	futex(uaddr, FUTEX_LOCK_PI|FUTEX_INTERRUPTIBLE|FUTEX_PRIVATE, \
	      val, timeout, NULL, 0)

#define futex_unlock_pi(uaddr, val) \
	futex(uaddr, FUTEX_UNLOCK_PI|FUTEX_PRIVATE, val, NULL, NULL, 0)

#define gettid() syscall(SYS_gettid)

#define SIGMUTEXINT SIGRTMIN

/*
 * Implement cmpxchg using gcc atomic builtins.
 * http://gcc.gnu.org/onlinedocs/gcc-4.1.0/gcc/Atomic-Builtins.html
 */
static inline int cmpxchg(volatile int32_t *addr, int32_t old, int32_t new)
{
	return __sync_val_compare_and_swap(addr, old, new);
}

struct mutexint {
	volatile int32_t lock_val;
};

struct mutexint_attr {
	/* placeholder for future expansion */
	u_int32_t flags;
};

void sigmutexint_handler(int signo)
{
	printf("handled cancel signal: %d\n", signo);
}

int mutexint_init(struct mutexint *mutex, struct mutexint_attr *attr)
{
	mutex->lock_val = 0;
	/* future: process attr->flags */
	return 0;
}

int mutexint_lock(struct mutexint *mutex)
{
	int32_t val;
	int ret = 0;
	pid_t tid = gettid();
	printf("%s: initial lock_val: 0x%x\n", __FUNCTION__, mutex->lock_val);
	if ((val = cmpxchg(&mutex->lock_val, 0, tid)) != 0) {
		printf("\tfirst cmpxchg old_val: 0x%x lock_val: 0x%x\n", val, mutex->lock_val);
		do {
			printf("\tdo cmpxchg old_val: 0x%x\n", val);
			if ((val & FUTEX_WAITERS) || 
				(val = cmpxchg(&mutex->lock_val, val, val | FUTEX_WAITERS))) {
				printf("\tinner cmpxchg old_val: 0x%x lock_val: 0x%x\n", val, mutex->lock_val);
				printf("\tcalling futex_lock_pi:\n");
				printf("\top: 0x%x\n", FUTEX_LOCK_PI | FUTEX_PRIVATE | FUTEX_INTERRUPTIBLE);
				printf("\t&lock_val: %p\n", &mutex->lock_val);
				ret = futex_lock_pi(&mutex->lock_val, val, NULL);
				if (ret == -1) {
					ret = -errno;
					printf("futex_lock_pi returned -1, errno is %d\n", ret);
				}
				/*
				 * If -EINTR is returned, the lock may no longer
				 * have owners, but we have no good way to detect that.
				 */
				printf("\tfutex_lock_pi returned. ret=%d lock_val=0x%x\n",
				       ret, mutex->lock_val);
				if (ret != -EWOULDBLOCK)
					break;
			}
		} while ((val = cmpxchg(&mutex->lock_val, 0, tid)) != 0);
	}

	/* 
	 * Catch the case where futex_lock_pi() returns EWOULDBLOCK, but the
	 * while conditional acquires the lock atomically and reset ret.
	 */
	if ((val & ~FUTEX_WAITERS) == tid)
		ret = 0;

	printf("%s: returning: %d\n", __FUNCTION__, ret);
	return ret;
}

int mutexint_unlock(struct mutexint *mutex)
{
	int32_t val;
	int ret = 0;
	pid_t tid = gettid();
	if ((val = cmpxchg(&mutex->lock_val, tid, 0)) != 0) {
		ret = futex_unlock_pi(&mutex->lock_val, val);
	}
	return ret;
}

int mutexint_interrupt(pthread_t pthread, struct mutexint *mutex)
{
	/* 
	 * FIXME: return -EINVAL if the pthread is blocked on a different
	 * thread ?
	 */
	return pthread_kill(pthread, SIGMUTEXINT);
}

int mutexint_prepare()
{
	struct sigaction sa;
	sa.sa_handler = sigmutexint_handler;
	sigemptyset(&sa.sa_mask);
	sa.sa_flags = 0;
	if (sigaction(SIGMUTEXINT, &sa, NULL)) {
		perror("sigaction");
		return 1;
	}
	return 0;
}

/* unit test */
#define UNIT_TEST 1
#ifdef UNIT_TEST

struct mutexint mutex;

void *child_thread(void *arg)
{
	int ret;
	mutexint_prepare();

	printf("\tchild acquiring the mutex (will block)\n");
	ret = mutexint_lock(&mutex);
	printf("\tchild returned %d\n", ret);

	*(int *)arg = ret;
	return NULL;
}

int main(int argc, char *argv[])
{
	struct mutexint_attr m_attr;
	pthread_t child;
	int child_ret;
	int ret = 0;

	/* prepare the mutex, ensuring we use PROCESS_PRIVATE */
	mutexint_init(&mutex, &m_attr);

	printf("\nTesting canceled mutex lock scenario.\n");
	printf("\tmain() acquiring the mutex\n");
	mutexint_lock(&mutex);
	/* prepare and start the child thread */
	ret = pthread_create(&child, NULL, child_thread, &child_ret);
	if (ret) {
		perror("Failed to create pthread");
		return ret;
	}

	/* ensure the child has blocked on the lock */
	sleep(3);

	/* cancel the child thread blocking on the mutex */
	printf("\tmain() canceling the child\n");
	ret = mutexint_interrupt(child, &mutex);
	printf("\tmain() pthread_kill returned %d\n", ret);
	
	pthread_join(child, NULL);

	printf("Result: %s\n", child_ret == -EINTR ? "PASS" : "FAIL");
	return ret;
}
#endif

-- 
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team

Re: [PATCH] RFC: futex: make futex_lock_pi interruptible

[Arnd Bergmann]

On Tuesday 27 October 2009, Darren Hart wrote:
> The following C test case demonstrates how this patch could be used to
> implement interruptible locking. There is an awful lot of debug code and
> some other relics of a hacked together test in there now, but if anyone
> wanted to test the futex changes, this will do the trick.
> 

Your test program uses a signal handler to interrupt the mutex. If you
are using a signal handler already to implement a user space mutex_cancel,
why can't you just do a longjmp out of the signal handler rather than
modifying the kernel?

	Arnd <><

Re: [PATCH] RFC: futex: make futex_lock_pi interruptible

[Darren Hart]

Arnd Bergmann wrote:
> On Tuesday 27 October 2009, Darren Hart wrote:
>> The following C test case demonstrates how this patch could be used to
>> implement interruptible locking. There is an awful lot of debug code and
>> some other relics of a hacked together test in there now, but if anyone
>> wanted to test the futex changes, this will do the trick.
>>
> 
> Your test program uses a signal handler to interrupt the mutex. If you
> are using a signal handler already to implement a user space mutex_cancel,
> why can't you just do a longjmp out of the signal handler rather than
> modifying the kernel?

Interesting... so something like the following diff to the test case?

This works on an unmodfied kernel... although I certainly don't feel
like I understand what it's doing well enough to recommend someone use
this in conjunction with glibc pthread_mutexes and condvars. Seems like
their internal state could suffer from this sort of thing. Hrm... and
this warning in the man pages is worth heeding:

  setjmp() and sigsetjmp() make programs hard to understand and maintain.
  If possible an alternative should be used.

However, if that is the only concern, it's certainly worth risking to
avoid any further complication of the futex code (and glibc!). I'll take
a stab at a pure pthread_mutex implementation and see if anything breaks.
I also need to understand signal handling a bit better to decide if I need
the sigsetjmp and siglongjmp versions.

Thanks for the suggestion!

--
Darren


>From c4fb07a73f8ce6e7d137a7e42634dba819ac4b88 Mon Sep 17 00:00:00 2001
From: Darren Vincent Hart <darren@dvhart.com>
Date: Thu, 29 Oct 2009 18:11:32 -0700
Subject: [PATCH] longjmp implementation

---
 mutexint.c |   27 +++++++++++++++++++--------
 1 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/mutexint.c b/mutexint.c
index 003717f..82a608b 100644
--- a/mutexint.c
+++ b/mutexint.c
@@ -37,6 +37,7 @@
 #include <sys/types.h>
 #include <linux/futex.h>
 #include <errno.h>
+#include <setjmp.h>
 
 /*
  * The futex() call has been removed from the include/futex.h header, implement
@@ -44,7 +45,8 @@
  */
 //#define SYS_futex 202
 #define FUTEX_PRIVATE 128
-#define FUTEX_INTERRUPTIBLE 512
+//#define FUTEX_INTERRUPTIBLE 512
+#define FUTEX_INTERRUPTIBLE 0
 #define futex(uaddr, op, val, timeout, uaddr2, val3) \
 	syscall(SYS_futex, uaddr, op, val, timeout, uaddr2, val3)
 
@@ -59,6 +61,9 @@
 
 #define SIGMUTEXINT SIGRTMIN
 
+/* Need some kind of per-thread variable here */
+jmp_buf env;
+
 /*
  * Implement cmpxchg using gcc atomic builtins.
  * http://gcc.gnu.org/onlinedocs/gcc-4.1.0/gcc/Atomic-Builtins.html
@@ -80,6 +85,7 @@ struct mutexint_attr {
 void sigmutexint_handler(int signo)
 {
 	printf("handled cancel signal: %d\n", signo);
+	longjmp(env, 1);
 }
 
 int mutexint_init(struct mutexint *mutex, struct mutexint_attr *attr)
@@ -102,13 +108,18 @@ int mutexint_lock(struct mutexint *mutex)
 			if ((val & FUTEX_WAITERS) || 
 				(val = cmpxchg(&mutex->lock_val, val, val | FUTEX_WAITERS))) {
 				printf("\tinner cmpxchg old_val: 0x%x lock_val: 0x%x\n", val, mutex->lock_val);
-				printf("\tcalling futex_lock_pi:\n");
-				printf("\top: 0x%x\n", FUTEX_LOCK_PI | FUTEX_PRIVATE | FUTEX_INTERRUPTIBLE);
-				printf("\t&lock_val: %p\n", &mutex->lock_val);
-				ret = futex_lock_pi(&mutex->lock_val, val, NULL);
-				if (ret == -1) {
-					ret = -errno;
-					printf("futex_lock_pi returned -1, errno is %d\n", ret);
+				if (setjmp(env)) {
+					printf("mutexint_lock canceled, aborting\n");
+					ret = -EINTR;
+				} else {
+					printf("\tcalling futex_lock_pi:\n");
+					printf("\top: 0x%x\n", FUTEX_LOCK_PI | FUTEX_PRIVATE | FUTEX_INTERRUPTIBLE);
+					printf("\t&lock_val: %p\n", &mutex->lock_val);
+					ret = futex_lock_pi(&mutex->lock_val, val, NULL);
+					if (ret == -1) {
+						ret = -errno;
+						printf("futex_lock_pi returned -1, errno is %d\n", ret);
+					}
 				}
 				/*
 				 * If -EINTR is returned, the lock may no longer
-- 
1.6.0.4


-- 
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team

Re: [PATCH] RFC: futex: make futex_lock_pi interruptible

[Darren Hart]

Darren Hart wrote:
> Arnd Bergmann wrote:
>> On Tuesday 27 October 2009, Darren Hart wrote:
>>> The following C test case demonstrates how this patch could be used to
>>> implement interruptible locking. There is an awful lot of debug code and
>>> some other relics of a hacked together test in there now, but if anyone
>>> wanted to test the futex changes, this will do the trick.
>>>
>>
>> Your test program uses a signal handler to interrupt the mutex. If you
>> are using a signal handler already to implement a user space 
>> mutex_cancel,
>> why can't you just do a longjmp out of the signal handler rather than
>> modifying the kernel?
> 
> Interesting... so something like the following diff to the test case?
> 
> This works on an unmodfied kernel... although I certainly don't feel
> like I understand what it's doing well enough to recommend someone use
> this in conjunction with glibc pthread_mutexes and condvars. Seems like
> their internal state could suffer from this sort of thing. Hrm... and
> this warning in the man pages is worth heeding:
> 
>  setjmp() and sigsetjmp() make programs hard to understand and maintain.
>  If possible an alternative should be used.
> 
> However, if that is the only concern, it's certainly worth risking to
> avoid any further complication of the futex code (and glibc!). I'll take
> a stab at a pure pthread_mutex implementation and see if anything breaks.

This appears to work fine. Can anyone think of a reason why this is an unsafe
thing to do? I'll have to create a much more elaborate test case and review
the glibc code of course to make sure the glibc mutex state isn't compromised.
My original plan had been to patch glibc to handle the return of EINTR if the
mutex was tagged as PTHREAD_MUTEX_INTERRUPTIBLE_NP or something like that, this
approach appears to negate the need for such a thing - which is great.

Thoughts?

/******************************************************************************
 *
 *   Copyright © International Business Machines  Corp., 2009
 *
 *   This program is free software;  you can redistribute it and/or modify
 *   it under the terms of the GNU General Public License as published by
 *   the Free Software Foundation; either version 2 of the License, or
 *   (at your option) any later version.
 *
 *   This program is distributed in the hope that it will be useful,
 *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
 *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
 *   the GNU General Public License for more details.
 *
 *   You should have received a copy of the GNU General Public License
 *   along with this program;  if not, write to the Free Software
 *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 *
 * NAME
 *      pmutexint.c
 *
 * DESCRIPTION
 *      Use set/longjmp to interrupt a PI pthread_mutex.
 *
 * AUTHOR
 *      Darren Hart <dvhltc@us.ibm.com>
 *
 * HISTORY
 *      2009-Oct-29: Initial version by Darren Hart <dvhltc@us.ibm.com>
 *
 *****************************************************************************/

#define _GNU_SOURCE 1

#include <pthread.h>
#include <stdio.h>
#include <signal.h>
#include <syscall.h>
#include <sys/types.h>
#include <linux/futex.h>
#include <errno.h>
#include <string.h>
#include <setjmp.h>

#define SIGMUTEXINT SIGRTMIN

/* Need some kind of per-thread variable here */
jmp_buf env;
pthread_mutex_t mutex;

void sigmutexint_handler(int signo)
{
	printf("\thandled cancel signal: %d\n", signo);
	longjmp(env, 1);
}

void *child_thread(void *arg)
{
	int ret;
	struct sigaction sa;
	sa.sa_handler = sigmutexint_handler;
	sigemptyset(&sa.sa_mask);
	sa.sa_flags = 0;
	if (sigaction(SIGMUTEXINT, &sa, NULL)) {
		perror("sigaction");
		ret = 1;
		goto out;
	}

	printf("\tchild acquiring the mutex (will block)\n");
	if (setjmp(env)) {
		ret = EINTR;
	} else {
		ret = pthread_mutex_lock(&mutex);
	}
	printf("\tchild returning: %s\n", strerror(ret));
 out:
	*(int *)arg = -ret;
	return NULL;
}

int main(int argc, char *argv[])
{
	pthread_t child;
	pthread_mutexattr_t mattr;
	int child_ret;
	int ret = 0;

	pthread_mutexattr_init(&mattr);
	pthread_mutexattr_setprotocol(&mattr, PTHREAD_PRIO_INHERIT);
	pthread_mutex_init(&mutex, &mattr);

	printf("\nTesting canceled pthread_mutex lock scenario.\n");
	printf("\tmain() acquiring the mutex\n");
	pthread_mutex_lock(&mutex);

	/* prepare and start the child thread */
	ret = pthread_create(&child, NULL, child_thread, &child_ret);
	if (ret) {
		perror("Failed to create pthread");
		return ret;
	}

	/* ensure the child has blocked on the lock */
	sleep(1);

	/* cancel the child thread blocking on the mutex */
	printf("\tmain() canceling the child\n");
	ret = pthread_kill(child, SIGMUTEXINT);
	
	pthread_join(child, NULL);

	printf("Result: %s\n", child_ret == -EINTR ? "PASS" : "FAIL");
	return ret;
}

-- 
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team

Re: [PATCH] RFC: futex: make futex_lock_pi interruptible

[Arnd Bergmann]

On Friday 30 October 2009, Darren Hart wrote:
> Darren Hart wrote:
> This appears to work fine. Can anyone think of a reason why this is an unsafe
> thing to do? I'll have to create a much more elaborate test case and review
> the glibc code of course to make sure the glibc mutex state isn't compromised.

The only reason I can see against it is the need to use one of the
rt signal numbers from library code, which may conflict with other
users of the signal. Being able to avoid a signal altogether would
be really nice, as in the futex_cancel extension you mentioned.

> /* Need some kind of per-thread variable here */
> jmp_buf env;
> pthread_mutex_t mutex;

Maybe instead of per-thread variables (which should work
fine), you could do

typedef struct {
	jmp_buf env;
	pthread_mutex_t mutex;
} interruptible_mutex_t;

> 	/* ensure the child has blocked on the lock */
> 	sleep(1);

In a real application, you might want to add some logic to avoid
this kind of race. For the test case, you probably need to do it
with the sleep.

	Arnd <><

Re: [PATCH] RFC: futex: make futex_lock_pi interruptible

[Darren Hart]

Arnd Bergmann wrote:
> On Friday 30 October 2009, Darren Hart wrote:
>> Darren Hart wrote:
>> This appears to work fine. Can anyone think of a reason why this is an unsafe
>> thing to do? I'll have to create a much more elaborate test case and review
>> the glibc code of course to make sure the glibc mutex state isn't compromised.
> 
> The only reason I can see against it is the need to use one of the
> rt signal numbers from library code, which may conflict with other
> users of the signal. Being able to avoid a signal altogether would
> be really nice, as in the futex_cancel extension you mentioned.

For the reason you mention, consumption of a signal number, the 
futex_cancel extension was how I originally set out to tackle this. 
However, Thomas and Peter both seemed to feel that the signal approach 
was a more standard way of interrupting a unix system call. One trick to 
the futex_cancel approach will be identifying which thread to cancel - 
since no other futex operation is thread specific. I suspect just 
overloading one of the argument to pass a TID would address that nicely. 
  This would allow us to return ECANCELED from the kernel, which I think 
is a much more direct implementation.

Peter and Thomas, could you comment on why the signal approach might be 
preferred over the futex_cancel extension?

>> /* Need some kind of per-thread variable here */
>> jmp_buf env;
>> pthread_mutex_t mutex;
> 
> Maybe instead of per-thread variables (which should work
> fine), you could do
> 
> typedef struct {
> 	jmp_buf env;
> 	pthread_mutex_t mutex;
> } interruptible_mutex_t;


I don't quite follow. There will be a 1:many relationship between 
mutex:threads, but there should be a 1:1 relationship between 
threads:env. Since multiple threads can block on one mutex, the above 
struct wouldn't provide the necessary number of env to set the jmp point 
for each one.... am I misunderstanding your suggestion?

>> 	/* ensure the child has blocked on the lock */
>> 	sleep(1);
> 
> In a real application, you might want to add some logic to avoid
> this kind of race. For the test case, you probably need to do it
> with the sleep.

This would likely need to be handled within glibc, just as it manages 
the sequence counters for the condvars to deal with wake-up races.

-- 
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team

Re: [PATCH] RFC: futex: make futex_lock_pi interruptible

[Arnd Bergmann]

On Friday 30 October 2009 16:23:12 Darren Hart wrote:
> I don't quite follow. There will be a 1:many relationship between 
> mutex:threads, but there should be a 1:1 relationship between 
> threads:env. Since multiple threads can block on one mutex, the above 
> struct wouldn't provide the necessary number of env to set the jmp point 
> for each one.... am I misunderstanding your suggestion?
> 

No, you are totally right with this. My suggestion would not work.

	Arnd <><

Re: [PATCH] RFC: futex: make futex_lock_pi interruptible

[Chris Friesen]

On 10/29/2009 07:45 PM, Darren Hart wrote:

> This appears to work fine. Can anyone think of a reason why this is an unsafe
> thing to do? I'll have to create a much more elaborate test case and review
> the glibc code of course to make sure the glibc mutex state isn't compromised.

Setting aside the specific code details, I would suggest that you not
review the glibc code but rather review the glibc documentation and the
susv3/posix specifications.  That way, if it behaves according to the
spec but breaks glibc you can push for a patch to glibc.

If it happens to work with current glibc but is not standards-compliant,
then it could break in the future.

Chris

Re: [PATCH] RFC: futex: make futex_lock_pi interruptible

[Darren Hart]

Chris Friesen wrote:
> On 10/29/2009 07:45 PM, Darren Hart wrote:
> 
>> This appears to work fine. Can anyone think of a reason why this is an unsafe
>> thing to do? I'll have to create a much more elaborate test case and review
>> the glibc code of course to make sure the glibc mutex state isn't compromised.
> 
> Setting aside the specific code details, I would suggest that you not
> review the glibc code but rather review the glibc documentation and the
> susv3/posix specifications.  That way, if it behaves according to the
> spec but breaks glibc you can push for a patch to glibc.
> 
> If it happens to work with current glibc but is not standards-compliant,
> then it could break in the future.

Unfortunately, pthread_mutex_lock() by spec shall not return -EINTR nor 
is it a valid pthread_cancelation point. We can do the latter with a 
non-posix extension in glibc. In order to provide an 
interruptible/cancelable mutex, we would also be non-posix - at least, 
if we provided a mechanism directly within glibc.

If folks think this signal based approach is the why to go, then you are 
correct, we just need to make sure that the spec allows us to use 
set/longjmp in this context.



-- 
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team