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
---
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 */
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Adeos-main mailing list [email protected] https://mail.gna.org/listinfo/adeos-main
