nucleus: Convert intrlock to a sleeping Linux lock

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 <[email protected]>

---

Quoted 11 lines · 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 <[email protected]>

---

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

Quoted 9 lines · Jan Kiszka

ksrc/nucleus/sched.c +5 -0

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

ksrc/nucleus/sched.c struct vfile_stat_data { · line 853

Quoted 3 lines

ksrc/nucleus/sched.c +1 -0

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

Quoted 11 lines · Philippe Gerum

Quoted 9 lines · Jan Kiszka

ksrc/nucleus/sched.c +5 -0

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

Quoted 8 lines

ksrc/nucleus/sched.c struct vfile_stat_data { · line 853

Quoted 3 lines

ksrc/nucleus/sched.c +1 -0

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

Quoted 21 lines · Jan Kiszka

Quoted 19 lines · Philippe Gerum

ksrc/nucleus/sched.c struct vfile_stat_data { · line 853

Quoted 12 lines · Jan Kiszka

ksrc/nucleus/sched.c +6 -0

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

Quoted 4 lines

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.

Quoted 22 lines · Jan Kiszka

Quoted 21 lines · Jan Kiszka

Quoted 19 lines · Philippe Gerum

ksrc/nucleus/sched.c struct vfile_stat_data { · line 853

Quoted 12 lines · Jan Kiszka

ksrc/nucleus/sched.c +6 -0

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

Quoted 8 lines

Quoted 4 lines

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.

Quoted 34 lines · Gilles Chanteperdrix

Quoted 22 lines · Jan Kiszka

Quoted 21 lines · Jan Kiszka

Quoted 19 lines · Philippe Gerum

ksrc/nucleus/sched.c struct vfile_stat_data { · line 853

Quoted 12 lines · Jan Kiszka

ksrc/nucleus/sched.c +6 -0

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

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 42 lines · Jan Kiszka

Quoted 34 lines · Gilles Chanteperdrix

Quoted 22 lines · Jan Kiszka

Quoted 21 lines · Jan Kiszka

Quoted 19 lines · Philippe Gerum

Review

Quoted 12 lines · Jan Kiszka

(quoted diff) +6 -0

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

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 42 lines · Jan Kiszka

Quoted 34 lines · Gilles Chanteperdrix

Quoted 22 lines · Jan Kiszka

Quoted 21 lines · Jan Kiszka

Quoted 19 lines · Philippe Gerum

Review

Quoted 12 lines · Jan Kiszka

(quoted diff) +6 -0

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

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 48 lines · Gilles Chanteperdrix

Quoted 42 lines · Jan Kiszka

Quoted 34 lines · Gilles Chanteperdrix

Quoted 22 lines · Jan Kiszka

Quoted 21 lines · Jan Kiszka

Quoted 19 lines · Philippe Gerum

Review

Quoted 12 lines · Jan Kiszka

(quoted diff) +6 -0

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

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

Well, we need to support 2.6 for quite a while here, so I'm interested in fixing such regressions,

Quoted 8 lines

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.

Quoted 3 lines

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.

Quoted 5 lines · Gilles Chanteperdrix

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 6 lines · Jan Kiszka

Quoted 5 lines · Gilles Chanteperdrix

Quoted 3 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 17 lines · Gilles Chanteperdrix

Quoted 6 lines · Jan Kiszka

Quoted 5 lines · Gilles Chanteperdrix

Quoted 3 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 23 lines · Jan Kiszka

Quoted 17 lines · Gilles Chanteperdrix

Quoted 6 lines · Jan Kiszka

Quoted 5 lines · Gilles Chanteperdrix

Quoted 3 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.