[Xenomai-core] [PATCH 0/2] Fix and improve task/thread inquire services

[Xenomai-core] [PATCH 0/2] Fix and improve task/thread inquire services

[Jan Kiszka]

This series fixes the issues around rt_task_inquire I posted yesterday.
Additionally, it introduces an analogous services pthread_inquire_np for
the POSIX skin. That allows, among other things, to implement test cases
for the upcoming fast xnsynch/mutex patches.

Jan

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

[Xenomai-core] [PATCH 1/2] Fix status values reported by rt_task_inquire

[Jan Kiszka]

[-- Attachment #1: fix-status-reported-by-rt_task_inquire.patch --]
[-- Type: text/plain, Size: 5115 bytes --]

So far rt_task_inquire simply copied the nucleus thread status, leaking
lots of undocumented or incorrectly described state values to the user.
This patch first of all fixes the T_* documentation, adds further
informative states, and then ensures that only those are reported back
in RT_TASK_INFO.status. This includes correct reporting of T_PRIMARY and
T_JOINABLE.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---
 include/native/task.h       |   38 +++++++++++++++++++++++---------------
 ksrc/skins/native/syscall.c |    7 +++++++
 ksrc/skins/native/task.c    |    5 ++++-
 3 files changed, 34 insertions(+), 16 deletions(-)

Index: b/include/native/task.h
===================================================================
--- a/include/native/task.h
+++ b/include/native/task.h
@@ -28,7 +28,6 @@
 
 /* Creation flags. */
 #define T_FPU     XNFPU
-#define T_SUSP    XNSUSP
 /* <!> High bits must not conflict with XNFPU|XNSHADOW|XNSHIELD|XNSUSP. */
 #define T_CPU(cpu) (1 << (24 + (cpu & 7))) /* Up to 8 cpus [0-7] */
 #define T_CPUMASK  0xff000000
@@ -40,22 +39,31 @@
   @{
  */
 
-#define T_BLOCKED  XNPEND     /**< See #XNPEND    */
-#define T_DELAYED  XNDELAY    /**< See #XNDELAY   */
-#define T_READY    XNREADY    /**< See #XNREADY   */
-#define T_DORMANT  XNDORMANT  /**< See #XNDORMANT */
-#define T_STARTED  XNSTARTED  /**< See #XNSTARTED */
-#define T_BOOST    XNBOOST    /**< See #XNBOOST   */
-#define T_LOCK     XNLOCK     /**< See #XNLOCK    */
-#define T_RRB      XNRRB      /**< See #XNRRB     */
-#define T_NOSIG    XNASDI     /**< See #XNASDI    */ 
-#define T_SHIELD   XNSHIELD   /**< See #XNSHIELD  */ 
-#define T_WARNSW   XNTRAPSW   /**< See #XNTRAPSW  */ 
-#define T_RPIOFF   XNRPIOFF   /**< See #XNRPIOFF  */ 
-#define T_PRIMARY  0x00000200	/* Recycle internal bits status which */
-#define T_JOINABLE 0x00000400	/* won't be passed to the nucleus.  */
+#define T_SUSP       XNSUSP	/**< Suspended. */
+#define T_BLOCKED    XNPEND	/**< Sleep-wait for a resource. */
+#define T_DELAYED    XNDELAY	/**< Delayed. */
+#define T_READY      XNREADY	/**< Linked to the ready queue. */
+#define T_DORMANT    XNDORMANT	/**< Not started yet or killed. */
+#define T_STARTED    XNSTARTED	/**< Thread has been started. */
+/* Reuses XNRELAX flag, but invertedly. */
+#define T_PRIMARY    0x00000200	/**< Running under Xenomai control (primary mode). */
+#define T_HELD       XNHELD	/**< Held thread from suspended partition. */
+#define T_BOOST      XNBOOST	/**< Undergoes a PIP boost. */
+#define T_DEBUG      XNDEBUG	/**< Hit a debugger breakpoint (user space only). */
+#define T_LOCK       XNLOCK	/**< Holds the scheduler lock (i.e. not preemptible). */
+#define T_RRB        XNRRB	/**< Undergoes a round-robin scheduling. */
+#define T_NOSIG      XNASDI	/**< ASR are disabled. */
+#define T_SHIELD     XNSHIELD	/**< IRQ shield is enabled (user space only). */
+#define T_WARNSW     XNTRAPSW	/**< Trap execution mode switches. */
+#define T_RPIOFF     XNRPIOFF	/**< Stop priority coupling (user space only). */
+#define T_USERSPACE  XNSHADOW	/**< User space task. */
+/* Reuses XNROOT (ROOT is not an addressable task). */
+#define T_JOINABLE   0x00400000	/**< Task electable for rt_task_join. */
 /*! @} */ /* Ends doxygen-group native_task_status */
 
+/* Used to save the T_JOINABLE state after initialization. */
+#define __T_JOINABLE XNTHREAD_INFO_SPARE0
+
 /* Task hook types. */
 #define T_HOOK_START  XNHOOK_THREAD_START
 #define T_HOOK_SWITCH XNHOOK_THREAD_SWITCH
Index: b/ksrc/skins/native/syscall.c
===================================================================
--- a/ksrc/skins/native/syscall.c
+++ b/ksrc/skins/native/syscall.c
@@ -159,6 +159,8 @@ static int __rt_task_create(struct pt_re
 	prio = bulk.a3;
 	/* Task init mode & CPU affinity. */
 	mode = bulk.a4 & (T_CPUMASK | T_SUSP | T_SHIELD);
+	if (bulk.a4 & T_JOINABLE)
+		mode |= __T_JOINABLE;
 
 	task = (RT_TASK *)xnmalloc(sizeof(*task));
 
@@ -516,6 +518,11 @@ static int __rt_task_inquire(struct pt_r
 	if (err)
 		return err;
 
+	if (info.status & __T_JOINABLE) {
+		info.status &= ~__T_JOINABLE;
+		info.status |= T_JOINABLE;
+	}
+
 	if (__xn_safe_copy_to_user((void __user *)__xn_reg_arg2(regs),
 				   &info, sizeof(info)))
 		return -EFAULT;
Index: b/ksrc/skins/native/task.c
===================================================================
--- a/ksrc/skins/native/task.c
+++ b/ksrc/skins/native/task.c
@@ -1129,7 +1129,10 @@ int rt_task_inquire(RT_TASK *task, RT_TA
 	strcpy(info->name, xnthread_name(&task->thread_base));
 	info->bprio = xnthread_base_priority(&task->thread_base);
 	info->cprio = xnthread_current_priority(&task->thread_base);
-	info->status = xnthread_state_flags(&task->thread_base);
+	info->status = xnthread_state_flags(&task->thread_base) &
+		~(XNZOMBIE | XNRESTART | XNMAPPED | XNFPU | XNROOT | XNSWLOCK
+		  | 0x0f000000);
+	info->status ^= T_PRIMARY; /* we use XNRELAX invertedly */
 	info->relpoint = xntimer_get_date(&task->thread_base.ptimer);
 	raw_exectime = xnthread_get_exectime(&task->thread_base);
 	if (task->thread_base.sched->runthread == &task->thread_base)

[Xenomai-core] [PATCH 2/2] Add pthread_inquire_np service

[Jan Kiszka]

[-- Attachment #1: add-pthread_inquire_np.patch --]
[-- Type: text/plain, Size: 8466 bytes --]

Almost identically to what rt_task_inquire provides for the native skin,
this introduces the pthread_inquire_np extension to the POSIX API. Only
supported thread status values are reported back, PTHREAD_PRIMARY
included.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---
 include/posix/pthread.h    |   53 ++++++++++++++++++++++++++++++++++++++----
 include/posix/syscall.h    |    1 
 ksrc/skins/posix/syscall.c |   23 ++++++++++++++++++
 ksrc/skins/posix/thread.c  |   56 +++++++++++++++++++++++++++++++++++++++++++++
 src/skins/posix/thread.c   |    7 +++++
 5 files changed, 135 insertions(+), 5 deletions(-)

Index: b/include/posix/pthread.h
===================================================================
--- a/include/posix/pthread.h
+++ b/include/posix/pthread.h
@@ -135,6 +135,7 @@ typedef struct
 #include_next <pthread.h>
 #include <nucleus/thread.h>
 #include <nucleus/intr.h>
+#include <nucleus/types.h>
 
 struct timespec;
 
@@ -144,11 +145,31 @@ struct timespec;
 #define PTHREAD_PRIO_INHERIT 1
 #define PTHREAD_PRIO_PROTECT 2
 
-#define PTHREAD_SHIELD     XNSHIELD
-#define PTHREAD_WARNSW     XNTRAPSW
-#define PTHREAD_LOCK_SCHED XNLOCK
-#define PTHREAD_RPIOFF     XNRPIOFF
-#define PTHREAD_PRIMARY    XNTHREAD_STATE_SPARE1
+/**
+ * @ingroup posix
+ * @defgroup thread_task_status Thread Status
+ * @brief Defines used to specify thread state and/or mode
+ * @{
+ */
+
+#define PTHREAD_SUSP       XNSUSP	/**< Suspended. */
+#define PTHREAD_BLOCKED    XNPEND	/**< Sleep-wait for a resource. */
+#define PTHREAD_DELAYED    XNDELAY	/**< Delayed. */
+#define PTHREAD_READY      XNREADY	/**< Linked to the ready queue. */
+#define PTHREAD_DORMANT    XNDORMANT	/**< Not started yet or killed. */
+#define PTHREAD_STARTED    XNSTARTED	/**< Thread has been started. */
+#define PTHREAD_HELD       XNHELD	/**< Held thread from suspended partition. */
+#define PTHREAD_BOOST      XNBOOST	/**< Undergoes a PIP boost. */
+#define PTHREAD_DEBUG      XNDEBUG	/**< Hit a debugger breakpoint (user space only). */
+#define PTHREAD_LOCK_SCHED XNLOCK	/**< Holds the scheduler lock (i.e. not preemptible). */
+#define PTHREAD_RRB        XNRRB	/**< Undergoes a round-robin scheduling. */
+#define PTHREAD_NOSIG      XNASDI	/**< ASR are disabled. */
+#define PTHREAD_SHIELD     XNSHIELD	/**< IRQ shield is enabled (user space only). */
+#define PTHREAD_WARNSW     XNTRAPSW	/**< Trap execution mode switches. */
+#define PTHREAD_RPIOFF     XNRPIOFF	/**< Stop priority coupling (user space only). */
+#define PTHREAD_USERSPACE  XNSHADOW	/**< User space task. */
+#define PTHREAD_PRIMARY    XNTHREAD_STATE_SPARE1 /**< Running under Xenomai control (primary mode). */
+/*! @} */ /* Ends doxygen-group thread_task_status */
 
 #define PTHREAD_INOAUTOENA  XN_ISR_NOENABLE
 #define PTHREAD_IPROPAGATE  XN_ISR_PROPAGATE
@@ -183,6 +204,24 @@ struct pse51_interrupt;
 
 typedef struct pse51_interrupt *pthread_intr_t;
 
+/**
+ * Structure containing thread information useful to users.
+ *
+ *  @see pthread_inquire_np()
+ */
+typedef struct {
+
+	int bprio; /**< Base priority. */
+	int cprio; /**< Current priority. May change through Priority Inheritance.*/
+	unsigned status; /**< Thread status. @see thread_state_flags */
+	struct timespec relpoint; /**< Time of next release.*/
+	char name[XNOBJECT_NAME_LEN];  /**< Symbolic name assigned at creation. */
+	struct timespec exectime; /**< Execution time in primary mode. */
+	int modeswitches; /**< Number of primary->secondary mode switches. */
+	int ctxswitches; /**< Number of context switches. */
+	int pagefaults; /**< Number of triggered page faults. */
+} pthread_info_t;
+
 #if defined(__KERNEL__) || defined(__XENO_SIM__)
 typedef struct pse51_mutexattr pthread_mutexattr_t;
 
@@ -385,6 +424,8 @@ int pthread_set_mode_np(int clrmask,
 int pthread_set_name_np(pthread_t thread,
 			const char *name);
 
+int pthread_inquire_np(pthread_t thread, pthread_info_t *info);
+
 int pthread_intr_attach_np(pthread_intr_t *intr,
 			   unsigned irq,
 			   xnisr_t isr,
@@ -429,6 +470,8 @@ int pthread_set_mode_np(int clrmask,
 int pthread_set_name_np(pthread_t thread,
 			const char *name);
 
+int pthread_inquire_np(pthread_t thread, pthread_info_t *info);
+
 int pthread_intr_attach_np(pthread_intr_t *intr,
 			   unsigned irq,
 			   int mode);
Index: b/include/posix/syscall.h
===================================================================
--- a/include/posix/syscall.h
+++ b/include/posix/syscall.h
@@ -102,6 +102,7 @@
 #define __pse51_thread_getschedparam  75
 #define __pse51_thread_kill           76
 #define __pse51_select                77
+#define __pse51_thread_inquire        78
 
 #ifdef __KERNEL__
 
Index: b/ksrc/skins/posix/syscall.c
===================================================================
--- a/ksrc/skins/posix/syscall.c
+++ b/ksrc/skins/posix/syscall.c
@@ -370,6 +370,28 @@ static int __pthread_set_name_np(struct
 	return -pthread_set_name_np(k_tid, name);
 }
 
+static int __pthread_inquire_np(struct pt_regs *regs)
+{
+	struct pse51_hkey hkey;
+	pthread_info_t info;
+	pthread_t k_tid;
+	int err;
+
+	hkey.u_tid = __xn_reg_arg1(regs);
+	hkey.mm = current->mm;
+	k_tid = __pthread_find(&hkey);
+
+	if (!k_tid)
+		return -ESRCH;
+
+	err = -pthread_inquire_np(k_tid, &info);
+	if (err)
+		return err;
+
+	return __xn_safe_copy_to_user((void __user *)__xn_reg_arg2(regs),
+				      &info, sizeof(info));
+}
+
 static int __pthread_kill(struct pt_regs *regs)
 {
 	struct pse51_hkey hkey;
@@ -2762,6 +2784,7 @@ static xnsysent_t __systab[] = {
 	[__pse51_condattr_setpshared] =
 	    {&__pthread_condattr_setpshared, __xn_exec_any},
 	[__pse51_select] = {&__select, __xn_exec_primary},
+	[__pse51_thread_inquire] = {&__pthread_inquire_np, __xn_exec_any},
 };
 
 static void __shadow_delete_hook(xnthread_t *thread)
Index: b/ksrc/skins/posix/thread.c
===================================================================
--- a/ksrc/skins/posix/thread.c
+++ b/ksrc/skins/posix/thread.c
@@ -698,6 +698,62 @@ int pthread_set_name_np(pthread_t thread
 	return 0;
 }
 
+/**
+ * Inquire about a thread.
+ *
+ * Return various information about the status of a given thread.
+ *
+ * This service is a non-portable extension of the POSIX interface.
+ *
+ * @param thread identifier of the thread to inquire;
+ *
+ * @param info address of a structure the thread information will be
+ * written to.
+ *
+ * @return 0 on success;
+ * @return an error number if:
+ * - ESRCH, @a thread is invalid;
+ *
+ */
+int pthread_inquire_np(pthread_t thread, pthread_info_t *info)
+{
+	xnticks_t raw_exectime;
+	int err = 0;
+	spl_t s;
+
+	xnlock_get_irqsave(&nklock, s);
+
+	if (!pse51_obj_active(thread, PSE51_THREAD_MAGIC, struct pse51_thread)) {
+		err = ESRCH;
+		goto unlock_and_exit;
+	}
+
+	strcpy(info->name, xnthread_name(&thread->threadbase));
+	info->bprio = xnthread_base_priority(&thread->threadbase);
+	info->cprio = xnthread_current_priority(&thread->threadbase);
+	info->status = xnthread_state_flags(&thread->threadbase) &
+		~(XNZOMBIE | XNRESTART | XNRELAX | XNMAPPED | XNFPU | XNROOT
+		  | XNSWLOCK | 0x0f000000);
+	if (!xnthread_test_state(&thread->threadbase, XNRELAX))
+		info->status |= PTHREAD_PRIMARY;
+	ticks2ts(&info->relpoint,
+		 xntimer_get_date(&thread->threadbase.ptimer));
+	raw_exectime = xnthread_get_exectime(&thread->threadbase);
+	if (thread->threadbase.sched->runthread == &thread->threadbase)
+		raw_exectime += xnstat_exectime_now() -
+			xnthread_get_lastswitch(&thread->threadbase);
+	ticks2ts(&info->exectime, xnarch_tsc_to_ns(raw_exectime));
+	info->modeswitches = xnstat_counter_get(&thread->threadbase.stat.ssw);
+	info->ctxswitches = xnstat_counter_get(&thread->threadbase.stat.csw);
+	info->pagefaults = xnstat_counter_get(&thread->threadbase.stat.pf);
+
+      unlock_and_exit:
+
+	xnlock_put_irqrestore(&nklock, s);
+
+	return err;
+}
+
 void pse51_thread_abort(pthread_t thread, void *status)
 {
 	thread_exit_status(thread) = status;
Index: b/src/skins/posix/thread.c
===================================================================
--- a/src/skins/posix/thread.c
+++ b/src/skins/posix/thread.c
@@ -270,6 +270,13 @@ int pthread_set_name_np(pthread_t thread
 				  __pse51_thread_set_name, thread, name);
 }
 
+int pthread_inquire_np(pthread_t thread, pthread_info_t *info)
+{
+	return -XENOMAI_SKINCALL2(__pse51_muxid,
+				  __pse51_thread_inquire,
+				  thread, info);
+}
+
 int __wrap_pthread_kill(pthread_t thread, int sig)
 {
 	int err;

Re: [Xenomai-core] [PATCH 0/2] Fix and improve task/thread inquire services

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> This series fixes the issues around rt_task_inquire I posted yesterday.
> Additionally, it introduces an analogous services pthread_inquire_np for
> the POSIX skin. That allows, among other things, to implement test cases
> for the upcoming fast xnsynch/mutex patches.

Ok. Since this applies only for debugging purpose, and displaying
whether a task is in primary mode may be use badly by users, maybe we
should make this service a shadow syscall, and not export any interface
to use it. This would further avoid duplication between the native and
posix skins.

Philippe, what do you think ?

-- 
                                                 Gilles.

Re: [Xenomai-core] [PATCH 0/2] Fix and improve task/thread inquire services

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> This series fixes the issues around rt_task_inquire I posted yesterday.
>> Additionally, it introduces an analogous services pthread_inquire_np for
>> the POSIX skin. That allows, among other things, to implement test cases
>> for the upcoming fast xnsynch/mutex patches.
> 
> Ok. Since this applies only for debugging purpose, and displaying
> whether a task is in primary mode may be use badly by users, maybe we
> should make this service a shadow syscall, and not export any interface
> to use it. This would further avoid duplication between the native and
> posix skins.

Debugging is not the holy, exclusive business of Xenomai hackers.

The inquire services are useful in libraries as well, when you want to
check if the caller complies to the call convention ("don't use in
primary mode", "caller's priority must not exceed X" or whatever).

That said, I'm open for unifying the code, maybe introducing some
xnthread_inquire.

Jan

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

Re: [Xenomai-core] [PATCH 0/2] Fix and improve task/thread inquire services

[Philippe Gerum]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> This series fixes the issues around rt_task_inquire I posted yesterday.
>>> Additionally, it introduces an analogous services pthread_inquire_np for
>>> the POSIX skin. That allows, among other things, to implement test cases
>>> for the upcoming fast xnsynch/mutex patches.
>> Ok. Since this applies only for debugging purpose, and displaying
>> whether a task is in primary mode may be use badly by users, maybe we
>> should make this service a shadow syscall, and not export any interface
>> to use it. This would further avoid duplication between the native and
>> posix skins.
> 
> Debugging is not the holy, exclusive business of Xenomai hackers.
> 
> The inquire services are useful in libraries as well, when you want to
> check if the caller complies to the call convention ("don't use in
> primary mode", "caller's priority must not exceed X" or whatever).
> 
> That said, I'm open for unifying the code, maybe introducing some
> xnthread_inquire.
>

That would be much better than publishing an open interface to fiddle even more
with thread modes via rt_task_set_mode(). I would definitely merge that.

> Jan
> 


-- 
Philippe.

Re: [Xenomai-core] [PATCH 0/2] Fix and improve task/thread inquire services

[Jan Kiszka]

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

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> This series fixes the issues around rt_task_inquire I posted yesterday.
>>>> Additionally, it introduces an analogous services pthread_inquire_np for
>>>> the POSIX skin. That allows, among other things, to implement test cases
>>>> for the upcoming fast xnsynch/mutex patches.
>>> Ok. Since this applies only for debugging purpose, and displaying
>>> whether a task is in primary mode may be use badly by users, maybe we
>>> should make this service a shadow syscall, and not export any interface
>>> to use it. This would further avoid duplication between the native and
>>> posix skins.
>> Debugging is not the holy, exclusive business of Xenomai hackers.
>>
>> The inquire services are useful in libraries as well, when you want to
>> check if the caller complies to the call convention ("don't use in
>> primary mode", "caller's priority must not exceed X" or whatever).
>>
>> That said, I'm open for unifying the code, maybe introducing some
>> xnthread_inquire.
>>
> 
> That would be much better than publishing an open interface to fiddle even more
> with thread modes via rt_task_set_mode(). I would definitely merge that.

Code refactoring is no problem, will work that out. I just want to keep
the user interface.

Jan


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

Re: [Xenomai-core] [PATCH 0/2] Fix and improve task/thread inquire services

[Jan Kiszka]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> This series fixes the issues around rt_task_inquire I posted yesterday.
>>>>> Additionally, it introduces an analogous services pthread_inquire_np for
>>>>> the POSIX skin. That allows, among other things, to implement test cases
>>>>> for the upcoming fast xnsynch/mutex patches.
>>>> Ok. Since this applies only for debugging purpose, and displaying
>>>> whether a task is in primary mode may be use badly by users, maybe we
>>>> should make this service a shadow syscall, and not export any interface
>>>> to use it. This would further avoid duplication between the native and
>>>> posix skins.
>>> Debugging is not the holy, exclusive business of Xenomai hackers.
>>>
>>> The inquire services are useful in libraries as well, when you want to
>>> check if the caller complies to the call convention ("don't use in
>>> primary mode", "caller's priority must not exceed X" or whatever).
>>>
>>> That said, I'm open for unifying the code, maybe introducing some
>>> xnthread_inquire.
>>>
>> That would be much better than publishing an open interface to fiddle even more
>> with thread modes via rt_task_set_mode(). I would definitely merge that.
> 
> Code refactoring is no problem, will work that out. I just want to keep
> the user interface.

Looking into this, I come to the conclusion that xnthread_inquire is
only then a gain if both rt_task_inquire and pthread_inquire_np use the
same data structure layout. And that means that both need to use the
same time encodings, not struct timespec vs. RTIME like it is now. That
would not be beautiful, but feasible (e.g. picking __u64 as type,
passing nanoseconds). Still, it does not yet convince me.

Jan

Re: [Xenomai-core] [PATCH 0/2] Fix and improve task/thread inquire services

[Philippe Gerum]

Jan Kiszka wrote:
> Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> This series fixes the issues around rt_task_inquire I posted yesterday.
>>>>>> Additionally, it introduces an analogous services pthread_inquire_np for
>>>>>> the POSIX skin. That allows, among other things, to implement test cases
>>>>>> for the upcoming fast xnsynch/mutex patches.
>>>>> Ok. Since this applies only for debugging purpose, and displaying
>>>>> whether a task is in primary mode may be use badly by users, maybe we
>>>>> should make this service a shadow syscall, and not export any interface
>>>>> to use it. This would further avoid duplication between the native and
>>>>> posix skins.
>>>> Debugging is not the holy, exclusive business of Xenomai hackers.
>>>>
>>>> The inquire services are useful in libraries as well, when you want to
>>>> check if the caller complies to the call convention ("don't use in
>>>> primary mode", "caller's priority must not exceed X" or whatever).
>>>>
>>>> That said, I'm open for unifying the code, maybe introducing some
>>>> xnthread_inquire.
>>>>
>>> That would be much better than publishing an open interface to fiddle even more
>>> with thread modes via rt_task_set_mode(). I would definitely merge that.
>> Code refactoring is no problem, will work that out. I just want to keep
>> the user interface.
> 
> Looking into this, I come to the conclusion that xnthread_inquire is
> only then a gain if both rt_task_inquire and pthread_inquire_np use the
> same data structure layout. And that means that both need to use the
> same time encodings, not struct timespec vs. RTIME like it is now. That
> would not be beautiful, but feasible (e.g. picking __u64 as type,
> passing nanoseconds). Still, it does not yet convince me.
>

The idea behind xnthread_inquire() is not about replacing rt_task_inquire()
under the hood, but rather to get back only fundamental values, such as the
current thread status, in order to determine whether XNRELAX is set or not for
instance.

XENOMAI_SYSCALL1(__xn_sys_inquire) => xnthread_inquire(xnpod_current_thread())

> Jan
> 


-- 
Philippe.

Re: [Xenomai-core] [PATCH 0/2] Fix and improve task/thread inquire services

[Jan Kiszka]

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

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Philippe Gerum wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> This series fixes the issues around rt_task_inquire I posted yesterday.
>>>>>>> Additionally, it introduces an analogous services pthread_inquire_np for
>>>>>>> the POSIX skin. That allows, among other things, to implement test cases
>>>>>>> for the upcoming fast xnsynch/mutex patches.
>>>>>> Ok. Since this applies only for debugging purpose, and displaying
>>>>>> whether a task is in primary mode may be use badly by users, maybe we
>>>>>> should make this service a shadow syscall, and not export any interface
>>>>>> to use it. This would further avoid duplication between the native and
>>>>>> posix skins.
>>>>> Debugging is not the holy, exclusive business of Xenomai hackers.
>>>>>
>>>>> The inquire services are useful in libraries as well, when you want to
>>>>> check if the caller complies to the call convention ("don't use in
>>>>> primary mode", "caller's priority must not exceed X" or whatever).
>>>>>
>>>>> That said, I'm open for unifying the code, maybe introducing some
>>>>> xnthread_inquire.
>>>>>
>>>> That would be much better than publishing an open interface to fiddle even more
>>>> with thread modes via rt_task_set_mode(). I would definitely merge that.
>>> Code refactoring is no problem, will work that out. I just want to keep
>>> the user interface.
>> Looking into this, I come to the conclusion that xnthread_inquire is
>> only then a gain if both rt_task_inquire and pthread_inquire_np use the
>> same data structure layout. And that means that both need to use the
>> same time encodings, not struct timespec vs. RTIME like it is now. That
>> would not be beautiful, but feasible (e.g. picking __u64 as type,
>> passing nanoseconds). Still, it does not yet convince me.
>>
> 
> The idea behind xnthread_inquire() is not about replacing rt_task_inquire()
> under the hood, but rather to get back only fundamental values, such as the
> current thread status, in order to determine whether XNRELAX is set or not for
> instance.
> 
> XENOMAI_SYSCALL1(__xn_sys_inquire) => xnthread_inquire(xnpod_current_thread())

OK, slowly getting to your core. Assume we had __xn_sys_inquire, what
should it return, and what should we mask from rt_task_inquire, and do
we still want pthread_inquire_np.

But I really dislike this approach of artificially
complicating/crippling interfaces just to hide XNRELAX (I can't imagine
you have problems with the other information rt_task_inquire returns).

Jan


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

Re: [Xenomai-core] [PATCH 0/2] Fix and improve task/thread inquire services

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Jan Kiszka wrote:
>>> Jan Kiszka wrote:
>>>> Philippe Gerum wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> This series fixes the issues around rt_task_inquire I posted yesterday.
>>>>>>>> Additionally, it introduces an analogous services pthread_inquire_np for
>>>>>>>> the POSIX skin. That allows, among other things, to implement test cases
>>>>>>>> for the upcoming fast xnsynch/mutex patches.
>>>>>>> Ok. Since this applies only for debugging purpose, and displaying
>>>>>>> whether a task is in primary mode may be use badly by users, maybe we
>>>>>>> should make this service a shadow syscall, and not export any interface
>>>>>>> to use it. This would further avoid duplication between the native and
>>>>>>> posix skins.
>>>>>> Debugging is not the holy, exclusive business of Xenomai hackers.
>>>>>>
>>>>>> The inquire services are useful in libraries as well, when you want to
>>>>>> check if the caller complies to the call convention ("don't use in
>>>>>> primary mode", "caller's priority must not exceed X" or whatever).
>>>>>>
>>>>>> That said, I'm open for unifying the code, maybe introducing some
>>>>>> xnthread_inquire.
>>>>>>
>>>>> That would be much better than publishing an open interface to fiddle even more
>>>>> with thread modes via rt_task_set_mode(). I would definitely merge that.
>>>> Code refactoring is no problem, will work that out. I just want to keep
>>>> the user interface.
>>> Looking into this, I come to the conclusion that xnthread_inquire is
>>> only then a gain if both rt_task_inquire and pthread_inquire_np use the
>>> same data structure layout. And that means that both need to use the
>>> same time encodings, not struct timespec vs. RTIME like it is now. That
>>> would not be beautiful, but feasible (e.g. picking __u64 as type,
>>> passing nanoseconds). Still, it does not yet convince me.
>>>
>> The idea behind xnthread_inquire() is not about replacing rt_task_inquire()
>> under the hood, but rather to get back only fundamental values, such as the
>> current thread status, in order to determine whether XNRELAX is set or not for
>> instance.
>>
>> XENOMAI_SYSCALL1(__xn_sys_inquire) => xnthread_inquire(xnpod_current_thread())
> 
> OK, slowly getting to your core. Assume we had __xn_sys_inquire, what
> should it return, and what should we mask from rt_task_inquire, and do
> we still want pthread_inquire_np.
>

__xn_sys_inquire would return both the informational and status bitmasks, for
debug and sanity check purposes.

> But I really dislike this approach of artificially
> complicating/crippling interfaces just to hide XNRELAX (I can't imagine
> you have problems with the other information rt_task_inquire returns).
> 

It is not about crippling that interface at all, it is about not adding the
XNRELAX bit - since it is NOT there ATM - to prevent further misuse of the
thread mode, which is a very real problem in lots of application code now.

Look, even Hannes who very well knows what co-kernel stuff is all about
misinterpreted the Xenomai API in that area:
http://www.captain.at/review-rtai-versus-xenomai.php

The reason is likely because people tend to think that if T_PRIMARY is
explicitly provided, that means that the real-time core does not handle the
issue implicitly, and then conclude that they ought to use rt_task_set_mode() to
do the job themselves. That's wrong, that's badly wrong.

Aside of this, using rt_task_inquire() in instrumentation/debug/sanity checking
code would be overkill most of the time. If the purpose is to implement
constructs like:

	if (!task_in_primary_mode()) {
		blah;
	}

Then, you likely don't want the overhead of copying useless things like the task
name, fetching the outstanding timeout values, or any other values we may want
to add in the future to RT_TASK_INFO (e.g. we did not use to send back any kind
of statistical information via that structure in 2.3.x, but we started to do so
with 2.4.x).

> Jan
> 


-- 
Philippe.

Re: [Xenomai-core] [PATCH 0/2] Fix and improve task/thread inquire services

[Jan Kiszka]

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

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> Jan Kiszka wrote:
>>>> Jan Kiszka wrote:
>>>>> Philippe Gerum wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> This series fixes the issues around rt_task_inquire I posted yesterday.
>>>>>>>>> Additionally, it introduces an analogous services pthread_inquire_np for
>>>>>>>>> the POSIX skin. That allows, among other things, to implement test cases
>>>>>>>>> for the upcoming fast xnsynch/mutex patches.
>>>>>>>> Ok. Since this applies only for debugging purpose, and displaying
>>>>>>>> whether a task is in primary mode may be use badly by users, maybe we
>>>>>>>> should make this service a shadow syscall, and not export any interface
>>>>>>>> to use it. This would further avoid duplication between the native and
>>>>>>>> posix skins.
>>>>>>> Debugging is not the holy, exclusive business of Xenomai hackers.
>>>>>>>
>>>>>>> The inquire services are useful in libraries as well, when you want to
>>>>>>> check if the caller complies to the call convention ("don't use in
>>>>>>> primary mode", "caller's priority must not exceed X" or whatever).
>>>>>>>
>>>>>>> That said, I'm open for unifying the code, maybe introducing some
>>>>>>> xnthread_inquire.
>>>>>>>
>>>>>> That would be much better than publishing an open interface to fiddle even more
>>>>>> with thread modes via rt_task_set_mode(). I would definitely merge that.
>>>>> Code refactoring is no problem, will work that out. I just want to keep
>>>>> the user interface.
>>>> Looking into this, I come to the conclusion that xnthread_inquire is
>>>> only then a gain if both rt_task_inquire and pthread_inquire_np use the
>>>> same data structure layout. And that means that both need to use the
>>>> same time encodings, not struct timespec vs. RTIME like it is now. That
>>>> would not be beautiful, but feasible (e.g. picking __u64 as type,
>>>> passing nanoseconds). Still, it does not yet convince me.
>>>>
>>> The idea behind xnthread_inquire() is not about replacing rt_task_inquire()
>>> under the hood, but rather to get back only fundamental values, such as the
>>> current thread status, in order to determine whether XNRELAX is set or not for
>>> instance.
>>>
>>> XENOMAI_SYSCALL1(__xn_sys_inquire) => xnthread_inquire(xnpod_current_thread())
>> OK, slowly getting to your core. Assume we had __xn_sys_inquire, what
>> should it return, and what should we mask from rt_task_inquire, and do
>> we still want pthread_inquire_np.
>>
> 
> __xn_sys_inquire would return both the informational and status bitmasks, for
> debug and sanity check purposes.
> 
>> But I really dislike this approach of artificially
>> complicating/crippling interfaces just to hide XNRELAX (I can't imagine
>> you have problems with the other information rt_task_inquire returns).
>>
> 
> It is not about crippling that interface at all, it is about not adding the
> XNRELAX bit - since it is NOT there ATM - to prevent further misuse of the

Look at the code, it _is_ practically there, it _is_ usable, but it is
inverted (T_PRIMARY==XNRELAX). That's why I insist on the fact that the
current rt_task_inquire implementation is broken when taking its
documentation as a reference.

> thread mode, which is a very real problem in lots of application code now.
> 
> Look, even Hannes who very well knows what co-kernel stuff is all about
> misinterpreted the Xenomai API in that area:
> http://www.captain.at/review-rtai-versus-xenomai.php

Not directly his fault, he just copied our old, incorrect serial test
code (/me failed to review it before it saw the light of the internet).
But I told him ages ago to update his page.

> 
> The reason is likely because people tend to think that if T_PRIMARY is
> explicitly provided, that means that the real-time core does not handle the
> issue implicitly, and then conclude that they ought to use rt_task_set_mode() to
> do the job themselves. That's wrong, that's badly wrong.

Again, don't shoot the messenger, let's discuss how to overcome the
explicit mode switch interface! You are looking at the wrong spot, IMHO.

BTW, RTnet no longer requires explicit mode switch. And for Xenomai 2.5,
I will remove rtdm_device.open_rt/socket_rt as well as
rtdm_operations.close_rt, ie. a whole bunch of handlers that motivated
most of the explicit switches from my POV. So I would be fine with
deprecating that switch service for 2.5 and maybe remove it with 2.6 (or
whatever comes next).

> 
> Aside of this, using rt_task_inquire() in instrumentation/debug/sanity checking
> code would be overkill most of the time. If the purpose is to implement
> constructs like:
> 
> 	if (!task_in_primary_mode()) {
> 		blah;
> 	}
> 
> Then, you likely don't want the overhead of copying useless things like the task
> name, fetching the outstanding timeout values, or any other values we may want
> to add in the future to RT_TASK_INFO (e.g. we did not use to send back any kind
> of statistical information via that structure in 2.3.x, but we started to do so
> with 2.4.x).

That overhead is negligible compared to the anyway required syscall. If
we needed to optimize debugging stuff (I doubt so), we would have to
push that data (also the thread prio etc.) continuously to user space.

Jan


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

Re: [Xenomai-core] [PATCH 0/2] Fix and improve task/thread inquire services

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Jan Kiszka wrote:
>>> Philippe Gerum wrote:
>>>> Jan Kiszka wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Philippe Gerum wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>> This series fixes the issues around rt_task_inquire I posted yesterday.
>>>>>>>>>> Additionally, it introduces an analogous services pthread_inquire_np for
>>>>>>>>>> the POSIX skin. That allows, among other things, to implement test cases
>>>>>>>>>> for the upcoming fast xnsynch/mutex patches.
>>>>>>>>> Ok. Since this applies only for debugging purpose, and displaying
>>>>>>>>> whether a task is in primary mode may be use badly by users, maybe we
>>>>>>>>> should make this service a shadow syscall, and not export any interface
>>>>>>>>> to use it. This would further avoid duplication between the native and
>>>>>>>>> posix skins.
>>>>>>>> Debugging is not the holy, exclusive business of Xenomai hackers.
>>>>>>>>
>>>>>>>> The inquire services are useful in libraries as well, when you want to
>>>>>>>> check if the caller complies to the call convention ("don't use in
>>>>>>>> primary mode", "caller's priority must not exceed X" or whatever).
>>>>>>>>
>>>>>>>> That said, I'm open for unifying the code, maybe introducing some
>>>>>>>> xnthread_inquire.
>>>>>>>>
>>>>>>> That would be much better than publishing an open interface to fiddle even more
>>>>>>> with thread modes via rt_task_set_mode(). I would definitely merge that.
>>>>>> Code refactoring is no problem, will work that out. I just want to keep
>>>>>> the user interface.
>>>>> Looking into this, I come to the conclusion that xnthread_inquire is
>>>>> only then a gain if both rt_task_inquire and pthread_inquire_np use the
>>>>> same data structure layout. And that means that both need to use the
>>>>> same time encodings, not struct timespec vs. RTIME like it is now. That
>>>>> would not be beautiful, but feasible (e.g. picking __u64 as type,
>>>>> passing nanoseconds). Still, it does not yet convince me.
>>>>>
>>>> The idea behind xnthread_inquire() is not about replacing rt_task_inquire()
>>>> under the hood, but rather to get back only fundamental values, such as the
>>>> current thread status, in order to determine whether XNRELAX is set or not for
>>>> instance.
>>>>
>>>> XENOMAI_SYSCALL1(__xn_sys_inquire) => xnthread_inquire(xnpod_current_thread())
>>> OK, slowly getting to your core. Assume we had __xn_sys_inquire, what
>>> should it return, and what should we mask from rt_task_inquire, and do
>>> we still want pthread_inquire_np.
>>>
>> __xn_sys_inquire would return both the informational and status bitmasks, for
>> debug and sanity check purposes.
>>
>>> But I really dislike this approach of artificially
>>> complicating/crippling interfaces just to hide XNRELAX (I can't imagine
>>> you have problems with the other information rt_task_inquire returns).
>>>
>> It is not about crippling that interface at all, it is about not adding the
>> XNRELAX bit - since it is NOT there ATM - to prevent further misuse of the
> 
> Look at the code, it _is_ practically there, it _is_ usable, but it is
> inverted (T_PRIMARY==XNRELAX). That's why I insist on the fact that the
> current rt_task_inquire implementation is broken when taking its
> documentation as a reference.
> 
>> thread mode, which is a very real problem in lots of application code now.
>>
>> Look, even Hannes who very well knows what co-kernel stuff is all about
>> misinterpreted the Xenomai API in that area:
>> http://www.captain.at/review-rtai-versus-xenomai.php
> 
> Not directly his fault, he just copied our old, incorrect serial test
> code (/me failed to review it before it saw the light of the internet).
> But I told him ages ago to update his page.
>

The point still stands, whoever made the mistake, that mistake was due to a
misunderstanding of the API. This is the gist of the matter: let's remove the
source of such issue.

>> The reason is likely because people tend to think that if T_PRIMARY is
>> explicitly provided, that means that the real-time core does not handle the
>> issue implicitly, and then conclude that they ought to use rt_task_set_mode() to
>> do the job themselves. That's wrong, that's badly wrong.
> 
> Again, don't shoot the messenger, let's discuss how to overcome the
> explicit mode switch interface! You are looking at the wrong spot, IMHO.
> 

I'm not shooting the messenger, I'm just telling the messenger that his message
is currently wrong.

Please look at the code again:

- __rt_task_inquire() does not remap XNRELAX to some still undefined T_PRIMARY
bit because I never wanted to do so.

- The comments attached to the status bits in question do make pretty clear that
I did not want those statuses to exist actually, there were just meant to be
placeholders for rt_task_set_mode() only. T_JOINABLE is arguably a good
candidate for conversion to an actual status , that is not disputed.

#define T_PRIMARY  0x00000200	/* Recycle internal bits status which */
#define T_JOINABLE 0x00000400	/* won't be passed to the nucleus.  */

- __rt_task_set_mode() is currently trying to set T_PRIMARY within the old mode
mask to be returned to the user to be consistent with the input parameters that
may set it, but that fails, and beyond that, it's plain silly (I wrote this
code, so I can tell you it is, right?). It is silly because __rt_task_set_mode()
has to be tagged as __xn_exec_primary; so, what is the point?

What we need is:

- to prevent the thread mode from being explicitly manipulated from the public
interface. So we need to get rid of T_PRIMARY in rt_task_set_mode(). At the same
time, we still need to provide a way for low-level code to control that mode,
That service already exists, i.e. XENOMAI_SYSCALL(__xn_sys_migrate).

- a way to get the current thread status for debugging/sanity checks/logging
etc; but since we want to move the primary mode bit away from the public
interface of rt_task_set_mode(), and that is XENOMAI_SYSCALL(__xn_sys_inquire).
There would be no point in returning T_PRIMARY back explicitly via
rt_task_inquire(). It would even be counter-productive. I don't care if we don't
mask out XNRELAX and other unwanted bits from RT_TASK_INFO::status; the point is
that we should not mention those internal bits in the rt_task_inquire()
documentation.

> BTW, RTnet no longer requires explicit mode switch. And for Xenomai 2.5,
> I will remove rtdm_device.open_rt/socket_rt as well as
> rtdm_operations.close_rt, ie. a whole bunch of handlers that motivated
> most of the explicit switches from my POV. So I would be fine with
> deprecating that switch service for 2.5 and maybe remove it with 2.6 (or
> whatever comes next).
> 
>> Aside of this, using rt_task_inquire() in instrumentation/debug/sanity checking
>> code would be overkill most of the time. If the purpose is to implement
>> constructs like:
>>
>> 	if (!task_in_primary_mode()) {
>> 		blah;
>> 	}
>>
>> Then, you likely don't want the overhead of copying useless things like the task
>> name, fetching the outstanding timeout values, or any other values we may want
>> to add in the future to RT_TASK_INFO (e.g. we did not use to send back any kind
>> of statistical information via that structure in 2.3.x, but we started to do so
>> with 2.4.x).
> 
> That overhead is negligible compared to the anyway required syscall. If
> we needed to optimize debugging stuff (I doubt so), we would have to
> push that data (also the thread prio etc.) continuously to user space.
>

It is not about debugging stuff only, you said you wanted to add sanity checks
as well, like I just sketched above and will redo below:

 	if (!task_in_primary_mode())
 		return -CANTDOTHAT;


In that case, you certainly don't want to pay for such overhead. Therefore, what
you need is XENOMAI_SYSCALL(__xn_sys_inquire).

> Jan
> 


-- 
Philippe.

Re: [Xenomai-core] [PATCH 0/2] Fix and improve task/thread inquire services

[Jan Kiszka]

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

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> Jan Kiszka wrote:
>>>> Philippe Gerum wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Philippe Gerum wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>> This series fixes the issues around rt_task_inquire I posted yesterday.
>>>>>>>>>>> Additionally, it introduces an analogous services pthread_inquire_np for
>>>>>>>>>>> the POSIX skin. That allows, among other things, to implement test cases
>>>>>>>>>>> for the upcoming fast xnsynch/mutex patches.
>>>>>>>>>> Ok. Since this applies only for debugging purpose, and displaying
>>>>>>>>>> whether a task is in primary mode may be use badly by users, maybe we
>>>>>>>>>> should make this service a shadow syscall, and not export any interface
>>>>>>>>>> to use it. This would further avoid duplication between the native and
>>>>>>>>>> posix skins.
>>>>>>>>> Debugging is not the holy, exclusive business of Xenomai hackers.
>>>>>>>>>
>>>>>>>>> The inquire services are useful in libraries as well, when you want to
>>>>>>>>> check if the caller complies to the call convention ("don't use in
>>>>>>>>> primary mode", "caller's priority must not exceed X" or whatever).
>>>>>>>>>
>>>>>>>>> That said, I'm open for unifying the code, maybe introducing some
>>>>>>>>> xnthread_inquire.
>>>>>>>>>
>>>>>>>> That would be much better than publishing an open interface to fiddle even more
>>>>>>>> with thread modes via rt_task_set_mode(). I would definitely merge that.
>>>>>>> Code refactoring is no problem, will work that out. I just want to keep
>>>>>>> the user interface.
>>>>>> Looking into this, I come to the conclusion that xnthread_inquire is
>>>>>> only then a gain if both rt_task_inquire and pthread_inquire_np use the
>>>>>> same data structure layout. And that means that both need to use the
>>>>>> same time encodings, not struct timespec vs. RTIME like it is now. That
>>>>>> would not be beautiful, but feasible (e.g. picking __u64 as type,
>>>>>> passing nanoseconds). Still, it does not yet convince me.
>>>>>>
>>>>> The idea behind xnthread_inquire() is not about replacing rt_task_inquire()
>>>>> under the hood, but rather to get back only fundamental values, such as the
>>>>> current thread status, in order to determine whether XNRELAX is set or not for
>>>>> instance.
>>>>>
>>>>> XENOMAI_SYSCALL1(__xn_sys_inquire) => xnthread_inquire(xnpod_current_thread())
>>>> OK, slowly getting to your core. Assume we had __xn_sys_inquire, what
>>>> should it return, and what should we mask from rt_task_inquire, and do
>>>> we still want pthread_inquire_np.
>>>>
>>> __xn_sys_inquire would return both the informational and status bitmasks, for
>>> debug and sanity check purposes.
>>>
>>>> But I really dislike this approach of artificially
>>>> complicating/crippling interfaces just to hide XNRELAX (I can't imagine
>>>> you have problems with the other information rt_task_inquire returns).
>>>>
>>> It is not about crippling that interface at all, it is about not adding the
>>> XNRELAX bit - since it is NOT there ATM - to prevent further misuse of the
>> Look at the code, it _is_ practically there, it _is_ usable, but it is
>> inverted (T_PRIMARY==XNRELAX). That's why I insist on the fact that the
>> current rt_task_inquire implementation is broken when taking its
>> documentation as a reference.
>>
>>> thread mode, which is a very real problem in lots of application code now.
>>>
>>> Look, even Hannes who very well knows what co-kernel stuff is all about
>>> misinterpreted the Xenomai API in that area:
>>> http://www.captain.at/review-rtai-versus-xenomai.php
>> Not directly his fault, he just copied our old, incorrect serial test
>> code (/me failed to review it before it saw the light of the internet).
>> But I told him ages ago to update his page.
>>
> 
> The point still stands, whoever made the mistake, that mistake was due to a
> misunderstanding of the API. This is the gist of the matter: let's remove the
> source of such issue.
> 
>>> The reason is likely because people tend to think that if T_PRIMARY is
>>> explicitly provided, that means that the real-time core does not handle the
>>> issue implicitly, and then conclude that they ought to use rt_task_set_mode() to
>>> do the job themselves. That's wrong, that's badly wrong.
>> Again, don't shoot the messenger, let's discuss how to overcome the
>> explicit mode switch interface! You are looking at the wrong spot, IMHO.
>>
> 
> I'm not shooting the messenger, I'm just telling the messenger that his message
> is currently wrong.
> 
> Please look at the code again:
> 
> - __rt_task_inquire() does not remap XNRELAX to some still undefined T_PRIMARY
> bit because I never wanted to do so.
> 
> - The comments attached to the status bits in question do make pretty clear that
> I did not want those statuses to exist actually, there were just meant to be
> placeholders for rt_task_set_mode() only. T_JOINABLE is arguably a good
> candidate for conversion to an actual status , that is not disputed.
> 
> #define T_PRIMARY  0x00000200	/* Recycle internal bits status which */
> #define T_JOINABLE 0x00000400	/* won't be passed to the nucleus.  */

The "won't be passed _to_ the nucleus", there is no comment about what
is returned _from_ it.

> 
> - __rt_task_set_mode() is currently trying to set T_PRIMARY within the old mode
> mask to be returned to the user to be consistent with the input parameters that
> may set it, but that fails, and beyond that, it's plain silly (I wrote this
> code, so I can tell you it is, right?). It is silly because __rt_task_set_mode()
> has to be tagged as __xn_exec_primary; so, what is the point?
> 
> What we need is:
> 
> - to prevent the thread mode from being explicitly manipulated from the public
> interface. So we need to get rid of T_PRIMARY in rt_task_set_mode(). At the same
> time, we still need to provide a way for low-level code to control that mode,
> That service already exists, i.e. XENOMAI_SYSCALL(__xn_sys_migrate).
> 
> - a way to get the current thread status for debugging/sanity checks/logging
> etc; but since we want to move the primary mode bit away from the public
> interface of rt_task_set_mode(), and that is XENOMAI_SYSCALL(__xn_sys_inquire).
> There would be no point in returning T_PRIMARY back explicitly via
> rt_task_inquire(). It would even be counter-productive. I don't care if we don't
> mask out XNRELAX and other unwanted bits from RT_TASK_INFO::status; the point is
> that we should not mention those internal bits in the rt_task_inquire()
> documentation.

When there is no official way to "manipulate" the mode, there is ZERO
point in hiding it, urging Native or Posix user to talk to
Xenomai-internal APIs. We actually should want users to be _very_ well
aware of the different modes, otherwise they continue to write too much
broken code (/wrt RT).

> 
>> BTW, RTnet no longer requires explicit mode switch. And for Xenomai 2.5,
>> I will remove rtdm_device.open_rt/socket_rt as well as
>> rtdm_operations.close_rt, ie. a whole bunch of handlers that motivated
>> most of the explicit switches from my POV. So I would be fine with
>> deprecating that switch service for 2.5 and maybe remove it with 2.6 (or
>> whatever comes next).
>>
>>> Aside of this, using rt_task_inquire() in instrumentation/debug/sanity checking
>>> code would be overkill most of the time. If the purpose is to implement
>>> constructs like:
>>>
>>> 	if (!task_in_primary_mode()) {
>>> 		blah;
>>> 	}
>>>
>>> Then, you likely don't want the overhead of copying useless things like the task
>>> name, fetching the outstanding timeout values, or any other values we may want
>>> to add in the future to RT_TASK_INFO (e.g. we did not use to send back any kind
>>> of statistical information via that structure in 2.3.x, but we started to do so
>>> with 2.4.x).
>> That overhead is negligible compared to the anyway required syscall. If
>> we needed to optimize debugging stuff (I doubt so), we would have to
>> push that data (also the thread prio etc.) continuously to user space.
>>
> 
> It is not about debugging stuff only, you said you wanted to add sanity checks
> as well, like I just sketched above and will redo below:
> 
>  	if (!task_in_primary_mode())
>  		return -CANTDOTHAT;
> 
> 
> In that case, you certainly don't want to pay for such overhead. Therefore, what
> you need is XENOMAI_SYSCALL(__xn_sys_inquire).

If you really insist on reducing the information which skin inquire
services are allowed to return and leave me with a special approach for
task_in_primary_mode(), then I will rather come up with a syscall-less
variant.

Jan


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

Re: [Xenomai-core] [PATCH 0/2] Fix and improve task/thread inquire services

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Jan Kiszka wrote:
>>> Philippe Gerum wrote:
>>>> Jan Kiszka wrote:
>>>>> Philippe Gerum wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Philippe Gerum wrote:
>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>>>> This series fixes the issues around rt_task_inquire I posted yesterday.
>>>>>>>>>>>> Additionally, it introduces an analogous services pthread_inquire_np for
>>>>>>>>>>>> the POSIX skin. That allows, among other things, to implement test cases
>>>>>>>>>>>> for the upcoming fast xnsynch/mutex patches.
>>>>>>>>>>> Ok. Since this applies only for debugging purpose, and displaying
>>>>>>>>>>> whether a task is in primary mode may be use badly by users, maybe we
>>>>>>>>>>> should make this service a shadow syscall, and not export any interface
>>>>>>>>>>> to use it. This would further avoid duplication between the native and
>>>>>>>>>>> posix skins.
>>>>>>>>>> Debugging is not the holy, exclusive business of Xenomai hackers.
>>>>>>>>>>
>>>>>>>>>> The inquire services are useful in libraries as well, when you want to
>>>>>>>>>> check if the caller complies to the call convention ("don't use in
>>>>>>>>>> primary mode", "caller's priority must not exceed X" or whatever).
>>>>>>>>>>
>>>>>>>>>> That said, I'm open for unifying the code, maybe introducing some
>>>>>>>>>> xnthread_inquire.
>>>>>>>>>>
>>>>>>>>> That would be much better than publishing an open interface to fiddle even more
>>>>>>>>> with thread modes via rt_task_set_mode(). I would definitely merge that.
>>>>>>>> Code refactoring is no problem, will work that out. I just want to keep
>>>>>>>> the user interface.
>>>>>>> Looking into this, I come to the conclusion that xnthread_inquire is
>>>>>>> only then a gain if both rt_task_inquire and pthread_inquire_np use the
>>>>>>> same data structure layout. And that means that both need to use the
>>>>>>> same time encodings, not struct timespec vs. RTIME like it is now. That
>>>>>>> would not be beautiful, but feasible (e.g. picking __u64 as type,
>>>>>>> passing nanoseconds). Still, it does not yet convince me.
>>>>>>>
>>>>>> The idea behind xnthread_inquire() is not about replacing rt_task_inquire()
>>>>>> under the hood, but rather to get back only fundamental values, such as the
>>>>>> current thread status, in order to determine whether XNRELAX is set or not for
>>>>>> instance.
>>>>>>
>>>>>> XENOMAI_SYSCALL1(__xn_sys_inquire) => xnthread_inquire(xnpod_current_thread())
>>>>> OK, slowly getting to your core. Assume we had __xn_sys_inquire, what
>>>>> should it return, and what should we mask from rt_task_inquire, and do
>>>>> we still want pthread_inquire_np.
>>>>>
>>>> __xn_sys_inquire would return both the informational and status bitmasks, for
>>>> debug and sanity check purposes.
>>>>
>>>>> But I really dislike this approach of artificially
>>>>> complicating/crippling interfaces just to hide XNRELAX (I can't imagine
>>>>> you have problems with the other information rt_task_inquire returns).
>>>>>
>>>> It is not about crippling that interface at all, it is about not adding the
>>>> XNRELAX bit - since it is NOT there ATM - to prevent further misuse of the
>>> Look at the code, it _is_ practically there, it _is_ usable, but it is
>>> inverted (T_PRIMARY==XNRELAX). That's why I insist on the fact that the
>>> current rt_task_inquire implementation is broken when taking its
>>> documentation as a reference.
>>>
>>>> thread mode, which is a very real problem in lots of application code now.
>>>>
>>>> Look, even Hannes who very well knows what co-kernel stuff is all about
>>>> misinterpreted the Xenomai API in that area:
>>>> http://www.captain.at/review-rtai-versus-xenomai.php
>>> Not directly his fault, he just copied our old, incorrect serial test
>>> code (/me failed to review it before it saw the light of the internet).
>>> But I told him ages ago to update his page.
>>>
>> The point still stands, whoever made the mistake, that mistake was due to a
>> misunderstanding of the API. This is the gist of the matter: let's remove the
>> source of such issue.
>>
>>>> The reason is likely because people tend to think that if T_PRIMARY is
>>>> explicitly provided, that means that the real-time core does not handle the
>>>> issue implicitly, and then conclude that they ought to use rt_task_set_mode() to
>>>> do the job themselves. That's wrong, that's badly wrong.
>>> Again, don't shoot the messenger, let's discuss how to overcome the
>>> explicit mode switch interface! You are looking at the wrong spot, IMHO.
>>>
>> I'm not shooting the messenger, I'm just telling the messenger that his message
>> is currently wrong.
>>
>> Please look at the code again:
>>
>> - __rt_task_inquire() does not remap XNRELAX to some still undefined T_PRIMARY
>> bit because I never wanted to do so.
>>
>> - The comments attached to the status bits in question do make pretty clear that
>> I did not want those statuses to exist actually, there were just meant to be
>> placeholders for rt_task_set_mode() only. T_JOINABLE is arguably a good
>> candidate for conversion to an actual status , that is not disputed.
>>
>> #define T_PRIMARY  0x00000200	/* Recycle internal bits status which */
>> #define T_JOINABLE 0x00000400	/* won't be passed to the nucleus.  */
> 
> The "won't be passed _to_ the nucleus", there is no comment about what
> is returned _from_ it.
>

You cannot return a value which has no existence. This is the purpose of my
comment in that code: that value has _no_ existence, it is just a placeholder,
and I still don't want that value to have any existence. Should I really
reformulate this differently again?

>> - __rt_task_set_mode() is currently trying to set T_PRIMARY within the old mode
>> mask to be returned to the user to be consistent with the input parameters that
>> may set it, but that fails, and beyond that, it's plain silly (I wrote this
>> code, so I can tell you it is, right?). It is silly because __rt_task_set_mode()
>> has to be tagged as __xn_exec_primary; so, what is the point?
>>
>> What we need is:
>>
>> - to prevent the thread mode from being explicitly manipulated from the public
>> interface. So we need to get rid of T_PRIMARY in rt_task_set_mode(). At the same
>> time, we still need to provide a way for low-level code to control that mode,
>> That service already exists, i.e. XENOMAI_SYSCALL(__xn_sys_migrate).
>>
>> - a way to get the current thread status for debugging/sanity checks/logging
>> etc; but since we want to move the primary mode bit away from the public
>> interface of rt_task_set_mode(), and that is XENOMAI_SYSCALL(__xn_sys_inquire).
>> There would be no point in returning T_PRIMARY back explicitly via
>> rt_task_inquire(). It would even be counter-productive. I don't care if we don't
>> mask out XNRELAX and other unwanted bits from RT_TASK_INFO::status; the point is
>> that we should not mention those internal bits in the rt_task_inquire()
>> documentation.
> 
> When there is no official way to "manipulate" the mode, there is ZERO
> point in hiding it, urging Native or Posix user to talk to
> Xenomai-internal APIs. We actually should want users to be _very_ well
> aware of the different modes, otherwise they continue to write too much
> broken code (/wrt RT).

Back again to your argument that we should not care and let people make the
correct assumption about the purpose of that particular API. The whole purpose
of this thread is to tell you that we should care instead, because facts show
that this API is routinely misused. Having the documentation say "T_PRIMARY may
be set or returned, but you should not use it or base any action on its presence
most of the time" would be preposterous.

> 
>>> BTW, RTnet no longer requires explicit mode switch. And for Xenomai 2.5,
>>> I will remove rtdm_device.open_rt/socket_rt as well as
>>> rtdm_operations.close_rt, ie. a whole bunch of handlers that motivated
>>> most of the explicit switches from my POV. So I would be fine with
>>> deprecating that switch service for 2.5 and maybe remove it with 2.6 (or
>>> whatever comes next).
>>>
>>>> Aside of this, using rt_task_inquire() in instrumentation/debug/sanity checking
>>>> code would be overkill most of the time. If the purpose is to implement
>>>> constructs like:
>>>>
>>>> 	if (!task_in_primary_mode()) {
>>>> 		blah;
>>>> 	}
>>>>
>>>> Then, you likely don't want the overhead of copying useless things like the task
>>>> name, fetching the outstanding timeout values, or any other values we may want
>>>> to add in the future to RT_TASK_INFO (e.g. we did not use to send back any kind
>>>> of statistical information via that structure in 2.3.x, but we started to do so
>>>> with 2.4.x).
>>> That overhead is negligible compared to the anyway required syscall. If
>>> we needed to optimize debugging stuff (I doubt so), we would have to
>>> push that data (also the thread prio etc.) continuously to user space.
>>>
>> It is not about debugging stuff only, you said you wanted to add sanity checks
>> as well, like I just sketched above and will redo below:
>>
>>  	if (!task_in_primary_mode())
>>  		return -CANTDOTHAT;
>>
>>
>> In that case, you certainly don't want to pay for such overhead. Therefore, what
>> you need is XENOMAI_SYSCALL(__xn_sys_inquire).
> 
> If you really insist on reducing the information which skin inquire
> services are allowed to return and leave me with a special approach for
> task_in_primary_mode(), then I will rather come up with a syscall-less
> variant.
> 

Export the thread status to userland as a side-effect of the fast mutex
implementationn as much as you want, this is a no-brainer for me. But I do
insist on not making some of the information available in the status field
public, indeed.

> Jan
> 


-- 
Philippe.