[Xenomai] [PATCH v2] nucleus: Convert intrlock to a sleeping Linux lock

[Xenomai] [PATCH v2] nucleus: Convert intrlock to a sleeping Linux lock

[Jan Kiszka]

Conceptually, all users of this lock are supposed to run over Linux
context already. Moreover, latest I-pipe patches complain that
ipipe_virtualize_irq is called with the Xenomai domain stalled which
invalidates that useful internal bug check.

So rework the locking for dumping scheduling statistics and convert
intrlock to a sleeping Linux variant to avoid the alarm. And to simplify
some code paths.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

This revealed the broken usage of XNARCH_TIMER_IRQ by xnintr_query_next.
A similar issue was visible in format_irq_proc even before this change.
Requires different patches, see corresponding posting.

 include/nucleus/intr.h |    4 ++
 ksrc/nucleus/intr.c    |   81 ++++++++++++-----------------------------------
 ksrc/nucleus/sched.c   |    6 +++
 3 files changed, 31 insertions(+), 60 deletions(-)

diff --git a/include/nucleus/intr.h b/include/nucleus/intr.h
index 20a625d..4105122 100644
--- a/include/nucleus/intr.h
+++ b/include/nucleus/intr.h
@@ -131,6 +131,10 @@ void xnintr_affinity(xnintr_t *intr,
 
 int xnintr_query_init(xnintr_iterator_t *iterator);
 
+void xnintr_put_query_lock(struct xnvfile *vfile);
+
+int xnintr_get_query_lock(struct xnvfile *vfile);
+
 int xnintr_query_next(int irq, xnintr_iterator_t *iterator,
 		      char *name_buf);
 
diff --git a/ksrc/nucleus/intr.c b/ksrc/nucleus/intr.c
index c75fcac..36f20fe 100644
--- a/ksrc/nucleus/intr.c
+++ b/ksrc/nucleus/intr.c
@@ -39,30 +39,11 @@
 
 #define XNINTR_MAX_UNHANDLED	1000
 
-DEFINE_PRIVATE_XNLOCK(intrlock);
+static DEFINE_BINARY_SEMAPHORE(intrlock);
 
 #ifdef CONFIG_XENO_OPT_STATS
 xnintr_t nkclock;	     /* Only for statistics */
 static int xnintr_count = 1; /* Number of attached xnintr objects + nkclock */
-static int xnintr_list_rev;  /* Modification counter of xnintr list */
-
-/* Both functions update xnintr_list_rev at the very end.
- * This guarantees that module.c::stat_seq_open() won't get
- * an up-to-date xnintr_list_rev and old xnintr_count. */
-
-static inline void xnintr_stat_counter_inc(void)
-{
-	xnintr_count++;
-	xnarch_memory_barrier();
-	xnintr_list_rev++;
-}
-
-static inline void xnintr_stat_counter_dec(void)
-{
-	xnintr_count--;
-	xnarch_memory_barrier();
-	xnintr_list_rev++;
-}
 
 static inline void xnintr_sync_stat_references(xnintr_t *intr)
 {
@@ -76,8 +57,6 @@ static inline void xnintr_sync_stat_references(xnintr_t *intr)
 	}
 }
 #else
-static inline void xnintr_stat_counter_inc(void) {}
-static inline void xnintr_stat_counter_dec(void) {}
 static inline void xnintr_sync_stat_references(xnintr_t *intr) {}
 #endif /* CONFIG_XENO_OPT_STATS */
 
@@ -709,7 +688,6 @@ EXPORT_SYMBOL_GPL(xnintr_destroy);
 int xnintr_attach(xnintr_t *intr, void *cookie)
 {
 	int ret;
-	spl_t s;
 
 	trace_mark(xn_nucleus, irq_attach, "irq %u name %s",
 		   intr->irq, intr->name);
@@ -721,7 +699,7 @@ int xnintr_attach(xnintr_t *intr, void *cookie)
 	xnarch_set_irq_affinity(intr->irq, nkaffinity);
 #endif /* CONFIG_SMP */
 
-	xnlock_get_irqsave(&intrlock, s);
+	down(&intrlock);
 
 	if (__testbits(intr->flags, XN_ISR_ATTACHED)) {
 		ret = -EBUSY;
@@ -733,9 +711,9 @@ int xnintr_attach(xnintr_t *intr, void *cookie)
 		goto out;
 
 	__setbits(intr->flags, XN_ISR_ATTACHED);
-	xnintr_stat_counter_inc();
+	xnintr_count++;
 out:
-	xnlock_put_irqrestore(&intrlock, s);
+	up(&intrlock);
 
 	return ret;
 }
@@ -775,11 +753,10 @@ EXPORT_SYMBOL_GPL(xnintr_attach);
 int xnintr_detach(xnintr_t *intr)
 {
 	int ret;
-	spl_t s;
 
 	trace_mark(xn_nucleus, irq_detach, "irq %u", intr->irq);
 
-	xnlock_get_irqsave(&intrlock, s);
+	down(&intrlock);
 
 	if (!__testbits(intr->flags, XN_ISR_ATTACHED)) {
 		ret = -EINVAL;
@@ -792,9 +769,9 @@ int xnintr_detach(xnintr_t *intr)
 	if (ret)
 		goto out;
 
-	xnintr_stat_counter_dec();
+	xnintr_count--;
  out:
-	xnlock_put_irqrestore(&intrlock, s);
+	up(&intrlock);
 
 	return ret;
 }
@@ -894,23 +871,21 @@ void xnintr_affinity(xnintr_t *intr, xnarch_cpumask_t cpumask)
 EXPORT_SYMBOL_GPL(xnintr_affinity);
 
 #ifdef CONFIG_XENO_OPT_STATS
+int xnintr_get_query_lock(struct xnvfile *vfile)
+{
+	return down_interruptible(&intrlock) ? -ERESTARTSYS : 0;
+}
+
+void xnintr_put_query_lock(struct xnvfile *vfile)
+{
+	up(&intrlock);
+}
+
 int xnintr_query_init(xnintr_iterator_t *iterator)
 {
 	iterator->cpu = -1;
 	iterator->prev = NULL;
 
-	/* The order is important here: first xnintr_list_rev then
-	 * xnintr_count.  On the other hand, xnintr_attach/detach()
-	 * update xnintr_count first and then xnintr_list_rev.  This
-	 * should guarantee that we can't get an up-to-date
-	 * xnintr_list_rev and old xnintr_count here. The other way
-	 * around is not a problem as xnintr_query() will notice this
-	 * fact later.  Should xnintr_list_rev change later,
-	 * xnintr_query() will trigger an appropriate error below. */
-
-	iterator->list_rev = xnintr_list_rev;
-	xnarch_memory_barrier();
-
 	return xnintr_count;
 }
 
@@ -919,20 +894,11 @@ int xnintr_query_next(int irq, xnintr_iterator_t *iterator, char *name_buf)
 	xnintr_t *intr;
 	xnticks_t last_switch;
 	int cpu_no = iterator->cpu + 1;
-	int err = 0;
-	spl_t s;
 
 	if (cpu_no == xnarch_num_online_cpus())
 		cpu_no = 0;
 	iterator->cpu = cpu_no;
 
-	xnlock_get_irqsave(&intrlock, s);
-
-	if (iterator->list_rev != xnintr_list_rev) {
-		err = -EAGAIN;
-		goto unlock_and_exit;
-	}
-
 	if (!iterator->prev) {
 		if (irq == XNARCH_TIMER_IRQ)
 			intr = &nkclock;
@@ -944,8 +910,7 @@ int xnintr_query_next(int irq, xnintr_iterator_t *iterator, char *name_buf)
 	if (!intr) {
 		cpu_no = -1;
 		iterator->prev = NULL;
-		err = -ENODEV;
-		goto unlock_and_exit;
+		return -ENODEV;
 	}
 
 	snprintf(name_buf, XNOBJECT_NAME_LEN, "IRQ%d: %s", irq, intr->name);
@@ -968,10 +933,7 @@ int xnintr_query_next(int irq, xnintr_iterator_t *iterator, char *name_buf)
 	if (cpu_no + 1 == xnarch_num_online_cpus())
 		iterator->prev = intr;
 
-     unlock_and_exit:
-	xnlock_put_irqrestore(&intrlock, s);
-
-	return err;
+	return 0;
 }
 #endif /* CONFIG_XENO_OPT_STATS */
 
@@ -983,7 +945,6 @@ static inline int format_irq_proc(unsigned int irq,
 				  struct xnvfile_regular_iterator *it)
 {
 	struct xnintr *intr;
-	spl_t s;
 
 	if (irq == XNARCH_TIMER_IRQ) {
 		xnvfile_puts(it, "         [timer]");
@@ -1005,7 +966,7 @@ static inline int format_irq_proc(unsigned int irq,
 		return 0;
 	}
 
-	xnlock_get_irqsave(&intrlock, s);
+	down(&intrlock);
 
 	intr = xnintr_shirq_first(irq);
 	if (intr) {
@@ -1018,7 +979,7 @@ static inline int format_irq_proc(unsigned int irq,
 		} while (intr);
 	}
 
-	xnlock_put_irqrestore(&intrlock, s);
+	up(&intrlock);
 
 	return 0;
 }
diff --git a/ksrc/nucleus/sched.c b/ksrc/nucleus/sched.c
index 61957e6..16a59f8 100644
--- a/ksrc/nucleus/sched.c
+++ b/ksrc/nucleus/sched.c
@@ -839,11 +839,17 @@ struct vfile_stat_data {
 
 static struct xnvfile_snapshot_ops vfile_stat_ops;
 
+static struct xnvfile_lock_ops vfile_stat_lockops = {
+	.get = xnintr_get_query_lock,
+	.put = xnintr_put_query_lock,
+};
+
 static struct xnvfile_snapshot stat_vfile = {
 	.privsz = sizeof(struct vfile_stat_priv),
 	.datasz = sizeof(struct vfile_stat_data),
 	.tag = &nkpod_struct.threadlist_tag,
 	.ops = &vfile_stat_ops,
+	.entry = { .lockops = &vfile_stat_lockops },
 };
 
 static int vfile_stat_rewind(struct xnvfile_snapshot_iterator *it)
-- 
1.7.3.4

Re: [Xenomai] [PATCH v2] nucleus: Convert intrlock to a sleeping Linux lock

[Jeff Webb]

On 01/02/2013 12:07 PM, Jan Kiszka wrote:
> Conceptually, all users of this lock are supposed to run over Linux
> context already. Moreover, latest I-pipe patches complain that
> ipipe_virtualize_irq is called with the Xenomai domain stalled which
> invalidates that useful internal bug check.
>
> So rework the locking for dumping scheduling statistics and convert
> intrlock to a sleeping Linux variant to avoid the alarm. And to simplify
> some code paths.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---

This patch seems to solve the original problem, but I get the following output in the kernel log, when I do a 'cat /proc/xenomai/stat'.

-Jeff

kernel: [  106.030870] ------------[ cut here ]------------
kernel: [  106.030878] WARNING: at kernel/xenomai/nucleus/intr.c:903 xnintr_query_next+0x189/0x1a0()
kernel: [  106.030880] Hardware name: Precision WorkStation T3400
kernel: [  106.030882] Modules linked in: bnep rfcomm bluetooth binfmt_misc dm_crypt snd_hda_codec_analog snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq usbhid snd_timer snd_seq_device psmouse mac_hid snd hid coretemp dcdbas x38_edac ppdev soundcore snd_page_alloc serio_raw edac_core parport_pc microcode lp parport reiserfs nouveau tg3 ttm drm_kms_helper drm i2c_algo_bit mxm_wmi video wmi
kernel: [  106.030932] Pid: 3106, comm: cat Not tainted 3.4.6-xenomai-2.6.2 #1
kernel: [  106.030934] Call Trace:
kernel: [  106.030941]  [<ffffffff8103cc0f>] warn_slowpath_common+0x7f/0xc0
kernel: [  106.030944]  [<ffffffff8103cc6a>] warn_slowpath_null+0x1a/0x20
kernel: [  106.030947]  [<ffffffff811060b9>] xnintr_query_next+0x189/0x1a0
kernel: [  106.030951]  [<ffffffff81119003>] vfile_stat_next+0x163/0x200
kernel: [  106.030955]  [<ffffffff8113063b>] vfile_snapshot_open+0x21b/0x2e0
kernel: [  106.030960]  [<ffffffff81263cc3>] proc_reg_open+0xa3/0x180
kernel: [  106.030963]  [<ffffffff81130160>] ? vfile_regular_release+0x50/0x50
kernel: [  106.030967]  [<ffffffff81263c20>] ? proc_alloc_inode+0xb0/0xb0
kernel: [  106.030972]  [<ffffffff81201ace>] __dentry_open+0x24e/0x310
kernel: [  106.030975]  [<ffffffff81202f51>] nameidata_to_filp+0x71/0x80
kernel: [  106.030979]  [<ffffffff81212afc>] do_last+0x26c/0x920
kernel: [  106.030983]  [<ffffffff812132c3>] path_openat+0xd3/0x3c0
kernel: [  106.030986]  [<ffffffff812136d2>] do_filp_open+0x42/0xa0
kernel: [  106.030990]  [<ffffffff8121fb8f>] ? alloc_fd+0x4f/0x130
kernel: [  106.030993]  [<ffffffff81203058>] do_sys_open+0xf8/0x1d0
kernel: [  106.030997]  [<ffffffff813a5c4a>] ? __ipipe_syscall_root_thunk+0x35/0x67
kernel: [  106.031001]  [<ffffffff81203151>] sys_open+0x21/0x30
kernel: [  106.031005]  [<ffffffff816c61e6>] system_call_fastpath+0x16/0x1b
kernel: [  106.031007] ---[ end trace 5ed7b1604d797a3f ]---

Re: [Xenomai] [PATCH v2] nucleus: Convert intrlock to a sleeping Linux lock

[Jan Kiszka]

On 2013-01-02 21:25, Jeff Webb wrote:
> On 01/02/2013 12:07 PM, Jan Kiszka wrote:
>> Conceptually, all users of this lock are supposed to run over Linux
>> context already. Moreover, latest I-pipe patches complain that
>> ipipe_virtualize_irq is called with the Xenomai domain stalled which
>> invalidates that useful internal bug check.
>>
>> So rework the locking for dumping scheduling statistics and convert
>> intrlock to a sleeping Linux variant to avoid the alarm. And to simplify
>> some code paths.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
> 
> This patch seems to solve the original problem, but I get the following output in the kernel log, when I do a 'cat /proc/xenomai/stat'.
> 
> -Jeff
> 
> kernel: [  106.030870] ------------[ cut here ]------------
> kernel: [  106.030878] WARNING: at kernel/xenomai/nucleus/intr.c:903 xnintr_query_next+0x189/0x1a0()
> kernel: [  106.030880] Hardware name: Precision WorkStation T3400
> kernel: [  106.030882] Modules linked in: bnep rfcomm bluetooth binfmt_misc dm_crypt snd_hda_codec_analog snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq usbhid snd_timer snd_seq_device psmouse mac_hid snd hid coretemp dcdbas x38_edac ppdev soundcore snd_page_alloc serio_raw edac_core parport_pc microcode lp parport reiserfs nouveau tg3 ttm drm_kms_helper drm i2c_algo_bit mxm_wmi video wmi
> kernel: [  106.030932] Pid: 3106, comm: cat Not tainted 3.4.6-xenomai-2.6.2 #1
> kernel: [  106.030934] Call Trace:
> kernel: [  106.030941]  [<ffffffff8103cc0f>] warn_slowpath_common+0x7f/0xc0
> kernel: [  106.030944]  [<ffffffff8103cc6a>] warn_slowpath_null+0x1a/0x20
> kernel: [  106.030947]  [<ffffffff811060b9>] xnintr_query_next+0x189/0x1a0
> kernel: [  106.030951]  [<ffffffff81119003>] vfile_stat_next+0x163/0x200
> kernel: [  106.030955]  [<ffffffff8113063b>] vfile_snapshot_open+0x21b/0x2e0
> kernel: [  106.030960]  [<ffffffff81263cc3>] proc_reg_open+0xa3/0x180
> kernel: [  106.030963]  [<ffffffff81130160>] ? vfile_regular_release+0x50/0x50
> kernel: [  106.030967]  [<ffffffff81263c20>] ? proc_alloc_inode+0xb0/0xb0
> kernel: [  106.030972]  [<ffffffff81201ace>] __dentry_open+0x24e/0x310
> kernel: [  106.030975]  [<ffffffff81202f51>] nameidata_to_filp+0x71/0x80
> kernel: [  106.030979]  [<ffffffff81212afc>] do_last+0x26c/0x920
> kernel: [  106.030983]  [<ffffffff812132c3>] path_openat+0xd3/0x3c0
> kernel: [  106.030986]  [<ffffffff812136d2>] do_filp_open+0x42/0xa0
> kernel: [  106.030990]  [<ffffffff8121fb8f>] ? alloc_fd+0x4f/0x130
> kernel: [  106.030993]  [<ffffffff81203058>] do_sys_open+0xf8/0x1d0
> kernel: [  106.030997]  [<ffffffff813a5c4a>] ? __ipipe_syscall_root_thunk+0x35/0x67
> kernel: [  106.031001]  [<ffffffff81203151>] sys_open+0x21/0x30
> kernel: [  106.031005]  [<ffffffff816c61e6>] system_call_fastpath+0x16/0x1b
> kernel: [  106.031007] ---[ end trace 5ed7b1604d797a3f ]---
> 
> 

Yes, that's the XNARCH_TIMER_IRQ bug I was referring to.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

[Xenomai] [PATCH v3] nucleus: Convert intrlock to a sleeping Linux lock

[Jan Kiszka]

Conceptually, all users of this lock are supposed to run over Linux
context already. Moreover, latest I-pipe patches complain that
ipipe_virtualize_irq is called with the Xenomai domain stalled which
invalidates that useful internal bug check.

So rework the locking for dumping scheduling statistics and convert
intrlock to a sleeping Linux variant to avoid the alarm. And to simplify
some code paths.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v3:
 - Fix OPT_STATS-less build

 include/nucleus/intr.h |    4 ++
 ksrc/nucleus/intr.c    |   84 +++++++++++++----------------------------------
 ksrc/nucleus/sched.c   |    6 +++
 3 files changed, 33 insertions(+), 61 deletions(-)

diff --git a/include/nucleus/intr.h b/include/nucleus/intr.h
index 20a625d..4105122 100644
--- a/include/nucleus/intr.h
+++ b/include/nucleus/intr.h
@@ -131,6 +131,10 @@ void xnintr_affinity(xnintr_t *intr,
 
 int xnintr_query_init(xnintr_iterator_t *iterator);
 
+void xnintr_put_query_lock(struct xnvfile *vfile);
+
+int xnintr_get_query_lock(struct xnvfile *vfile);
+
 int xnintr_query_next(int irq, xnintr_iterator_t *iterator,
 		      char *name_buf);
 
diff --git a/ksrc/nucleus/intr.c b/ksrc/nucleus/intr.c
index c75fcac..e623372 100644
--- a/ksrc/nucleus/intr.c
+++ b/ksrc/nucleus/intr.c
@@ -39,30 +39,12 @@
 
 #define XNINTR_MAX_UNHANDLED	1000
 
-DEFINE_PRIVATE_XNLOCK(intrlock);
+static DEFINE_BINARY_SEMAPHORE(intrlock);
 
-#ifdef CONFIG_XENO_OPT_STATS
-xnintr_t nkclock;	     /* Only for statistics */
 static int xnintr_count = 1; /* Number of attached xnintr objects + nkclock */
-static int xnintr_list_rev;  /* Modification counter of xnintr list */
-
-/* Both functions update xnintr_list_rev at the very end.
- * This guarantees that module.c::stat_seq_open() won't get
- * an up-to-date xnintr_list_rev and old xnintr_count. */
-
-static inline void xnintr_stat_counter_inc(void)
-{
-	xnintr_count++;
-	xnarch_memory_barrier();
-	xnintr_list_rev++;
-}
 
-static inline void xnintr_stat_counter_dec(void)
-{
-	xnintr_count--;
-	xnarch_memory_barrier();
-	xnintr_list_rev++;
-}
+#ifdef CONFIG_XENO_OPT_STATS
+xnintr_t nkclock;	     /* Only for statistics */
 
 static inline void xnintr_sync_stat_references(xnintr_t *intr)
 {
@@ -76,8 +58,6 @@ static inline void xnintr_sync_stat_references(xnintr_t *intr)
 	}
 }
 #else
-static inline void xnintr_stat_counter_inc(void) {}
-static inline void xnintr_stat_counter_dec(void) {}
 static inline void xnintr_sync_stat_references(xnintr_t *intr) {}
 #endif /* CONFIG_XENO_OPT_STATS */
 
@@ -709,7 +689,6 @@ EXPORT_SYMBOL_GPL(xnintr_destroy);
 int xnintr_attach(xnintr_t *intr, void *cookie)
 {
 	int ret;
-	spl_t s;
 
 	trace_mark(xn_nucleus, irq_attach, "irq %u name %s",
 		   intr->irq, intr->name);
@@ -721,7 +700,7 @@ int xnintr_attach(xnintr_t *intr, void *cookie)
 	xnarch_set_irq_affinity(intr->irq, nkaffinity);
 #endif /* CONFIG_SMP */
 
-	xnlock_get_irqsave(&intrlock, s);
+	down(&intrlock);
 
 	if (__testbits(intr->flags, XN_ISR_ATTACHED)) {
 		ret = -EBUSY;
@@ -733,9 +712,9 @@ int xnintr_attach(xnintr_t *intr, void *cookie)
 		goto out;
 
 	__setbits(intr->flags, XN_ISR_ATTACHED);
-	xnintr_stat_counter_inc();
+	xnintr_count++;
 out:
-	xnlock_put_irqrestore(&intrlock, s);
+	up(&intrlock);
 
 	return ret;
 }
@@ -775,11 +754,10 @@ EXPORT_SYMBOL_GPL(xnintr_attach);
 int xnintr_detach(xnintr_t *intr)
 {
 	int ret;
-	spl_t s;
 
 	trace_mark(xn_nucleus, irq_detach, "irq %u", intr->irq);
 
-	xnlock_get_irqsave(&intrlock, s);
+	down(&intrlock);
 
 	if (!__testbits(intr->flags, XN_ISR_ATTACHED)) {
 		ret = -EINVAL;
@@ -792,9 +770,9 @@ int xnintr_detach(xnintr_t *intr)
 	if (ret)
 		goto out;
 
-	xnintr_stat_counter_dec();
+	xnintr_count--;
  out:
-	xnlock_put_irqrestore(&intrlock, s);
+	up(&intrlock);
 
 	return ret;
 }
@@ -894,23 +872,21 @@ void xnintr_affinity(xnintr_t *intr, xnarch_cpumask_t cpumask)
 EXPORT_SYMBOL_GPL(xnintr_affinity);
 
 #ifdef CONFIG_XENO_OPT_STATS
+int xnintr_get_query_lock(struct xnvfile *vfile)
+{
+	return down_interruptible(&intrlock) ? -ERESTARTSYS : 0;
+}
+
+void xnintr_put_query_lock(struct xnvfile *vfile)
+{
+	up(&intrlock);
+}
+
 int xnintr_query_init(xnintr_iterator_t *iterator)
 {
 	iterator->cpu = -1;
 	iterator->prev = NULL;
 
-	/* The order is important here: first xnintr_list_rev then
-	 * xnintr_count.  On the other hand, xnintr_attach/detach()
-	 * update xnintr_count first and then xnintr_list_rev.  This
-	 * should guarantee that we can't get an up-to-date
-	 * xnintr_list_rev and old xnintr_count here. The other way
-	 * around is not a problem as xnintr_query() will notice this
-	 * fact later.  Should xnintr_list_rev change later,
-	 * xnintr_query() will trigger an appropriate error below. */
-
-	iterator->list_rev = xnintr_list_rev;
-	xnarch_memory_barrier();
-
 	return xnintr_count;
 }
 
@@ -919,20 +895,11 @@ int xnintr_query_next(int irq, xnintr_iterator_t *iterator, char *name_buf)
 	xnintr_t *intr;
 	xnticks_t last_switch;
 	int cpu_no = iterator->cpu + 1;
-	int err = 0;
-	spl_t s;
 
 	if (cpu_no == xnarch_num_online_cpus())
 		cpu_no = 0;
 	iterator->cpu = cpu_no;
 
-	xnlock_get_irqsave(&intrlock, s);
-
-	if (iterator->list_rev != xnintr_list_rev) {
-		err = -EAGAIN;
-		goto unlock_and_exit;
-	}
-
 	if (!iterator->prev) {
 		if (irq == XNARCH_TIMER_IRQ)
 			intr = &nkclock;
@@ -944,8 +911,7 @@ int xnintr_query_next(int irq, xnintr_iterator_t *iterator, char *name_buf)
 	if (!intr) {
 		cpu_no = -1;
 		iterator->prev = NULL;
-		err = -ENODEV;
-		goto unlock_and_exit;
+		return -ENODEV;
 	}
 
 	snprintf(name_buf, XNOBJECT_NAME_LEN, "IRQ%d: %s", irq, intr->name);
@@ -968,10 +934,7 @@ int xnintr_query_next(int irq, xnintr_iterator_t *iterator, char *name_buf)
 	if (cpu_no + 1 == xnarch_num_online_cpus())
 		iterator->prev = intr;
 
-     unlock_and_exit:
-	xnlock_put_irqrestore(&intrlock, s);
-
-	return err;
+	return 0;
 }
 #endif /* CONFIG_XENO_OPT_STATS */
 
@@ -983,7 +946,6 @@ static inline int format_irq_proc(unsigned int irq,
 				  struct xnvfile_regular_iterator *it)
 {
 	struct xnintr *intr;
-	spl_t s;
 
 	if (irq == XNARCH_TIMER_IRQ) {
 		xnvfile_puts(it, "         [timer]");
@@ -1005,7 +967,7 @@ static inline int format_irq_proc(unsigned int irq,
 		return 0;
 	}
 
-	xnlock_get_irqsave(&intrlock, s);
+	down(&intrlock);
 
 	intr = xnintr_shirq_first(irq);
 	if (intr) {
@@ -1018,7 +980,7 @@ static inline int format_irq_proc(unsigned int irq,
 		} while (intr);
 	}
 
-	xnlock_put_irqrestore(&intrlock, s);
+	up(&intrlock);
 
 	return 0;
 }
diff --git a/ksrc/nucleus/sched.c b/ksrc/nucleus/sched.c
index 61957e6..16a59f8 100644
--- a/ksrc/nucleus/sched.c
+++ b/ksrc/nucleus/sched.c
@@ -839,11 +839,17 @@ struct vfile_stat_data {
 
 static struct xnvfile_snapshot_ops vfile_stat_ops;
 
+static struct xnvfile_lock_ops vfile_stat_lockops = {
+	.get = xnintr_get_query_lock,
+	.put = xnintr_put_query_lock,
+};
+
 static struct xnvfile_snapshot stat_vfile = {
 	.privsz = sizeof(struct vfile_stat_priv),
 	.datasz = sizeof(struct vfile_stat_data),
 	.tag = &nkpod_struct.threadlist_tag,
 	.ops = &vfile_stat_ops,
+	.entry = { .lockops = &vfile_stat_lockops },
 };
 
 static int vfile_stat_rewind(struct xnvfile_snapshot_iterator *it)
-- 
1.7.3.4

Re: [Xenomai] [PATCH v3] nucleus: Convert intrlock to a sleeping Linux lock

[Philippe Gerum]

On 01/03/2013 03:09 PM, Jan Kiszka wrote:

> +static struct xnvfile_lock_ops vfile_stat_lockops = {
> +	.get = xnintr_get_query_lock,
> +	.put = xnintr_put_query_lock,
> +};
> +
>  static struct xnvfile_snapshot stat_vfile = {
>  	.privsz = sizeof(struct vfile_stat_priv),
>  	.datasz = sizeof(struct vfile_stat_data),
>  	.tag = &nkpod_struct.threadlist_tag,
>  	.ops = &vfile_stat_ops,
> +	.entry = { .lockops = &vfile_stat_lockops },
>  };

This would introduce a regression. stat_vfile's rewind/next handlers
compete with code altering the global thread queue which may run in
primary mode, such as xnpod_delete_thread() when deleting a kernel task.
So we can't use a linux lock to protect it.

-- 
Philippe.

Re: [Xenomai] [PATCH v3] nucleus: Convert intrlock to a sleeping Linux lock

[Jan Kiszka]

On 2013-01-06 11:14, Philippe Gerum wrote:
> On 01/03/2013 03:09 PM, Jan Kiszka wrote:
> 
>> +static struct xnvfile_lock_ops vfile_stat_lockops = {
>> +	.get = xnintr_get_query_lock,
>> +	.put = xnintr_put_query_lock,
>> +};
>> +
>>  static struct xnvfile_snapshot stat_vfile = {
>>  	.privsz = sizeof(struct vfile_stat_priv),
>>  	.datasz = sizeof(struct vfile_stat_data),
>>  	.tag = &nkpod_struct.threadlist_tag,
>>  	.ops = &vfile_stat_ops,
>> +	.entry = { .lockops = &vfile_stat_lockops },
>>  };
> 
> This would introduce a regression. stat_vfile's rewind/next handlers
> compete with code altering the global thread queue which may run in
> primary mode, such as xnpod_delete_thread() when deleting a kernel task.
> So we can't use a linux lock to protect it.

The Linux lock is held around this rewind loop (which is still in
place), and it is not protecting anything thread related, just the
appearance and disappearance of IRQs. I fail to see a conflict, and
there are also no I-pipe/Xenomai warnings triggered with this pattern.

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 261 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130106/51171f9c/attachment.pgp>

Re: [Xenomai] [PATCH v3] nucleus: Convert intrlock to a sleeping Linux lock

[Jan Kiszka]

On 2013-01-06 11:55, Jan Kiszka wrote:
> On 2013-01-06 11:14, Philippe Gerum wrote:
>> On 01/03/2013 03:09 PM, Jan Kiszka wrote:
>>
>>> +static struct xnvfile_lock_ops vfile_stat_lockops = {
>>> +	.get = xnintr_get_query_lock,
>>> +	.put = xnintr_put_query_lock,
>>> +};
>>> +
>>>  static struct xnvfile_snapshot stat_vfile = {
>>>  	.privsz = sizeof(struct vfile_stat_priv),
>>>  	.datasz = sizeof(struct vfile_stat_data),
>>>  	.tag = &nkpod_struct.threadlist_tag,
>>>  	.ops = &vfile_stat_ops,
>>> +	.entry = { .lockops = &vfile_stat_lockops },
>>>  };
>>
>> This would introduce a regression. stat_vfile's rewind/next handlers
>> compete with code altering the global thread queue which may run in
>> primary mode, such as xnpod_delete_thread() when deleting a kernel task.
>> So we can't use a linux lock to protect it.
> 
> The Linux lock is held around this rewind loop (which is still in
> place), and it is not protecting anything thread related, just the
> appearance and disappearance of IRQs. I fail to see a conflict, and
> there are also no I-pipe/Xenomai warnings triggered with this pattern.

Oh, now I see: The nklock is automatically taken if there are no
lockops. That needs to be done in the stat lockups as well, of course.
Will fix.

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 261 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130106/f836c9a4/attachment.pgp>

Re: [Xenomai] [PATCH v3] nucleus: Convert intrlock to a sleeping Linux lock

[Gilles Chanteperdrix]

On 01/06/2013 12:03 PM, Jan Kiszka wrote:

> On 2013-01-06 11:55, Jan Kiszka wrote:
>> On 2013-01-06 11:14, Philippe Gerum wrote:
>>> On 01/03/2013 03:09 PM, Jan Kiszka wrote:
>>>
>>>> +static struct xnvfile_lock_ops vfile_stat_lockops = {
>>>> +	.get = xnintr_get_query_lock,
>>>> +	.put = xnintr_put_query_lock,
>>>> +};
>>>> +
>>>>  static struct xnvfile_snapshot stat_vfile = {
>>>>  	.privsz = sizeof(struct vfile_stat_priv),
>>>>  	.datasz = sizeof(struct vfile_stat_data),
>>>>  	.tag = &nkpod_struct.threadlist_tag,
>>>>  	.ops = &vfile_stat_ops,
>>>> +	.entry = { .lockops = &vfile_stat_lockops },
>>>>  };
>>>
>>> This would introduce a regression. stat_vfile's rewind/next handlers
>>> compete with code altering the global thread queue which may run in
>>> primary mode, such as xnpod_delete_thread() when deleting a kernel task.
>>> So we can't use a linux lock to protect it.
>>
>> The Linux lock is held around this rewind loop (which is still in
>> place), and it is not protecting anything thread related, just the
>> appearance and disappearance of IRQs. I fail to see a conflict, and
>> there are also no I-pipe/Xenomai warnings triggered with this pattern.
> 
> Oh, now I see: The nklock is automatically taken if there are no
> lockops. That needs to be done in the stat lockups as well, of course.
> Will fix.


The more I think about this, the more I find we should not be doing
this. Xenomai 2.6.x is a stable branch, let us stop making such changes,
this will allow us to start working on -forge. The warning is
essentially a false-positive, I do not see any problem calling
ipipe_virtualize_irq with the root domain stalled. We can skip it in the
I-pipe core patches when compiling with CONFIG_IPIPE_LEGACY.

-- 
                                                                Gilles.

Re: [Xenomai] [PATCH v3] nucleus: Convert intrlock to a sleeping Linux lock

[Jan Kiszka]

On 2013-01-06 14:55, Gilles Chanteperdrix wrote:
> On 01/06/2013 12:03 PM, Jan Kiszka wrote:
> 
>> On 2013-01-06 11:55, Jan Kiszka wrote:
>>> On 2013-01-06 11:14, Philippe Gerum wrote:
>>>> On 01/03/2013 03:09 PM, Jan Kiszka wrote:
>>>>
>>>>> +static struct xnvfile_lock_ops vfile_stat_lockops = {
>>>>> +	.get = xnintr_get_query_lock,
>>>>> +	.put = xnintr_put_query_lock,
>>>>> +};
>>>>> +
>>>>>  static struct xnvfile_snapshot stat_vfile = {
>>>>>  	.privsz = sizeof(struct vfile_stat_priv),
>>>>>  	.datasz = sizeof(struct vfile_stat_data),
>>>>>  	.tag = &nkpod_struct.threadlist_tag,
>>>>>  	.ops = &vfile_stat_ops,
>>>>> +	.entry = { .lockops = &vfile_stat_lockops },
>>>>>  };
>>>>
>>>> This would introduce a regression. stat_vfile's rewind/next handlers
>>>> compete with code altering the global thread queue which may run in
>>>> primary mode, such as xnpod_delete_thread() when deleting a kernel task.
>>>> So we can't use a linux lock to protect it.
>>>
>>> The Linux lock is held around this rewind loop (which is still in
>>> place), and it is not protecting anything thread related, just the
>>> appearance and disappearance of IRQs. I fail to see a conflict, and
>>> there are also no I-pipe/Xenomai warnings triggered with this pattern.
>>
>> Oh, now I see: The nklock is automatically taken if there are no
>> lockops. That needs to be done in the stat lockups as well, of course.
>> Will fix.
> 
> 
> The more I think about this, the more I find we should not be doing
> this. Xenomai 2.6.x is a stable branch, let us stop making such changes,
> this will allow us to start working on -forge. The warning is
> essentially a false-positive, I do not see any problem calling
> ipipe_virtualize_irq with the root domain stalled. We can skip it in the
> I-pipe core patches when compiling with CONFIG_IPIPE_LEGACY.

Well, we need to support 2.6 for quite a while here, so I'm interested
in fixing such regressions, also on instrumentations, without increasing
our local queue size. The next version will be less invasive than the
current one. Finally, fixing this issue revealed also other proc output
bugs.

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 261 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130106/10ffe864/attachment.pgp>

Re: [Xenomai] [PATCH v3] nucleus: Convert intrlock to a sleeping Linux lock

[Gilles Chanteperdrix]

On 01/06/2013 03:20 PM, Jan Kiszka wrote:

> On 2013-01-06 14:55, Gilles Chanteperdrix wrote:
>> On 01/06/2013 12:03 PM, Jan Kiszka wrote:
>>
>>> On 2013-01-06 11:55, Jan Kiszka wrote:
>>>> On 2013-01-06 11:14, Philippe Gerum wrote:
>>>>> On 01/03/2013 03:09 PM, Jan Kiszka wrote:
>>>>>
>>>>>> +static struct xnvfile_lock_ops vfile_stat_lockops = {
>>>>>> +	.get = xnintr_get_query_lock,
>>>>>> +	.put = xnintr_put_query_lock,
>>>>>> +};
>>>>>> +
>>>>>>  static struct xnvfile_snapshot stat_vfile = {
>>>>>>  	.privsz = sizeof(struct vfile_stat_priv),
>>>>>>  	.datasz = sizeof(struct vfile_stat_data),
>>>>>>  	.tag = &nkpod_struct.threadlist_tag,
>>>>>>  	.ops = &vfile_stat_ops,
>>>>>> +	.entry = { .lockops = &vfile_stat_lockops },
>>>>>>  };
>>>>>
>>>>> This would introduce a regression. stat_vfile's rewind/next handlers
>>>>> compete with code altering the global thread queue which may run in
>>>>> primary mode, such as xnpod_delete_thread() when deleting a kernel task.
>>>>> So we can't use a linux lock to protect it.
>>>>
>>>> The Linux lock is held around this rewind loop (which is still in
>>>> place), and it is not protecting anything thread related, just the
>>>> appearance and disappearance of IRQs. I fail to see a conflict, and
>>>> there are also no I-pipe/Xenomai warnings triggered with this pattern.
>>>
>>> Oh, now I see: The nklock is automatically taken if there are no
>>> lockops. That needs to be done in the stat lockups as well, of course.
>>> Will fix.
>>
>>
>> The more I think about this, the more I find we should not be doing
>> this. Xenomai 2.6.x is a stable branch, let us stop making such changes,
>> this will allow us to start working on -forge. The warning is
>> essentially a false-positive, I do not see any problem calling
>> ipipe_virtualize_irq with the root domain stalled. We can skip it in the
>> I-pipe core patches when compiling with CONFIG_IPIPE_LEGACY.
> 
> Well, we need to support 2.6 for quite a while here, so I'm interested
> in fixing such regressions,


I repeat again: there is no regression, it is not invalid to call
ipipe_virtualize_irq with the root domain stalled, and never was, the
warning is new and a false positive. I am opposed to changing intrlock
into a mutex with a lot of cascading consequences, the ones we see, the
ones we will discover when people start using the next release.

If you want to benefit from the full instrumentation of the I-pipe core,
use xenomai-forge.

> also on instrumentations, without increasing
> our local queue size. The next version will be less invasive than the
> current one. Finally, fixing this issue revealed also other proc output
> bugs.


If you are talking about the XNARCH_TIMER_IRQ mess, then yes, we should
fix this one.

-- 
                                                                Gilles.

Re: [Xenomai] [PATCH v3] nucleus: Convert intrlock to a sleeping Linux lock

[Gilles Chanteperdrix]

On 01/06/2013 03:26 PM, Gilles Chanteperdrix wrote:

> On 01/06/2013 03:20 PM, Jan Kiszka wrote:
> 
>> On 2013-01-06 14:55, Gilles Chanteperdrix wrote:
>>> On 01/06/2013 12:03 PM, Jan Kiszka wrote:
>>>
>>>> On 2013-01-06 11:55, Jan Kiszka wrote:
>>>>> On 2013-01-06 11:14, Philippe Gerum wrote:
>>>>>> On 01/03/2013 03:09 PM, Jan Kiszka wrote:
>>>>>>
>>>>>>> +static struct xnvfile_lock_ops vfile_stat_lockops = {
>>>>>>> +	.get = xnintr_get_query_lock,
>>>>>>> +	.put = xnintr_put_query_lock,
>>>>>>> +};
>>>>>>> +
>>>>>>>  static struct xnvfile_snapshot stat_vfile = {
>>>>>>>  	.privsz = sizeof(struct vfile_stat_priv),
>>>>>>>  	.datasz = sizeof(struct vfile_stat_data),
>>>>>>>  	.tag = &nkpod_struct.threadlist_tag,
>>>>>>>  	.ops = &vfile_stat_ops,
>>>>>>> +	.entry = { .lockops = &vfile_stat_lockops },
>>>>>>>  };
>>>>>>
>>>>>> This would introduce a regression. stat_vfile's rewind/next handlers
>>>>>> compete with code altering the global thread queue which may run in
>>>>>> primary mode, such as xnpod_delete_thread() when deleting a kernel task.
>>>>>> So we can't use a linux lock to protect it.
>>>>>
>>>>> The Linux lock is held around this rewind loop (which is still in
>>>>> place), and it is not protecting anything thread related, just the
>>>>> appearance and disappearance of IRQs. I fail to see a conflict, and
>>>>> there are also no I-pipe/Xenomai warnings triggered with this pattern.
>>>>
>>>> Oh, now I see: The nklock is automatically taken if there are no
>>>> lockops. That needs to be done in the stat lockups as well, of course.
>>>> Will fix.
>>>
>>>
>>> The more I think about this, the more I find we should not be doing
>>> this. Xenomai 2.6.x is a stable branch, let us stop making such changes,
>>> this will allow us to start working on -forge. The warning is
>>> essentially a false-positive, I do not see any problem calling
>>> ipipe_virtualize_irq with the root domain stalled. We can skip it in the
>>> I-pipe core patches when compiling with CONFIG_IPIPE_LEGACY.
>>
>> Well, we need to support 2.6 for quite a while here, so I'm interested
>> in fixing such regressions,
> 
> 
> I repeat again: there is no regression, it is not invalid to call
> ipipe_virtualize_irq with the root domain stalled, and never was, the
> warning is new and a false positive. I am opposed to changing intrlock
> into a mutex with a lot of cascading consequences, the ones we see, the
> ones we will discover when people start using the next release.
> 
> If you want to benefit from the full instrumentation of the I-pipe core,
> use xenomai-forge.
> 
>> also on instrumentations, without increasing
>> our local queue size. The next version will be less invasive than the
>> current one. Finally, fixing this issue revealed also other proc output
>> bugs.
> 
> 
> If you are talking about the XNARCH_TIMER_IRQ mess, then yes, we should
> fix this one.
> 


And this mess is because I made the same mistake as you: I wanted to
benefit from the I-pipe core improvements in the 2.6 branch, by using
the I-pipe timers, in retrospect, this was a mistake, we could have
created a compatibility glue for the architectures such as ARM where the
timer abstraction was provided by the I-pipe, and keep unchanged the
architectures which handle their timer themselves such as x86. See how
many ripples it did, let us not make the same mistake again.

-- 
                                                                Gilles.

Re: [Xenomai] [PATCH v3] nucleus: Convert intrlock to a sleeping Linux lock

[Jan Kiszka]

On 2013-01-06 15:26, Gilles Chanteperdrix wrote:
> On 01/06/2013 03:20 PM, Jan Kiszka wrote:
> 
>> On 2013-01-06 14:55, Gilles Chanteperdrix wrote:
>>> On 01/06/2013 12:03 PM, Jan Kiszka wrote:
>>>
>>>> On 2013-01-06 11:55, Jan Kiszka wrote:
>>>>> On 2013-01-06 11:14, Philippe Gerum wrote:
>>>>>> On 01/03/2013 03:09 PM, Jan Kiszka wrote:
>>>>>>
>>>>>>> +static struct xnvfile_lock_ops vfile_stat_lockops = {
>>>>>>> +	.get = xnintr_get_query_lock,
>>>>>>> +	.put = xnintr_put_query_lock,
>>>>>>> +};
>>>>>>> +
>>>>>>>  static struct xnvfile_snapshot stat_vfile = {
>>>>>>>  	.privsz = sizeof(struct vfile_stat_priv),
>>>>>>>  	.datasz = sizeof(struct vfile_stat_data),
>>>>>>>  	.tag = &nkpod_struct.threadlist_tag,
>>>>>>>  	.ops = &vfile_stat_ops,
>>>>>>> +	.entry = { .lockops = &vfile_stat_lockops },
>>>>>>>  };
>>>>>>
>>>>>> This would introduce a regression. stat_vfile's rewind/next handlers
>>>>>> compete with code altering the global thread queue which may run in
>>>>>> primary mode, such as xnpod_delete_thread() when deleting a kernel task.
>>>>>> So we can't use a linux lock to protect it.
>>>>>
>>>>> The Linux lock is held around this rewind loop (which is still in
>>>>> place), and it is not protecting anything thread related, just the
>>>>> appearance and disappearance of IRQs. I fail to see a conflict, and
>>>>> there are also no I-pipe/Xenomai warnings triggered with this pattern.
>>>>
>>>> Oh, now I see: The nklock is automatically taken if there are no
>>>> lockops. That needs to be done in the stat lockups as well, of course.
>>>> Will fix.
>>>
>>>
>>> The more I think about this, the more I find we should not be doing
>>> this. Xenomai 2.6.x is a stable branch, let us stop making such changes,
>>> this will allow us to start working on -forge. The warning is
>>> essentially a false-positive, I do not see any problem calling
>>> ipipe_virtualize_irq with the root domain stalled. We can skip it in the
>>> I-pipe core patches when compiling with CONFIG_IPIPE_LEGACY.
>>
>> Well, we need to support 2.6 for quite a while here, so I'm interested
>> in fixing such regressions,
> 
> 
> I repeat again: there is no regression, it is not invalid to call
> ipipe_virtualize_irq with the root domain stalled, and never was, the
> warning is new and a false positive. I am opposed to changing intrlock
> into a mutex with a lot of cascading consequences, the ones we see, the
> ones we will discover when people start using the next release.
> 
> If you want to benefit from the full instrumentation of the I-pipe core,
> use xenomai-forge.

Unfortunately, forge wasn't ready for our workload last time we checked.
That's why we are stabilizing I-pipe x86 over 2.6 here, and that's why
we need working instrumentations also in legacy mode.

If you prefer to skip the warning in ipipe, then I will send a
corresponding patch. The lock conversion is still necessary for forge,
though.

Jan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 261 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130106/efc4a149/attachment.pgp>

Re: [Xenomai] [PATCH v3] nucleus: Convert intrlock to a sleeping Linux lock

[Gilles Chanteperdrix]

On 01/06/2013 03:46 PM, Jan Kiszka wrote:

> On 2013-01-06 15:26, Gilles Chanteperdrix wrote:
>> On 01/06/2013 03:20 PM, Jan Kiszka wrote:
>>
>>> On 2013-01-06 14:55, Gilles Chanteperdrix wrote:
>>>> On 01/06/2013 12:03 PM, Jan Kiszka wrote:
>>>>
>>>>> On 2013-01-06 11:55, Jan Kiszka wrote:
>>>>>> On 2013-01-06 11:14, Philippe Gerum wrote:
>>>>>>> On 01/03/2013 03:09 PM, Jan Kiszka wrote:
>>>>>>>
>>>>>>>> +static struct xnvfile_lock_ops vfile_stat_lockops = {
>>>>>>>> +	.get = xnintr_get_query_lock,
>>>>>>>> +	.put = xnintr_put_query_lock,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>>  static struct xnvfile_snapshot stat_vfile = {
>>>>>>>>  	.privsz = sizeof(struct vfile_stat_priv),
>>>>>>>>  	.datasz = sizeof(struct vfile_stat_data),
>>>>>>>>  	.tag = &nkpod_struct.threadlist_tag,
>>>>>>>>  	.ops = &vfile_stat_ops,
>>>>>>>> +	.entry = { .lockops = &vfile_stat_lockops },
>>>>>>>>  };
>>>>>>>
>>>>>>> This would introduce a regression. stat_vfile's rewind/next handlers
>>>>>>> compete with code altering the global thread queue which may run in
>>>>>>> primary mode, such as xnpod_delete_thread() when deleting a kernel task.
>>>>>>> So we can't use a linux lock to protect it.
>>>>>>
>>>>>> The Linux lock is held around this rewind loop (which is still in
>>>>>> place), and it is not protecting anything thread related, just the
>>>>>> appearance and disappearance of IRQs. I fail to see a conflict, and
>>>>>> there are also no I-pipe/Xenomai warnings triggered with this pattern.
>>>>>
>>>>> Oh, now I see: The nklock is automatically taken if there are no
>>>>> lockops. That needs to be done in the stat lockups as well, of course.
>>>>> Will fix.
>>>>
>>>>
>>>> The more I think about this, the more I find we should not be doing
>>>> this. Xenomai 2.6.x is a stable branch, let us stop making such changes,
>>>> this will allow us to start working on -forge. The warning is
>>>> essentially a false-positive, I do not see any problem calling
>>>> ipipe_virtualize_irq with the root domain stalled. We can skip it in the
>>>> I-pipe core patches when compiling with CONFIG_IPIPE_LEGACY.
>>>
>>> Well, we need to support 2.6 for quite a while here, so I'm interested
>>> in fixing such regressions,
>>
>>
>> I repeat again: there is no regression, it is not invalid to call
>> ipipe_virtualize_irq with the root domain stalled, and never was, the
>> warning is new and a false positive. I am opposed to changing intrlock
>> into a mutex with a lot of cascading consequences, the ones we see, the
>> ones we will discover when people start using the next release.
>>
>> If you want to benefit from the full instrumentation of the I-pipe core,
>> use xenomai-forge.
> 
> Unfortunately, forge wasn't ready for our workload last time we checked.
> That's why we are stabilizing I-pipe x86 over 2.6 here, and that's why
> we need working instrumentations also in legacy mode.
> 
> If you prefer to skip the warning in ipipe, then I will send a
> corresponding patch. The lock conversion is still necessary for forge,
> though.


There are other ways around, ipipe_virtualize_irq is already a wrapper
used only by 2.6, so we can add some code here. So, we can only check
ipipe_root_p here, and disable the warning in ipipe_request_irq.


-- 
                                                                Gilles.

Re: [Xenomai] [PATCH v3] nucleus: Convert intrlock to a sleeping Linux lock

[Jan Kiszka]

On 2013-01-06 16:09, Gilles Chanteperdrix wrote:
>> If you prefer to skip the warning in ipipe, then I will send a
>> corresponding patch. The lock conversion is still necessary for forge,
>> though.
> 
> 
> There are other ways around, ipipe_virtualize_irq is already a wrapper
> used only by 2.6, so we can add some code here. So, we can only check
> ipipe_root_p here, and disable the warning in ipipe_request_irq.

Err, we rather need unchecked __ipipe_request_irq, called by
ipipe_request_irq after testing the context and by ipipe_virtualize_irq
without that test.

Jan


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 261 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130106/38aa9c95/attachment.pgp>

Re: [Xenomai] [PATCH v3] nucleus: Convert intrlock to a sleeping Linux lock

[Gilles Chanteperdrix]

On 01/06/2013 04:11 PM, Jan Kiszka wrote:

> On 2013-01-06 16:09, Gilles Chanteperdrix wrote:
>>> If you prefer to skip the warning in ipipe, then I will send a
>>> corresponding patch. The lock conversion is still necessary for forge,
>>> though.
>>
>>
>> There are other ways around, ipipe_virtualize_irq is already a wrapper
>> used only by 2.6, so we can add some code here. So, we can only check
>> ipipe_root_p here, and disable the warning in ipipe_request_irq.
> 
> Err, we rather need unchecked __ipipe_request_irq, called by
> ipipe_request_irq after testing the context and by ipipe_virtualize_irq
> without that test.


ipipe_root_only does two tests, it tests whether the current domain is
root, and whether root is stalled. The problem, as far as I understood,
come from the second test, what I mean is that we can arrange to keep
the first test for ipipe_virtualize_irq.


-- 
                                                                Gilles.

Re: [Xenomai] [PATCH v3] nucleus: Convert intrlock to a sleeping Linux lock

[Jan Kiszka]

On 2013-01-06 16:15, Gilles Chanteperdrix wrote:
> On 01/06/2013 04:11 PM, Jan Kiszka wrote:
> 
>> On 2013-01-06 16:09, Gilles Chanteperdrix wrote:
>>>> If you prefer to skip the warning in ipipe, then I will send a
>>>> corresponding patch. The lock conversion is still necessary for forge,
>>>> though.
>>>
>>>
>>> There are other ways around, ipipe_virtualize_irq is already a wrapper
>>> used only by 2.6, so we can add some code here. So, we can only check
>>> ipipe_root_p here, and disable the warning in ipipe_request_irq.
>>
>> Err, we rather need unchecked __ipipe_request_irq, called by
>> ipipe_request_irq after testing the context and by ipipe_virtualize_irq
>> without that test.
> 
> 
> ipipe_root_only does two tests, it tests whether the current domain is
> root, and whether root is stalled. The problem, as far as I understood,
> come from the second test, what I mean is that we can arrange to keep
> the first test for ipipe_virtualize_irq.

Well, if we disallow non-root invocations of ipipe_virtualize_irq, then
we can also convert the lock to a Linux version. I thought your concerns
are related to restricting the usage of that services (or services that
call it) in stable 2.6.

Jan


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 261 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130106/b6b0d47e/attachment.pgp>

Re: [Xenomai] [PATCH v3] nucleus: Convert intrlock to a sleeping Linux lock

[Gilles Chanteperdrix]

On 01/06/2013 04:19 PM, Jan Kiszka wrote:

> On 2013-01-06 16:15, Gilles Chanteperdrix wrote:
>> On 01/06/2013 04:11 PM, Jan Kiszka wrote:
>>
>>> On 2013-01-06 16:09, Gilles Chanteperdrix wrote:
>>>>> If you prefer to skip the warning in ipipe, then I will send a
>>>>> corresponding patch. The lock conversion is still necessary for forge,
>>>>> though.
>>>>
>>>>
>>>> There are other ways around, ipipe_virtualize_irq is already a wrapper
>>>> used only by 2.6, so we can add some code here. So, we can only check
>>>> ipipe_root_p here, and disable the warning in ipipe_request_irq.
>>>
>>> Err, we rather need unchecked __ipipe_request_irq, called by
>>> ipipe_request_irq after testing the context and by ipipe_virtualize_irq
>>> without that test.
>>
>>
>> ipipe_root_only does two tests, it tests whether the current domain is
>> root, and whether root is stalled. The problem, as far as I understood,
>> come from the second test, what I mean is that we can arrange to keep
>> the first test for ipipe_virtualize_irq.
> 
> Well, if we disallow non-root invocations of ipipe_virtualize_irq, then
> we can also convert the lock to a Linux version. I thought your concerns
> are related to restricting the usage of that services (or services that
> call it) in stable 2.6.


No, my concerns are about the many regression that turning a spinlock
into a mutex could bring. If turning a spinlock into a mutex was such a
harmless matter, maybe the -rt patch would have been merged for years
;-)

Your patches changes some behaviour, for instance, with the current
implementation, if an interrupt is taken while reading cat
/proc/xenomai/irq, the display to /proc is restarted, after your patch,
this behaviour changes. I do not think that it matters, but you get the
point: there are many consequences possible with this change, yet
unforeseen but which will pop up as soon as people start using the
modified version and cause us to keep working on 2.6 whereas we should
be working on -forge.

Besides, if 2.6 benefits from all the changes made to the I-pipe core,
what will be left to advertise when 3.0 is out ;-)

-- 
                                                                Gilles.

Re: [Xenomai] [PATCH v3] nucleus: Convert intrlock to a sleeping Linux lock

[Jan Kiszka]

On 2013-01-06 16:26, Gilles Chanteperdrix wrote:
> On 01/06/2013 04:19 PM, Jan Kiszka wrote:
> 
>> On 2013-01-06 16:15, Gilles Chanteperdrix wrote:
>>> On 01/06/2013 04:11 PM, Jan Kiszka wrote:
>>>
>>>> On 2013-01-06 16:09, Gilles Chanteperdrix wrote:
>>>>>> If you prefer to skip the warning in ipipe, then I will send a
>>>>>> corresponding patch. The lock conversion is still necessary for forge,
>>>>>> though.
>>>>>
>>>>>
>>>>> There are other ways around, ipipe_virtualize_irq is already a wrapper
>>>>> used only by 2.6, so we can add some code here. So, we can only check
>>>>> ipipe_root_p here, and disable the warning in ipipe_request_irq.
>>>>
>>>> Err, we rather need unchecked __ipipe_request_irq, called by
>>>> ipipe_request_irq after testing the context and by ipipe_virtualize_irq
>>>> without that test.
>>>
>>>
>>> ipipe_root_only does two tests, it tests whether the current domain is
>>> root, and whether root is stalled. The problem, as far as I understood,
>>> come from the second test, what I mean is that we can arrange to keep
>>> the first test for ipipe_virtualize_irq.
>>
>> Well, if we disallow non-root invocations of ipipe_virtualize_irq, then
>> we can also convert the lock to a Linux version. I thought your concerns
>> are related to restricting the usage of that services (or services that
>> call it) in stable 2.6.
> 
> 
> No, my concerns are about the many regression that turning a spinlock
> into a mutex could bring. If turning a spinlock into a mutex was such a
> harmless matter, maybe the -rt patch would have been merged for years
> ;-)
> 
> Your patches changes some behaviour, for instance, with the current
> implementation, if an interrupt is taken while reading cat
> /proc/xenomai/irq, the display to /proc is restarted, after your patch,
> this behaviour changes.

This change was broken, the next version (or the one for forge) will not
do this mistake.

> I do not think that it matters, but you get the
> point: there are many consequences possible with this change, yet
> unforeseen but which will pop up as soon as people start using the
> modified version and cause us to keep working on 2.6 whereas we should
> be working on -forge.

I cannot chose yet where I'd like to work on. Getting a working combo of
Xenomai 2.6 with kernel 3.x will hopefully give us the room to move on.

Jan


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 261 bytes
Desc: OpenPGP digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130106/ef0fe961/attachment.pgp>