On Tue, Sep 25, 2007 at 01:22:03PM -0400, Steven Rostedt wrote:
>
> --
> >
> > Passes light testing (five rounds of kernbench) on an x86_64 box.
> >
> > Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
> > ---
> >
> > include/linux/hardirq.h | 4 +++-
> > kernel/irq/handle.c | 2 ++
> > kernel/irq/manage.c | 25 +++++++++++++++++++++++++
> > 3 files changed, 30 insertions(+), 1 deletion(-)
> >
> > diff -urpNa -X dontdiff linux-2.6.23-rc4-rt1/include/linux/hardirq.h linux-2.6.23-rc4-rt1-sairq/include/linux/hardirq.h
> > --- linux-2.6.23-rc4-rt1/include/linux/hardirq.h 2007-09-20 17:34:52.000000000 -0700
> > +++ linux-2.6.23-rc4-rt1-sairq/include/linux/hardirq.h 2007-09-20 18:35:53.000000000 -0700
> > @@ -105,8 +105,10 @@
> >
> > #ifdef CONFIG_SMP
> > extern void synchronize_irq(unsigned int irq);
> > +extern void synchronize_all_irqs(void);
> > #else
> > -# define synchronize_irq(irq) barrier()
> > +# define synchronize_irq(irq) barrier()
> > +# define synchronize_all_irqs(irq) barrier()
> > #endif
> >
> > struct task_struct;
> > diff -urpNa -X dontdiff linux-2.6.23-rc4-rt1/kernel/irq/handle.c linux-2.6.23-rc4-rt1-sairq/kernel/irq/handle.c
> > --- linux-2.6.23-rc4-rt1/kernel/irq/handle.c 2007-09-20 17:34:51.000000000 -0700
> > +++ linux-2.6.23-rc4-rt1-sairq/kernel/irq/handle.c 2007-09-22 23:34:13.000000000 -0700
> > @@ -150,7 +150,9 @@ irqreturn_t handle_IRQ_event(unsigned in
> > do {
> > unsigned int preempt_count = preempt_count();
> >
> > + rcu_read_lock();
> > ret = action->handler(irq, action->dev_id);
> > + rcu_read_unlock();
> > if (preempt_count() != preempt_count) {
> > stop_trace();
> > print_symbol("BUG: unbalanced irq-handler preempt count in %s!\n", (unsigned long) action->handler);
> > diff -urpNa -X dontdiff linux-2.6.23-rc4-rt1/kernel/irq/manage.c linux-2.6.23-rc4-rt1-sairq/kernel/irq/manage.c
> > --- linux-2.6.23-rc4-rt1/kernel/irq/manage.c 2007-09-20 17:34:51.000000000 -0700
> > +++ linux-2.6.23-rc4-rt1-sairq/kernel/irq/manage.c 2007-09-22 23:34:15.000000000 -0700
> > @@ -45,6 +45,30 @@ void synchronize_irq(unsigned int irq)
> > EXPORT_SYMBOL(synchronize_irq);
> >
> > /**
> > + * synchronize_all_irqs - wait for all pending IRQ handlers (on other CPUs)
> > + *
> > + * This function waits for any pending IRQ handlers for this interrupt
> > + * to complete before returning. If you use this function while
> > + * holding a resource the IRQ handler may need you will deadlock.
> > + * If you use this function from an IRQ handler, you will immediately
> > + * self-deadlock.
> > + *
> > + * Note that this function waits for -handlers-, not for pending
> > + * interrupts, and most especially not for pending interrupts that
> > + * have not yet been delivered to the CPU. So if an interrupt
> > + * handler was just about to start executing when this function was
> > + * called, and if there are no other interrupt handlers executing,
> > + * this function is within its rights to return immediately.
> > + */
> > +void synchronize_all_irqs(void)
> > +{
> > + if (hardirq_preemption)
> > + synchronize_rcu(); /* wait for threaded irq handlers. */
> > + synchronize_sched(); /* wait for hardware irq handlers. */
>
> I don't undrestand the synchronize_sched part above. How does that wait
> for all hardware irq handlers? Wouldn't the synchronize_rcu be sufficient?
In practice, given the current implementations of RCU, agreed. However,
an RCU implementation would be within its rights to return immediately
from synchronize_rcu() if it could somehow deduce that there happened
to be no RCU read-side critical sections in progress at that moment.
This could leave hardware interrupt handlers still running on other CPUs.
In fact, the QRCU implementation (http://lkml.org/lkml/2007/2/25/18)
is an example RCU implementation that is capable of returning from
synchronize_qrcu() extremely quickly if there are no QRCU readers (a
few tens of nanoseconds on a recent x86-64 box). Hardware irq handlers
could easily be running for much longer (like if they take even a single
cache miss).
That said, having serialized delay that is almost never necessary is
not such a good thing. One thing I could do would be to open-code
synchronize_rcu() in synchronize_all_irqs(), so that the delays for RCU
and for synchronize_sched() happened in parallel. Something like:
void synchronize_all_irqs(void)
{
struct rcu_synchronize rcu;
if (hardirq_preemption) {
init_completion(&rcu.completion);
/* Will wake me after RCU finished */
call_rcu(&rcu.head, wakeme_after_rcu);
}
/* wait for hardware irq handlers. */
synchronize_sched();
/* wait for threaded irq handlers. */
if (hardirq_preemption)
wait_for_completion(&rcu.completion);
}
This would of course require that synchronize_all_irqs() be in the
RCU code rather than the irq code so that it could access the static
wakeme_after_rcu() definition and the rcu_synchronize structure.
Thoughts?
Thanx, Paul
> > +EXPORT_SYMBOL_GPL(synchronize_all_irqs);
> > +
> > +/**
> > * irq_can_set_affinity - Check if the affinity of a given irq can be set
> > * @irq: Interrupt to check
> > *
> > @@ -886,3 +910,4 @@ void __init early_init_hardirqs(void)
> > for (i = 0; i < NR_IRQS; i++)
> > init_waitqueue_head(&irq_desc[i].wait_for_handler);
> > }
> > +
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> >
> -
> To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
-
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
