[Xenomai-core] [RFC][PATCH 0/3] Handle-based fast mutex owner tracking

[Xenomai-core] [RFC][PATCH 0/3] Handle-based fast mutex owner tracking

[Jan Kiszka]

Hi Gilles,

this is a prototype patch series to explore handle-based lock owner
tracking in the fast mutex lock variable. It appears to work, so I'm
rolling it out for collecting early comments, specifically from you.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Xenomai-core] [RFC][PATCH 1/3] Always register threads by their base

[Jan Kiszka]

In order to use xnthread_handle(thread) for the fast mutex lock
variable, we have to ensure that all skins pass the generic object on
xnregistry_enter. The following patch ensures this for preexisting
registrations.

---
 ksrc/skins/native/syscall.c  |   56 +++++++++++++++++++++++++------------------
 ksrc/skins/native/task.c     |    2 -
 ksrc/skins/psos+/syscall.c   |   14 +++++-----
 ksrc/skins/psos+/task.c      |    7 +++--
 ksrc/skins/vxworks/syscall.c |   28 +++++++++++----------
 ksrc/skins/vxworks/taskLib.c |    4 +--
 6 files changed, 63 insertions(+), 48 deletions(-)

Index: b/ksrc/skins/native/syscall.c
===================================================================
--- a/ksrc/skins/native/syscall.c
+++ b/ksrc/skins/native/syscall.c
@@ -52,7 +52,8 @@ int __native_muxid;
 static int __rt_bind_helper(struct task_struct *p,
 			    struct pt_regs *regs,
 			    xnhandle_t *handlep,
-			    unsigned magic, void **objaddrp)
+			    unsigned magic, void **objaddrp,
+			    unsigned long objoffs)
 {
 	char name[XNOBJECT_NAME_LEN];
 	RTIME timeout;
@@ -82,7 +83,7 @@ static int __rt_bind_helper(struct task_
 
 	/* Also validate the type of the bound object. */
 
-	if (xeno_test_magic(objaddr, magic)) {
+	if (xeno_test_magic(objaddr + objoffs, magic)) {
 		if (objaddrp)
 			*objaddrp = objaddr;
 	} else
@@ -223,7 +224,9 @@ static int __rt_task_bind(struct pt_regs
 	RT_TASK_PLACEHOLDER ph;
 	int err;
 
-	err = __rt_bind_helper(p, regs, &ph.opaque, XENO_TASK_MAGIC, NULL);
+	err =
+	    __rt_bind_helper(p, regs, &ph.opaque, XENO_TASK_MAGIC, NULL,
+			     -offsetof(RT_TASK, thread_base));
 
 	if (err)
 		return err;
@@ -253,7 +256,7 @@ static int __rt_task_start(struct pt_reg
 				     sizeof(ph)))
 		return -EFAULT;
 
-	task = (RT_TASK *)xnregistry_fetch(ph.opaque);
+	task = thread2rtask((xnthread_t *)xnregistry_fetch(ph.opaque));
 
 	if (!task)
 		return -ESRCH;
@@ -279,7 +282,7 @@ static int __rt_task_suspend(struct pt_r
 					     sizeof(ph)))
 			return -EFAULT;
 
-		task = (RT_TASK *)xnregistry_fetch(ph.opaque);
+		task = thread2rtask((xnthread_t *)xnregistry_fetch(ph.opaque));
 	} else
 		task = __rt_task_current(p);
 
@@ -302,7 +305,7 @@ static int __rt_task_resume(struct pt_re
 				     sizeof(ph)))
 		return -EFAULT;
 
-	task = (RT_TASK *)xnregistry_fetch(ph.opaque);
+	task = thread2rtask((xnthread_t *)xnregistry_fetch(ph.opaque));
 
 	if (!task)
 		return -ESRCH;
@@ -326,7 +329,7 @@ static int __rt_task_delete(struct pt_re
 					     sizeof(ph)))
 			return -EFAULT;
 
-		task = (RT_TASK *)xnregistry_fetch(ph.opaque);
+		task = thread2rtask((xnthread_t *)xnregistry_fetch(ph.opaque));
 	} else
 		task = __rt_task_current(p);
 
@@ -364,7 +367,7 @@ static int __rt_task_set_periodic(struct
 					     sizeof(ph)))
 			return -EFAULT;
 
-		task = (RT_TASK *)xnregistry_fetch(ph.opaque);
+		task = thread2rtask((xnthread_t *)xnregistry_fetch(ph.opaque));
 	} else
 		task = __rt_task_current(p);
 
@@ -418,7 +421,7 @@ static int __rt_task_set_priority(struct
 					     sizeof(ph)))
 			return -EFAULT;
 
-		task = (RT_TASK *)xnregistry_fetch(ph.opaque);
+		task = thread2rtask((xnthread_t *)xnregistry_fetch(ph.opaque));
 	} else
 		task = __rt_task_current(p);
 
@@ -473,7 +476,7 @@ static int __rt_task_unblock(struct pt_r
 				     sizeof(ph)))
 		return -EFAULT;
 
-	task = (RT_TASK *)xnregistry_fetch(ph.opaque);
+	task = thread2rtask((xnthread_t *)xnregistry_fetch(ph.opaque));
 
 	if (!task)
 		return -ESRCH;
@@ -500,7 +503,7 @@ static int __rt_task_inquire(struct pt_r
 					     sizeof(ph)))
 			return -EFAULT;
 
-		task = (RT_TASK *)xnregistry_fetch(ph.opaque);
+		task = thread2rtask((xnthread_t *)xnregistry_fetch(ph.opaque));
 	} else
 		task = __rt_task_current(p);
 
@@ -541,7 +544,7 @@ static int __rt_task_notify(struct pt_re
 					     sizeof(ph)))
 			return -EFAULT;
 
-		task = (RT_TASK *)xnregistry_fetch(ph.opaque);
+		task = thread2rtask((xnthread_t *)xnregistry_fetch(ph.opaque));
 	} else
 		task = __rt_task_current(p);
 
@@ -627,7 +630,7 @@ static int __rt_task_slice(struct pt_reg
 					     sizeof(ph)))
 			return -EFAULT;
 
-		task = (RT_TASK *)xnregistry_fetch(ph.opaque);
+		task = thread2rtask((xnthread_t *)xnregistry_fetch(ph.opaque));
 	} else
 		task = __rt_task_current(current);
 
@@ -667,7 +670,7 @@ static int __rt_task_send(struct pt_regs
 					     sizeof(ph)))
 			return -EFAULT;
 
-		task = (RT_TASK *)xnregistry_fetch(ph.opaque);
+		task = thread2rtask((xnthread_t *)xnregistry_fetch(ph.opaque));
 	} else
 		task = __rt_task_current(current);
 
@@ -1104,7 +1107,9 @@ static int __rt_sem_bind(struct pt_regs
 	RT_SEM_PLACEHOLDER ph;
 	int err;
 
-	err = __rt_bind_helper(current, regs, &ph.opaque, XENO_SEM_MAGIC, NULL);
+	err =
+	    __rt_bind_helper(current, regs, &ph.opaque, XENO_SEM_MAGIC,
+			     NULL, 0);
 
 	if (err)
 		return err;
@@ -1324,7 +1329,8 @@ static int __rt_event_bind(struct pt_reg
 	int err;
 
 	err =
-	    __rt_bind_helper(current, regs, &ph.opaque, XENO_EVENT_MAGIC, NULL);
+	    __rt_bind_helper(current, regs, &ph.opaque, XENO_EVENT_MAGIC,
+			     NULL, 0);
 
 	if (err)
 		return err;
@@ -1567,7 +1573,8 @@ static int __rt_mutex_bind(struct pt_reg
 	int err;
 
 	err =
-	    __rt_bind_helper(current, regs, &ph.opaque, XENO_MUTEX_MAGIC, NULL);
+	    __rt_bind_helper(current, regs, &ph.opaque, XENO_MUTEX_MAGIC,
+			     NULL, 0);
 
 	if (err)
 		return err;
@@ -1758,7 +1765,8 @@ static int __rt_cond_bind(struct pt_regs
 	int err;
 
 	err =
-	    __rt_bind_helper(current, regs, &ph.opaque, XENO_COND_MAGIC, NULL);
+	    __rt_bind_helper(current, regs, &ph.opaque, XENO_COND_MAGIC,
+			     NULL, 0);
 
 	if (err)
 		return err;
@@ -2008,7 +2016,7 @@ static int __rt_queue_bind(struct pt_reg
 
 	err =
 	    __rt_bind_helper(p, regs, &ph.opaque, XENO_QUEUE_MAGIC,
-			     (void **)&q);
+			     (void **)&q, 0);
 
 	if (err)
 		goto unlock_and_exit;
@@ -2507,7 +2515,7 @@ static int __rt_heap_bind(struct pt_regs
 
 	err =
 	    __rt_bind_helper(p, regs, &ph.opaque, XENO_HEAP_MAGIC,
-			     (void **)&heap);
+			     (void **)&heap, 0);
 
 	if (err)
 		goto unlock_and_exit;
@@ -3018,7 +3026,7 @@ static int __rt_intr_bind(struct pt_regs
 	RT_INTR_PLACEHOLDER ph;
 	int err;
 
-	err = __rt_bind_helper(p, regs, &ph.opaque, XENO_INTR_MAGIC, NULL);
+	err = __rt_bind_helper(p, regs, &ph.opaque, XENO_INTR_MAGIC, NULL, 0);
 
 	if (err)
 		return err;
@@ -3275,7 +3283,7 @@ static int __rt_pipe_bind(struct pt_regs
 	RT_PIPE_PLACEHOLDER ph;
 	int err;
 
-	err = __rt_bind_helper(p, regs, &ph.opaque, XENO_PIPE_MAGIC, NULL);
+	err = __rt_bind_helper(p, regs, &ph.opaque, XENO_PIPE_MAGIC, NULL, 0);
 
 	if (err)
 		return err;
@@ -3551,7 +3559,9 @@ static int __rt_buffer_bind(struct pt_re
 	RT_BUFFER_PLACEHOLDER ph;
 	int ret;
 
-	ret = __rt_bind_helper(current, regs, &ph.opaque, XENO_BUFFER_MAGIC, NULL);
+	ret =
+	    __rt_bind_helper(current, regs, &ph.opaque, XENO_BUFFER_MAGIC,
+			     NULL, 0);
 	if (ret)
 		return ret;
 
Index: b/ksrc/skins/native/task.c
===================================================================
--- a/ksrc/skins/native/task.c
+++ b/ksrc/skins/native/task.c
@@ -292,7 +292,7 @@ int rt_task_create(RT_TASK *task,
 
 	if (name) {
 		err = xnregistry_enter(task->rname,
-				       task,
+				       &task->thread_base,
 				       &xnthread_handle(&task->thread_base),
 				       NULL);
 		if (err)
Index: b/ksrc/skins/psos+/syscall.c
===================================================================
--- a/ksrc/skins/psos+/syscall.c
+++ b/ksrc/skins/psos+/syscall.c
@@ -131,7 +131,7 @@ static int __t_start(struct pt_regs *reg
 	psostask_t *task;
 
 	handle = __xn_reg_arg1(regs);
-	task = (psostask_t *)xnregistry_fetch(handle);
+	task = thread2psostask((xnthread_t *)xnregistry_fetch(handle));
 
 	if (!task)
 		return ERR_OBJID;
@@ -162,7 +162,7 @@ static int __t_delete(struct pt_regs *re
 	handle = __xn_reg_arg1(regs);
 
 	if (handle)
-		task = (psostask_t *)xnregistry_fetch(handle);
+		task = thread2psostask((xnthread_t *)xnregistry_fetch(handle));
 	else
 		task = __psos_task_current(current);
 
@@ -182,7 +182,7 @@ static int __t_suspend(struct pt_regs *r
 	psostask_t *task;
 
 	if (handle)
-		task = (psostask_t *)xnregistry_fetch(handle);
+		task = thread2psostask((xnthread_t *)xnregistry_fetch(handle));
 	else
 		task = __psos_task_current(current);
 
@@ -202,7 +202,7 @@ static int __t_resume(struct pt_regs *re
 	psostask_t *task;
 
 	if (handle)
-		task = (psostask_t *)xnregistry_fetch(handle);
+		task = thread2psostask((xnthread_t *)xnregistry_fetch(handle));
 	else
 		task = __psos_task_current(current);
 
@@ -284,7 +284,7 @@ static int __t_setpri(struct pt_regs *re
 	psostask_t *task;
 
 	if (handle)
-		task = (psostask_t *)xnregistry_fetch(handle);
+		task = thread2psostask((xnthread_t *)xnregistry_fetch(handle));
 	else
 		task = __psos_task_current(current);
 
@@ -314,7 +314,7 @@ static int __ev_send(struct pt_regs *reg
 	u_long events;
 
 	if (handle)
-		task = (psostask_t *)xnregistry_fetch(handle);
+		task = thread2psostask((xnthread_t *)xnregistry_fetch(handle));
 	else
 		task = __psos_task_current(current);
 
@@ -1317,7 +1317,7 @@ static int __as_send(struct pt_regs *reg
 	psostask_t *task;
 
 	if (handle)
-		task = (psostask_t *)xnregistry_fetch(handle);
+		task = thread2psostask((xnthread_t *)xnregistry_fetch(handle));
 	else
 		task = __psos_task_current(current);
 
Index: b/ksrc/skins/psos+/task.c
===================================================================
--- a/ksrc/skins/psos+/task.c
+++ b/ksrc/skins/psos+/task.c
@@ -161,8 +161,11 @@ u_long t_create(const char *name,
 
 #ifdef CONFIG_XENO_OPT_REGISTRY
 	{
-		u_long err = xnregistry_enter(task->name,
-					      task, &xnthread_handle(&task->threadbase), NULL);
+		u_long err =
+			xnregistry_enter(task->name,
+					 &task->threadbase,
+					 &xnthread_handle(&task->threadbase),
+					 NULL);
 		if (err) {
 			t_delete((u_long)task);
 			return err;
Index: b/ksrc/skins/vxworks/syscall.c
===================================================================
--- a/ksrc/skins/vxworks/syscall.c
+++ b/ksrc/skins/vxworks/syscall.c
@@ -146,7 +146,8 @@ out:
 
 static int __wind_task_activate(struct pt_regs *regs)
 {
-	WIND_TCB *pTcb = (WIND_TCB *)xnregistry_fetch(__xn_reg_arg1(regs));
+	WIND_TCB *pTcb = thread2wind_task(
+		(xnthread_t *)xnregistry_fetch(__xn_reg_arg1(regs)));
 
 	if (!pTcb)
 		return S_objLib_OBJ_ID_ERROR;
@@ -167,7 +168,7 @@ static int __wind_task_deleteforce(struc
 	WIND_TCB *pTcb;
 
 	if (handle)
-		pTcb = (WIND_TCB *)xnregistry_fetch(handle);
+		pTcb = thread2wind_task((xnthread_t *)xnregistry_fetch(handle));
 	else
 		pTcb = __wind_task_current(current);
 
@@ -190,7 +191,7 @@ static int __wind_task_delete(struct pt_
 	WIND_TCB *pTcb;
 
 	if (handle)
-		pTcb = (WIND_TCB *)xnregistry_fetch(handle);
+		pTcb = thread2wind_task((xnthread_t *)xnregistry_fetch(handle));
 	else
 		pTcb = __wind_task_current(current);
 
@@ -213,7 +214,7 @@ static int __wind_task_suspend(struct pt
 	WIND_TCB *pTcb;
 
 	if (handle)
-		pTcb = (WIND_TCB *)xnregistry_fetch(handle);
+		pTcb = thread2wind_task((xnthread_t *)xnregistry_fetch(handle));
 	else
 		pTcb = __wind_task_current(current);
 
@@ -232,7 +233,8 @@ static int __wind_task_suspend(struct pt
 
 static int __wind_task_resume(struct pt_regs *regs)
 {
-	WIND_TCB *pTcb = (WIND_TCB *)xnregistry_fetch(__xn_reg_arg1(regs));
+	WIND_TCB *pTcb = thread2wind_task(
+		(xnthread_t *)xnregistry_fetch(__xn_reg_arg1(regs)));
 
 	if (!pTcb)
 		return S_objLib_OBJ_ID_ERROR;
@@ -275,7 +277,7 @@ static int __wind_task_priorityset(struc
 	WIND_TCB *pTcb;
 
 	if (handle)
-		pTcb = (WIND_TCB *)xnregistry_fetch(handle);
+		pTcb = thread2wind_task((xnthread_t *)xnregistry_fetch(handle));
 	else
 		pTcb = __wind_task_current(current);
 
@@ -299,7 +301,7 @@ static int __wind_task_priorityget(struc
 	int prio;
 
 	if (handle)
-		pTcb = (WIND_TCB *)xnregistry_fetch(handle);
+		pTcb = thread2wind_task((xnthread_t *)xnregistry_fetch(handle));
 	else
 		pTcb = __wind_task_current(current);
 
@@ -374,7 +376,7 @@ static int __wind_task_verifyid(struct p
 	xnhandle_t handle = __xn_reg_arg1(regs);
 	WIND_TCB *pTcb;
 
-	pTcb = (WIND_TCB *)xnregistry_fetch(handle);
+	pTcb = (thread2wind_task((xnthread_t *)xnregistry_fetch(handle)));
 
 	if (!pTcb)
 		return S_objLib_OBJ_ID_ERROR;
@@ -574,7 +576,7 @@ static int __wind_taskinfo_name(struct p
 	const char *name;
 	WIND_TCB *pTcb;
 
-	pTcb = (WIND_TCB *)xnregistry_fetch(handle);
+	pTcb = thread2wind_task((xnthread_t *)xnregistry_fetch(handle));
 
 	if (!pTcb)
 		return S_objLib_OBJ_ID_ERROR;
@@ -618,7 +620,7 @@ static int __wind_taskinfo_status(struct
 
 	xnlock_get_irqsave(&nklock, s);
 
-	pTcb = (WIND_TCB *)xnregistry_fetch(handle);
+	pTcb = thread2wind_task((xnthread_t *)xnregistry_fetch(handle));
 
 	if (!pTcb || pTcb->magic != WIND_TASK_MAGIC) {
 		xnlock_put_irqrestore(&nklock, s);
@@ -643,7 +645,7 @@ static int __wind_taskinfo_get(struct pt
 	WIND_TCB *pTcb;
 	int err;
 
-	pTcb = (WIND_TCB *)xnregistry_fetch(handle);
+	pTcb = thread2wind_task((xnthread_t *)xnregistry_fetch(handle));
 	if (!pTcb)
 		return S_objLib_OBJ_ID_ERROR;
 
@@ -673,7 +675,7 @@ static int __wind_errno_taskset(struct p
  		return 0;
  	}
  
- 	pTcb = (WIND_TCB *)xnregistry_fetch(handle);
+ 	pTcb = thread2wind_task((xnthread_t *)xnregistry_fetch(handle));
 	if (!pTcb)
 		return S_objLib_OBJ_ID_ERROR;
 
@@ -696,7 +698,7 @@ static int __wind_errno_taskget(struct p
  	if (!handle)
  		errcode = wind_errnoget();
  	else {
- 		pTcb = (WIND_TCB *)xnregistry_fetch(handle);
+ 		pTcb = thread2wind_task((xnthread_t *)xnregistry_fetch(handle));
  		if (!pTcb)
  			return S_objLib_OBJ_ID_ERROR;
  
Index: b/ksrc/skins/vxworks/taskLib.c
===================================================================
--- a/ksrc/skins/vxworks/taskLib.c
+++ b/ksrc/skins/vxworks/taskLib.c
@@ -171,8 +171,8 @@ STATUS taskInit(WIND_TCB *pTcb,
 	xnlock_put_irqrestore(&nklock, s);
 
 #ifdef CONFIG_XENO_OPT_REGISTRY
-	if (xnregistry_enter(pTcb->name,
-			     pTcb, &xnthread_handle(&pTcb->threadbase), NULL)) {
+	if (xnregistry_enter(pTcb->name, &pTcb->threadbase,
+			     &xnthread_handle(&pTcb->threadbase), NULL)) {
 		wind_errnoset(S_objLib_OBJ_ID_ERROR);
 		taskDeleteForce((TASK_ID) pTcb);
 		return ERROR;

[Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[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.

---
 include/asm-generic/bits/bind.h    |    9 ++++-
 include/asm-generic/bits/current.h |    5 +-
 include/nucleus/types.h            |   13 +++++++
 ksrc/nucleus/shadow.c              |   31 +++++++++++++++--
 ksrc/skins/posix/Kconfig           |    1 
 ksrc/skins/posix/cb_lock.h         |   15 ++++++--
 ksrc/skins/posix/cond.c            |   12 +++---
 ksrc/skins/posix/mutex.c           |   21 +++++++----
 ksrc/skins/posix/mutex.h           |   66 +++++++++++++++++++++++++------------
 ksrc/skins/posix/syscall.c         |   11 +++---
 ksrc/skins/posix/thread.c          |    8 ++++
 src/skins/posix/mutex.c            |   27 +++++++--------
 12 files changed, 158 insertions(+), 61 deletions(-)

Index: b/include/asm-generic/bits/current.h
===================================================================
--- a/include/asm-generic/bits/current.h
+++ b/include/asm-generic/bits/current.h
@@ -2,14 +2,15 @@
 #define _XENO_ASM_GENERIC_CURRENT_H
 
 #include <pthread.h>
+#include <nucleus/types.h>
 
 extern pthread_key_t xeno_current_key;
 
 extern void xeno_set_current(void);
 
-static inline void *xeno_get_current(void)
+static inline xnhandle_t xeno_get_current(void)
 {
-	return pthread_getspecific(xeno_current_key);
+	return (xnhandle_t)pthread_getspecific(xeno_current_key);
 }
 
 #endif /* _XENO_ASM_GENERIC_CURRENT_H */
Index: b/include/nucleus/types.h
===================================================================
--- a/include/nucleus/types.h
+++ b/include/nucleus/types.h
@@ -61,6 +61,19 @@ typedef unsigned long xnhandle_t;
 
 #define XN_NO_HANDLE ((xnhandle_t)0)
 
+#define XN_HANDLE_SPARE0	((xnhandle_t)0x10000000)
+#define XN_HANDLE_SPARE1	((xnhandle_t)0x20000000)
+#define XN_HANDLE_SPARE2	((xnhandle_t)0x40000000)
+#define XN_HANDLE_SPARE3	((xnhandle_t)0x80000000)
+#define XN_HANDLE_SPARE_MASK	((xnhandle_t)0xf0000000)
+
+#define xnhandle_mask_spare(handle)  ((handle) & ~XN_HANDLE_SPARE_MASK)
+#define xnhandle_test_spare(handle, bits)  (!!((handle) & (bits)))
+#define xnhandle_set_spare(handle, bits) \
+	do { (handle) |= (bits); } while (0)
+#define xnhandle_clear_spare(handle, bits) \
+	do { (handle) &= ~(bits); } while (0)
+
 struct xnintr;
 
 typedef int (*xnisr_t)(struct xnintr *intr);
Index: b/ksrc/nucleus/shadow.c
===================================================================
--- a/ksrc/nucleus/shadow.c
+++ b/ksrc/nucleus/shadow.c
@@ -52,6 +52,7 @@
 #include <nucleus/trace.h>
 #include <nucleus/stat.h>
 #include <nucleus/sys_ppd.h>
+#include <nucleus/registry.h>
 #include <asm/xenomai/features.h>
 #include <asm/xenomai/syscall.h>
 #include <asm/xenomai/bits/shadow.h>
@@ -1907,13 +1908,35 @@ static int xnshadow_sys_sem_heap(struct
 	return __xn_safe_copy_to_user(us_hinfo, &hinfo, sizeof(*us_hinfo));
 }
 
+#ifdef CONFIG_XENO_OPT_REGISTRY
 static int xnshadow_sys_current(struct pt_regs *regs)
 {
-	xnthread_t * __user *us_current, *cur = xnshadow_thread(current);
-	us_current = (xnthread_t *__user *) __xn_reg_arg1(regs);
+	xnthread_t *cur = xnshadow_thread(current);
+	xnhandle_t cur_handle, __user *us_handle;
+	int err = 0;
+	spl_t s;
+
+	xnlock_get_irqsave(&nklock, s);
+
+	cur_handle = xnthread_handle(cur);
+
+	/* We need at least an anonymous registry entry to obtain a handle. */
+	if (!cur_handle) {
+		err = xnregistry_enter("", cur, &xnthread_handle(cur), NULL);
+		cur_handle = xnthread_handle(cur);
+	}
+
+	xnlock_put_irqrestore(&nklock, s);
+
+	if (err)
+		return err;
+
+	us_handle = (xnhandle_t __user *) __xn_reg_arg1(regs);
 
-	return __xn_safe_copy_to_user(us_current, &cur, sizeof(*us_current));
+	return __xn_safe_copy_to_user(us_handle, &cur_handle,
+				      sizeof(*us_handle));
 }
+#endif /* CONFIG_XENO_OPT_REGISTRY */
 
 static xnsysent_t __systab[] = {
 	[__xn_sys_migrate] = {&xnshadow_sys_migrate, __xn_exec_current},
@@ -1924,7 +1947,9 @@ static xnsysent_t __systab[] = {
 	[__xn_sys_barrier] = {&xnshadow_sys_barrier, __xn_exec_lostage},
 	[__xn_sys_trace] = {&xnshadow_sys_trace, __xn_exec_any},
 	[__xn_sys_sem_heap] = {&xnshadow_sys_sem_heap, __xn_exec_any},
+#ifdef CONFIG_XENO_OPT_REGISTRY
 	[__xn_sys_current] = {&xnshadow_sys_current, __xn_exec_any},
+#endif /* CONFIG_XENO_OPT_REGISTRY */
 };
 
 static void *xnshadow_sys_event(int event, void *data)
Index: b/ksrc/skins/posix/cb_lock.h
===================================================================
--- a/ksrc/skins/posix/cb_lock.h
+++ b/ksrc/skins/posix/cb_lock.h
@@ -3,15 +3,22 @@
 
 #include <asm/xenomai/atomic.h>
 #include <nucleus/compiler.h>
+#include <nucleus/types.h>
 
 #ifndef __KERNEL__
 typedef void xnthread_t;
 #endif /* __KERNEL__ */
 
-#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)))
+#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; \
+})
 
 #ifdef CONFIG_XENO_FASTSEM
 
Index: b/ksrc/skins/posix/cond.c
===================================================================
--- a/ksrc/skins/posix/cond.c
+++ b/ksrc/skins/posix/cond.c
@@ -230,18 +230,20 @@ static inline int mutex_save_count(xnthr
 
 	mutex = shadow->mutex;
 
-	if (clear_claimed(xnarch_atomic_intptr_get(mutex->owner)) != cur)
+	if (clear_claimed(xnarch_atomic_get(mutex->owner)) !=
+	    xnthread_handle(cur))
 		return EPERM;
 
 	*count_ptr = shadow->lockcnt;
 
-	if (likely(xnarch_atomic_intptr_cmpxchg(mutex->owner, cur, NULL) == cur))
+	if (likely(xnarch_atomic_cmpxchg(mutex->owner, cur, XN_NO_HANDLE) ==
+		   xnthread_handle(cur)))
 		return 0;
 
 	owner = xnsynch_wakeup_one_sleeper(&mutex->synchbase);
-	xnarch_atomic_intptr_set
-		(mutex->owner,
-		 set_claimed(owner,xnsynch_nsleepers(&mutex->synchbase)));
+	xnarch_atomic_set(mutex->owner,
+		 	  set_claimed(xnthread_handle(owner),
+				      xnsynch_nsleepers(&mutex->synchbase)));
 
 	/* Do not reschedule here, releasing the mutex and suspension must be
 	   done atomically in pthread_cond_*wait. */
Index: b/ksrc/skins/posix/mutex.c
===================================================================
--- a/ksrc/skins/posix/mutex.c
+++ b/ksrc/skins/posix/mutex.c
@@ -82,7 +82,7 @@ int pse51_mutex_check_init(struct __shad
 
 int pse51_mutex_init_internal(struct __shadow_mutex *shadow,
 			      pse51_mutex_t *mutex,
-			      xnarch_atomic_intptr_t *ownerp,
+			      xnarch_atomic_t *ownerp,
 			      const pthread_mutexattr_t *attr)
 {
 	xnflags_t synch_flags = XNSYNCH_PRIO | XNSYNCH_NOPIP;
@@ -118,7 +118,7 @@ int pse51_mutex_init_internal(struct __s
 	mutex->owner = ownerp;
 	mutex->owningq = kq;
 	mutex->sleepers = 0;
-	xnarch_atomic_intptr_set(ownerp, NULL);
+	xnarch_atomic_set(ownerp, XN_NO_HANDLE);
 
 	xnlock_get_irqsave(&nklock, s);
 	appendq(&kq->mutexq, &mutex->link);
@@ -159,7 +159,7 @@ int pthread_mutex_init(pthread_mutex_t *
 	    &((union __xeno_mutex *)mx)->shadow_mutex;
 	DECLARE_CB_LOCK_FLAGS(s);
 	pse51_mutex_t *mutex;
-	xnarch_atomic_intptr_t *ownerp;
+	xnarch_atomic_t *ownerp;
 	int err;
 
 	if (!attr)
@@ -185,9 +185,9 @@ int pthread_mutex_init(pthread_mutex_t *
 	if (!mutex)
 		return ENOMEM;
 
-	ownerp = (xnarch_atomic_intptr_t *)
+	ownerp = (xnarch_atomic_t *)
 		xnheap_alloc(&xnsys_ppd_get(attr->pshared)->sem_heap,
-			     sizeof(xnarch_atomic_intptr_t));
+			     sizeof(xnarch_atomic_t));
 	if (!ownerp) {
 		xnfree(mutex);
 		return EAGAIN;
@@ -266,7 +266,7 @@ int pthread_mutex_destroy(pthread_mutex_
 		return EPERM;
 	}
 
-	if (xnarch_atomic_intptr_get(mutex->owner)) {
+	if (xnarch_atomic_get(mutex->owner)) {
 		cb_write_unlock(&shadow->lock, s);
 		return EBUSY;
 	}
@@ -290,6 +290,10 @@ int pse51_mutex_timedlock_break(struct _
 	spl_t s;
 	int err;
 
+	/* We need a valid thread handle for the fast lock. */
+	if (!xnthread_handle(cur))
+		return -EPERM;
+
 	err = pse51_mutex_timedlock_internal(cur, shadow, 1, timed, abs_to);
 	if (err != -EBUSY)
 		goto unlock_and_return;
@@ -392,7 +396,7 @@ int pthread_mutex_trylock(pthread_mutex_
 		return -PTR_ERR(owner);
 
 	err = EBUSY;
-	if (clear_claimed(owner) == cur) {
+	if (owner == cur) {
 		pse51_mutex_t *mutex = shadow->mutex;
 
 		if (mutex->attr.type == PTHREAD_MUTEX_RECURSIVE) {
@@ -573,7 +577,8 @@ int pthread_mutex_unlock(pthread_mutex_t
 
 	mutex = shadow->mutex;
 	
-	if (clear_claimed(xnarch_atomic_intptr_get(mutex->owner)) != cur) {
+	if (clear_claimed(xnarch_atomic_get(mutex->owner)) !=
+	    xnthread_handle(cur)) {
 		err = EPERM;
 		goto out;
 	}
Index: b/ksrc/skins/posix/mutex.h
===================================================================
--- a/ksrc/skins/posix/mutex.h
+++ b/ksrc/skins/posix/mutex.h
@@ -34,7 +34,7 @@ union __xeno_mutex {
 		xnarch_atomic_t lock;
 		union {
 			unsigned owner_offset;
-			xnarch_atomic_intptr_t *owner;
+			xnarch_atomic_t *owner;
 		};
 		struct pse51_mutexattr attr;
 #endif /* CONFIG_XENO_FASTSEM */
@@ -43,6 +43,7 @@ union __xeno_mutex {
 
 #ifdef __KERNEL__
 
+#include <nucleus/registry.h>
 #include <posix/internal.h>
 #include <posix/thread.h>
 #include <posix/cb_lock.h>
@@ -54,7 +55,7 @@ typedef struct pse51_mutex {
 #define link2mutex(laddr)                                               \
 	((pse51_mutex_t *)(((char *)laddr) - offsetof(pse51_mutex_t, link)))
 
-	xnarch_atomic_intptr_t *owner;
+	xnarch_atomic_t *owner;
 	pthread_mutexattr_t attr;
 	unsigned sleepers;
 	pse51_kqueues_t *owningq;
@@ -77,7 +78,7 @@ int pse51_mutex_check_init(struct __shad
 
 int pse51_mutex_init_internal(struct __shadow_mutex *shadow,
 			      pse51_mutex_t *mutex,
-			      xnarch_atomic_intptr_t *ownerp,
+			      xnarch_atomic_t *ownerp,
 			      const pthread_mutexattr_t *attr);
 
 void pse51_mutex_destroy_internal(pse51_mutex_t *mutex,
@@ -88,6 +89,7 @@ pse51_mutex_trylock_internal(xnthread_t
 			     struct __shadow_mutex *shadow, unsigned count)
 {
 	pse51_mutex_t *mutex = shadow->mutex;
+	xnhandle_t ownerh;
 	xnthread_t *owner;
 
 	if (xnpod_unblockable_p())
@@ -101,9 +103,14 @@ pse51_mutex_trylock_internal(xnthread_t
 		return ERR_PTR(-EPERM);
 #endif /* XENO_DEBUG(POSIX) */
 
-	owner = xnarch_atomic_intptr_cmpxchg(mutex->owner, NULL, cur);
-	if (unlikely(owner != NULL))
+	ownerh = xnarch_atomic_cmpxchg(mutex->owner, XN_NO_HANDLE,
+				       xnthread_handle(cur));
+	if (unlikely(ownerh)) {
+		owner = xnregistry_fetch(clear_claimed(ownerh));
+		if (!owner)
+			return ERR_PTR(-EINVAL);
 		return owner;
+	}
 
 	shadow->lockcnt = count;
 	return NULL;
@@ -118,7 +125,8 @@ static inline int pse51_mutex_timedlock_
 
 {
 	pse51_mutex_t *mutex;
-	xnthread_t *owner, *old;
+	xnthread_t *owner;
+	xnhandle_t ownerh, old;
 	spl_t s;
 	int err;
 
@@ -128,27 +136,42 @@ static inline int pse51_mutex_timedlock_
 		return PTR_ERR(owner);
 
 	mutex = shadow->mutex;
-	if (clear_claimed(owner) == cur)
+	if (owner == cur)
 		return -EBUSY;
 
 	/* Set bit 0, so that mutex_unlock will know that the mutex is claimed.
 	   Hold the nklock, for mutual exclusion with slow mutex_unlock. */
 	xnlock_get_irqsave(&nklock, s);
-	while(!test_claimed(owner)) {
-		old = xnarch_atomic_intptr_cmpxchg(mutex->owner,
-						   owner, set_claimed(owner, 1));
-		if (likely(old == owner))
+
+	ownerh = xnarch_atomic_get(mutex->owner);
+	while (!test_claimed(ownerh)) {
+		old = xnarch_atomic_cmpxchg(mutex->owner, ownerh,
+					    set_claimed(ownerh, 1));
+		if (likely(old == ownerh))
 			break;
-		if (old == NULL) {
+		if (old == 0) {
 			/* Owner called fast mutex_unlock
 			   (on another cpu) */
 			xnlock_put_irqrestore(&nklock, s);
 			goto retry_lock;
 		}
-		owner = old;
+
+		ownerh = old;
+		owner = xnregistry_fetch(clear_claimed(ownerh));
+
+		if (unlikely(!owner)) {
+			err = -EINVAL;
+			goto error;
+		}
+	}
+
+	/* Consistency check for owner handle - is the object a thread? */
+	if (unlikely(xnthread_handle(owner) != clear_claimed(ownerh))) {
+		err = -EINVAL;
+		goto error;
 	}
 
-	xnsynch_set_owner(&mutex->synchbase, clear_claimed(owner));
+	xnsynch_set_owner(&mutex->synchbase, owner);
 	++mutex->sleepers;
 	if (timed)
 		xnsynch_sleep_on(&mutex->synchbase, abs_to, XN_REALTIME);
@@ -169,7 +192,8 @@ static inline int pse51_mutex_timedlock_
 		goto error;
 	}
 
-	xnarch_atomic_intptr_set(mutex->owner,set_claimed(cur, mutex->sleepers));
+	ownerh = set_claimed(xnthread_handle(cur), mutex->sleepers);
+	xnarch_atomic_set(mutex->owner, ownerh);
 	shadow->lockcnt = count;
 	xnlock_put_irqrestore(&nklock, s);
 
@@ -177,9 +201,9 @@ static inline int pse51_mutex_timedlock_
 
   error:
 	if (!mutex->sleepers)
-		xnarch_atomic_intptr_set
+		xnarch_atomic_set
 			(mutex->owner,
-			 clear_claimed(xnarch_atomic_intptr_get(mutex->owner)));
+			 clear_claimed(xnarch_atomic_get(mutex->owner)));
 	xnlock_put_irqrestore(&nklock, s);
 	return err;
 }
@@ -187,16 +211,18 @@ static inline int pse51_mutex_timedlock_
 static inline void pse51_mutex_unlock_internal(xnthread_t *cur,
 					       pse51_mutex_t *mutex)
 {
+	xnhandle_t ownerh;
 	xnthread_t *owner;
 	spl_t s;
 
-	if (likely(xnarch_atomic_intptr_cmpxchg(mutex->owner, cur, NULL) == cur))
+	if (likely(xnarch_atomic_cmpxchg(mutex->owner, cur, XN_NO_HANDLE) ==
+		   xnthread_handle(cur)))
 		return;
 
 	xnlock_get_irqsave(&nklock, s);
 	owner = xnsynch_wakeup_one_sleeper(&mutex->synchbase);
-	xnarch_atomic_intptr_set(mutex->owner,
-				 set_claimed(owner, mutex->sleepers));
+	ownerh = set_claimed(xnthread_handle(owner), mutex->sleepers);
+	xnarch_atomic_set(mutex->owner, ownerh);
 	if (owner)
 		xnpod_schedule();
 	xnlock_put_irqrestore(&nklock, s);
Index: b/ksrc/skins/posix/syscall.c
===================================================================
--- a/ksrc/skins/posix/syscall.c
+++ b/ksrc/skins/posix/syscall.c
@@ -1060,7 +1060,8 @@ static int __pthread_mutex_unlock(struct
 
 	mutex = shadow->mutex;
 
-	if (clear_claimed(xnarch_atomic_intptr_get(mutex->owner)) != cur) {
+	if (clear_claimed(xnarch_atomic_get(mutex->owner)) !=
+	    xnthread_handle(cur)) {
 		err = -EPERM;
 		goto out;
 	}
@@ -1119,7 +1120,7 @@ static int __pthread_mutex_init(struct p
 	pthread_mutexattr_t locattr, *attr, *uattrp;
 	union __xeno_mutex mx, *umx;
 	pse51_mutex_t *mutex;
-	xnarch_atomic_intptr_t *ownerp;
+	xnarch_atomic_t *ownerp;
 	int err;
 
 	umx = (union __xeno_mutex *)__xn_reg_arg1(regs);
@@ -1144,9 +1145,9 @@ static int __pthread_mutex_init(struct p
 	if (!mutex)
 		return -ENOMEM;
 
-	ownerp = (xnarch_atomic_intptr_t *)
+	ownerp = (xnarch_atomic_t *)
 		xnheap_alloc(&xnsys_ppd_get(attr->pshared)->sem_heap,
-			     sizeof(xnarch_atomic_intptr_t));
+			     sizeof(xnarch_atomic_t));
 	if (!ownerp) {
 		xnfree(mutex);
 		return -EAGAIN;
@@ -1185,7 +1186,7 @@ static int __pthread_mutex_destroy(struc
 	if (pse51_kqueues(mutex->attr.pshared) != mutex->owningq)
 		return -EPERM;
 
-	if (xnarch_atomic_intptr_get(mutex->owner))
+	if (xnarch_atomic_get(mutex->owner))
 		return -EBUSY;
 
 	pse51_mark_deleted(shadow);
Index: b/src/skins/posix/mutex.c
===================================================================
--- a/src/skins/posix/mutex.c
+++ b/src/skins/posix/mutex.c
@@ -31,12 +31,12 @@ extern int __pse51_muxid;
 
 extern unsigned long xeno_sem_heap[2];
 
-static xnarch_atomic_intptr_t *get_ownerp(struct __shadow_mutex *shadow)
+static xnarch_atomic_t *get_ownerp(struct __shadow_mutex *shadow)
 {
 	if (likely(!shadow->attr.pshared))
 		return shadow->owner;
 	
-	return (xnarch_atomic_intptr_t *) (xeno_sem_heap[1] + shadow->owner_offset);
+	return (xnarch_atomic_t *) (xeno_sem_heap[1] + shadow->owner_offset);
 }
 #endif /* CONFIG_XENO_FASTSEM */
 
@@ -117,7 +117,7 @@ int __wrap_pthread_mutex_init(pthread_mu
 
 #ifdef CONFIG_XENO_FASTSEM
 	if (!shadow->attr.pshared)
-		shadow->owner = (xnarch_atomic_intptr_t *)
+		shadow->owner = (xnarch_atomic_t *)
 			(xeno_sem_heap[0] + shadow->owner_offset);
 	
 	cb_write_unlock(&shadow->lock, s);
@@ -149,7 +149,7 @@ int __wrap_pthread_mutex_lock(pthread_mu
 	int err = 0;
 
 #ifdef CONFIG_XENO_FASTSEM
-	xnthread_t *cur, *owner;
+	xnhandle_t cur, owner;
 
 	cur = xeno_get_current();
 	if (!cur)
@@ -163,7 +163,7 @@ int __wrap_pthread_mutex_lock(pthread_mu
 		goto out;
 	}
 
-	owner = xnarch_atomic_intptr_cmpxchg(get_ownerp(shadow), NULL, cur);
+	owner = xnarch_atomic_cmpxchg(get_ownerp(shadow), XN_NO_HANDLE, cur);
 	if (likely(!owner)) {
 		shadow->lockcnt = 1;
 		cb_read_unlock(&shadow->lock, s);
@@ -210,7 +210,7 @@ int __wrap_pthread_mutex_timedlock(pthre
 	int err = 0;
 
 #ifdef CONFIG_XENO_FASTSEM
-	xnthread_t *cur, *owner;
+	xnhandle_t cur, owner;
 
 	cur = xeno_get_current();
 	if (!cur)
@@ -224,7 +224,7 @@ int __wrap_pthread_mutex_timedlock(pthre
 		goto out;
 	}	
 
-	owner = xnarch_atomic_intptr_cmpxchg(get_ownerp(shadow), NULL, cur);
+	owner = xnarch_atomic_cmpxchg(get_ownerp(shadow), XN_NO_HANDLE, cur);
 	if (likely(!owner)) {
 		shadow->lockcnt = 1;
 		cb_read_unlock(&shadow->lock, s);
@@ -271,7 +271,7 @@ int __wrap_pthread_mutex_trylock(pthread
 	int err = 0;
 
 #ifdef CONFIG_XENO_FASTSEM
-	xnthread_t *cur, *owner;
+	xnhandle_t cur, owner;
 
 	cur = xeno_get_current();
 	if (!cur)
@@ -285,7 +285,7 @@ int __wrap_pthread_mutex_trylock(pthread
 		goto out;
 	}	
 
-	owner = xnarch_atomic_intptr_cmpxchg(get_ownerp(shadow), NULL, cur);
+	owner = xnarch_atomic_cmpxchg(get_ownerp(shadow), XN_NO_HANDLE, cur);
 	if (likely(!owner)) {
 		shadow->lockcnt = 1;
 		cb_read_unlock(&shadow->lock, s);
@@ -325,8 +325,8 @@ int __wrap_pthread_mutex_unlock(pthread_
 	int err = 0;
 
 #ifdef CONFIG_XENO_FASTSEM
-	xnarch_atomic_intptr_t *ownerp;
-	xnthread_t *cur;
+	xnarch_atomic_t *ownerp;
+	xnhandle_t cur, owner;
 
 	cur = xeno_get_current();
 	if (!cur)
@@ -341,7 +341,8 @@ int __wrap_pthread_mutex_unlock(pthread_
 	}
 
 	ownerp = get_ownerp(shadow);
-	if (unlikely(clear_claimed(xnarch_atomic_intptr_get(ownerp)) != cur)) {
+	owner = clear_claimed(xnarch_atomic_get(ownerp));
+	if (unlikely(owner != cur)) {
 		err = -EPERM;
 		goto out_err;
 	}
@@ -352,7 +353,7 @@ int __wrap_pthread_mutex_unlock(pthread_
 		goto out;
 	}
 
-	if (likely(xnarch_atomic_intptr_cmpxchg(ownerp, cur, NULL) == cur)) {
+	if (likely(xnarch_atomic_cmpxchg(ownerp, cur, XN_NO_HANDLE) == cur)) {
 	  out:
 		cb_read_unlock(&shadow->lock, s);
 		return 0;
Index: b/ksrc/skins/posix/thread.c
===================================================================
--- a/ksrc/skins/posix/thread.c
+++ b/ksrc/skins/posix/thread.c
@@ -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);
 
+#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 */
+
 #ifdef CONFIG_XENO_OPT_PERVASIVE
 	thread->hkey.u_tid = 0;
 	thread->hkey.mm = NULL;
Index: b/ksrc/skins/posix/Kconfig
===================================================================
--- a/ksrc/skins/posix/Kconfig
+++ b/ksrc/skins/posix/Kconfig
@@ -1,5 +1,6 @@
 menuconfig XENO_SKIN_POSIX
 	depends on XENO_OPT_NUCLEUS 
+	select XENO_OPT_REGISTRY if XENO_FASTSEM
 	tristate "POSIX API"
 	default y
 	help
Index: b/include/asm-generic/bits/bind.h
===================================================================
--- a/include/asm-generic/bits/bind.h
+++ b/include/asm-generic/bits/bind.h
@@ -22,7 +22,14 @@ __attribute__ ((weak))
 void xeno_set_current(void)
 {
 	void *kthread_cb;
-	XENOMAI_SYSCALL1(__xn_sys_current, &kthread_cb);
+	int err;
+
+	err = XENOMAI_SYSCALL1(__xn_sys_current, &kthread_cb);
+	if (err) {
+		fprintf(stderr, "Xenomai: error obtaining handle for current "
+			"thread: %s\n", strerror(err));
+		exit(1);
+	}
 	pthread_setspecific(xeno_current_key, kthread_cb);
 }
 

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> 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...

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> 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...

Find it below (PATCH 4/3).

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

Jan

---
 src/skins/native/task.c     |    3 +++
 src/skins/psos+/task.c      |    3 +++
 src/skins/uitron/task.c     |    3 +++
 src/skins/vrtx/task.c       |    3 +++
 src/skins/vxworks/taskLib.c |    3 +++
 5 files changed, 15 insertions(+)

Index: b/src/skins/native/task.c
===================================================================
--- a/src/skins/native/task.c
+++ b/src/skins/native/task.c
@@ -26,6 +26,7 @@
 #include <limits.h>
 #include <native/syscall.h>
 #include <native/task.h>
+#include <asm-generic/bits/current.h>
 #include "wrappers.h"
 
 extern pthread_key_t __native_tskey;
@@ -88,6 +89,8 @@ static void *rt_task_trampoline(void *co
 	if (err)
 		goto fail;
 
+	xeno_set_current();
+
 	/* Wait on the barrier for the task to be started. The barrier
 	   could be released in order to process Linux signals while the
 	   Xenomai shadow is still dormant; in such a case, resume wait. */
Index: b/src/skins/psos+/task.c
===================================================================
--- a/src/skins/psos+/task.c
+++ b/src/skins/psos+/task.c
@@ -26,6 +26,7 @@
 #include <memory.h>
 #include <string.h>
 #include <psos+/psos.h>
+#include <asm-generic/bits/current.h>
 
 extern int __psos_muxid;
 
@@ -89,6 +90,8 @@ static void *psos_task_trampoline(void *
 	if (err)
 		goto fail;
 
+	xeno_set_current();
+
 	/* Wait on the barrier for the task to be started. The barrier
 	   could be released in order to process Linux signals while the
 	   Xenomai shadow is still dormant; in such a case, resume wait. */
Index: b/src/skins/uitron/task.c
===================================================================
--- a/src/skins/uitron/task.c
+++ b/src/skins/uitron/task.c
@@ -25,6 +25,7 @@
 #include <limits.h>
 #include <asm/xenomai/system.h>
 #include <uitron/uitron.h>
+#include <asm-generic/bits/current.h>
 
 extern int __uitron_muxid;
 
@@ -89,6 +90,8 @@ static void *uitron_task_trampoline(void
 	if (err)
 		goto fail;
 
+	xeno_set_current();
+
 	/* iargs->pk_ctsk might not be valid anymore, after our parent
 	   was released from the completion sync, so do not
 	   dereference this pointer. */
Index: b/src/skins/vrtx/task.c
===================================================================
--- a/src/skins/vrtx/task.c
+++ b/src/skins/vrtx/task.c
@@ -27,6 +27,7 @@
 #include <errno.h>
 #include <limits.h>
 #include <vrtx/vrtx.h>
+#include <asm-generic/bits/current.h>
 
 extern pthread_key_t __vrtx_tskey;
 
@@ -106,6 +107,8 @@ static void *vrtx_task_trampoline(void *
 	if (err)
 		goto fail;
 
+	xeno_set_current();
+
 	/* Wait on the barrier for the task to be started. The barrier
 	   could be released in order to process Linux signals while the
 	   Xenomai shadow is still dormant; in such a case, resume wait. */
Index: b/src/skins/vxworks/taskLib.c
===================================================================
--- a/src/skins/vxworks/taskLib.c
+++ b/src/skins/vxworks/taskLib.c
@@ -27,6 +27,7 @@
 #include <errno.h>
 #include <limits.h>
 #include <vxworks/vxworks.h>
+#include <asm-generic/bits/current.h>
 #include "wrappers.h"
 
 extern pthread_key_t __vxworks_tskey;
@@ -117,6 +118,8 @@ static void *wind_task_trampoline(void *
 	if (err)
 		goto fail;
 
+	xeno_set_current();
+
 	/* Wait on the barrier for the task to be started. The barrier
 	   could be released in order to process Linux signals while the
 	   Xenomai shadow is still dormant; in such a case, resume wait. */

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> 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...
> 
> 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.

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> 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...
>> 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.

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.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> 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...
>>> 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.
> 
> 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.

Then it looks redundant with the code that does associate a handle with
a new thread in the posix skin.

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> 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...
>>> 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.
> 
> 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.

Yes please rework that and do the registration at thread creation in
kernel-space. Since the system call is not called for kernel-space
threads, the on-demand registry registration would not work for them,
unless.... no, I can not imagine that.

-- 
					    Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:

> -#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)))
> +#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.

> +	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.

> +	/* 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 ?


> Index: b/ksrc/skins/posix/thread.c
> ===================================================================
> --- a/ksrc/skins/posix/thread.c
> +++ b/ksrc/skins/posix/thread.c
> @@ -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);
>  
> +#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 ?

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
> 
>> -#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)))
>> +#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)?

> 
>> +	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.

> 
>> +	/* 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).

> 
> 
>> Index: b/ksrc/skins/posix/thread.c
>> ===================================================================
>> --- a/ksrc/skins/posix/thread.c
>> +++ b/ksrc/skins/posix/thread.c
>> @@ -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);
>>  
>> +#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.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>
>>> -#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)))
>>> +#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)?
> 
>>> +	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.
> 
>>> +	/* 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).

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

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>
>>>> -#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)))
>>>> +#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)?
>>
>>>> +	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.
>>
>>>> +	/* 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).
> 
> 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.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>
>>>>> -#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)))
>>>>> +#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)?
>>>
>>>>> +	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.
>>>
>>>>> +	/* 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).
>> 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.

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)).

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>
>>>>>> -#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)))
>>>>>> +#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)?
>>>>
>>>>>> +	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.
>>>>
>>>>>> +	/* 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).
>>> 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.
> 
> 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.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>
>>>>>>> -#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)))
>>>>>>> +#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)?
>>>>>
>>>>>>> +	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.
>>>>>
>>>>>>> +	/* 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).
>>>> 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.
>> 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...

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>
>>>>>>>> -#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)))
>>>>>>>> +#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)?
>>>>>>
>>>>>>>> +	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.
>>>>>>
>>>>>>>> +	/* 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).
>>>>> 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.
>>> 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...

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.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>
>>>>>>>>> -#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)))
>>>>>>>>> +#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)?
>>>>>>>
>>>>>>>>> +	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.
>>>>>>>
>>>>>>>>> +	/* 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).
>>>>>> 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.
>>>> 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...
> 
> 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.

Ok. Got it. Leave the check then.

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> 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.

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> 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:
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.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> 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:
> 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.

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> 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:
>> 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.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> 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:
>>> 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.

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

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> 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:
>>>> 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.
> 
> 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).

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> 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:
>>>>> 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.
>> 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).

Ok, but using hash tables instead of static tables allow you to have
more objects in the hash than hash buckets, and a "registry slot"
occupies simply a bit in a multi-level bitfield when free, so you can
have preposterous registry slots limits like 2048 or 4096.

-- 
					    Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>
>>> -#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)))
>>> +#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)?
> 
>>> +	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.
> 
>>> +	/* 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).
> 
>>
>>> Index: b/ksrc/skins/posix/thread.c
>>> ===================================================================
>>> --- a/ksrc/skins/posix/thread.c
>>> +++ b/ksrc/skins/posix/thread.c
>>> @@ -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);
>>>  
>>> +#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.

Ok. But we need to add the register in all skins then.

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>
>>> -#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)))
>>> +#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)?

the if is ok. I guess the compiler does !! with an if too...

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> 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.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> 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.

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).

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> 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.
> 
> 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?

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> 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.
>> 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.

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> 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.
>>> 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.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> 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.
>>>> 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.

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

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3788 bytes --]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> 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.
>>>>> 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.
> 
> 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.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> 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.
>>>>>> 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.
>> 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.

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".

-- 
					    Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>> 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.
>>>>>>> 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.
>>> 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.
> 
> 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".

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.

-- 
					    Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 5387 bytes --]

Gilles Chanteperdrix wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>> 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.
>>>>>>>> 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.
>>>> 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.
>> 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".
> 
> 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.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>> 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.
>>>>>>>>> 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.
>>>>> 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.
>>> 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".
>> 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.

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.

-- 
					    Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 6149 bytes --]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>>> 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.
>>>>>>>>>> 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.
>>>>>> 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.
>>>> 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".
>>> 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.
> 
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>>>> 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.
>>>>>>>>>>> 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.
>>>>>>> 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.
>>>>> 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".
>>>> 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.
>> 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.

Here is an exerpt of the logs:
2008-08-27 16:55:54 1KYMR8-0004Uw-2w SMTP error from remote mailer after
initial connection: host mail.gna.org [88.191.250.46]: 421 mail.gna.org
service not available, please try later.
Then:
2008-08-27 16:55:55 1KYMR8-0004Uw-2w => jan.kiszka@domain.hid
R=dnslookup T=remote_smtp H=hephaistos.siemens.com [217.194.34.77]
X=TLS-1.0:RSA_AES_256_CBC_SHA:32
2
And finally:
2008-08-27 17:13:08 1KYMR8-0004Uw-2w => xenomai@xenomai.org R=dnslookup
T=remote_smtp H=mail.gna.org [88.191.250.46]*
2008-08-27 17:13:08 1KYMR8-0004Uw-2w Completed

-- 
					    Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 5947 bytes --]

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.

Jan

---
 ksrc/skins/posix/mutex.c |   10 +++++++---
 ksrc/skins/posix/mutex.h |   17 +++++++++++------
 2 files changed, 18 insertions(+), 9 deletions(-)

Index: b/ksrc/skins/posix/mutex.c
===================================================================
--- a/ksrc/skins/posix/mutex.c
+++ b/ksrc/skins/posix/mutex.c
@@ -117,7 +117,6 @@ int pse51_mutex_init_internal(struct __s
 	mutex->attr = *attr;
 	mutex->owner = ownerp;
 	mutex->owningq = kq;
-	mutex->sleepers = 0;
 	xnarch_atomic_set(ownerp, XN_NO_HANDLE);
 
 	xnlock_get_irqsave(&nklock, s);
@@ -305,14 +304,12 @@ int pse51_mutex_timedlock_break(struct _
 		/* Attempting to relock a normal mutex, deadlock. */
 		xnlock_get_irqsave(&nklock, s);
 		for (;;) {
-			++mutex->sleepers;
 			if (timed)
 				xnsynch_sleep_on(&mutex->synchbase,
 						 abs_to, XN_REALTIME);
 			else
 				xnsynch_sleep_on(&mutex->synchbase,
 						 XN_INFINITE, XN_RELATIVE);
-			--mutex->sleepers;
 
 			if (xnthread_test_info(cur, XNBREAK)) {
 				err = -EINTR;
@@ -329,6 +326,13 @@ int pse51_mutex_timedlock_break(struct _
 				break;
 			}
 		}
+		if (!xnsynch_nsleepers(&mutex->synchbase)) {
+			xnarch_atomic_set
+				(mutex->owner,
+				 clear_claimed
+					(xnarch_atomic_get(mutex->owner)));
+			xnsynch_set_owner(&mutex->synchbase, NULL);
+		}
 		xnlock_put_irqrestore(&nklock, s);
 
 		break;
Index: b/ksrc/skins/posix/mutex.h
===================================================================
--- a/ksrc/skins/posix/mutex.h
+++ b/ksrc/skins/posix/mutex.h
@@ -57,7 +57,6 @@ typedef struct pse51_mutex {
 
 	xnarch_atomic_t *owner;
 	pthread_mutexattr_t attr;
-	unsigned sleepers;
 	pse51_kqueues_t *owningq;
 } pse51_mutex_t;
 
@@ -172,12 +171,10 @@ static inline int pse51_mutex_timedlock_
 	}
 
 	xnsynch_set_owner(&mutex->synchbase, owner);
-	++mutex->sleepers;
 	if (timed)
 		xnsynch_sleep_on(&mutex->synchbase, abs_to, XN_REALTIME);
 	else
 		xnsynch_sleep_on(&mutex->synchbase, XN_INFINITE, XN_RELATIVE);
-	--mutex->sleepers;
 
 	if (xnthread_test_info(cur, XNBREAK)) {
 		err = -EINTR;
@@ -192,7 +189,11 @@ static inline int pse51_mutex_timedlock_
 		goto error;
 	}
 
-	ownerh = set_claimed(xnthread_handle(cur), mutex->sleepers);
+	ownerh = xnthread_handle(cur);
+	if (xnsynch_nsleepers(&mutex->synchbase))
+		ownerh = set_claimed(ownerh, 1);
+	else
+		xnsynch_set_owner(&mutex->synchbase, NULL);
 	xnarch_atomic_set(mutex->owner, ownerh);
 	shadow->lockcnt = count;
 	xnlock_put_irqrestore(&nklock, s);
@@ -200,10 +201,12 @@ static inline int pse51_mutex_timedlock_
 	return 0;
 
   error:
-	if (!mutex->sleepers)
+	if (!xnsynch_nsleepers(&mutex->synchbase)) {
 		xnarch_atomic_set
 			(mutex->owner,
 			 clear_claimed(xnarch_atomic_get(mutex->owner)));
+		xnsynch_set_owner(&mutex->synchbase, NULL);
+	}
 	xnlock_put_irqrestore(&nklock, s);
 	return err;
 }
@@ -221,7 +224,9 @@ static inline void pse51_mutex_unlock_in
 
 	xnlock_get_irqsave(&nklock, s);
 	owner = xnsynch_wakeup_one_sleeper(&mutex->synchbase);
-	ownerh = set_claimed(xnthread_handle(owner), mutex->sleepers);
+	ownerh =
+	    set_claimed(xnthread_handle(owner),
+			xnsynch_nsleepers(&mutex->synchbase));
 	xnarch_atomic_set(mutex->owner, ownerh);
 	if (owner)
 		xnpod_schedule();


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> 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.

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.

-- 
					    Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Philippe Gerum]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> 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.
> 
> 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.

-- 
Philippe.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Philippe Gerum wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> 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.
>> 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.

... but would also not be able to provide PIP. Unfortunately, we want
PIP here, but we do separate owner tracking, so this differentiation
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.

> 
> 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

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> 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.
>>> 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.
> 
> ... but would also not be able to provide PIP. Unfortunately, we want
> PIP here,

I don't follow you here. You just can't provide PIP if you don't have ownership.

 but we do separate owner tracking, so this differentiation
> 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.
> 

Mmfff...

>> 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...
> 

This is why I don't think moving part of the core logic to the skins would be a
good idea.

> Jan
> 


-- 
Philippe.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Philippe Gerum wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> 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.
>> 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.

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.

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 5582 bytes --]

Gilles Chanteperdrix wrote:
> Philippe Gerum wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> 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.
>>> 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.
> 
> 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).

> 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.

For now we have two files already, but maybe we can merge them later
based on your proposal.

> 
> 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.

I have some intermediate version here that breaks up the
retry-on-XNROBBED loop, doing it at skin level instead. This is
targeting the first customer delivery.

I also have two implementations of fast locking now, native and posix,
and they don't look that bad /wrt moving quite a few bits into xnsynch -
later.

Jan

PS: The test cases + my /proc/xenomai/registry/usage probably unveiled
another pending bug: Native user space tasks are leaking handles, even
on vanilla Xenomai SVN (didn't test 2.4.x yet). Will look into this later.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Philippe Gerum wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> 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.
>>>> 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.
>> 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.

-- 
					    Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4801 bytes --]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Philippe Gerum wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> 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.
>>>>> 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.
>>> 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.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 258 bytes --]

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Philippe Gerum wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> 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.
>>>>>> 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.
>>>> 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.

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Philippe Gerum wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> 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.
>>>>>>> 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.
>>>>> 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.

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.

Jan

PS: Handle leak fixed, #4156, was SVN-only, I caused it.

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Philippe Gerum wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> 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.
>>>>>>>> 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.
>>>>>> 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.
> 
> 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.

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Philippe Gerum wrote:
>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>> 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.
>>>>>>>>> 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.
>>>>>>> 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.
>> 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.

Once the dust settles here, I will have a look at the stable series as
well, providing a fix for the "old" mutexes.

> 
> 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... ;)

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> 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... ;)

The way xnsynch_sleep_on checks the owner field and retries for the
implementation of the lock stealing already furiously look like a mutex
compare and exchange...


-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Philippe Gerum]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Philippe Gerum wrote:
>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>> 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.
>>>>>>>>> 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.
>>>>>>> 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.
>> 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.

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


-- 
Philippe.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Philippe Gerum wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Philippe Gerum wrote:
>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>> 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.
>>>>>>>>>> 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.
>>>>>>>> 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.
>>> 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.

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

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>> Philippe Gerum wrote:
>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>>> 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.
>>>>>>>>>>> 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.
>>>>>>>>> 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.
>>>> 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.
> 
> 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.
>

It's very fundamentally crappy. Please don't do that. xnsynch_sleep_on() must do
what it is supposed to mean.

> Jan
> 


-- 
Philippe.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>> Philippe Gerum wrote:
>>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>>>> 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.
>>>>>>>>>>>> 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.
>>>>>>>>>> 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.
>>>>> 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.
>> 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.
>>
> 
> It's very fundamentally crappy. Please don't do that. xnsynch_sleep_on() must do
> what it is supposed to mean.

So we need xnsynch_try_to_steal + lots of new special-case code in the
skins to re-implement what is already there? No, I can't imagine that
this is what you want to see in the end.

Then lets better rename xnsynch_sleep_on to something like
xnsynch_acquire - will be logically needed anyway when we push fast
mutex maintenance into it.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Jan Kiszka wrote:
>>> Philippe Gerum wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>> Philippe Gerum wrote:
>>>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>>>>> 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.
>>>>>>>>>>>>> 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.
>>>>>>>>>>> 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.
>>>>>> 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.
>>> 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.
>>>
>> It's very fundamentally crappy. Please don't do that. xnsynch_sleep_on() must do
>> what it is supposed to mean.
> 
> So we need xnsynch_try_to_steal + lots of new special-case code in the
> skins to re-implement what is already there? No, I can't imagine that
> this is what you want to see in the end.
>

What I don't want is to see code running a routine called "sleep_on" knowing
that it may not sleep. What I don't want either, is to go through
xnsynch_sleep_on -> xnpod_suspend_thread -> xntimer_start ->
xnsynch_forget_sleeper to get things right with !PIP objects, albeit the caller
knows from the beginning that it does not want to sleep. So much for the fast
path to trylocking.

> Then lets better rename xnsynch_sleep_on to something like
> xnsynch_acquire - will be logically needed anyway when we push fast
> mutex maintenance into it.
> 

This would not fix issue #2. xnsynch_try_acquire() that would attempt stealing
is the way to go. It's even possible that some code could be shared between that
routine, and the resource stealing support currently provided by
xnsynch_sleep_on(). Only resource acquisition routines that enforce exclusive
ownership would be impacted, not each and every xnsynch_sleep_on caller.

> Jan
> 


-- 
Philippe.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> Jan Kiszka wrote:
>>>> Philippe Gerum wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>>> Philippe Gerum wrote:
>>>>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>>>>>> 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.
>>>>>>>>>>>>>> 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.
>>>>>>>>>>>> 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.
>>>>>>> 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.
>>>> 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.
>>>>
>>> It's very fundamentally crappy. Please don't do that. xnsynch_sleep_on() must do
>>> what it is supposed to mean.
>> So we need xnsynch_try_to_steal + lots of new special-case code in the
>> skins to re-implement what is already there? No, I can't imagine that
>> this is what you want to see in the end.
>>
> 
> What I don't want is to see code running a routine called "sleep_on" knowing
> that it may not sleep. What I don't want either, is to go through
> xnsynch_sleep_on -> xnpod_suspend_thread -> xntimer_start ->
> xnsynch_forget_sleeper to get things right with !PIP objects, albeit the caller
> knows from the beginning that it does not want to sleep. So much for the fast
> path to trylocking.

OK, point taken. But as this is a generic API issue, I would like to see
it fixed in the generic code, not in some to-be-duplicated branches in
all the skins.

My point is that our timeout API nicely defines the case XN_NONBLOCK
now, throughout all layers. We just need to pass the arguments as-is
down, the core will handle them. Let's do optimizations at the lowest
possible layer, i.e. in xnsynch_sleep_on (or whatever it may be called
better). A simple 'if' before suspend should be enough. And it will keep
the code small.

> 
>> Then lets better rename xnsynch_sleep_on to something like
>> xnsynch_acquire - will be logically needed anyway when we push fast
>> mutex maintenance into it.
>>
> 
> This would not fix issue #2. xnsynch_try_acquire() that would attempt stealing
> is the way to go. It's even possible that some code could be shared between that
> routine, and the resource stealing support currently provided by
> xnsynch_sleep_on(). Only resource acquisition routines that enforce exclusive
> ownership would be impacted, not each and every xnsynch_sleep_on caller.

For now it will be a PIP show, but atomic_cmpxchg-based semaphores might
be an xnsynch extension in the future as well.

Jan

---
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> 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.
> 
> Jan
> 
> ---
>  ksrc/skins/posix/mutex.c |   10 +++++++---
>  ksrc/skins/posix/mutex.h |   17 +++++++++++------
>  2 files changed, 18 insertions(+), 9 deletions(-)
> 
> Index: b/ksrc/skins/posix/mutex.c
> ===================================================================
> --- a/ksrc/skins/posix/mutex.c
> +++ b/ksrc/skins/posix/mutex.c
> @@ -117,7 +117,6 @@ int pse51_mutex_init_internal(struct __s
>  	mutex->attr = *attr;
>  	mutex->owner = ownerp;
>  	mutex->owningq = kq;
> -	mutex->sleepers = 0;
>  	xnarch_atomic_set(ownerp, XN_NO_HANDLE);
>  
>  	xnlock_get_irqsave(&nklock, s);
> @@ -305,14 +304,12 @@ int pse51_mutex_timedlock_break(struct _
>  		/* Attempting to relock a normal mutex, deadlock. */
>  		xnlock_get_irqsave(&nklock, s);
>  		for (;;) {
> -			++mutex->sleepers;
>  			if (timed)
>  				xnsynch_sleep_on(&mutex->synchbase,
>  						 abs_to, XN_REALTIME);
>  			else
>  				xnsynch_sleep_on(&mutex->synchbase,
>  						 XN_INFINITE, XN_RELATIVE);
> -			--mutex->sleepers;
>  
>  			if (xnthread_test_info(cur, XNBREAK)) {
>  				err = -EINTR;
> @@ -329,6 +326,13 @@ int pse51_mutex_timedlock_break(struct _
>  				break;
>  			}
>  		}
> +		if (!xnsynch_nsleepers(&mutex->synchbase)) {
> +			xnarch_atomic_set
> +				(mutex->owner,
> +				 clear_claimed
> +					(xnarch_atomic_get(mutex->owner)));
> +			xnsynch_set_owner(&mutex->synchbase, NULL);
> +		}

I think an atomic_cmpxchg would be better here.

-- 
					    Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4499 bytes --]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> 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.
>>
>> Jan
>>
>> ---
>>  ksrc/skins/posix/mutex.c |   10 +++++++---
>>  ksrc/skins/posix/mutex.h |   17 +++++++++++------
>>  2 files changed, 18 insertions(+), 9 deletions(-)
>>
>> Index: b/ksrc/skins/posix/mutex.c
>> ===================================================================
>> --- a/ksrc/skins/posix/mutex.c
>> +++ b/ksrc/skins/posix/mutex.c
>> @@ -117,7 +117,6 @@ int pse51_mutex_init_internal(struct __s
>>  	mutex->attr = *attr;
>>  	mutex->owner = ownerp;
>>  	mutex->owningq = kq;
>> -	mutex->sleepers = 0;
>>  	xnarch_atomic_set(ownerp, XN_NO_HANDLE);
>>  
>>  	xnlock_get_irqsave(&nklock, s);
>> @@ -305,14 +304,12 @@ int pse51_mutex_timedlock_break(struct _
>>  		/* Attempting to relock a normal mutex, deadlock. */
>>  		xnlock_get_irqsave(&nklock, s);
>>  		for (;;) {
>> -			++mutex->sleepers;
>>  			if (timed)
>>  				xnsynch_sleep_on(&mutex->synchbase,
>>  						 abs_to, XN_REALTIME);
>>  			else
>>  				xnsynch_sleep_on(&mutex->synchbase,
>>  						 XN_INFINITE, XN_RELATIVE);
>> -			--mutex->sleepers;
>>  
>>  			if (xnthread_test_info(cur, XNBREAK)) {
>>  				err = -EINTR;
>> @@ -329,6 +326,13 @@ int pse51_mutex_timedlock_break(struct _
>>  				break;
>>  			}
>>  		}
>> +		if (!xnsynch_nsleepers(&mutex->synchbase)) {
>> +			xnarch_atomic_set
>> +				(mutex->owner,
>> +				 clear_claimed
>> +					(xnarch_atomic_get(mutex->owner)));
>> +			xnsynch_set_owner(&mutex->synchbase, NULL);
>> +		}
> 
> I think an atomic_cmpxchg would be better here.
> 

Not really. The claimed bit is set, so no one can change the owner
without entering the kernel. But there we are also holding nklock. BTW,
pse51_mutex_timedlock_internal does the same already.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> 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.
>>>
>>> Jan
>>>
>>> ---
>>>  ksrc/skins/posix/mutex.c |   10 +++++++---
>>>  ksrc/skins/posix/mutex.h |   17 +++++++++++------
>>>  2 files changed, 18 insertions(+), 9 deletions(-)
>>>
>>> Index: b/ksrc/skins/posix/mutex.c
>>> ===================================================================
>>> --- a/ksrc/skins/posix/mutex.c
>>> +++ b/ksrc/skins/posix/mutex.c
>>> @@ -117,7 +117,6 @@ int pse51_mutex_init_internal(struct __s
>>>  	mutex->attr = *attr;
>>>  	mutex->owner = ownerp;
>>>  	mutex->owningq = kq;
>>> -	mutex->sleepers = 0;
>>>  	xnarch_atomic_set(ownerp, XN_NO_HANDLE);
>>>  
>>>  	xnlock_get_irqsave(&nklock, s);
>>> @@ -305,14 +304,12 @@ int pse51_mutex_timedlock_break(struct _
>>>  		/* Attempting to relock a normal mutex, deadlock. */
>>>  		xnlock_get_irqsave(&nklock, s);
>>>  		for (;;) {
>>> -			++mutex->sleepers;
>>>  			if (timed)
>>>  				xnsynch_sleep_on(&mutex->synchbase,
>>>  						 abs_to, XN_REALTIME);
>>>  			else
>>>  				xnsynch_sleep_on(&mutex->synchbase,
>>>  						 XN_INFINITE, XN_RELATIVE);
>>> -			--mutex->sleepers;
>>>  
>>>  			if (xnthread_test_info(cur, XNBREAK)) {
>>>  				err = -EINTR;
>>> @@ -329,6 +326,13 @@ int pse51_mutex_timedlock_break(struct _
>>>  				break;
>>>  			}
>>>  		}
>>> +		if (!xnsynch_nsleepers(&mutex->synchbase)) {
>>> +			xnarch_atomic_set
>>> +				(mutex->owner,
>>> +				 clear_claimed
>>> +					(xnarch_atomic_get(mutex->owner)));
>>> +			xnsynch_set_owner(&mutex->synchbase, NULL);
>>> +		}
>> I think an atomic_cmpxchg would be better here.
>>
> 
> Not really. The claimed bit is set, so no one can change the owner
> without entering the kernel. But there we are also holding nklock. BTW,
> pse51_mutex_timedlock_internal does the same already.

I meant from an assembly generated code point of view. atomic_set +
atomic_get generates two atomic operations (well, except on x86),
whereas atomic_cmpxchg is just an atomic sequence. Except that we will
have to do the atomic_get also.

And we do not give a damn about this code. It is the implementation of
the deadlock that happens when trying to relock a non-recursive mutex.

-- 
					    Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> @@ -329,6 +326,13 @@ int pse51_mutex_timedlock_break(struct _
>  				break;
>  			}
>  		}
> +		if (!xnsynch_nsleepers(&mutex->synchbase)) {
> +			xnarch_atomic_set
> +				(mutex->owner,
> +				 clear_claimed
> +					(xnarch_atomic_get(mutex->owner)));
> +			xnsynch_set_owner(&mutex->synchbase, NULL);
> +		}
>  		xnlock_put_irqrestore(&nklock, s);

I do not like this at all. I mean, unless I am mistaken, we loose more
than we gain, we are adding a couple of atomic, hence heavy, operations
in a common case for handling a corner case. I still prefer emitting a
system call in the corner case.

-- 
					    Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 999 bytes --]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> @@ -329,6 +326,13 @@ int pse51_mutex_timedlock_break(struct _
>>  				break;
>>  			}
>>  		}
>> +		if (!xnsynch_nsleepers(&mutex->synchbase)) {
>> +			xnarch_atomic_set
>> +				(mutex->owner,
>> +				 clear_claimed
>> +					(xnarch_atomic_get(mutex->owner)));
>> +			xnsynch_set_owner(&mutex->synchbase, NULL);
>> +		}
>>  		xnlock_put_irqrestore(&nklock, s);
> 
> I do not like this at all. I mean, unless I am mistaken, we loose more
> than we gain, we are adding a couple of atomic, hence heavy, operations
> in a common case for handling a corner case. I still prefer emitting a
> system call in the corner case.

The hunk above is in the mutex' deadlock path - I wouldn't call this a
common case. Moreover, we don't any expensive cmpxchg here.

I haven't counted ops, but my strong feeling is that this patch actually
shortens the common code paths + it safely avoids one syscall in the
lock stealing case.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> @@ -329,6 +326,13 @@ int pse51_mutex_timedlock_break(struct _
>>>  				break;
>>>  			}
>>>  		}
>>> +		if (!xnsynch_nsleepers(&mutex->synchbase)) {
>>> +			xnarch_atomic_set
>>> +				(mutex->owner,
>>> +				 clear_claimed
>>> +					(xnarch_atomic_get(mutex->owner)));
>>> +			xnsynch_set_owner(&mutex->synchbase, NULL);
>>> +		}
>>>  		xnlock_put_irqrestore(&nklock, s);
>> I do not like this at all. I mean, unless I am mistaken, we loose more
>> than we gain, we are adding a couple of atomic, hence heavy, operations
>> in a common case for handling a corner case. I still prefer emitting a
>> system call in the corner case.
> 
> The hunk above is in the mutex' deadlock path - I wouldn't call this a
> common case. Moreover, we don't any expensive cmpxchg here.
> 
> I haven't counted ops, but my strong feeling is that this patch actually
> shortens the common code paths + it safely avoids one syscall in the
> lock stealing case.

I have not yet understood how this happens.

-- 
                                                 Gilles.

Re: [Xenomai-core] [RFC][PATCH 2/3] Switch to handle-based fast mutex owners

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> @@ -329,6 +326,13 @@ int pse51_mutex_timedlock_break(struct _
>>>>  				break;
>>>>  			}
>>>>  		}
>>>> +		if (!xnsynch_nsleepers(&mutex->synchbase)) {
>>>> +			xnarch_atomic_set
>>>> +				(mutex->owner,
>>>> +				 clear_claimed
>>>> +					(xnarch_atomic_get(mutex->owner)));
>>>> +			xnsynch_set_owner(&mutex->synchbase, NULL);
>>>> +		}
>>>>  		xnlock_put_irqrestore(&nklock, s);
>>> I do not like this at all. I mean, unless I am mistaken, we loose more
>>> than we gain, we are adding a couple of atomic, hence heavy, operations
>>> in a common case for handling a corner case. I still prefer emitting a
>>> system call in the corner case.
>> The hunk above is in the mutex' deadlock path - I wouldn't call this a
>> common case. Moreover, we don't any expensive cmpxchg here.
>>
>> I haven't counted ops, but my strong feeling is that this patch actually
>> shortens the common code paths + it safely avoids one syscall in the
>> lock stealing case.
> 
> I have not yet understood how this happens.

Argh, it won't work for all cases: When the robbed owner retries the
acquisition while the current owner still holds the lock, that clearing
of xnsynch_owner has the fatal effect of letting the former thread
proceed as well. So we cannot get around to extend xnsynch, teaching it
about the two-level ownership management - or actually folding fast lock
support into xnsynch.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[Xenomai-core] [RFC][PATCH 3/3] Remove xnarch_atomic_intptr wrappers

[Jan Kiszka]

No more users, no more use cases in sight, let's get rid of it.

---
 include/asm-arm/atomic.h        |    3 ---
 include/asm-blackfin/atomic.h   |    2 --
 include/asm-generic/Makefile.am |    2 +-
 include/asm-generic/Makefile.in |    2 +-
 include/asm-generic/atomic.h    |   24 ------------------------
 include/asm-ia64/atomic.h       |    2 --
 include/asm-powerpc/atomic.h    |    2 --
 include/asm-x86/atomic.h        |    2 --
 8 files changed, 2 insertions(+), 37 deletions(-)

Index: b/include/asm-arm/atomic.h
===================================================================
--- a/include/asm-arm/atomic.h
+++ b/include/asm-arm/atomic.h
@@ -412,9 +412,6 @@ static __inline__ void xnarch_atomic_cle
 
 typedef unsigned long atomic_flags_t;
 
-/* Add support for xnarch_atomic_intptr_t */
-#include <asm-generic/xenomai/atomic.h>
-
 #endif /* !_XENO_ASM_ARM_ATOMIC_H */
 
 // vim: ts=4 et sw=4 sts=4
Index: b/include/asm-blackfin/atomic.h
===================================================================
--- a/include/asm-blackfin/atomic.h
+++ b/include/asm-blackfin/atomic.h
@@ -42,8 +42,6 @@
 typedef atomic_t atomic_counter_t;
 typedef atomic_t xnarch_atomic_t;
 
-#include <asm-generic/xenomai/atomic.h>
-
 #else /* !__KERNEL__ */
 
 #include <asm/xenomai/syscall.h>
Index: b/include/asm-generic/Makefile.am
===================================================================
--- a/include/asm-generic/Makefile.am
+++ b/include/asm-generic/Makefile.am
@@ -1,5 +1,5 @@
 includesubdir = $(includedir)/asm-generic
 
-includesub_HEADERS = arith.h atomic.h features.h hal.h syscall.h system.h wrappers.h
+includesub_HEADERS = arith.h features.h hal.h syscall.h system.h wrappers.h
 
 SUBDIRS = bits
Index: b/include/asm-generic/Makefile.in
===================================================================
--- a/include/asm-generic/Makefile.in
+++ b/include/asm-generic/Makefile.in
@@ -226,7 +226,7 @@ target_vendor = @target_vendor@
 top_builddir = @top_builddir@
 top_srcdir = @top_srcdir@
 includesubdir = $(includedir)/asm-generic
-includesub_HEADERS = arith.h atomic.h features.h hal.h syscall.h system.h wrappers.h
+includesub_HEADERS = arith.h features.h hal.h syscall.h system.h wrappers.h
 SUBDIRS = bits
 all: all-recursive
 
Index: b/include/asm-generic/atomic.h
===================================================================
--- a/include/asm-generic/atomic.h
+++ /dev/null
@@ -1,24 +0,0 @@
-#ifndef _XENO_ASM_GENERIC_ATOMIC_H
-#define _XENO_ASM_GENERIC_ATOMIC_H
-
-typedef xnarch_atomic_t xnarch_atomic_intptr_t;
-
-static inline void *xnarch_atomic_intptr_get(xnarch_atomic_intptr_t *l)
-{
-        xnarch_atomic_t *v = (xnarch_atomic_t *)l;
-
-        return (void *)xnarch_atomic_get(v);
-}
-
-static inline void xnarch_atomic_intptr_set(xnarch_atomic_intptr_t *l, void *i)
-{
-        xnarch_atomic_t *v = (xnarch_atomic_t *)l;
-
-        xnarch_atomic_set(v, (long)i);
-}
-
-#define xnarch_atomic_intptr_cmpxchg(l, old, newval) \
-        (void *)(xnarch_atomic_cmpxchg((xnarch_atomic_t *)(l), \
-				       (long)(old), (long)(newval)))
-
-#endif /* _XENO_ASM_GENERIC_ATOMIC_H */
Index: b/include/asm-ia64/atomic.h
===================================================================
--- a/include/asm-ia64/atomic.h
+++ b/include/asm-ia64/atomic.h
@@ -71,8 +71,6 @@ static inline void atomic_clear_mask(uns
 #define xnarch_atomic_cmpxchg(pcounter,old,new) \
 	cmpxchg((&(pcounter)->counter),(old),(new))
 
-#include <asm-generic/xenomai/atomic.h>
-
 #else /* !__KERNEL__ */
 
 #include <asm/xenomai/features.h>
Index: b/include/asm-powerpc/atomic.h
===================================================================
--- a/include/asm-powerpc/atomic.h
+++ b/include/asm-powerpc/atomic.h
@@ -262,6 +262,4 @@ xnarch_atomic_cmpxchg(xnarch_atomic_t *p
 
 typedef unsigned long atomic_flags_t;
 
-#include <asm-generic/xenomai/atomic.h>
-
 #endif /* !_XENO_ASM_POWERPC_ATOMIC_H */
Index: b/include/asm-x86/atomic.h
===================================================================
--- a/include/asm-x86/atomic.h
+++ b/include/asm-x86/atomic.h
@@ -136,6 +136,4 @@ xnarch_atomic_cmpxchg(xnarch_atomic_t *v
 
 #endif /* __KERNEL__ */
 
-#include <asm-generic/xenomai/atomic.h>
-
 #endif /* !_XENO_ASM_X86_ATOMIC_64_H */