Handle-based fast mutex owner tracking

To improve robustness of the fast mutex implementation in POSIX (and later on in native), it is better to track the mutex owner by handle instead of kernel object pointer. Therefore, this patch changes __xn_sys_current (xeno_set_current) so that it returns xnthread_handle(current_thread). It furthermore converts the POSIX mutex implementation to pick up and store the lock owner as handle in the kernel/user-shared mutex. Finally it ensures that at least POSIX threads always have an (anonymous) handle assigned.

As the value stored in the mutex variable is now an integer, we can switch over to xnarch_atomic_t, removing all atomic_intptr users.

Quoted 9 lines · Jan Kiszka

To improve robustness of the fast mutex implementation in POSIX (and later on in native), it is better to track the mutex owner by handle instead of kernel object pointer. Therefore, this patch changes __xn_sys_current (xeno_set_current) so that it returns xnthread_handle(current_thread). It furthermore converts the POSIX mutex implementation to pick up and store the lock owner as handle in the kernel/user-shared mutex. Finally it ensures that at least POSIX threads always have an (anonymous) handle assigned.

Quoted 2 lines

As the value stored in the mutex variable is now an integer, we can switch over to xnarch_atomic_t, removing all atomic_intptr users.

Ok. I do not know if this should be part of this patch, or in another one, but we should call xeno_set_current in the trampolines of all skins, so that they can use the native and posix mutexes.

This is another thing that I have left in a state of flux...

Quoted 16 lines · Gilles Chanteperdrix

Quoted 11 lines · Jan Kiszka

To improve robustness of the fast mutex implementation in POSIX (and later on in native), it is better to track the mutex owner by handle instead of kernel object pointer. Therefore, this patch changes __xn_sys_current (xeno_set_current) so that it returns xnthread_handle(current_thread). It furthermore converts the POSIX mutex implementation to pick up and store the lock owner as handle in the kernel/user-shared mutex. Finally it ensures that at least POSIX threads always have an (anonymous) handle assigned.

As the value stored in the mutex variable is now an integer, we can switch over to xnarch_atomic_t, removing all atomic_intptr users.

Ok. I do not know if this should be part of this patch, or in another one, but we should call xeno_set_current in the trampolines of all skins, so that they can use the native and posix mutexes.

Quoted 1 line

This is another thing that I have left in a state of flux...

Find it below (PATCH 4/3).

BTW, should we better invoke pthread_exit in xeno_set_current in case of a failure?

Quoted 18 lines · Jan Kiszka

Quoted 17 lines · Gilles Chanteperdrix

Quoted 11 lines · Jan Kiszka

To improve robustness of the fast mutex implementation in POSIX (and later on in native), it is better to track the mutex owner by handle instead of kernel object pointer. Therefore, this patch changes __xn_sys_current (xeno_set_current) so that it returns xnthread_handle(current_thread). It furthermore converts the POSIX mutex implementation to pick up and store the lock owner as handle in the kernel/user-shared mutex. Finally it ensures that at least POSIX threads always have an (anonymous) handle assigned.

As the value stored in the mutex variable is now an integer, we can switch over to xnarch_atomic_t, removing all atomic_intptr users.

Ok. I do not know if this should be part of this patch, or in another one, but we should call xeno_set_current in the trampolines of all skins, so that they can use the native and posix mutexes.

This is another thing that I have left in a state of flux...

Quoted 4 lines

Find it below (PATCH 4/3).

BTW, should we better invoke pthread_exit in xeno_set_current in case of a failure?

I prefer my application to die. This way I would know that something went wrong. And by the way, how could this syscall fail ? I mean if xenomai or the posix skin are not loaded, we would have known it earlier.

Quoted 23 lines · Gilles Chanteperdrix

Quoted 22 lines · Jan Kiszka

Quoted 10 lines · Gilles Chanteperdrix

Quoted 9 lines · Jan Kiszka

To improve robustness of the fast mutex implementation in POSIX (and later on in native), it is better to track the mutex owner by handle instead of kernel object pointer. Therefore, this patch changes __xn_sys_current (xeno_set_current) so that it returns xnthread_handle(current_thread). It furthermore converts the POSIX mutex implementation to pick up and store the lock owner as handle in the kernel/user-shared mutex. Finally it ensures that at least POSIX threads always have an (anonymous) handle assigned.

Quoted 7 lines

Quoted 2 lines

As the value stored in the mutex variable is now an integer, we can switch over to xnarch_atomic_t, removing all atomic_intptr users.

Ok. I do not know if this should be part of this patch, or in another one, but we should call xeno_set_current in the trampolines of all skins, so that they can use the native and posix mutexes.

This is another thing that I have left in a state of flux...

Find it below (PATCH 4/3).

BTW, should we better invoke pthread_exit in xeno_set_current in case of a failure?

Quoted 3 lines

I prefer my application to die. This way I would know that something went wrong. And by the way, how could this syscall fail ? I mean if xenomai or the posix skin are not loaded, we would have known it earlier.

So far this syscall can trigger a thread handle registration if the current thread has no handle yet. But that could go away if all skins ensure that there is at least an anonymous handle for their threads after creation.

Quoted 23 lines · Gilles Chanteperdrix

Quoted 22 lines · Jan Kiszka

Quoted 10 lines · Gilles Chanteperdrix

Quoted 9 lines · Jan Kiszka

To improve robustness of the fast mutex implementation in POSIX (and later on in native), it is better to track the mutex owner by handle instead of kernel object pointer. Therefore, this patch changes __xn_sys_current (xeno_set_current) so that it returns xnthread_handle(current_thread). It furthermore converts the POSIX mutex implementation to pick up and store the lock owner as handle in the kernel/user-shared mutex. Finally it ensures that at least POSIX threads always have an (anonymous) handle assigned.

Quoted 7 lines

Quoted 2 lines

As the value stored in the mutex variable is now an integer, we can switch over to xnarch_atomic_t, removing all atomic_intptr users.

Ok. I do not know if this should be part of this patch, or in another one, but we should call xeno_set_current in the trampolines of all skins, so that they can use the native and posix mutexes.

This is another thing that I have left in a state of flux...

Find it below (PATCH 4/3).

BTW, should we better invoke pthread_exit in xeno_set_current in case of a failure?

Quoted 3 lines

I prefer my application to die. This way I would know that something went wrong. And by the way, how could this syscall fail ? I mean if xenomai or the posix skin are not loaded, we would have known it earlier.

So far this syscall can trigger a thread handle registration if the current thread has no handle yet. But that could go away if all skins ensure that there is at least an anonymous handle for their threads after creation.

ksrc/skins/posix/cb_lock.h +0 -4

-#define test_claimed(owner) ((long) (owner) & 1)
-#define clear_claimed(owner) ((xnthread_t *) ((long) (owner) & ~1))
-#define set_claimed(owner, bit) \
-        ((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))

Index: b/ksrc/skins/posix/thread.c
===================================================================

ksrc/skins/posix/thread.c +1 -0

@@ -28,6 +28,7 @@
  * 
  *@{*/
 
+#include <nucleus/registry.h>
 #include <posix/thread.h>
 #include <posix/cancel.h>
 #include <posix/timer.h>

Quoted 4 lines · Jan Kiszka

ksrc/skins/posix/cb_lock.h +0 -4

-#define test_claimed(owner) ((long) (owner) & 1)
-#define clear_claimed(owner) ((xnthread_t *) ((long) (owner) & ~1))
-#define set_claimed(owner, bit) \
-        ((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))

ksrc/skins/posix/cb_lock.h line 21

Quoted 10 lines

ksrc/skins/posix/cb_lock.h +10 -0

+#define __CLAIMED_BIT		XN_HANDLE_SPARE3
+
+#define test_claimed(owner)	xnhandle_test_spare(owner, __CLAIMED_BIT)
+#define clear_claimed(owner)	xnhandle_mask_spare(owner)
+#define set_claimed(owner, bit) ({ \
+	xnhandle_t __tmp = xnhandle_mask_spare(owner); \
+	if (bit) \
+		xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
+	__tmp; \
+})

I liked doing this with no conditional.

ksrc/skins/posix/cond.c static inline int mutex_save_count(xnthr · line 246

Quoted 4 lines

ksrc/skins/posix/cond.c +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

ksrc/skins/posix/mutex.h static inline int pse51_mutex_timedlock_ · line 172

Quoted 6 lines

ksrc/skins/posix/mutex.h +4 -0

+	/* Consistency check for owner handle - is the object a thread? */
+	if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
+		err = -EINVAL;
+		goto error;
 	}

What is this ?

Quoted 18 lines

Index: b/ksrc/skins/posix/thread.c
===================================================================

ksrc/skins/posix/thread.c +1 -0

@@ -28,6 +28,7 @@
  * 
  *@{*/
 
+#include <nucleus/registry.h>
 #include <posix/thread.h>
 #include <posix/cancel.h>
 #include <posix/timer.h>

@@ -229,6 +230,13 @@ int pthread_create(pthread_t *tid,
 	appendq(thread->container, &thread->link);
 	xnlock_put_irqrestore(&nklock, s);
 

ksrc/skins/posix/thread.c int pthread_create(pthread_t *tid, · line 239

Quoted 7 lines

ksrc/skins/posix/thread.c +7 -0

+#ifdef CONFIG_XENO_OPT_REGISTRY
+	/* We need an anonymous registry entry to obtain a handle for fast
+	   mutex locking. */
+	xnregistry_enter("", &thread->threadbase,
+			 &xnthread_handle(&thread->threadbase), NULL);
+#endif /* CONFIG_XENO_OPT_REGISTRY */
+

Since we need this for all threads (I want a thread from any skin to be able to use the posix skin mutexes), why doing it in the skin ?

Quoted 17 lines · Gilles Chanteperdrix

Quoted 4 lines · Jan Kiszka

ksrc/skins/posix/cb_lock.h +0 -4

-#define test_claimed(owner) ((long) (owner) & 1)
-#define clear_claimed(owner) ((xnthread_t *) ((long) (owner) & ~1))
-#define set_claimed(owner, bit) \
-        ((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))

ksrc/skins/posix/cb_lock.h line 21

Quoted 10 lines

ksrc/skins/posix/cb_lock.h +10 -0

+#define __CLAIMED_BIT		XN_HANDLE_SPARE3
+
+#define test_claimed(owner)	xnhandle_test_spare(owner, __CLAIMED_BIT)
+#define clear_claimed(owner)	xnhandle_mask_spare(owner)
+#define set_claimed(owner, bit) ({ \
+	xnhandle_t __tmp = xnhandle_mask_spare(owner); \
+	if (bit) \
+		xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
+	__tmp; \
+})

I liked doing this with no conditional.

You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter would require some renaming then, I guess)?

Quoted 5 lines

ksrc/skins/posix/cond.c static inline int mutex_save_count(xnthr · line 246

Quoted 3 lines

ksrc/skins/posix/cond.c +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

OK. Will you fix? I will rebase then.

Quoted 6 lines

ksrc/skins/posix/mutex.h static inline int pse51_mutex_timedlock_ · line 172

Quoted 5 lines

ksrc/skins/posix/mutex.h +4 -0

+	/* Consistency check for owner handle - is the object a thread? */
+	if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
+		err = -EINVAL;
+		goto error;
 	}

What is this ?

This ensures that we are not dereferences some arbitrary registry object due to a borken handle in the mutex variable: The the object registered on this handle is an xnthread, we will find the very same handle value at the well-known place in the object. Kind of magic for xnthread objects (instead of adding a magic field to them).

Quoted 34 lines · Jan Kiszka

Quoted 17 lines · Gilles Chanteperdrix

Quoted 4 lines · Jan Kiszka

ksrc/skins/posix/cb_lock.h +0 -4

-#define test_claimed(owner) ((long) (owner) & 1)
-#define clear_claimed(owner) ((xnthread_t *) ((long) (owner) & ~1))
-#define set_claimed(owner, bit) \
-        ((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))

ksrc/skins/posix/cb_lock.h line 21

Quoted 10 lines

ksrc/skins/posix/cb_lock.h +10 -0

+#define __CLAIMED_BIT		XN_HANDLE_SPARE3
+
+#define test_claimed(owner)	xnhandle_test_spare(owner, __CLAIMED_BIT)
+#define clear_claimed(owner)	xnhandle_mask_spare(owner)
+#define set_claimed(owner, bit) ({ \
+	xnhandle_t __tmp = xnhandle_mask_spare(owner); \
+	if (bit) \
+		xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
+	__tmp; \
+})

I liked doing this with no conditional.

You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter would require some renaming then, I guess)?

Quoted 5 lines

ksrc/skins/posix/cond.c static inline int mutex_save_count(xnthr · line 246

Quoted 3 lines

ksrc/skins/posix/cond.c +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

OK. Will you fix? I will rebase then.

Quoted 6 lines

ksrc/skins/posix/mutex.h static inline int pse51_mutex_timedlock_ · line 172

Quoted 5 lines

ksrc/skins/posix/mutex.h +4 -0

+	/* Consistency check for owner handle - is the object a thread? */
+	if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
+		err = -EINVAL;
+		goto error;
 	}

What is this ?

Quoted 5 lines

This ensures that we are not dereferences some arbitrary registry object due to a borken handle in the mutex variable: The the object registered on this handle is an xnthread, we will find the very same handle value at the well-known place in the object. Kind of magic for xnthread objects (instead of adding a magic field to them).

Yes, but the point is: how can the handle be borken ? It looks like a useless check to me.

Quoted 40 lines · Gilles Chanteperdrix

Quoted 39 lines · Jan Kiszka

Quoted 17 lines · Gilles Chanteperdrix

Quoted 4 lines · Jan Kiszka

ksrc/skins/posix/cb_lock.h +0 -4

-#define test_claimed(owner) ((long) (owner) & 1)
-#define clear_claimed(owner) ((xnthread_t *) ((long) (owner) & ~1))
-#define set_claimed(owner, bit) \
-        ((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))

ksrc/skins/posix/cb_lock.h line 21

Quoted 10 lines

ksrc/skins/posix/cb_lock.h +10 -0

+#define __CLAIMED_BIT		XN_HANDLE_SPARE3
+
+#define test_claimed(owner)	xnhandle_test_spare(owner, __CLAIMED_BIT)
+#define clear_claimed(owner)	xnhandle_mask_spare(owner)
+#define set_claimed(owner, bit) ({ \
+	xnhandle_t __tmp = xnhandle_mask_spare(owner); \
+	if (bit) \
+		xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
+	__tmp; \
+})

I liked doing this with no conditional.

You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter would require some renaming then, I guess)?

Quoted 5 lines

ksrc/skins/posix/cond.c static inline int mutex_save_count(xnthr · line 246

Quoted 3 lines

ksrc/skins/posix/cond.c +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

OK. Will you fix? I will rebase then.

Quoted 6 lines

ksrc/skins/posix/mutex.h static inline int pse51_mutex_timedlock_ · line 172

Quoted 5 lines

ksrc/skins/posix/mutex.h +4 -0

+	/* Consistency check for owner handle - is the object a thread? */
+	if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
+		err = -EINVAL;
+		goto error;
 	}

What is this ?

This ensures that we are not dereferences some arbitrary registry object due to a borken handle in the mutex variable: The the object registered on this handle is an xnthread, we will find the very same handle value at the well-known place in the object. Kind of magic for xnthread objects (instead of adding a magic field to them).

Quoted 2 lines

Yes, but the point is: how can the handle be borken ? It looks like a useless check to me.

This is user memory. Everything can be borken. And I don't want to create yet another source for user-triggered kernel bugs (including silent ones) like with our heap management structures that are corruptible by userland. That's headache guarantee.

Quoted 43 lines · Jan Kiszka

Quoted 42 lines · Gilles Chanteperdrix

Quoted 34 lines · Jan Kiszka

Quoted 17 lines · Gilles Chanteperdrix

Quoted 4 lines · Jan Kiszka

ksrc/skins/posix/cb_lock.h +0 -4

-#define test_claimed(owner) ((long) (owner) & 1)
-#define clear_claimed(owner) ((xnthread_t *) ((long) (owner) & ~1))
-#define set_claimed(owner, bit) \
-        ((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))

ksrc/skins/posix/cb_lock.h line 21

Quoted 10 lines

ksrc/skins/posix/cb_lock.h +10 -0

+#define __CLAIMED_BIT		XN_HANDLE_SPARE3
+
+#define test_claimed(owner)	xnhandle_test_spare(owner, __CLAIMED_BIT)
+#define clear_claimed(owner)	xnhandle_mask_spare(owner)
+#define set_claimed(owner, bit) ({ \
+	xnhandle_t __tmp = xnhandle_mask_spare(owner); \
+	if (bit) \
+		xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
+	__tmp; \
+})

I liked doing this with no conditional.

You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter would require some renaming then, I guess)?

Quoted 5 lines

ksrc/skins/posix/cond.c static inline int mutex_save_count(xnthr · line 246

Quoted 3 lines

ksrc/skins/posix/cond.c +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

OK. Will you fix? I will rebase then.

Quoted 6 lines

ksrc/skins/posix/mutex.h static inline int pse51_mutex_timedlock_ · line 172

Quoted 5 lines

ksrc/skins/posix/mutex.h +4 -0

+	/* Consistency check for owner handle - is the object a thread? */
+	if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
+		err = -EINVAL;
+		goto error;
 	}

What is this ?

Quoted 5 lines

This ensures that we are not dereferences some arbitrary registry object due to a borken handle in the mutex variable: The the object registered on this handle is an xnthread, we will find the very same handle value at the well-known place in the object. Kind of magic for xnthread objects (instead of adding a magic field to them).

Yes, but the point is: how can the handle be borken ? It looks like a useless check to me.

Quoted 4 lines

This is user memory. Everything can be borken. And I don't want to create yet another source for user-triggered kernel bugs (including silent ones) like with our heap management structures that are corruptible by userland. That's headache guarantee.

No, we assume that the user application will only clobber our heap if it is broken too. Add this with a XENO_DEBUG option if you want (not XENO_DEBUG(POSIX)).

Quoted 48 lines · Gilles Chanteperdrix

Quoted 47 lines · Jan Kiszka

Quoted 35 lines · Gilles Chanteperdrix

Quoted 34 lines · Jan Kiszka

Quoted 17 lines · Gilles Chanteperdrix

Quoted 4 lines · Jan Kiszka

(quoted diff) +0 -4

-#define test_claimed(owner) ((long) (owner) & 1)
-#define clear_claimed(owner) ((xnthread_t *) ((long) (owner) & ~1))
-#define set_claimed(owner, bit) \
-        ((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))

Review

Quoted 10 lines

(quoted diff) +10 -0

+#define __CLAIMED_BIT		XN_HANDLE_SPARE3
+
+#define test_claimed(owner)	xnhandle_test_spare(owner, __CLAIMED_BIT)
+#define clear_claimed(owner)	xnhandle_mask_spare(owner)
+#define set_claimed(owner, bit) ({ \
+	xnhandle_t __tmp = xnhandle_mask_spare(owner); \
+	if (bit) \
+		xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
+	__tmp; \
+})

I liked doing this with no conditional.

You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter would require some renaming then, I guess)?

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

OK. Will you fix? I will rebase then.

Quoted 6 lines

Review

Quoted 5 lines

(quoted diff) +4 -0

+	/* Consistency check for owner handle - is the object a thread? */
+	if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
+		err = -EINVAL;
+		goto error;
 	}

What is this ?

Quoted 7 lines

Quoted 5 lines

This ensures that we are not dereferences some arbitrary registry object due to a borken handle in the mutex variable: The the object registered on this handle is an xnthread, we will find the very same handle value at the well-known place in the object. Kind of magic for xnthread objects (instead of adding a magic field to them).

Yes, but the point is: how can the handle be borken ? It looks like a useless check to me.

This is user memory. Everything can be borken. And I don't want to create yet another source for user-triggered kernel bugs (including silent ones) like with our heap management structures that are corruptible by userland. That's headache guarantee.

Quoted 3 lines

No, we assume that the user application will only clobber our heap if it is broken too. Add this with a XENO_DEBUG option if you want (not XENO_DEBUG(POSIX)).

No problem for me. This check is almost 100% identical (specifically overhead-wise) to the usual object magic checks after querying the registry, and as those are unconditionally performed, I saw no need to handle this case differently.

Quoted 49 lines · Jan Kiszka

Quoted 48 lines · Gilles Chanteperdrix

Quoted 47 lines · Jan Kiszka

Quoted 35 lines · Gilles Chanteperdrix

Quoted 34 lines · Jan Kiszka

Quoted 17 lines · Gilles Chanteperdrix

Quoted 4 lines · Jan Kiszka

(quoted diff) +0 -4

-#define test_claimed(owner) ((long) (owner) & 1)
-#define clear_claimed(owner) ((xnthread_t *) ((long) (owner) & ~1))
-#define set_claimed(owner, bit) \
-        ((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))

Review

Quoted 10 lines

(quoted diff) +10 -0

+#define __CLAIMED_BIT		XN_HANDLE_SPARE3
+
+#define test_claimed(owner)	xnhandle_test_spare(owner, __CLAIMED_BIT)
+#define clear_claimed(owner)	xnhandle_mask_spare(owner)
+#define set_claimed(owner, bit) ({ \
+	xnhandle_t __tmp = xnhandle_mask_spare(owner); \
+	if (bit) \
+		xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
+	__tmp; \
+})

I liked doing this with no conditional.

You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter would require some renaming then, I guess)?

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

OK. Will you fix? I will rebase then.

Quoted 6 lines

Review

Quoted 5 lines

(quoted diff) +4 -0

+	/* Consistency check for owner handle - is the object a thread? */
+	if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
+		err = -EINVAL;
+		goto error;
 	}

What is this ?

Quoted 7 lines

Quoted 5 lines

This ensures that we are not dereferences some arbitrary registry object due to a borken handle in the mutex variable: The the object registered on this handle is an xnthread, we will find the very same handle value at the well-known place in the object. Kind of magic for xnthread objects (instead of adding a magic field to them).

Yes, but the point is: how can the handle be borken ? It looks like a useless check to me.

This is user memory. Everything can be borken. And I don't want to create yet another source for user-triggered kernel bugs (including silent ones) like with our heap management structures that are corruptible by userland. That's headache guarantee.

Quoted 7 lines

Quoted 3 lines

No, we assume that the user application will only clobber our heap if it is broken too. Add this with a XENO_DEBUG option if you want (not XENO_DEBUG(POSIX)).

No problem for me. This check is almost 100% identical (specifically overhead-wise) to the usual object magic checks after querying the registry, and as those are unconditionally performed, I saw no need to handle this case differently.

But the registry lookup is already a validation in itself. I mean, if an application uses 5 as a file descriptor, there are only two cases:
- the file descriptor 5 exists in the application and the kernel has no other choice than to believe that the application is using this file descriptor, and nothing will go wrong because this is a valid descriptor;
- the file descriptor does not exist, and we will return EBADF, and nothing wrong will happen either...

Quoted 57 lines · Gilles Chanteperdrix

Quoted 49 lines · Jan Kiszka

Quoted 48 lines · Gilles Chanteperdrix

Quoted 47 lines · Jan Kiszka

Quoted 35 lines · Gilles Chanteperdrix

Quoted 34 lines · Jan Kiszka

Quoted 17 lines · Gilles Chanteperdrix

Quoted 4 lines · Jan Kiszka

(quoted diff) +0 -4

-#define test_claimed(owner) ((long) (owner) & 1)
-#define clear_claimed(owner) ((xnthread_t *) ((long) (owner) & ~1))
-#define set_claimed(owner, bit) \
-        ((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))

Review

Quoted 10 lines

(quoted diff) +10 -0

+#define __CLAIMED_BIT		XN_HANDLE_SPARE3
+
+#define test_claimed(owner)	xnhandle_test_spare(owner, __CLAIMED_BIT)
+#define clear_claimed(owner)	xnhandle_mask_spare(owner)
+#define set_claimed(owner, bit) ({ \
+	xnhandle_t __tmp = xnhandle_mask_spare(owner); \
+	if (bit) \
+		xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
+	__tmp; \
+})

I liked doing this with no conditional.

You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter would require some renaming then, I guess)?

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

OK. Will you fix? I will rebase then.

Quoted 6 lines

Review

Quoted 5 lines

(quoted diff) +4 -0

+	/* Consistency check for owner handle - is the object a thread? */
+	if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
+		err = -EINVAL;
+		goto error;
 	}

What is this ?

Quoted 7 lines

Quoted 5 lines

This ensures that we are not dereferences some arbitrary registry object due to a borken handle in the mutex variable: The the object registered on this handle is an xnthread, we will find the very same handle value at the well-known place in the object. Kind of magic for xnthread objects (instead of adding a magic field to them).

Yes, but the point is: how can the handle be borken ? It looks like a useless check to me.

This is user memory. Everything can be borken. And I don't want to create yet another source for user-triggered kernel bugs (including silent ones) like with our heap management structures that are corruptible by userland. That's headache guarantee.

Quoted 7 lines

Quoted 3 lines

No, we assume that the user application will only clobber our heap if it is broken too. Add this with a XENO_DEBUG option if you want (not XENO_DEBUG(POSIX)).

No problem for me. This check is almost 100% identical (specifically overhead-wise) to the usual object magic checks after querying the registry, and as those are unconditionally performed, I saw no need to handle this case differently.

Quoted 7 lines

But the registry lookup is already a validation in itself. I mean, if an application uses 5 as a file descriptor, there are only two cases:
- the file descriptor 5 exists in the application and the kernel has no other choice than to believe that the application is using this file descriptor, and nothing will go wrong because this is a valid descriptor;
- the file descriptor does not exist, and we will return EBADF, and nothing wrong will happen either...

File descriptors are all identically structured objects, so at worst you ruin some other app's day. But the registry contains arbitrary objects with different internal layout. If you start assuming object_a * is object_b * and use the pointer etc. included in a as if they have the meaning of b, you quickly ruin the kernel's day as well. Therefore, native, e.g., does magic checks after fetching from the registry. As I said, this test here works differently, but it has the same effect and impact.

File descriptors are all identically structured objects, so at worst you ruin some other app's day. But the registry contains arbitrary objects with different internal layout. If you start assuming object_a * is object_b * and use the pointer etc. included in a as if they have the meaning of b, you quickly ruin the kernel's day as well. Therefore, native, e.g., does magic checks after fetching from the registry. As I said, this test here works differently, but it has the same effect and impact.

Quoted 8 lines · Jan Kiszka

File descriptors are all identically structured objects, so at worst you ruin some other app's day. But the registry contains arbitrary objects with different internal layout. If you start assuming object_a * is object_b * and use the pointer etc. included in a as if they have the meaning of b, you quickly ruin the kernel's day as well. Therefore, native, e.g., does magic checks after fetching from the registry. As I said, this test here works differently, but it has the same effect and impact.

By the way, would not it make sense to have separate hash tables for separate objects types ? I mean then we would not need any validation, and several object types could use the same name.

From that POV a good idea. The only issue I see is a management problem:

Quoted 9 lines · Gilles Chanteperdrix

Quoted 8 lines · Jan Kiszka

File descriptors are all identically structured objects, so at worst you ruin some other app's day. But the registry contains arbitrary objects with different internal layout. If you start assuming object_a * is object_b * and use the pointer etc. included in a as if they have the meaning of b, you quickly ruin the kernel's day as well. Therefore, native, e.g., does magic checks after fetching from the registry. As I said, this test here works differently, but it has the same effect and impact.

Quoted 3 lines

By the way, would not it make sense to have separate hash tables for separate objects types ? I mean then we would not need any validation, and several object types could use the same name.

From that POV a good idea. The only issue I see is a management problem:
How many mutex, thread, queue, whatever slots do you want in your system? One knob for them all? Or countless knobs for all object types of all skins? That's hairy, I'm afraid.

Quoted 13 lines · Jan Kiszka

Quoted 12 lines · Gilles Chanteperdrix

Quoted 8 lines · Jan Kiszka

File descriptors are all identically structured objects, so at worst you ruin some other app's day. But the registry contains arbitrary objects with different internal layout. If you start assuming object_a * is object_b * and use the pointer etc. included in a as if they have the meaning of b, you quickly ruin the kernel's day as well. Therefore, native, e.g., does magic checks after fetching from the registry. As I said, this test here works differently, but it has the same effect and impact.

By the way, would not it make sense to have separate hash tables for separate objects types ? I mean then we would not need any validation, and several object types could use the same name.

Quoted 4 lines

From that POV a good idea. The only issue I see is a management problem:
How many mutex, thread, queue, whatever slots do you want in your system? One knob for them all? Or countless knobs for all object types of all skins? That's hairy, I'm afraid.

xnmalloc, the pool size is the limit.

Quoted 14 lines · Gilles Chanteperdrix

Quoted 13 lines · Jan Kiszka

Quoted 12 lines · Gilles Chanteperdrix

Quoted 8 lines · Jan Kiszka

File descriptors are all identically structured objects, so at worst you ruin some other app's day. But the registry contains arbitrary objects with different internal layout. If you start assuming object_a * is object_b * and use the pointer etc. included in a as if they have the meaning of b, you quickly ruin the kernel's day as well. Therefore, native, e.g., does magic checks after fetching from the registry. As I said, this test here works differently, but it has the same effect and impact.

By the way, would not it make sense to have separate hash tables for separate objects types ? I mean then we would not need any validation, and several object types could use the same name.

Quoted 5 lines

Quoted 4 lines

From that POV a good idea. The only issue I see is a management problem:
How many mutex, thread, queue, whatever slots do you want in your system? One knob for them all? Or countless knobs for all object types of all skins? That's hairy, I'm afraid.

xnmalloc, the pool size is the limit.

You mean kind of "xnrealloc", including atomically copying potentially large descriptor tables over? Sounds not that attractive.

Quoted 19 lines · Jan Kiszka

Quoted 18 lines · Gilles Chanteperdrix

Quoted 17 lines · Jan Kiszka

Quoted 9 lines · Gilles Chanteperdrix

Quoted 8 lines · Jan Kiszka

File descriptors are all identically structured objects, so at worst you ruin some other app's day. But the registry contains arbitrary objects with different internal layout. If you start assuming object_a * is object_b * and use the pointer etc. included in a as if they have the meaning of b, you quickly ruin the kernel's day as well. Therefore, native, e.g., does magic checks after fetching from the registry. As I said, this test here works differently, but it has the same effect and impact.

Quoted 3 lines

By the way, would not it make sense to have separate hash tables for separate objects types ? I mean then we would not need any validation, and several object types could use the same name.

From that POV a good idea. The only issue I see is a management problem:
How many mutex, thread, queue, whatever slots do you want in your system? One knob for them all? Or countless knobs for all object types of all skins? That's hairy, I'm afraid.

Quoted 3 lines

Quoted 1 line

xnmalloc, the pool size is the limit.

You mean kind of "xnrealloc", including atomically copying potentially large descriptor tables over? Sounds not that attractive.

No, I mean the hash table contains pointers, and the objects are allocated dynamically...

Quoted 20 lines · Gilles Chanteperdrix

Quoted 19 lines · Jan Kiszka

Quoted 18 lines · Gilles Chanteperdrix

Quoted 17 lines · Jan Kiszka

Quoted 9 lines · Gilles Chanteperdrix

Quoted 8 lines · Jan Kiszka

File descriptors are all identically structured objects, so at worst you ruin some other app's day. But the registry contains arbitrary objects with different internal layout. If you start assuming object_a * is object_b * and use the pointer etc. included in a as if they have the meaning of b, you quickly ruin the kernel's day as well. Therefore, native, e.g., does magic checks after fetching from the registry. As I said, this test here works differently, but it has the same effect and impact.

Quoted 3 lines

By the way, would not it make sense to have separate hash tables for separate objects types ? I mean then we would not need any validation, and several object types could use the same name.

From that POV a good idea. The only issue I see is a management problem:
How many mutex, thread, queue, whatever slots do you want in your system? One knob for them all? Or countless knobs for all object types of all skins? That's hairy, I'm afraid.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

xnmalloc, the pool size is the limit.

You mean kind of "xnrealloc", including atomically copying potentially large descriptor tables over? Sounds not that attractive.

No, I mean the hash table contains pointers, and the objects are allocated dynamically...

Handle lookup is not going through hash tables. It's index based (like file descriptors).

Quoted 17 lines · Gilles Chanteperdrix

Quoted 4 lines · Jan Kiszka

ksrc/skins/posix/cb_lock.h +0 -4

-#define test_claimed(owner) ((long) (owner) & 1)
-#define clear_claimed(owner) ((xnthread_t *) ((long) (owner) & ~1))
-#define set_claimed(owner, bit) \
-        ((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))

ksrc/skins/posix/cb_lock.h line 21

Quoted 10 lines

ksrc/skins/posix/cb_lock.h +10 -0

+#define __CLAIMED_BIT		XN_HANDLE_SPARE3
+
+#define test_claimed(owner)	xnhandle_test_spare(owner, __CLAIMED_BIT)
+#define clear_claimed(owner)	xnhandle_mask_spare(owner)
+#define set_claimed(owner, bit) ({ \
+	xnhandle_t __tmp = xnhandle_mask_spare(owner); \
+	if (bit) \
+		xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
+	__tmp; \
+})

I liked doing this with no conditional.

You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter would require some renaming then, I guess)?

Quoted 5 lines

ksrc/skins/posix/cond.c static inline int mutex_save_count(xnthr · line 246

Quoted 3 lines

ksrc/skins/posix/cond.c +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

OK. Will you fix? I will rebase then.

Quoted 6 lines

ksrc/skins/posix/mutex.h static inline int pse51_mutex_timedlock_ · line 172

Quoted 5 lines

ksrc/skins/posix/mutex.h +4 -0

+	/* Consistency check for owner handle - is the object a thread? */
+	if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
+		err = -EINVAL;
+		goto error;
 	}

What is this ?

This ensures that we are not dereferences some arbitrary registry object due to a borken handle in the mutex variable: The the object registered on this handle is an xnthread, we will find the very same handle value at the well-known place in the object. Kind of magic for xnthread objects (instead of adding a magic field to them).

Quoted 17 lines

Quoted 17 lines

Index: b/ksrc/skins/posix/thread.c
===================================================================

ksrc/skins/posix/thread.c +1 -0

@@ -28,6 +28,7 @@
  * 
  *@{*/
 
+#include <nucleus/registry.h>
 #include <posix/thread.h>
 #include <posix/cancel.h>
 #include <posix/timer.h>

@@ -229,6 +230,13 @@ int pthread_create(pthread_t *tid,
 	appendq(thread->container, &thread->link);
 	xnlock_put_irqrestore(&nklock, s);
 

Quoted 9 lines

ksrc/skins/posix/thread.c int pthread_create(pthread_t *tid, · line 239

Quoted 7 lines

ksrc/skins/posix/thread.c +7 -0

+#ifdef CONFIG_XENO_OPT_REGISTRY
+	/* We need an anonymous registry entry to obtain a handle for fast
+	   mutex locking. */
+	xnregistry_enter("", &thread->threadbase,
+			 &xnthread_handle(&thread->threadbase), NULL);
+#endif /* CONFIG_XENO_OPT_REGISTRY */
+

Since we need this for all threads (I want a thread from any skin to be able to use the posix skin mutexes), why doing it in the skin ?

Some skins register their threads unconditionally, some based on names, some not at all. The "not at all" case has to be fixed, but we need to respect the other two as well.

Quoted 17 lines · Gilles Chanteperdrix

Quoted 4 lines · Jan Kiszka

ksrc/skins/posix/cb_lock.h +0 -4

-#define test_claimed(owner) ((long) (owner) & 1)
-#define clear_claimed(owner) ((xnthread_t *) ((long) (owner) & ~1))
-#define set_claimed(owner, bit) \
-        ((xnthread_t *) ((long) clear_claimed(owner) | !!(bit)))

ksrc/skins/posix/cb_lock.h line 21

Quoted 10 lines

ksrc/skins/posix/cb_lock.h +10 -0

+#define __CLAIMED_BIT		XN_HANDLE_SPARE3
+
+#define test_claimed(owner)	xnhandle_test_spare(owner, __CLAIMED_BIT)
+#define clear_claimed(owner)	xnhandle_mask_spare(owner)
+#define set_claimed(owner, bit) ({ \
+	xnhandle_t __tmp = xnhandle_mask_spare(owner); \
+	if (bit) \
+		xnhandle_set_spare(__tmp, __CLAIMED_BIT); \
+	__tmp; \
+})

I liked doing this with no conditional.

You mean let the caller pass 'bit' as 0 or __CLAIMED_BIT (the latter would require some renaming then, I guess)?

ksrc/skins/posix/cond.c static inline int mutex_save_count(xnthr · line 246

Quoted 3 lines · Jan Kiszka

ksrc/skins/posix/cond.c +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

Quoted 6 lines · Gilles Chanteperdrix

ksrc/skins/posix/cond.c static inline int mutex_save_count(xnthr · line 246

Quoted 3 lines · Jan Kiszka

ksrc/skins/posix/cond.c +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

BTW, why do you need to track sleepers separately in POSIX? Native doesn't do so, e.g.

Quoted 7 lines · Jan Kiszka

Quoted 6 lines · Gilles Chanteperdrix

ksrc/skins/posix/cond.c static inline int mutex_save_count(xnthr · line 246

Quoted 3 lines · Jan Kiszka

ksrc/skins/posix/cond.c +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

Quoted 2 lines

BTW, why do you need to track sleepers separately in POSIX? Native doesn't do so, e.g.

Because of the "syscall-needed-when-unlocking-stolen-mutex" issue I already explained (sleepers - xnsynch_nsleepers is precisely the count of pending threads which have been awake then robbed the mutex).

Quoted 10 lines · Gilles Chanteperdrix

Quoted 9 lines · Jan Kiszka

Quoted 6 lines · Gilles Chanteperdrix

ksrc/skins/posix/cond.c static inline int mutex_save_count(xnthr · line 246

Quoted 3 lines · Jan Kiszka

ksrc/skins/posix/cond.c +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

BTW, why do you need to track sleepers separately in POSIX? Native doesn't do so, e.g.

Quoted 3 lines

Because of the "syscall-needed-when-unlocking-stolen-mutex" issue I already explained (sleepers - xnsynch_nsleepers is precisely the count of pending threads which have been awake then robbed the mutex).

Hmm, sounds like the new lock owner should better clear the 'claimed' bit then, not the old one on return from unlock. Or where is the pitfall? How does the futex algorithm handle this scenario?

Quoted 14 lines · Jan Kiszka

Quoted 13 lines · Gilles Chanteperdrix

Quoted 9 lines · Jan Kiszka

Quoted 6 lines · Gilles Chanteperdrix

ksrc/skins/posix/cond.c static inline int mutex_save_count(xnthr · line 246

Quoted 3 lines · Jan Kiszka

ksrc/skins/posix/cond.c +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

BTW, why do you need to track sleepers separately in POSIX? Native doesn't do so, e.g.

Because of the "syscall-needed-when-unlocking-stolen-mutex" issue I already explained (sleepers - xnsynch_nsleepers is precisely the count of pending threads which have been awake then robbed the mutex).

Quoted 3 lines

Hmm, sounds like the new lock owner should better clear the 'claimed' bit then, not the old one on return from unlock. Or where is the pitfall? How does the futex algorithm handle this scenario?

Ok. Please read my explanation again, I have already explained this in another mail.

Quoted 12 lines · Gilles Chanteperdrix

Quoted 11 lines · Jan Kiszka

Quoted 10 lines · Gilles Chanteperdrix

Quoted 9 lines · Jan Kiszka

Quoted 6 lines · Gilles Chanteperdrix

Review

Quoted 3 lines · Jan Kiszka

(quoted diff) +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

BTW, why do you need to track sleepers separately in POSIX? Native doesn't do so, e.g.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Because of the "syscall-needed-when-unlocking-stolen-mutex" issue I already explained (sleepers - xnsynch_nsleepers is precisely the count of pending threads which have been awake then robbed the mutex).

Hmm, sounds like the new lock owner should better clear the 'claimed' bit then, not the old one on return from unlock. Or where is the pitfall? How does the futex algorithm handle this scenario?

Ok. Please read my explanation again, I have already explained this in another mail.

I did this, but I'm unable to derive the answer for my question from it. Let's go through it in more details:

When we pass a mutex to a new owner, we set its reference in the fast lock variable + set the claimed bit if there are more waiters. Instead, I would simple set that bit if there is a new owner. That owner will then pick up the mutex eventually and clear 'claimed' on exit from it lock service (if there are no further waiters then). If the new owner is not able to run and we steal the lock, we simple keep the 'claimed' bit as is. On exit from the stolen lock we find it set, thus we are forced to issue a syscall as it should be.

OK, what happens if some waiter wants to leave the party while we are holding the stolen lock? Then the sleeper number must be correct - that is one pitfall!

I will have to dig into this more deeply, considering more cases. But the additional "sleepers" field remains at least misplaced IMHO. xnsynch_sleepers should better be fixed to respect lock stealing, as lock stealing is an xnsynch property, nothing POSIX-specific.

Quoted 37 lines · Jan Kiszka

Quoted 12 lines · Gilles Chanteperdrix

Quoted 11 lines · Jan Kiszka

Quoted 10 lines · Gilles Chanteperdrix

Quoted 9 lines · Jan Kiszka

Quoted 6 lines · Gilles Chanteperdrix

Review

Quoted 3 lines · Jan Kiszka

(quoted diff) +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

BTW, why do you need to track sleepers separately in POSIX? Native doesn't do so, e.g.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Because of the "syscall-needed-when-unlocking-stolen-mutex" issue I already explained (sleepers - xnsynch_nsleepers is precisely the count of pending threads which have been awake then robbed the mutex).

Hmm, sounds like the new lock owner should better clear the 'claimed' bit then, not the old one on return from unlock. Or where is the pitfall? How does the futex algorithm handle this scenario?

Ok. Please read my explanation again, I have already explained this in another mail.

I did this, but I'm unable to derive the answer for my question from it. Let's go through it in more details:

When we pass a mutex to a new owner, we set its reference in the fast lock variable + set the claimed bit if there are more waiters. Instead, I would simple set that bit if there is a new owner. That owner will then pick up the mutex eventually and clear 'claimed' on exit from it lock service (if there are no further waiters then). If the new owner is not able to run and we steal the lock, we simple keep the 'claimed' bit as is. On exit from the stolen lock we find it set, thus we are forced to issue a syscall as it should be.

OK, what happens if some waiter wants to leave the party while we are holding the stolen lock? Then the sleeper number must be correct - that is one pitfall!

Quoted 4 lines

I will have to dig into this more deeply, considering more cases. But the additional "sleepers" field remains at least misplaced IMHO. xnsynch_sleepers should better be fixed to respect lock stealing, as lock stealing is an xnsynch property, nothing POSIX-specific.

Ok. I have read this but did not get what you mean. I will read it again
quietly from home.

Quoted 38 lines · Gilles Chanteperdrix

Quoted 37 lines · Jan Kiszka

Quoted 12 lines · Gilles Chanteperdrix

Quoted 11 lines · Jan Kiszka

Quoted 10 lines · Gilles Chanteperdrix

Quoted 9 lines · Jan Kiszka

Quoted 6 lines · Gilles Chanteperdrix

Review

Quoted 3 lines · Jan Kiszka

(quoted diff) +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

BTW, why do you need to track sleepers separately in POSIX? Native doesn't do so, e.g.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Because of the "syscall-needed-when-unlocking-stolen-mutex" issue I already explained (sleepers - xnsynch_nsleepers is precisely the count of pending threads which have been awake then robbed the mutex).

Hmm, sounds like the new lock owner should better clear the 'claimed' bit then, not the old one on return from unlock. Or where is the pitfall? How does the futex algorithm handle this scenario?

Ok. Please read my explanation again, I have already explained this in another mail.

I did this, but I'm unable to derive the answer for my question from it. Let's go through it in more details:

When we pass a mutex to a new owner, we set its reference in the fast lock variable + set the claimed bit if there are more waiters. Instead, I would simple set that bit if there is a new owner. That owner will then pick up the mutex eventually and clear 'claimed' on exit from it lock service (if there are no further waiters then). If the new owner is not able to run and we steal the lock, we simple keep the 'claimed' bit as is. On exit from the stolen lock we find it set, thus we are forced to issue a syscall as it should be.

OK, what happens if some waiter wants to leave the party while we are holding the stolen lock? Then the sleeper number must be correct - that is one pitfall!

Quoted 6 lines

Quoted 4 lines

I will have to dig into this more deeply, considering more cases. But the additional "sleepers" field remains at least misplaced IMHO. xnsynch_sleepers should better be fixed to respect lock stealing, as lock stealing is an xnsynch property, nothing POSIX-specific.

Ok. I have read this but did not get what you mean. I will read it again
quietly from home.

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

Quoted 60 lines · Jan Kiszka

Quoted 38 lines · Gilles Chanteperdrix

Quoted 37 lines · Jan Kiszka

Quoted 12 lines · Gilles Chanteperdrix

Quoted 11 lines · Jan Kiszka

Quoted 10 lines · Gilles Chanteperdrix

Quoted 9 lines · Jan Kiszka

Quoted 6 lines · Gilles Chanteperdrix

Review

Quoted 3 lines · Jan Kiszka

(quoted diff) +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

BTW, why do you need to track sleepers separately in POSIX? Native doesn't do so, e.g.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Because of the "syscall-needed-when-unlocking-stolen-mutex" issue I already explained (sleepers - xnsynch_nsleepers is precisely the count of pending threads which have been awake then robbed the mutex).

Hmm, sounds like the new lock owner should better clear the 'claimed' bit then, not the old one on return from unlock. Or where is the pitfall? How does the futex algorithm handle this scenario?

Ok. Please read my explanation again, I have already explained this in another mail.

I did this, but I'm unable to derive the answer for my question from it. Let's go through it in more details:

When we pass a mutex to a new owner, we set its reference in the fast lock variable + set the claimed bit if there are more waiters. Instead, I would simple set that bit if there is a new owner. That owner will then pick up the mutex eventually and clear 'claimed' on exit from it lock service (if there are no further waiters then). If the new owner is not able to run and we steal the lock, we simple keep the 'claimed' bit as is. On exit from the stolen lock we find it set, thus we are forced to issue a syscall as it should be.

OK, what happens if some waiter wants to leave the party while we are holding the stolen lock? Then the sleeper number must be correct - that is one pitfall!

Quoted 6 lines

Quoted 4 lines

I will have to dig into this more deeply, considering more cases. But the additional "sleepers" field remains at least misplaced IMHO. xnsynch_sleepers should better be fixed to respect lock stealing, as lock stealing is an xnsynch property, nothing POSIX-specific.

Ok. I have read this but did not get what you mean. I will read it again
quietly from home.

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

Quoted 78 lines · Gilles Chanteperdrix

Quoted 60 lines · Jan Kiszka

Quoted 38 lines · Gilles Chanteperdrix

Quoted 37 lines · Jan Kiszka

Quoted 12 lines · Gilles Chanteperdrix

Quoted 11 lines · Jan Kiszka

Quoted 10 lines · Gilles Chanteperdrix

Quoted 9 lines · Jan Kiszka

Quoted 6 lines · Gilles Chanteperdrix

Review

Quoted 3 lines · Jan Kiszka

(quoted diff) +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

BTW, why do you need to track sleepers separately in POSIX? Native doesn't do so, e.g.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Because of the "syscall-needed-when-unlocking-stolen-mutex" issue I already explained (sleepers - xnsynch_nsleepers is precisely the count of pending threads which have been awake then robbed the mutex).

Hmm, sounds like the new lock owner should better clear the 'claimed' bit then, not the old one on return from unlock. Or where is the pitfall? How does the futex algorithm handle this scenario?

Ok. Please read my explanation again, I have already explained this in another mail.

I did this, but I'm unable to derive the answer for my question from it. Let's go through it in more details:

When we pass a mutex to a new owner, we set its reference in the fast lock variable + set the claimed bit if there are more waiters. Instead, I would simple set that bit if there is a new owner. That owner will then pick up the mutex eventually and clear 'claimed' on exit from it lock service (if there are no further waiters then). If the new owner is not able to run and we steal the lock, we simple keep the 'claimed' bit as is. On exit from the stolen lock we find it set, thus we are forced to issue a syscall as it should be.

OK, what happens if some waiter wants to leave the party while we are holding the stolen lock? Then the sleeper number must be correct - that is one pitfall!

Quoted 6 lines

Quoted 4 lines

I will have to dig into this more deeply, considering more cases. But the additional "sleepers" field remains at least misplaced IMHO. xnsynch_sleepers should better be fixed to respect lock stealing, as lock stealing is an xnsynch property, nothing POSIX-specific.

Ok. I have read this but did not get what you mean. I will read it again
quietly from home.

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

By the way, I think we should stop sending mails to our personal addresses in addition to the mailing list, because this results in mailing list mails being received out of orders, which make the threads hard to follow.

Quoted 79 lines · Gilles Chanteperdrix

Quoted 78 lines · Gilles Chanteperdrix

Quoted 60 lines · Jan Kiszka

Quoted 38 lines · Gilles Chanteperdrix

Quoted 37 lines · Jan Kiszka

Quoted 12 lines · Gilles Chanteperdrix

Quoted 11 lines · Jan Kiszka

Quoted 10 lines · Gilles Chanteperdrix

Quoted 9 lines · Jan Kiszka

Quoted 6 lines · Gilles Chanteperdrix

Review

Quoted 3 lines · Jan Kiszka

(quoted diff) +3 -0

+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

BTW, why do you need to track sleepers separately in POSIX? Native doesn't do so, e.g.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Because of the "syscall-needed-when-unlocking-stolen-mutex" issue I already explained (sleepers - xnsynch_nsleepers is precisely the count of pending threads which have been awake then robbed the mutex).

Hmm, sounds like the new lock owner should better clear the 'claimed' bit then, not the old one on return from unlock. Or where is the pitfall? How does the futex algorithm handle this scenario?

Ok. Please read my explanation again, I have already explained this in another mail.

I did this, but I'm unable to derive the answer for my question from it. Let's go through it in more details:

When we pass a mutex to a new owner, we set its reference in the fast lock variable + set the claimed bit if there are more waiters. Instead, I would simple set that bit if there is a new owner. That owner will then pick up the mutex eventually and clear 'claimed' on exit from it lock service (if there are no further waiters then). If the new owner is not able to run and we steal the lock, we simple keep the 'claimed' bit as is. On exit from the stolen lock we find it set, thus we are forced to issue a syscall as it should be.

OK, what happens if some waiter wants to leave the party while we are holding the stolen lock? Then the sleeper number must be correct - that is one pitfall!

Quoted 6 lines

Quoted 4 lines

I will have to dig into this more deeply, considering more cases. But the additional "sleepers" field remains at least misplaced IMHO. xnsynch_sleepers should better be fixed to respect lock stealing, as lock stealing is an xnsynch property, nothing POSIX-specific.

Ok. I have read this but did not get what you mean. I will read it again
quietly from home.

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 6 lines

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

By the way, I think we should stop sending mails to our personal addresses in addition to the mailing list, because this results in mailing list mails being received out of orders, which make the threads hard to follow.

That's common practice on most mailing lists I know of, and I personally don't want to change this. IMO, it would only make replying more complicated, and it would bear the risk to drop CCs to non-subscribers.

I think the problem was only temporarily, maybe caused by some weird interaction of gna.org and the Siemens mailserver (we were too fast for them). Meanwhile, archives and inboxes should contain all messages. Gmane, e.g., lists them in the correct order now.

Quoted 90 lines · Jan Kiszka

Quoted 79 lines · Gilles Chanteperdrix

Quoted 78 lines · Gilles Chanteperdrix

Quoted 60 lines · Jan Kiszka

Quoted 38 lines · Gilles Chanteperdrix

Quoted 37 lines · Jan Kiszka

Quoted 12 lines · Gilles Chanteperdrix

Quoted 11 lines · Jan Kiszka

Quoted 10 lines · Gilles Chanteperdrix

Quoted 9 lines · Jan Kiszka

Quoted 6 lines · Gilles Chanteperdrix

Quoted 3 lines · Jan Kiszka

+ xnarch_atomic_set(mutex->owner,
+ set_claimed(xnthread_handle(owner),
+ xnsynch_nsleepers(&mutex->synchbase)));

Ok. I think you have spotted a bug here. This should be mutex->sleepers instead of xnsynch_nsleepers.

BTW, why do you need to track sleepers separately in POSIX? Native doesn't do so, e.g.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Because of the "syscall-needed-when-unlocking-stolen-mutex" issue I already explained (sleepers - xnsynch_nsleepers is precisely the count of pending threads which have been awake then robbed the mutex).

Hmm, sounds like the new lock owner should better clear the 'claimed' bit then, not the old one on return from unlock. Or where is the pitfall? How does the futex algorithm handle this scenario?

Ok. Please read my explanation again, I have already explained this in another mail.

I did this, but I'm unable to derive the answer for my question from it. Let's go through it in more details:

When we pass a mutex to a new owner, we set its reference in the fast lock variable + set the claimed bit if there are more waiters. Instead, I would simple set that bit if there is a new owner. That owner will then pick up the mutex eventually and clear 'claimed' on exit from it lock service (if there are no further waiters then). If the new owner is not able to run and we steal the lock, we simple keep the 'claimed' bit as is. On exit from the stolen lock we find it set, thus we are forced to issue a syscall as it should be.

OK, what happens if some waiter wants to leave the party while we are holding the stolen lock? Then the sleeper number must be correct - that is one pitfall!

Quoted 6 lines

Quoted 4 lines

I will have to dig into this more deeply, considering more cases. But the additional "sleepers" field remains at least misplaced IMHO. xnsynch_sleepers should better be fixed to respect lock stealing, as lock stealing is an xnsynch property, nothing POSIX-specific.

Ok. I have read this but did not get what you mean. I will read it again
quietly from home.

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 6 lines

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

By the way, I think we should stop sending mails to our personal addresses in addition to the mailing list, because this results in mailing list mails being received out of orders, which make the threads hard to follow.

That's common practice on most mailing lists I know of, and I personally don't want to change this. IMO, it would only make replying more complicated, and it would bear the risk to drop CCs to non-subscribers.

Quoted 4 lines

I think the problem was only temporarily, maybe caused by some weird interaction of gna.org and the Siemens mailserver (we were too fast for them). Meanwhile, archives and inboxes should contain all messages. Gmane, e.g., lists them in the correct order now.

The "weird interaction" is actually a feature and called "grey listing". It delays mails for at least 5 minutes, the real result depending on the eagerness of your relay mail server to re-transmit the delayed message.

Quoted 95 lines · Gilles Chanteperdrix

Quoted 94 lines · Jan Kiszka

Quoted 79 lines · Gilles Chanteperdrix

Quoted 78 lines · Gilles Chanteperdrix

Quoted 60 lines · Jan Kiszka

Quoted 38 lines · Gilles Chanteperdrix

Quoted 37 lines · Jan Kiszka

Quoted 12 lines · Gilles Chanteperdrix

Quoted 11 lines · Jan Kiszka

Quoted 10 lines · Gilles Chanteperdrix

Quoted 9 lines · Jan Kiszka

Quoted 6 lines · Gilles Chanteperdrix

Jan Kiszka wrote:
+ xnarch_atomic_set(mutex->owner,
+ set_claimed(xnthread_handle(owner),
+ xnsynch_nsleepers(&mutex->synchbase)));
Ok. I think you have spotted a bug here. This should be mutex->sleepers
instead of xnsynch_nsleepers.

BTW, why do you need to track sleepers separately in POSIX? Native doesn't do so, e.g.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Because of the "syscall-needed-when-unlocking-stolen-mutex" issue I already explained (sleepers - xnsynch_nsleepers is precisely the count of pending threads which have been awake then robbed the mutex).

Hmm, sounds like the new lock owner should better clear the 'claimed' bit then, not the old one on return from unlock. Or where is the pitfall? How does the futex algorithm handle this scenario?

Ok. Please read my explanation again, I have already explained this in another mail.

I did this, but I'm unable to derive the answer for my question from it. Let's go through it in more details:

When we pass a mutex to a new owner, we set its reference in the fast lock variable + set the claimed bit if there are more waiters. Instead, I would simple set that bit if there is a new owner. That owner will then pick up the mutex eventually and clear 'claimed' on exit from it lock service (if there are no further waiters then). If the new owner is not able to run and we steal the lock, we simple keep the 'claimed' bit as is. On exit from the stolen lock we find it set, thus we are forced to issue a syscall as it should be.

OK, what happens if some waiter wants to leave the party while we are holding the stolen lock? Then the sleeper number must be correct - that is one pitfall!

Quoted 6 lines

Quoted 4 lines

I will have to dig into this more deeply, considering more cases. But the additional "sleepers" field remains at least misplaced IMHO. xnsynch_sleepers should better be fixed to respect lock stealing, as lock stealing is an xnsynch property, nothing POSIX-specific.

Ok. I have read this but did not get what you mean. I will read it again
quietly from home.

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 6 lines

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

By the way, I think we should stop sending mails to our personal addresses in addition to the mailing list, because this results in mailing list mails being received out of orders, which make the threads hard to follow.

That's common practice on most mailing lists I know of, and I personally don't want to change this. IMO, it would only make replying more complicated, and it would bear the risk to drop CCs to non-subscribers.

I think the problem was only temporarily, maybe caused by some weird interaction of gna.org and the Siemens mailserver (we were too fast for them). Meanwhile, archives and inboxes should contain all messages. Gmane, e.g., lists them in the correct order now.

Quoted 3 lines

The "weird interaction" is actually a feature and called "grey listing". It delays mails for at least 5 minutes, the real result depending on the eagerness of your relay mail server to re-transmit the delayed message.

I'm aware of such anti-spam approaches (and their varying effect). But as I've seen specifically gna.org struggling with other mailservers as well, just recently, I'm careful with blaming solely our corporate server. Logs would help to clarify, but I don't have access to either side.

Jan Kiszka wrote:

Quoted 15 lines

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

Quoted 1 line · Gilles Chanteperdrix

Jan Kiszka wrote:

...

Quoted 32 lines

Quoted 15 lines

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

I'm yet having difficulties to imagine how this should look like when it's implemented. Would it be simpler than my second idea?

Anyway, here is a patch (on top of my handle-based lock series) for the approach that clears xnsynch_owner when there are no waiters. At least it causes no regression based on your test, but I haven't checked lock stealing yet. In theory, everything still appears to be fine to me. This approach basically restores the state we find when some thread just acquired the lock in user space.

Quoted 40 lines · Jan Kiszka

Quoted 1 line · Gilles Chanteperdrix

Jan Kiszka wrote:

...

Quoted 32 lines

Quoted 15 lines

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

I'm yet having difficulties to imagine how this should look like when it's implemented. Would it be simpler than my second idea?

Quoted 6 lines

Anyway, here is a patch (on top of my handle-based lock series) for the approach that clears xnsynch_owner when there are no waiters. At least it causes no regression based on your test, but I haven't checked lock stealing yet. In theory, everything still appears to be fine to me. This approach basically restores the state we find when some thread just acquired the lock in user space.

Yes, I did not think about the stealing when writing my test, but I think it could be a good idea to add it to the test, especially if you want to port the test to the native API.

I let Philippe decide here. He is the one who did the stealing stuff and probably knows better.

Quoted 47 lines · Gilles Chanteperdrix

Quoted 40 lines · Jan Kiszka

Quoted 1 line · Gilles Chanteperdrix

Jan Kiszka wrote:

...

Quoted 32 lines

Quoted 15 lines

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

I'm yet having difficulties to imagine how this should look like when it's implemented. Would it be simpler than my second idea?

Quoted 6 lines

Anyway, here is a patch (on top of my handle-based lock series) for the approach that clears xnsynch_owner when there are no waiters. At least it causes no regression based on your test, but I haven't checked lock stealing yet. In theory, everything still appears to be fine to me. This approach basically restores the state we find when some thread just acquired the lock in user space.

Quoted 7 lines

Yes, I did not think about the stealing when writing my test, but I think it could be a good idea to add it to the test, especially if you want to port the test to the native API.

I let Philippe decide here. He is the one who did the stealing stuff and probably knows better.

Currently, the xnsynch strongly couples PIP and ownership, which seems to impede your various proposals. I would suggest to decouple that: the basic property of some xnsynch that we may want to handle is exclusiveness, then dynamic priority inheritance is another property, that could stack its own semantics on top of exclusiveness.

XNSYNCH_EXCLUSIVE would cover all ownership-related actions, XNSYNCH_PIP would simply add dynamic priority management. Non exclusive object would not require any xnsynch_set_owner() handling.

Just to give a clear signal here: I will happily consider any change to the xnsynch object that may ease the implementation of fast ownership handling (i.e. userland-to-userland transfer). The only thing is that such code is very much prone to regressions, so a testsuite must come with core changes in that area, but I guess you know that already.

Quoted 61 lines · Philippe Gerum

Quoted 47 lines · Gilles Chanteperdrix

Quoted 40 lines · Jan Kiszka

Quoted 1 line · Gilles Chanteperdrix

Jan Kiszka wrote:

...

Quoted 32 lines

Quoted 15 lines

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

I'm yet having difficulties to imagine how this should look like when it's implemented. Would it be simpler than my second idea?

Quoted 6 lines

Anyway, here is a patch (on top of my handle-based lock series) for the approach that clears xnsynch_owner when there are no waiters. At least it causes no regression based on your test, but I haven't checked lock stealing yet. In theory, everything still appears to be fine to me. This approach basically restores the state we find when some thread just acquired the lock in user space.

Quoted 7 lines

Yes, I did not think about the stealing when writing my test, but I think it could be a good idea to add it to the test, especially if you want to port the test to the native API.

I let Philippe decide here. He is the one who did the stealing stuff and probably knows better.

Currently, the xnsynch strongly couples PIP and ownership, which seems to impede your various proposals. I would suggest to decouple that: the basic property of some xnsynch that we may want to handle is exclusiveness, then dynamic priority inheritance is another property, that could stack its own semantics on top of exclusiveness.

Quoted 3 lines

XNSYNCH_EXCLUSIVE would cover all ownership-related actions, XNSYNCH_PIP would simply add dynamic priority management. Non exclusive object would not require any xnsynch_set_owner() handling.

... but would also not be able to provide PIP. Unfortunately, we want PIP here,

alone won't help.

When it comes to find a minimal invasive approach, I'm more in favor of breaking up that lock stealing loop in xnsynch_sleep_on, letting the caller handle XNROBBED instead. That feature could easily be made an add-on to the current internal handling, without risking regressions for other users.

Quoted 5 lines

Just to give a clear signal here: I will happily consider any change to the xnsynch object that may ease the implementation of fast ownership handling (i.e. userland-to-userland transfer). The only thing is that such code is very much prone to regressions, so a testsuite must come with core changes in that area, but I guess you know that already.

Yeah, it's clear. I'm currently considering the options and the efforts. There are some smaller differences the way POSIX does fast locking right now and native may do it, so it is also not yet clear to me how to abstract a generic service. Nevertheless, it would help a lot avoiding to invent too many skin-specific bugs when porting the algorithms back and forth...

Jan

Quoted 47 lines · Gilles Chanteperdrix

Quoted 40 lines · Jan Kiszka

Quoted 1 line · Gilles Chanteperdrix

Jan Kiszka wrote:

...

Quoted 32 lines

Quoted 15 lines

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

I'm yet having difficulties to imagine how this should look like when it's implemented. Would it be simpler than my second idea?

Quoted 6 lines

Anyway, here is a patch (on top of my handle-based lock series) for the approach that clears xnsynch_owner when there are no waiters. At least it causes no regression based on your test, but I haven't checked lock stealing yet. In theory, everything still appears to be fine to me. This approach basically restores the state we find when some thread just acquired the lock in user space.

Quoted 7 lines

Yes, I did not think about the stealing when writing my test, but I think it could be a good idea to add it to the test, especially if you want to port the test to the native API.

I let Philippe decide here. He is the one who did the stealing stuff and probably knows better.

Currently, the xnsynch strongly couples PIP and ownership, which seems to impede your various proposals. I would suggest to decouple that: the basic property of some xnsynch that we may want to handle is exclusiveness, then dynamic priority inheritance is another property, that could stack its own semantics on top of exclusiveness.

XNSYNCH_EXCLUSIVE would cover all ownership-related actions, XNSYNCH_PIP would simply add dynamic priority management. Non exclusive object would not require any xnsynch_set_owner() handling.

Just to give a clear signal here: I will happily consider any change to the xnsynch object that may ease the implementation of fast ownership handling (i.e. userland-to-userland transfer). The only thing is that such code is very much prone to regressions, so a testsuite must come with core changes in that area, but I guess you know that already.

Quoted 65 lines · Philippe Gerum

Quoted 47 lines · Gilles Chanteperdrix

Quoted 40 lines · Jan Kiszka

Quoted 1 line · Gilles Chanteperdrix

Jan Kiszka wrote:

...

Quoted 32 lines

Quoted 15 lines

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

I'm yet having difficulties to imagine how this should look like when it's implemented. Would it be simpler than my second idea?

Quoted 6 lines

Anyway, here is a patch (on top of my handle-based lock series) for the approach that clears xnsynch_owner when there are no waiters. At least it causes no regression based on your test, but I haven't checked lock stealing yet. In theory, everything still appears to be fine to me. This approach basically restores the state we find when some thread just acquired the lock in user space.

Quoted 7 lines

Yes, I did not think about the stealing when writing my test, but I think it could be a good idea to add it to the test, especially if you want to port the test to the native API.

I let Philippe decide here. He is the one who did the stealing stuff and probably knows better.

Currently, the xnsynch strongly couples PIP and ownership, which seems to impede your various proposals. I would suggest to decouple that: the basic property of some xnsynch that we may want to handle is exclusiveness, then dynamic priority inheritance is another property, that could stack its own semantics on top of exclusiveness.

XNSYNCH_EXCLUSIVE would cover all ownership-related actions, XNSYNCH_PIP would simply add dynamic priority management. Non exclusive object would not require any xnsynch_set_owner() handling.

Quoted 5 lines

Just to give a clear signal here: I will happily consider any change to the xnsynch object that may ease the implementation of fast ownership handling (i.e. userland-to-userland transfer). The only thing is that such code is very much prone to regressions, so a testsuite must come with core changes in that area, but I guess you know that already.

Ok. I think unit_mutex.c is a good start. It only lacks testing XNROBBED.

And well, cases of more than two waiters are not tested as well, but I would not think it really matters. It could be extended as well to allow switching at start time between the posix api and the native api, after all we do not care much about the overhead of calling function pointers in a a test application.

As for evolution of xnsynch, I am all for it, but it would break anything implemented before, so it will probably be on Jan's critical path.

Quoted 71 lines · Gilles Chanteperdrix

Quoted 70 lines · Philippe Gerum

Quoted 47 lines · Gilles Chanteperdrix

Quoted 40 lines · Jan Kiszka

Quoted 1 line · Gilles Chanteperdrix

Jan Kiszka wrote:

...

Quoted 32 lines

Quoted 15 lines

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

I'm yet having difficulties to imagine how this should look like when it's implemented. Would it be simpler than my second idea?

Quoted 6 lines

Anyway, here is a patch (on top of my handle-based lock series) for the approach that clears xnsynch_owner when there are no waiters. At least it causes no regression based on your test, but I haven't checked lock stealing yet. In theory, everything still appears to be fine to me. This approach basically restores the state we find when some thread just acquired the lock in user space.

Quoted 7 lines

Yes, I did not think about the stealing when writing my test, but I think it could be a good idea to add it to the test, especially if you want to port the test to the native API.

I let Philippe decide here. He is the one who did the stealing stuff and probably knows better.

Currently, the xnsynch strongly couples PIP and ownership, which seems to impede your various proposals. I would suggest to decouple that: the basic property of some xnsynch that we may want to handle is exclusiveness, then dynamic priority inheritance is another property, that could stack its own semantics on top of exclusiveness.

XNSYNCH_EXCLUSIVE would cover all ownership-related actions, XNSYNCH_PIP would simply add dynamic priority management. Non exclusive object would not require any xnsynch_set_owner() handling.

Just to give a clear signal here: I will happily consider any change to the xnsynch object that may ease the implementation of fast ownership handling (i.e. userland-to-userland transfer). The only thing is that such code is very much prone to regressions, so a testsuite must come with core changes in that area, but I guess you know that already.

Quoted 2 lines

Ok. I think unit_mutex.c is a good start. It only lacks testing XNROBBED.

My colleague sent me an extension. It's native-only so far, but it already pointed out a bug in my try-acquire implementation that should be present in posix as well (trylock must go through the slow path).

Quoted 72 lines · Jan Kiszka

Quoted 71 lines · Gilles Chanteperdrix

Quoted 70 lines · Philippe Gerum

Quoted 47 lines · Gilles Chanteperdrix

Quoted 40 lines · Jan Kiszka

Quoted 1 line · Gilles Chanteperdrix

Jan Kiszka wrote:

...

Quoted 32 lines

Quoted 15 lines

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

I'm yet having difficulties to imagine how this should look like when it's implemented. Would it be simpler than my second idea?

Quoted 6 lines

Anyway, here is a patch (on top of my handle-based lock series) for the approach that clears xnsynch_owner when there are no waiters. At least it causes no regression based on your test, but I haven't checked lock stealing yet. In theory, everything still appears to be fine to me. This approach basically restores the state we find when some thread just acquired the lock in user space.

Quoted 7 lines

Yes, I did not think about the stealing when writing my test, but I think it could be a good idea to add it to the test, especially if you want to port the test to the native API.

I let Philippe decide here. He is the one who did the stealing stuff and probably knows better.

Currently, the xnsynch strongly couples PIP and ownership, which seems to impede your various proposals. I would suggest to decouple that: the basic property of some xnsynch that we may want to handle is exclusiveness, then dynamic priority inheritance is another property, that could stack its own semantics on top of exclusiveness.

XNSYNCH_EXCLUSIVE would cover all ownership-related actions, XNSYNCH_PIP would simply add dynamic priority management. Non exclusive object would not require any xnsynch_set_owner() handling.

Just to give a clear signal here: I will happily consider any change to the xnsynch object that may ease the implementation of fast ownership handling (i.e. userland-to-userland transfer). The only thing is that such code is very much prone to regressions, so a testsuite must come with core changes in that area, but I guess you know that already.

Quoted 5 lines

Quoted 2 lines

Ok. I think unit_mutex.c is a good start. It only lacks testing XNROBBED.

My colleague sent me an extension. It's native-only so far, but it already pointed out a bug in my try-acquire implementation that should be present in posix as well (trylock must go through the slow path).

I do not see why. If mutex lock can lock without a syscall, the same goes for trylock.

Quoted 75 lines · Gilles Chanteperdrix

Quoted 74 lines · Jan Kiszka

Quoted 73 lines · Gilles Chanteperdrix

Quoted 65 lines · Philippe Gerum

Quoted 47 lines · Gilles Chanteperdrix

Quoted 40 lines · Jan Kiszka

Quoted 1 line · Gilles Chanteperdrix

Jan Kiszka wrote:

...

Quoted 32 lines

Quoted 15 lines

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

I'm yet having difficulties to imagine how this should look like when it's implemented. Would it be simpler than my second idea?

Quoted 6 lines

Anyway, here is a patch (on top of my handle-based lock series) for the approach that clears xnsynch_owner when there are no waiters. At least it causes no regression based on your test, but I haven't checked lock stealing yet. In theory, everything still appears to be fine to me. This approach basically restores the state we find when some thread just acquired the lock in user space.

Quoted 7 lines

Yes, I did not think about the stealing when writing my test, but I think it could be a good idea to add it to the test, especially if you want to port the test to the native API.

I let Philippe decide here. He is the one who did the stealing stuff and probably knows better.

Currently, the xnsynch strongly couples PIP and ownership, which seems to impede your various proposals. I would suggest to decouple that: the basic property of some xnsynch that we may want to handle is exclusiveness, then dynamic priority inheritance is another property, that could stack its own semantics on top of exclusiveness.

XNSYNCH_EXCLUSIVE would cover all ownership-related actions, XNSYNCH_PIP would simply add dynamic priority management. Non exclusive object would not require any xnsynch_set_owner() handling.

Quoted 5 lines

Just to give a clear signal here: I will happily consider any change to the xnsynch object that may ease the implementation of fast ownership handling (i.e. userland-to-userland transfer). The only thing is that such code is very much prone to regressions, so a testsuite must come with core changes in that area, but I guess you know that already.

Ok. I think unit_mutex.c is a good start. It only lacks testing XNROBBED.

Quoted 5 lines

Quoted 3 lines

My colleague sent me an extension. It's native-only so far, but it already pointed out a bug in my try-acquire implementation that should be present in posix as well (trylock must go through the slow path).

I do not see why. If mutex lock can lock without a syscall, the same goes for trylock.

Lock stealing requires the slow path.

Quoted 81 lines · Jan Kiszka

Quoted 80 lines · Gilles Chanteperdrix

Quoted 72 lines · Jan Kiszka

Quoted 71 lines · Gilles Chanteperdrix

Quoted 70 lines · Philippe Gerum

Quoted 47 lines · Gilles Chanteperdrix

Quoted 40 lines · Jan Kiszka

Quoted 1 line · Gilles Chanteperdrix

Jan Kiszka wrote:

...

Quoted 32 lines

Quoted 15 lines

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

I'm yet having difficulties to imagine how this should look like when it's implemented. Would it be simpler than my second idea?

Quoted 6 lines

Anyway, here is a patch (on top of my handle-based lock series) for the approach that clears xnsynch_owner when there are no waiters. At least it causes no regression based on your test, but I haven't checked lock stealing yet. In theory, everything still appears to be fine to me. This approach basically restores the state we find when some thread just acquired the lock in user space.

Quoted 7 lines

Yes, I did not think about the stealing when writing my test, but I think it could be a good idea to add it to the test, especially if you want to port the test to the native API.

I let Philippe decide here. He is the one who did the stealing stuff and probably knows better.

Currently, the xnsynch strongly couples PIP and ownership, which seems to impede your various proposals. I would suggest to decouple that: the basic property of some xnsynch that we may want to handle is exclusiveness, then dynamic priority inheritance is another property, that could stack its own semantics on top of exclusiveness.

XNSYNCH_EXCLUSIVE would cover all ownership-related actions, XNSYNCH_PIP would simply add dynamic priority management. Non exclusive object would not require any xnsynch_set_owner() handling.

Just to give a clear signal here: I will happily consider any change to the xnsynch object that may ease the implementation of fast ownership handling (i.e. userland-to-userland transfer). The only thing is that such code is very much prone to regressions, so a testsuite must come with core changes in that area, but I guess you know that already.

Quoted 5 lines

Quoted 2 lines

Ok. I think unit_mutex.c is a good start. It only lacks testing XNROBBED.

My colleague sent me an extension. It's native-only so far, but it already pointed out a bug in my try-acquire implementation that should be present in posix as well (trylock must go through the slow path).

I do not see why. If mutex lock can lock without a syscall, the same goes for trylock.

Quoted 1 line

Lock stealing requires the slow path.

Ah ok. I thought you mean that trylock had to go systematically through syscall.

As for lock stealing, I already said that it was not tested in the current test.

Quoted 86 lines · Gilles Chanteperdrix

Quoted 74 lines · Jan Kiszka

Quoted 73 lines · Gilles Chanteperdrix

Quoted 72 lines · Jan Kiszka

Quoted 71 lines · Gilles Chanteperdrix

Quoted 70 lines · Philippe Gerum

Quoted 47 lines · Gilles Chanteperdrix

Quoted 40 lines · Jan Kiszka

Quoted 1 line · Gilles Chanteperdrix

Jan Kiszka wrote:

...

Quoted 32 lines

Quoted 15 lines

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

I'm yet having difficulties to imagine how this should look like when it's implemented. Would it be simpler than my second idea?

Quoted 6 lines

Anyway, here is a patch (on top of my handle-based lock series) for the approach that clears xnsynch_owner when there are no waiters. At least it causes no regression based on your test, but I haven't checked lock stealing yet. In theory, everything still appears to be fine to me. This approach basically restores the state we find when some thread just acquired the lock in user space.

Quoted 7 lines

Yes, I did not think about the stealing when writing my test, but I think it could be a good idea to add it to the test, especially if you want to port the test to the native API.

I let Philippe decide here. He is the one who did the stealing stuff and probably knows better.

Currently, the xnsynch strongly couples PIP and ownership, which seems to impede your various proposals. I would suggest to decouple that: the basic property of some xnsynch that we may want to handle is exclusiveness, then dynamic priority inheritance is another property, that could stack its own semantics on top of exclusiveness.

XNSYNCH_EXCLUSIVE would cover all ownership-related actions, XNSYNCH_PIP would simply add dynamic priority management. Non exclusive object would not require any xnsynch_set_owner() handling.

Just to give a clear signal here: I will happily consider any change to the xnsynch object that may ease the implementation of fast ownership handling (i.e. userland-to-userland transfer). The only thing is that such code is very much prone to regressions, so a testsuite must come with core changes in that area, but I guess you know that already.

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Ok. I think unit_mutex.c is a good start. It only lacks testing XNROBBED.

My colleague sent me an extension. It's native-only so far, but it already pointed out a bug in my try-acquire implementation that should be present in posix as well (trylock must go through the slow path).

I do not see why. If mutex lock can lock without a syscall, the same goes for trylock.

Lock stealing requires the slow path.

Ah ok. I thought you mean that trylock had to go systematically through syscall.

Quoted 2 lines

As for lock stealing, I already said that it was not tested in the current test.

In fact, that bug is also present the current native skin (SVN, 2.4.x should suffer as well). Probably also current 2.4.x posix is affected, at least from a first glance.

Quoted 87 lines · Jan Kiszka

Quoted 86 lines · Gilles Chanteperdrix

Quoted 74 lines · Jan Kiszka

Quoted 73 lines · Gilles Chanteperdrix

Quoted 72 lines · Jan Kiszka

Quoted 71 lines · Gilles Chanteperdrix

Quoted 70 lines · Philippe Gerum

Quoted 47 lines · Gilles Chanteperdrix

Quoted 40 lines · Jan Kiszka

Quoted 1 line · Gilles Chanteperdrix

Jan Kiszka wrote:

...

Quoted 32 lines

Quoted 15 lines

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

I'm yet having difficulties to imagine how this should look like when it's implemented. Would it be simpler than my second idea?

Quoted 6 lines

Anyway, here is a patch (on top of my handle-based lock series) for the approach that clears xnsynch_owner when there are no waiters. At least it causes no regression based on your test, but I haven't checked lock stealing yet. In theory, everything still appears to be fine to me. This approach basically restores the state we find when some thread just acquired the lock in user space.

Quoted 7 lines

Yes, I did not think about the stealing when writing my test, but I think it could be a good idea to add it to the test, especially if you want to port the test to the native API.

I let Philippe decide here. He is the one who did the stealing stuff and probably knows better.

Currently, the xnsynch strongly couples PIP and ownership, which seems to impede your various proposals. I would suggest to decouple that: the basic property of some xnsynch that we may want to handle is exclusiveness, then dynamic priority inheritance is another property, that could stack its own semantics on top of exclusiveness.

XNSYNCH_EXCLUSIVE would cover all ownership-related actions, XNSYNCH_PIP would simply add dynamic priority management. Non exclusive object would not require any xnsynch_set_owner() handling.

Just to give a clear signal here: I will happily consider any change to the xnsynch object that may ease the implementation of fast ownership handling (i.e. userland-to-userland transfer). The only thing is that such code is very much prone to regressions, so a testsuite must come with core changes in that area, but I guess you know that already.

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Ok. I think unit_mutex.c is a good start. It only lacks testing XNROBBED.

My colleague sent me an extension. It's native-only so far, but it already pointed out a bug in my try-acquire implementation that should be present in posix as well (trylock must go through the slow path).

I do not see why. If mutex lock can lock without a syscall, the same goes for trylock.

Lock stealing requires the slow path.

Ah ok. I thought you mean that trylock had to go systematically through syscall.

Quoted 5 lines

Quoted 2 lines

As for lock stealing, I already said that it was not tested in the current test.

In fact, that bug is also present the current native skin (SVN, 2.4.x should suffer as well). Probably also current 2.4.x posix is affected, at least from a first glance.

Well, yes, now that you mention it, calling xnsynch_sleep_on does not seem obvious for the implementation of a trylock.

And by the way, I think that with the mutex/xnsynch shared owner, we can implement stealing without a syscall.

Probably. But it will definitely need full fast locking support inside xnsynch, which is basically about defining and maintaining a generic user-shared lock word from within xnsynch services that has at least a 'claimed' and a 'assignment pending' bit. Looks like we will not run out of work soon... ;)

Quoted 87 lines · Jan Kiszka

Quoted 86 lines · Gilles Chanteperdrix

Quoted 74 lines · Jan Kiszka

Quoted 73 lines · Gilles Chanteperdrix

Quoted 72 lines · Jan Kiszka

Quoted 71 lines · Gilles Chanteperdrix

Quoted 70 lines · Philippe Gerum

Quoted 47 lines · Gilles Chanteperdrix

Quoted 40 lines · Jan Kiszka

Quoted 1 line · Gilles Chanteperdrix

Jan Kiszka wrote:

...

Quoted 32 lines

Quoted 15 lines

I think I'm getting closer to the issue. Our actual problem comes from the fact that the xnsynch_owner is easily out of sync with the real owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B, causing it to become the new xnsynch owner, and clears the claimed bit as there are no further sleepers. B returns, and when it wants to release the mutex, it does this happily in user space because claimed is not set. Now the fast lock variable is 'unlocked', while xnsynch still reports B being the owner. This is no problem as the next time two threads fight over this lock the waiter will simply overwrite the xnsynch_owner before it falls asleep. But this "trick" doesn't work for waiters that have been robbed. They will spin inside xnsynch_sleep_on and stumble over this inconsistency.

Quoted 4 lines

I have two approaches in mind now: First one is something like XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED so that the robbed thread spins one level higher in the skin code - which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return from kernel with the lock held while there are no more xnsynch_sleepers. That should work with even less changes and save us one syscall in the robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

I'm yet having difficulties to imagine how this should look like when it's implemented. Would it be simpler than my second idea?

Quoted 6 lines

Anyway, here is a patch (on top of my handle-based lock series) for the approach that clears xnsynch_owner when there are no waiters. At least it causes no regression based on your test, but I haven't checked lock stealing yet. In theory, everything still appears to be fine to me. This approach basically restores the state we find when some thread just acquired the lock in user space.

Quoted 7 lines

Yes, I did not think about the stealing when writing my test, but I think it could be a good idea to add it to the test, especially if you want to port the test to the native API.

I let Philippe decide here. He is the one who did the stealing stuff and probably knows better.

Currently, the xnsynch strongly couples PIP and ownership, which seems to impede your various proposals. I would suggest to decouple that: the basic property of some xnsynch that we may want to handle is exclusiveness, then dynamic priority inheritance is another property, that could stack its own semantics on top of exclusiveness.

XNSYNCH_EXCLUSIVE would cover all ownership-related actions, XNSYNCH_PIP would simply add dynamic priority management. Non exclusive object would not require any xnsynch_set_owner() handling.

Just to give a clear signal here: I will happily consider any change to the xnsynch object that may ease the implementation of fast ownership handling (i.e. userland-to-userland transfer). The only thing is that such code is very much prone to regressions, so a testsuite must come with core changes in that area, but I guess you know that already.

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Ok. I think unit_mutex.c is a good start. It only lacks testing XNROBBED.

My colleague sent me an extension. It's native-only so far, but it already pointed out a bug in my try-acquire implementation that should be present in posix as well (trylock must go through the slow path).

I do not see why. If mutex lock can lock without a syscall, the same goes for trylock.

Lock stealing requires the slow path.

Ah ok. I thought you mean that trylock had to go systematically through syscall.

Quoted 5 lines

Quoted 2 lines

As for lock stealing, I already said that it was not tested in the current test.

In fact, that bug is also present the current native skin (SVN, 2.4.x should suffer as well). Probably also current 2.4.x posix is affected, at least from a first glance.

Well, yes, now that you mention it, calling xnsynch_sleep_on does not seem obvious for the implementation of a trylock.

And by the way, I think that with the mutex/xnsynch shared owner, we can implement stealing without a syscall.

Quoted 90 lines · Gilles Chanteperdrix

Quoted 89 lines · Jan Kiszka

Quoted 88 lines · Gilles Chanteperdrix

Quoted 74 lines · Jan Kiszka

Quoted 73 lines · Gilles Chanteperdrix

Quoted 72 lines · Jan Kiszka

Quoted 71 lines · Gilles Chanteperdrix

Quoted 70 lines · Philippe Gerum

Quoted 47 lines · Gilles Chanteperdrix

Quoted 40 lines · Jan Kiszka

Quoted 1 line · Gilles Chanteperdrix

Jan Kiszka wrote:

...

Quoted 32 lines

Quoted 19 lines

I think I'm getting closer to the issue. Our actual problem comes from
the fact that the xnsynch_owner is easily out of sync with the real
owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B,
causing it to become the new xnsynch owner, and clears the claimed bit
as there are no further sleepers. B returns, and when it wants to
release the mutex, it does this happily in user space because claimed is
not set. Now the fast lock variable is 'unlocked', while xnsynch still
reports B being the owner. This is no problem as the next time two
threads fight over this lock the waiter will simply overwrite the
xnsynch_owner before it falls asleep. But this "trick" doesn't work for
waiters that have been robbed. They will spin inside xnsynch_sleep_on
and stumble over this inconsistency.

I have two approaches in mind now: First one is something like
XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED
so that the robbed thread spins one level higher in the skin code -
which would have to be extended a bit.

No, the stealing is the xnsynch job.

Quoted 4 lines

Option two is to clear xnsynch_owner once a new owner is about to return
from kernel with the lock held while there are no more xnsynch_sleepers.
That should work with even less changes and save us one syscall in the
robbed case. Need to think about it more, though.

In fact the only time when the owner is required to be in sync is when PIP occurs, and this is guaranteed to work, because when PIP is needed a syscall is emitted anyway. To the extent that xnsynch does not even track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Quoted 2 lines

Ok. And what about the idea of the xnsynch bit to tell him "hey, the owner is tracked in the upper layer, go there to find it".

I'm yet having difficulties to imagine how this should look like when it's implemented. Would it be simpler than my second idea?

Quoted 6 lines

Anyway, here is a patch (on top of my handle-based lock series) for the approach that clears xnsynch_owner when there are no waiters. At least it causes no regression based on your test, but I haven't checked lock stealing yet. In theory, everything still appears to be fine to me. This approach basically restores the state we find when some thread just acquired the lock in user space.

Quoted 7 lines

Yes, I did not think about the stealing when writing my test, but I think it could be a good idea to add it to the test, especially if you want to port the test to the native API.

I let Philippe decide here. He is the one who did the stealing stuff and probably knows better.

Currently, the xnsynch strongly couples PIP and ownership, which seems to impede your various proposals. I would suggest to decouple that: the basic property of some xnsynch that we may want to handle is exclusiveness, then dynamic priority inheritance is another property, that could stack its own semantics on top of exclusiveness.

XNSYNCH_EXCLUSIVE would cover all ownership-related actions, XNSYNCH_PIP would simply add dynamic priority management. Non exclusive object would not require any xnsynch_set_owner() handling.

Just to give a clear signal here: I will happily consider any change to the xnsynch object that may ease the implementation of fast ownership handling (i.e. userland-to-userland transfer). The only thing is that such code is very much prone to regressions, so a testsuite must come with core changes in that area, but I guess you know that already.

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Ok. I think unit_mutex.c is a good start. It only lacks testing XNROBBED.

My colleague sent me an extension. It's native-only so far, but it already pointed out a bug in my try-acquire implementation that should be present in posix as well (trylock must go through the slow path).

I do not see why. If mutex lock can lock without a syscall, the same goes for trylock.

Lock stealing requires the slow path.

Ah ok. I thought you mean that trylock had to go systematically through syscall.

As for lock stealing, I already said that it was not tested in the current test.

Quoted 6 lines

Quoted 3 lines

In fact, that bug is also present the current native skin (SVN, 2.4.x should suffer as well). Probably also current 2.4.x posix is affected, at least from a first glance.

Well, yes, now that you mention it, calling xnsynch_sleep_on does not seem obvious for the implementation of a trylock.

Don't even try calling xnsynch_sleep_on with a nonblocking-type timeout.

Quoted 94 lines · Philippe Gerum

Quoted 93 lines · Gilles Chanteperdrix

Quoted 92 lines · Jan Kiszka

Quoted 80 lines · Gilles Chanteperdrix

Quoted 79 lines · Jan Kiszka

Quoted 78 lines · Gilles Chanteperdrix

Quoted 77 lines · Jan Kiszka

Quoted 66 lines · Gilles Chanteperdrix

Quoted 65 lines · Philippe Gerum

Quoted 47 lines · Gilles Chanteperdrix

Quoted 40 lines · Jan Kiszka

Quoted 1 line · Gilles Chanteperdrix

Jan Kiszka wrote:

...

Quoted 34 lines

I think I'm getting closer to the issue. Our actual problem comes from
the fact that the xnsynch_owner is easily out of sync with the real
owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B,
causing it to become the new xnsynch owner, and clears the claimed bit
as there are no further sleepers. B returns, and when it wants to
release the mutex, it does this happily in user space because claimed is
not set. Now the fast lock variable is 'unlocked', while xnsynch still
reports B being the owner. This is no problem as the next time two
threads fight over this lock the waiter will simply overwrite the
xnsynch_owner before it falls asleep. But this "trick" doesn't work for
waiters that have been robbed. They will spin inside xnsynch_sleep_on
and stumble over this inconsistency.

I have two approaches in mind now: First one is something like
XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED
so that the robbed thread spins one level higher in the skin code -
which would have to be extended a bit.
No, the stealing is the xnsynch job.

Option two is to clear xnsynch_owner once a new owner is about to return
from kernel with the lock held while there are no more xnsynch_sleepers.
That should work with even less changes and save us one syscall in the
robbed case. Need to think about it more, though.
In fact the only time when the owner is required to be in sync is when
PIP occurs, and this is guaranteed to work, because when PIP is needed a
syscall is emitted anyway. To the extent that xnsynch does not even
track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Ok. And what about the idea of the xnsynch bit to tell him "hey, the
owner is tracked in the upper layer, go there to find it".

I'm yet having difficulties to imagine how this should look like when it's implemented. Would it be simpler than my second idea?

Quoted 6 lines

Anyway, here is a patch (on top of my handle-based lock series) for the approach that clears xnsynch_owner when there are no waiters. At least it causes no regression based on your test, but I haven't checked lock stealing yet. In theory, everything still appears to be fine to me. This approach basically restores the state we find when some thread just acquired the lock in user space.

Quoted 7 lines

Yes, I did not think about the stealing when writing my test, but I think it could be a good idea to add it to the test, especially if you want to port the test to the native API.

I let Philippe decide here. He is the one who did the stealing stuff and probably knows better.

Currently, the xnsynch strongly couples PIP and ownership, which seems to impede your various proposals. I would suggest to decouple that: the basic property of some xnsynch that we may want to handle is exclusiveness, then dynamic priority inheritance is another property, that could stack its own semantics on top of exclusiveness.

XNSYNCH_EXCLUSIVE would cover all ownership-related actions, XNSYNCH_PIP would simply add dynamic priority management. Non exclusive object would not require any xnsynch_set_owner() handling.

Quoted 7 lines

Quoted 5 lines

Just to give a clear signal here: I will happily consider any change to the xnsynch object that may ease the implementation of fast ownership handling (i.e. userland-to-userland transfer). The only thing is that such code is very much prone to regressions, so a testsuite must come with core changes in that area, but I guess you know that already.

Ok. I think unit_mutex.c is a good start. It only lacks testing XNROBBED.

My colleague sent me an extension. It's native-only so far, but it already pointed out a bug in my try-acquire implementation that should be present in posix as well (trylock must go through the slow path).

Quoted 8 lines

Quoted 3 lines

Quoted 2 lines

I do not see why. If mutex lock can lock without a syscall, the same goes for trylock.

Lock stealing requires the slow path.

Ah ok. I thought you mean that trylock had to go systematically through syscall.

As for lock stealing, I already said that it was not tested in the current test.

In fact, that bug is also present the current native skin (SVN, 2.4.x should suffer as well). Probably also current 2.4.x posix is affected, at least from a first glance.

Quoted 4 lines

Quoted 3 lines

Well, yes, now that you mention it, calling xnsynch_sleep_on does not seem obvious for the implementation of a trylock.

Don't even try calling xnsynch_sleep_on with a nonblocking-type timeout.

Works nicely today (using (XN_NONBLOCK, XN_RELATIVE)) due to early timeout checks in xntimer_start. You just have to translate XNTIMEO into EWOULDBLOCK on return.

Jan

Quoted 98 lines · Jan Kiszka

Quoted 97 lines · Philippe Gerum

Quoted 96 lines · Gilles Chanteperdrix

Quoted 84 lines · Jan Kiszka

Quoted 83 lines · Gilles Chanteperdrix

Quoted 82 lines · Jan Kiszka

Quoted 73 lines · Gilles Chanteperdrix

Quoted 72 lines · Jan Kiszka

Quoted 71 lines · Gilles Chanteperdrix

Quoted 70 lines · Philippe Gerum

Quoted 47 lines · Gilles Chanteperdrix

Quoted 46 lines · Jan Kiszka

Gilles Chanteperdrix wrote:
Jan Kiszka wrote:
...
I think I'm getting closer to the issue. Our actual problem comes from
the fact that the xnsynch_owner is easily out of sync with the real
owner, it even sometimes points to a former owner:

Thread A releases a mutex on which thread B pends. It wakes up B,
causing it to become the new xnsynch owner, and clears the claimed bit
as there are no further sleepers. B returns, and when it wants to
release the mutex, it does this happily in user space because claimed is
not set. Now the fast lock variable is 'unlocked', while xnsynch still
reports B being the owner. This is no problem as the next time two
threads fight over this lock the waiter will simply overwrite the
xnsynch_owner before it falls asleep. But this "trick" doesn't work for
waiters that have been robbed. They will spin inside xnsynch_sleep_on
and stumble over this inconsistency.

I have two approaches in mind now: First one is something like
XNSYNCH_STEALNOINFORM, i.e. causing xnsynch_sleep_on to not set XNROBBED
so that the robbed thread spins one level higher in the skin code -
which would have to be extended a bit.
No, the stealing is the xnsynch job.

Option two is to clear xnsynch_owner once a new owner is about to return
from kernel with the lock held while there are no more xnsynch_sleepers.
That should work with even less changes and save us one syscall in the
robbed case. Need to think about it more, though.
In fact the only time when the owner is required to be in sync is when
PIP occurs, and this is guaranteed to work, because when PIP is needed a
syscall is emitted anyway. To the extent that xnsynch does not even
track the owner on non PIP synch (which is why the posix skin originally
forcibly set the synch owner, and it was simply kept to get the fastsem
stuff working).

Ok. And what about the idea of the xnsynch bit to tell him "hey, the
owner is tracked in the upper layer, go there to find it".
I'm yet having difficulties to imagine how this should look like when
it's implemented. Would it be simpler than my second idea?

Anyway, here is a patch (on top of my handle-based lock series) for the
approach that clears xnsynch_owner when there are no waiters. At least
it causes no regression based on your test, but I haven't checked lock
stealing yet. In theory, everything still appears to be fine to me. This
approach basically restores the state we find when some thread just
acquired the lock in user space.

Quoted 7 lines

Yes, I did not think about the stealing when writing my test, but I think it could be a good idea to add it to the test, especially if you want to port the test to the native API.

I let Philippe decide here. He is the one who did the stealing stuff and probably knows better.

Currently, the xnsynch strongly couples PIP and ownership, which seems to impede your various proposals. I would suggest to decouple that: the basic property of some xnsynch that we may want to handle is exclusiveness, then dynamic priority inheritance is another property, that could stack its own semantics on top of exclusiveness.

XNSYNCH_EXCLUSIVE would cover all ownership-related actions, XNSYNCH_PIP would simply add dynamic priority management. Non exclusive object would not require any xnsynch_set_owner() handling.

Just to give a clear signal here: I will happily consider any change to the xnsynch object that may ease the implementation of fast ownership handling (i.e. userland-to-userland transfer). The only thing is that such code is very much prone to regressions, so a testsuite must come with core changes in that area, but I guess you know that already.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Ok. I think unit_mutex.c is a good start. It only lacks testing XNROBBED.

My colleague sent me an extension. It's native-only so far, but it already pointed out a bug in my try-acquire implementation that should be present in posix as well (trylock must go through the slow path).

I do not see why. If mutex lock can lock without a syscall, the same goes for trylock.

Lock stealing requires the slow path.

Quoted 8 lines

Quoted 5 lines

Ah ok. I thought you mean that trylock had to go systematically through syscall.

As for lock stealing, I already said that it was not tested in the current test.

In fact, that bug is also present the current native skin (SVN, 2.4.x should suffer as well). Probably also current 2.4.x posix is affected, at least from a first glance.

Well, yes, now that you mention it, calling xnsynch_sleep_on does not seem obvious for the implementation of a trylock.