Re: [Adeos-main] [BUG] evsync is not SMP-safe

[Jan Kiszka <jan.kiszka@domain.hid>]

[-- Attachment #1.1: Type: text/plain, Size: 2685 bytes --]

Jan Kiszka wrote:
> Jan Kiszka wrote:
>> Philippe,
>>
>> this
>>
>> int fastcall __ipipe_dispatch_event (unsigned event, void *data)
>> ...
>> 	ipipe_cpudom_var(next_domain, evsync) |= (1LL << event);
>> 	local_irq_restore_hw(flags);
>> 	propagate = !evhand(event, start_domain, data);
>> 	local_irq_save_hw(flags);
>> 	ipipe_cpudom_var(next_domain, evsync) &= ~(1LL << event);
>>
>> doesn't fly on SMP. While the invoked event handler is running, it may
>> happen that the caller gets migrated to another CPU. The result is an
>> inconsistent evsync state that causes ipipe_catch_event to stall (test
>> case: invoke Jerome's system() test a few times, them try to unload
>> Xenomai skins and nucleus).
>>
>> First idea (I've nothing implemented to far, would happily leave it to
>> someone else's hand): Track event handler entry/exit with an, say, 8 bit
>> per-cpu counter. On event deregistration, just summarize over the
>> per-cpu counters and wait for the sum to become 0. This has just the
>> drawback that it may cause livelocks on large SMP boxes when trying to
>> wait for a busy event. I've no perfect idea so far.
> 
> Second approach to solve this (currently) last known ipipe issue cleanly:
> 
> As long as some task in the system has __ipipe_dispatch_event (and thus
> some of the registered event handlers) on its stack, it is not safe to
> unregister some handler. For simplicity reasons, I don't think we should
> make any difference which handlers are precisely busy. So, how to find
> out if there are such tasks?
> 
> I checked the code and derived three classes of preconditions for the
> invocation of __ipipe_dispatch_event:
> 
>  1) ipipe_init_notify and ipipe_cleanup_notify -> no preconditions
> 
>  2) ipipe_trap_notify -> some Linux task has PF_EVNOTIFY set or
>                          there are tasks with custom stacks present
> 
>  3) all other invocations -> some Linux task has PF_EVNOTIFY set
> 
> This means by walking the full Linux task list and checking that are no
> more PF_EVNOTIFY-tasks present, we can easily exclude 3). In fact, 2)
> can also be excluded this way because we can reasonably demand that any
> I-pipe user fiddling with event handlers has killed all non-Linux tasks
> beforehand. This leaves us with 1) which should be handled via classic
> RCU as Linux offers it. Viable? It would even shorten the fast path a bit!
> 
> Still no code, I would like to collect feedback on this new idea first.

OK, here is some code. Seems to work fine (now that I fixed the cleanup
race in Xenomai as well - the issue happened to pop up right after
applying this patch :-/).

Jan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: rework-event-deregistration.patch --]
[-- Type: text/x-patch; name="rework-event-deregistration.patch", Size: 4830 bytes --]

---
 include/linux/ipipe.h        |   11 +++++++++--
 include/linux/ipipe_percpu.h |    1 -
 kernel/ipipe/core.c          |   42 ++++++++++++++++++++++++------------------
 3 files changed, 33 insertions(+), 21 deletions(-)

Index: linux-2.6.24-xeno_64/include/linux/ipipe_percpu.h
===================================================================
--- linux-2.6.24-xeno_64.orig/include/linux/ipipe_percpu.h
+++ linux-2.6.24-xeno_64/include/linux/ipipe_percpu.h
@@ -33,7 +33,6 @@ struct ipipe_percpu_domain_data {
 	unsigned long irqpend_lomask[IPIPE_IRQ_IWORDS];
 	unsigned long irqheld_mask[IPIPE_IRQ_IWORDS];
 	unsigned long irqall[IPIPE_NR_IRQS];
-	u64 evsync;
 };
 
 #ifdef CONFIG_SMP
Index: linux-2.6.24-xeno_64/kernel/ipipe/core.c
===================================================================
--- linux-2.6.24-xeno_64.orig/kernel/ipipe/core.c
+++ linux-2.6.24-xeno_64/kernel/ipipe/core.c
@@ -260,8 +260,6 @@ void __ipipe_init_stage(struct ipipe_dom
 
 		for (n = 0; n < IPIPE_NR_IRQS; n++)
 			ipipe_percpudom(ipd, irqall, cpu)[n] = 0;
-
-		ipipe_percpudom(ipd, evsync, cpu) = 0;
 	}
 
 	for (n = 0; n < IPIPE_NR_IRQS; n++) {
@@ -554,7 +552,6 @@ void fastcall __ipipe_walk_pipeline(stru
 				__ipipe_sync_pipeline(IPIPE_IRQMASK_ANY);
 			else {
 
-				ipipe_cpudom_var(this_domain, evsync) = 0;
 				ipipe_current_domain = next_domain;
 				ipipe_suspend_domain();	/* Sync stage and propagate interrupts. */
 
@@ -822,11 +819,9 @@ int fastcall __ipipe_dispatch_event (uns
 
 		if (evhand != NULL) {
 			ipipe_current_domain = next_domain;
-			ipipe_cpudom_var(next_domain, evsync) |= (1LL << event);
 			local_irq_restore_hw(flags);
 			propagate = !evhand(event, start_domain, data);
 			local_irq_save_hw(flags);
-			ipipe_cpudom_var(next_domain, evsync) &= ~(1LL << event);
 			if (ipipe_current_domain != next_domain)
 				this_domain = ipipe_current_domain;
 		}
@@ -1252,7 +1247,7 @@ ipipe_event_handler_t ipipe_catch_event(
 {
 	ipipe_event_handler_t old_handler;
 	unsigned long flags;
-	int self = 0, cpu;
+	int self = 0;
 
 	if (event & IPIPE_EVENT_SELF) {
 		event &= ~IPIPE_EVENT_SELF;
@@ -1293,20 +1288,31 @@ ipipe_event_handler_t ipipe_catch_event(
 		 * domain, we have to wait for any current invocation
 		 * to drain, since our caller might subsequently unmap
 		 * the target domain. To this aim, this code
-		 * synchronizes with __ipipe_dispatch_event(),
-		 * guaranteeing that either the dispatcher sees a null
-		 * handler in which case it discards the invocation
-		 * (which also prevents from entering a livelock), or
-		 * finds a valid handler and calls it. Symmetrically,
-		 * ipipe_catch_event() ensures that the called code
-		 * won't be unmapped under our feet until the event
-		 * synchronization flag is cleared for the given event
-		 * on all CPUs.
+		 * synchronizes on RCU sections to ensure that event
+		 * handlers protected by this mechanisms were all left.
+		 * Moreover, it is checked that there are no more tasks
+		 * in system which have the PF_EVNOTIFY flag set, i.e.
+		 * may still have the deregistered handler on their
+		 * stack.
 		 */
 
-		for_each_online_cpu(cpu) {
-			while (ipipe_percpudom(ipd, evsync, cpu) & (1LL << event))
-				schedule_timeout_interruptible(HZ / 50);
+		synchronize_rcu();
+
+		while (1) {
+			int evnotify_tasks = 0;
+			struct task_struct *p, *t;
+
+			read_lock(&tasklist_lock);
+			do_each_thread(p, t) {
+				if (t->flags & PF_EVNOTIFY)
+					evnotify_tasks++;
+			} while_each_thread(p, t);
+			read_unlock(&tasklist_lock);
+
+			if (!evnotify_tasks)
+				break;
+
+			schedule_timeout_interruptible(HZ / 50);
 		}
 	}
 
Index: linux-2.6.24-xeno_64/include/linux/ipipe.h
===================================================================
--- linux-2.6.24-xeno_64.orig/include/linux/ipipe.h
+++ linux-2.6.24-xeno_64/include/linux/ipipe.h
@@ -27,6 +27,7 @@
 #include <linux/percpu.h>
 #include <linux/mutex.h>
 #include <linux/linkage.h>
+#include <linux/rcupdate.h>
 #include <linux/ipipe_base.h>
 #include <linux/ipipe_compat.h>
 #include <asm/ipipe.h>
@@ -294,16 +295,22 @@ do {									\
 
 static inline void ipipe_init_notify(struct task_struct *p)
 {
-	if (__ipipe_event_monitored_p(IPIPE_EVENT_INIT))
+	if (__ipipe_event_monitored_p(IPIPE_EVENT_INIT)) {
+		rcu_read_lock();
 		__ipipe_dispatch_event(IPIPE_EVENT_INIT,p);
+		rcu_read_unlock();
+	}
 }
 
 struct mm_struct;
 
 static inline void ipipe_cleanup_notify(struct mm_struct *mm)
 {
-	if (__ipipe_event_monitored_p(IPIPE_EVENT_CLEANUP))
+	if (__ipipe_event_monitored_p(IPIPE_EVENT_CLEANUP)) {
+		rcu_read_lock();
 		__ipipe_dispatch_event(IPIPE_EVENT_CLEANUP,mm);
+		rcu_read_unlock();
+	}
 }
 
 /* Public interface */

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