[PATCH RFC -rt] synchronize_all_irqs implementation

[PATCH RFC -rt] synchronize_all_irqs implementation

[Paul E. McKenney]

Hello!

This patch adds a sychronize_all_irqs(), which waits for all outstanding
interrupt handlers, both threaded and IRQF_NODELAY, to complete.  This
functionality is provided in non-rt kernels by synchronize_sched(), but
this approach fails in face of the threaded interrupt handlers present
in -rt.  The trick sychronize_all_irqs() uses is to recognize that it
has no way of waiting for pending interrupts that have not yet made it
to the CPU, and that the existing synchronize_irq() will in fact fail
to wait for delivered interrupts that have not yet managed to set the
IRQ_INPROGRESS status flag.

This patch takes this thought one step farther, and guarantees only to
wait for interrupts that have already started executing in their handler.

One particular concern among many: should the rcu_read_lock() and
rcu_read_unlock() be pushed down closer to the interrupt handlers?
The do-while loop in thread_edge_irq() is a case in point.  Can this
do-while execute indefinitely in real systems?

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

 include/linux/hardirq.h |    4 +++-
 kernel/irq/manage.c     |   27 +++++++++++++++++++++++++++
 2 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/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-20 21:51:53.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. */
+}
+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
  *
@@ -750,6 +774,7 @@ static void do_hardirq(struct irq_desc *
 	if (!(desc->status & IRQ_INPROGRESS))
 		goto out;
 
+	rcu_read_lock();
 	if (desc->handle_irq == handle_simple_irq)
 		thread_simple_irq(desc);
 	else if (desc->handle_irq == handle_level_irq)
@@ -760,6 +785,7 @@ static void do_hardirq(struct irq_desc *
 		thread_edge_irq(desc);
 	else
 		thread_do_irq(desc);
+	rcu_read_unlock();
  out:
 	spin_unlock_irqrestore(&desc->lock, flags);
 
@@ -886,3 +912,4 @@ void __init early_init_hardirqs(void)
 	for (i = 0; i < NR_IRQS; i++)
 		init_waitqueue_head(&irq_desc[i].wait_for_handler);
 }
+

[PATCH RFC -rt] updated synchronize_all_irqs implementation

[Paul E. McKenney]

Hello!

This updated patch adds a sychronize_all_irqs(), which waits for
all outstanding interrupt handlers, both threaded and IRQF_NODELAY,
to complete.  This functionality is provided in non-rt kernels by
synchronize_sched(), but this approach fails in face of the threaded
interrupt handlers present in -rt.  The trick sychronize_all_irqs() uses
is to recognize that it has no way of waiting for pending interrupts that
have not yet made it to the CPU, and that the existing synchronize_irq()
will in fact fail to wait for delivered interrupts that have not yet
managed to set the IRQ_INPROGRESS status flag.

This patch takes this thought one step farther, and guarantees only to
wait for interrupts that have already started executing in their handler.

This patch pushes the rcu_read_lock() and rcu_read_unlock() doen into
handle_IRQ_event() to as to avoid problems with the do-while loop in
thread_adge_irq().

Passes light testing (five rounds of kernbench) on an x86_64 box.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---

 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. */
+}
+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);
 }
+

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Steven Rostedt]

--
>
> Passes light testing (five rounds of kernbench) on an x86_64 box.
>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>
>  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?

-- Steve

> +}
> +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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Paul E. McKenney]

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@linux.vnet.ibm.com>
> > ---
> >
> >  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@vger.kernel.org
> > 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Steven Rostedt]

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

I do like this better. Anyone else care to comment?

-- Steve

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Peter Zijlstra]

On Tue, 25 Sep 2007 16:02:45 -0400 (EDT) Steven Rostedt
<rostedt@goodmis.org> wrote:


> > 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?
> >
> 
> I do like this better. Anyone else care to comment?
> 

I'm still wondering why the IRQ users cannot user proper RCU as it
stands:

  rcu_read_lock();
  foo = rcu_dereference(bar);
  if (foo)
    foo();
  rcu_read_unlock();

vs

  rcu_assign(foo, NULL);
  synchronize_rcu();

and the like.

The implicit rcu_read_lock() as placed in handle_IRQ_event() seems
misplaced.

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Paul E. McKenney]

On Wed, Sep 26, 2007 at 01:24:47AM +0200, Peter Zijlstra wrote:
> On Tue, 25 Sep 2007 16:02:45 -0400 (EDT) Steven Rostedt
> <rostedt@goodmis.org> wrote:
> 
> > > 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?
> > 
> > I do like this better. Anyone else care to comment?
> 
> I'm still wondering why the IRQ users cannot user proper RCU as it
> stands:

Well, that was my initial proposal.  ;-)

>   rcu_read_lock();
>   foo = rcu_dereference(bar);
>   if (foo)
>     foo();
>   rcu_read_unlock();
> 
> vs
> 
>   rcu_assign(foo, NULL);
>   synchronize_rcu();

For this last, it would be necessary to use SRCU -- also, not sure
we would want the IRQ handler to block this way.  Or am I missing 
something?

> and the like.
> 
> The implicit rcu_read_lock() as placed in handle_IRQ_event() seems
> misplaced.

OK -- where would you put them instead?  I have them covering the
call to the handler, so what am I missing here?

							Thanx, Paul

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Peter Zijlstra]

On Tue, 25 Sep 2007 18:11:39 -0700 "Paul E. McKenney"
<paulmck@linux.vnet.ibm.com> wrote:

> On Wed, Sep 26, 2007 at 01:24:47AM +0200, Peter Zijlstra wrote:
> > On Tue, 25 Sep 2007 16:02:45 -0400 (EDT) Steven Rostedt
> > <rostedt@goodmis.org> wrote:
> > 
> > > > 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?
> > > 
> > > I do like this better. Anyone else care to comment?
> > 
> > I'm still wondering why the IRQ users cannot user proper RCU as it
> > stands:
> 
> Well, that was my initial proposal.  ;-)

handler: 
> >   rcu_read_lock();
> >   foo = rcu_dereference(bar);
> >   if (foo)
> >     foo();
> >   rcu_read_unlock();
>

control routine (!handler)
> > vs
> > 
> >   rcu_assign(foo, NULL);
> >   synchronize_rcu();

> > The implicit rcu_read_lock() as placed in handle_IRQ_event() seems
> > misplaced.
> 
> OK -- where would you put them instead?  I have them covering the
> call to the handler, so what am I missing here?

in do_hardirq() (-rt) that is specific to threaded interrupts.

That said, I'm wondering if we need this whole extra sync_all_irqs()
thing. I'm just not getting why IRQ handlers should be an implicit RCU
safe context.

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Paul E. McKenney]

On Wed, Sep 26, 2007 at 10:28:33AM +0200, Peter Zijlstra wrote:
> On Tue, 25 Sep 2007 18:11:39 -0700 "Paul E. McKenney"
> <paulmck@linux.vnet.ibm.com> wrote:
> 
> > On Wed, Sep 26, 2007 at 01:24:47AM +0200, Peter Zijlstra wrote:
> > > On Tue, 25 Sep 2007 16:02:45 -0400 (EDT) Steven Rostedt
> > > <rostedt@goodmis.org> wrote:
> > > 
> > > > > 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?
> > > > 
> > > > I do like this better. Anyone else care to comment?
> > > 
> > > I'm still wondering why the IRQ users cannot user proper RCU as it
> > > stands:
> > 
> > Well, that was my initial proposal.  ;-)
> 
> handler: 
> > >   rcu_read_lock();
> > >   foo = rcu_dereference(bar);
> > >   if (foo)
> > >     foo();
> > >   rcu_read_unlock();
> >
> 
> control routine (!handler)
> > > vs
> > > 
> > >   rcu_assign(foo, NULL);
> > >   synchronize_rcu();

Ah, OK -- yes, that was what I originally proposed -- that individual
handlers using RCU place the rcu_read_lock() and rcu_read_unlock() as
needed.

> > > The implicit rcu_read_lock() as placed in handle_IRQ_event() seems
> > > misplaced.
> > 
> > OK -- where would you put them instead?  I have them covering the
> > call to the handler, so what am I missing here?
> 
> in do_hardirq() (-rt) that is specific to threaded interrupts.

My concern there is that some of the functions called from do_hardirq()
can loop processing multiple interrupts.  An interrupt storm, otherwise
harmless in -rt, could cause a very long RCU read-side critical section
if it happened within thread_edge_irq().

> That said, I'm wondering if we need this whole extra sync_all_irqs()
> thing. I'm just not getting why IRQ handlers should be an implicit RCU
> safe context.

Because they are in non-rt -- synchronize_sched() is guaranteed to
wait for all interrupt handlers.  In contrast, in -rt, synchronize_sched()
only waits for hardirq.  So Dmitry Torokhov asked for a primitive
that would wait for all irq handlers, whether threaded or not.

But given that he has not responded to this thread, perhaps he
found that synchronize_irq() worked for him.

						Thanx, Paul

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Dmitry Torokhov]

Hi Paul,

On 9/26/07, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Sep 26, 2007 at 10:28:33AM +0200, Peter Zijlstra wrote:
> > On Tue, 25 Sep 2007 18:11:39 -0700 "Paul E. McKenney"
> > <paulmck@linux.vnet.ibm.com> wrote:
> >
> > > On Wed, Sep 26, 2007 at 01:24:47AM +0200, Peter Zijlstra wrote:
> > > > On Tue, 25 Sep 2007 16:02:45 -0400 (EDT) Steven Rostedt
> > > > <rostedt@goodmis.org> wrote:
> > > >
> > > > > > 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?
> > > > >
> > > > > I do like this better. Anyone else care to comment?
> > > >
> > > > I'm still wondering why the IRQ users cannot user proper RCU as it
> > > > stands:
> > >
> > > Well, that was my initial proposal.  ;-)
> >
> > handler:
> > > >   rcu_read_lock();
> > > >   foo = rcu_dereference(bar);
> > > >   if (foo)
> > > >     foo();
> > > >   rcu_read_unlock();
> > >
> >
> > control routine (!handler)
> > > > vs
> > > >
> > > >   rcu_assign(foo, NULL);
> > > >   synchronize_rcu();
>
> Ah, OK -- yes, that was what I originally proposed -- that individual
> handlers using RCU place the rcu_read_lock() and rcu_read_unlock() as
> needed.
>
> > > > The implicit rcu_read_lock() as placed in handle_IRQ_event() seems
> > > > misplaced.
> > >
> > > OK -- where would you put them instead?  I have them covering the
> > > call to the handler, so what am I missing here?
> >
> > in do_hardirq() (-rt) that is specific to threaded interrupts.
>
> My concern there is that some of the functions called from do_hardirq()
> can loop processing multiple interrupts.  An interrupt storm, otherwise
> harmless in -rt, could cause a very long RCU read-side critical section
> if it happened within thread_edge_irq().
>
> > That said, I'm wondering if we need this whole extra sync_all_irqs()
> > thing. I'm just not getting why IRQ handlers should be an implicit RCU
> > safe context.
>
> Because they are in non-rt -- synchronize_sched() is guaranteed to
> wait for all interrupt handlers.  In contrast, in -rt, synchronize_sched()
> only waits for hardirq.  So Dmitry Torokhov asked for a primitive
> that would wait for all irq handlers, whether threaded or not.
>

That is correct. IIRC synchronize_sched() was introduced to show that
it is not related (other than implementation-wise) to RCU mechanisms.

> But given that he has not responded to this thread, perhaps he
> found that synchronize_irq() worked for him.
>

Sorry, I am just being slow.

No, I don't think synchronize_irq() will work for me. While in i8042 I
know there are 2 possible IRQs (so I'd need 2 calls to
synchronize_irq()) other drivers may not know what IRQ triggered their
handler (or whether it was an IRQ at all).

Actually, I need clarifucation on what you mean by "interrupt
handlers" in sync_all_irqs(). Right now (if I understand it correctly)
synchronize_sched() in mainline will wait for completion of all
IRQ-like contexts. By IRQ-like context I mean code guardede by
spinlock + IRQ off. Serio (input) drivers have their "interrupt"
routines run in that IRQ-like context. They may be invoked as a result
of real IRQ being raised but they also be invoked as a result of
userspace action of some sort. It all depends on implementation of
underlying serio port. So if sync_all_irqs() only waits for real IRQ
handlers to complete it is not sufficient in my case...

Thanks.

-- 
Dmitry

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Paul E. McKenney]

On Wed, Sep 26, 2007 at 09:16:55AM -0400, Dmitry Torokhov wrote:
> Hi Paul,
> 
> On 9/26/07, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> > On Wed, Sep 26, 2007 at 10:28:33AM +0200, Peter Zijlstra wrote:
> > > On Tue, 25 Sep 2007 18:11:39 -0700 "Paul E. McKenney"
> > > <paulmck@linux.vnet.ibm.com> wrote:
> > >
> > > > On Wed, Sep 26, 2007 at 01:24:47AM +0200, Peter Zijlstra wrote:
> > > > > On Tue, 25 Sep 2007 16:02:45 -0400 (EDT) Steven Rostedt
> > > > > <rostedt@goodmis.org> wrote:
> > > > >
> > > > > > > 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?
> > > > > >
> > > > > > I do like this better. Anyone else care to comment?
> > > > >
> > > > > I'm still wondering why the IRQ users cannot user proper RCU as it
> > > > > stands:
> > > >
> > > > Well, that was my initial proposal.  ;-)
> > >
> > > handler:
> > > > >   rcu_read_lock();
> > > > >   foo = rcu_dereference(bar);
> > > > >   if (foo)
> > > > >     foo();
> > > > >   rcu_read_unlock();
> > > >
> > >
> > > control routine (!handler)
> > > > > vs
> > > > >
> > > > >   rcu_assign(foo, NULL);
> > > > >   synchronize_rcu();
> >
> > Ah, OK -- yes, that was what I originally proposed -- that individual
> > handlers using RCU place the rcu_read_lock() and rcu_read_unlock() as
> > needed.
> >
> > > > > The implicit rcu_read_lock() as placed in handle_IRQ_event() seems
> > > > > misplaced.
> > > >
> > > > OK -- where would you put them instead?  I have them covering the
> > > > call to the handler, so what am I missing here?
> > >
> > > in do_hardirq() (-rt) that is specific to threaded interrupts.
> >
> > My concern there is that some of the functions called from do_hardirq()
> > can loop processing multiple interrupts.  An interrupt storm, otherwise
> > harmless in -rt, could cause a very long RCU read-side critical section
> > if it happened within thread_edge_irq().
> >
> > > That said, I'm wondering if we need this whole extra sync_all_irqs()
> > > thing. I'm just not getting why IRQ handlers should be an implicit RCU
> > > safe context.
> >
> > Because they are in non-rt -- synchronize_sched() is guaranteed to
> > wait for all interrupt handlers.  In contrast, in -rt, synchronize_sched()
> > only waits for hardirq.  So Dmitry Torokhov asked for a primitive
> > that would wait for all irq handlers, whether threaded or not.
> 
> That is correct. IIRC synchronize_sched() was introduced to show that
> it is not related (other than implementation-wise) to RCU mechanisms.

Yep, it was split out a few years back.

> > But given that he has not responded to this thread, perhaps he
> > found that synchronize_irq() worked for him.
> 
> Sorry, I am just being slow.

No problem!

> No, I don't think synchronize_irq() will work for me. While in i8042 I
> know there are 2 possible IRQs (so I'd need 2 calls to
> synchronize_irq()) other drivers may not know what IRQ triggered their
> handler (or whether it was an IRQ at all).
> 
> Actually, I need clarifucation on what you mean by "interrupt
> handlers" in sync_all_irqs(). Right now (if I understand it correctly)
> synchronize_sched() in mainline will wait for completion of all
> IRQ-like contexts. By IRQ-like context I mean code guardede by
> spinlock + IRQ off. Serio (input) drivers have their "interrupt"
> routines run in that IRQ-like context. They may be invoked as a result
> of real IRQ being raised but they also be invoked as a result of
> userspace action of some sort. It all depends on implementation of
> underlying serio port. So if sync_all_irqs() only waits for real IRQ
> handlers to complete it is not sufficient in my case...

The synchronize_all_irqs() will not return until:

1.	All pre-existing hardirqs have completed.

2.	All pre-existing threaded irqs have completed.

3.	All preempt_disable() regions of code have completed.

4.	All irq-disable regions of code have completed.

It will not necessarily wait for all softirqs to complete, but
then again, synchronize_sched() in non-rt might not wait for all
softirqs either, for example, if ksoftirqd is handling softirqs.

Does that do what you need, or am I missing a case that needs
to be covered?

						Thanx, Paul

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Dmitry Torokhov]

On 9/26/07, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Sep 26, 2007 at 09:16:55AM -0400, Dmitry Torokhov wrote:
>
> > No, I don't think synchronize_irq() will work for me. While in i8042 I
> > know there are 2 possible IRQs (so I'd need 2 calls to
> > synchronize_irq()) other drivers may not know what IRQ triggered their
> > handler (or whether it was an IRQ at all).
> >
> > Actually, I need clarifucation on what you mean by "interrupt
> > handlers" in sync_all_irqs(). Right now (if I understand it correctly)
> > synchronize_sched() in mainline will wait for completion of all
> > IRQ-like contexts. By IRQ-like context I mean code guardede by
> > spinlock + IRQ off. Serio (input) drivers have their "interrupt"
> > routines run in that IRQ-like context. They may be invoked as a result
> > of real IRQ being raised but they also be invoked as a result of
> > userspace action of some sort. It all depends on implementation of
> > underlying serio port. So if sync_all_irqs() only waits for real IRQ
> > handlers to complete it is not sufficient in my case...
>
> The synchronize_all_irqs() will not return until:
>
> 1.      All pre-existing hardirqs have completed.
>
> 2.      All pre-existing threaded irqs have completed.
>
> 3.      All preempt_disable() regions of code have completed.
>
> 4.      All irq-disable regions of code have completed.
>
> It will not necessarily wait for all softirqs to complete, but
> then again, synchronize_sched() in non-rt might not wait for all
> softirqs either, for example, if ksoftirqd is handling softirqs.
>
> Does that do what you need, or am I missing a case that needs
> to be covered?

As long as the list includes code guarded by
spin_lock_irqsave()/spin_unlock_irqrestore() I am happy. Does this
count as an irq-disable region? Peter said earlier that in -rt such
code still runs with IRQs enabled...

-- 
Dmitry

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Paul E. McKenney]

On Wed, Sep 26, 2007 at 01:19:05PM -0400, Dmitry Torokhov wrote:
> On 9/26/07, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> > On Wed, Sep 26, 2007 at 09:16:55AM -0400, Dmitry Torokhov wrote:
> >
> > > No, I don't think synchronize_irq() will work for me. While in i8042 I
> > > know there are 2 possible IRQs (so I'd need 2 calls to
> > > synchronize_irq()) other drivers may not know what IRQ triggered their
> > > handler (or whether it was an IRQ at all).
> > >
> > > Actually, I need clarifucation on what you mean by "interrupt
> > > handlers" in sync_all_irqs(). Right now (if I understand it correctly)
> > > synchronize_sched() in mainline will wait for completion of all
> > > IRQ-like contexts. By IRQ-like context I mean code guardede by
> > > spinlock + IRQ off. Serio (input) drivers have their "interrupt"
> > > routines run in that IRQ-like context. They may be invoked as a result
> > > of real IRQ being raised but they also be invoked as a result of
> > > userspace action of some sort. It all depends on implementation of
> > > underlying serio port. So if sync_all_irqs() only waits for real IRQ
> > > handlers to complete it is not sufficient in my case...
> >
> > The synchronize_all_irqs() will not return until:
> >
> > 1.      All pre-existing hardirqs have completed.
> >
> > 2.      All pre-existing threaded irqs have completed.
> >
> > 3.      All preempt_disable() regions of code have completed.
> >
> > 4.      All irq-disable regions of code have completed.
> >
> > It will not necessarily wait for all softirqs to complete, but
> > then again, synchronize_sched() in non-rt might not wait for all
> > softirqs either, for example, if ksoftirqd is handling softirqs.
> >
> > Does that do what you need, or am I missing a case that needs
> > to be covered?
> 
> As long as the list includes code guarded by
> spin_lock_irqsave()/spin_unlock_irqrestore() I am happy. Does this
> count as an irq-disable region? Peter said earlier that in -rt such
> code still runs with IRQs enabled...

If I remember correctly, it depends on whether the underlying spinlock
is spinlock_t or raw_spinlock_t.

However, doesn't spin_lock_irqsave() disable preemption in both cases
(either explicitly for the spinlock_t case or implicitly via irq
disable in the raw_spinlock_t case)?  If so, synchronize_all_irqs()
should do what you want in both cases.

						Thanx, Paul

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Steven Rostedt]

--

On Wed, 26 Sep 2007, Paul E. McKenney wrote:
> >
> > As long as the list includes code guarded by
> > spin_lock_irqsave()/spin_unlock_irqrestore() I am happy. Does this
> > count as an irq-disable region? Peter said earlier that in -rt such
> > code still runs with IRQs enabled...
>
> If I remember correctly, it depends on whether the underlying spinlock
> is spinlock_t or raw_spinlock_t.
>
> However, doesn't spin_lock_irqsave() disable preemption in both cases
> (either explicitly for the spinlock_t case or implicitly via irq
> disable in the raw_spinlock_t case)?  If so, synchronize_all_irqs()
> should do what you want in both cases.

Nope not at all. In RT spin_lock_irqsave does not disable preemption.
That's part of the "features" of RT. And also why we want everyone to use
spin_lock_irqsave instead of local_irq_save. Because the later does
prevent preemption and adds latencies.

-- Steve

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Steven Rostedt]

--

On Wed, 26 Sep 2007, Dmitry Torokhov wrote:

> >
> > The synchronize_all_irqs() will not return until:
> >
> > 1.      All pre-existing hardirqs have completed.
> >
> > 2.      All pre-existing threaded irqs have completed.
> >
> > 3.      All preempt_disable() regions of code have completed.
> >
> > 4.      All irq-disable regions of code have completed.
> >
> > It will not necessarily wait for all softirqs to complete, but
> > then again, synchronize_sched() in non-rt might not wait for all
> > softirqs either, for example, if ksoftirqd is handling softirqs.
> >
> > Does that do what you need, or am I missing a case that needs
> > to be covered?
>
> As long as the list includes code guarded by
> spin_lock_irqsave()/spin_unlock_irqrestore() I am happy. Does this
> count as an irq-disable region? Peter said earlier that in -rt such
> code still runs with IRQs enabled...

No it does not guarantee that in -rt.  Peter is correct that in the -rt
patch, spin_lock_irqsave and restore do not touch actual interrupts (when
PREEMPT_RT is configured).  And the above protections that Paul has shown
also does not guarantee that all spin-locks will be released.

spin_locks in -rt become mutexes, and if some low priority process grabs
some spin_lock somewhere that no other process cares about, and then this
process gets preempted by a bunch of RT hogs, that lock could be held for
an awfully long time.

Paul, has there been any work to guarantee such a thing?

-- Steve

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Paul E. McKenney]

On Wed, Sep 26, 2007 at 01:44:22PM -0400, Steven Rostedt wrote:
> --
> On Wed, 26 Sep 2007, Dmitry Torokhov wrote:
> > > The synchronize_all_irqs() will not return until:
> > >
> > > 1.      All pre-existing hardirqs have completed.
> > >
> > > 2.      All pre-existing threaded irqs have completed.
> > >
> > > 3.      All preempt_disable() regions of code have completed.
> > >
> > > 4.      All irq-disable regions of code have completed.
> > >
> > > It will not necessarily wait for all softirqs to complete, but
> > > then again, synchronize_sched() in non-rt might not wait for all
> > > softirqs either, for example, if ksoftirqd is handling softirqs.
> > >
> > > Does that do what you need, or am I missing a case that needs
> > > to be covered?
> >
> > As long as the list includes code guarded by
> > spin_lock_irqsave()/spin_unlock_irqrestore() I am happy. Does this
> > count as an irq-disable region? Peter said earlier that in -rt such
> > code still runs with IRQs enabled...
> 
> No it does not guarantee that in -rt.  Peter is correct that in the -rt
> patch, spin_lock_irqsave and restore do not touch actual interrupts (when
> PREEMPT_RT is configured).  And the above protections that Paul has shown
> also does not guarantee that all spin-locks will be released.
> 
> spin_locks in -rt become mutexes, and if some low priority process grabs
> some spin_lock somewhere that no other process cares about, and then this
> process gets preempted by a bunch of RT hogs, that lock could be held for
> an awfully long time.
> 
> Paul, has there been any work to guarantee such a thing?

Well, we could make spin_lock_irqsave() invoke rcu_read_lock() and
spin_lock_irqrestore() invoke rcu_read_unlock(), with similar adjustments
to the other primitives in this group.  Then RCU priority boosting would
kick in if configured.

Of course, someone who is not quite so closely linked to RCU might be
able to come up with some other solution.  ;-)

						Thanx, Paul

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Peter Zijlstra]

On Wed, 2007-09-26 at 12:55 -0700, Paul E. McKenney wrote:

> Well, we could make spin_lock_irqsave() invoke rcu_read_lock() and
> spin_lock_irqrestore() invoke rcu_read_unlock(), with similar adjustments
> to the other primitives in this group.  Then RCU priority boosting would
> kick in if configured.

Might be me, but 'hiding' the RCU scope like that makes my skin crawl.
What is wrong with using rcu_read_lock() explicitly where you depend on
it? It even makes the code cleaner in that it documents the usage.

These blanket locks like lock_kernel(), preempt_disable(),
local_irq_disable() etc. are very hard to get rid of because they don't
document what is protected.

Please lets not add to this mess and keep all this explicit.

Yes, -rt changes the preemption model, and that has concequences. But
IMHO the resulting code is cleaner in most cases.

I'd go as far as to depricate synchronize_sched() and replace all its
users with proper RCU.

The more I think of it, the more I dislike this synchronize_all_irqs()
and the 'magic' property of irq handlers.

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Steven Rostedt]

--
On Wed, 26 Sep 2007, Peter Zijlstra wrote:
>
> On Wed, 2007-09-26 at 12:55 -0700, Paul E. McKenney wrote:
>
> > Well, we could make spin_lock_irqsave() invoke rcu_read_lock() and
> > spin_lock_irqrestore() invoke rcu_read_unlock(), with similar adjustments
> > to the other primitives in this group.  Then RCU priority boosting would
> > kick in if configured.
>
> Might be me, but 'hiding' the RCU scope like that makes my skin crawl.
> What is wrong with using rcu_read_lock() explicitly where you depend on
> it? It even makes the code cleaner in that it documents the usage.
>
> These blanket locks like lock_kernel(), preempt_disable(),
> local_irq_disable() etc. are very hard to get rid of because they don't
> document what is protected.
>
> Please lets not add to this mess and keep all this explicit.
>
> Yes, -rt changes the preemption model, and that has concequences. But
> IMHO the resulting code is cleaner in most cases.
>
> I'd go as far as to depricate synchronize_sched() and replace all its
> users with proper RCU.
>
> The more I think of it, the more I dislike this synchronize_all_irqs()
> and the 'magic' property of irq handlers.


Thinking a little more about this, I fully agree with Peter.

What code needs to know that all spin_locks have released, or you want to
know all irq handlers are done?

Seems that this is a broad data to ask for and will be a bitch to clean up
when we find out that this is not scalable. This all sounds like
reinventing the BKL.

If you simply want a RCU type of scheme, then simply use RCU.  I second
deprecating "synchronize_sched". Everything that does RCU type locking
(that is waiting for some kind of grace period to finish) should simply
use RCU and not a "wait for all processes to voluntarily schedule" or
"wait for all interrupt handlers to finish".

-- Steve

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Paul E. McKenney]

On Wed, Sep 26, 2007 at 05:07:33PM -0400, Steven Rostedt wrote:
> 
> --
> On Wed, 26 Sep 2007, Peter Zijlstra wrote:
> >
> > On Wed, 2007-09-26 at 12:55 -0700, Paul E. McKenney wrote:
> >
> > > Well, we could make spin_lock_irqsave() invoke rcu_read_lock() and
> > > spin_lock_irqrestore() invoke rcu_read_unlock(), with similar adjustments
> > > to the other primitives in this group.  Then RCU priority boosting would
> > > kick in if configured.
> >
> > Might be me, but 'hiding' the RCU scope like that makes my skin crawl.
> > What is wrong with using rcu_read_lock() explicitly where you depend on
> > it? It even makes the code cleaner in that it documents the usage.
> >
> > These blanket locks like lock_kernel(), preempt_disable(),
> > local_irq_disable() etc. are very hard to get rid of because they don't
> > document what is protected.
> >
> > Please lets not add to this mess and keep all this explicit.
> >
> > Yes, -rt changes the preemption model, and that has concequences. But
> > IMHO the resulting code is cleaner in most cases.
> >
> > I'd go as far as to depricate synchronize_sched() and replace all its
> > users with proper RCU.
> >
> > The more I think of it, the more I dislike this synchronize_all_irqs()
> > and the 'magic' property of irq handlers.
> 
> Thinking a little more about this, I fully agree with Peter.

Again, it is not me that you need to convince.  ;-)

> What code needs to know that all spin_locks have released, or you want to
> know all irq handlers are done?
> 
> Seems that this is a broad data to ask for and will be a bitch to clean up
> when we find out that this is not scalable. This all sounds like
> reinventing the BKL.

It would scale just fine, and inherently does not need priority boosting
(since preempted/blocked things cannot hold up sychronize_sched()).
That said, I do agree with you that having things explicit is a good
thing.

> If you simply want a RCU type of scheme, then simply use RCU.  I second
> deprecating "synchronize_sched". Everything that does RCU type locking
> (that is waiting for some kind of grace period to finish) should simply
> use RCU and not a "wait for all processes to voluntarily schedule" or
> "wait for all interrupt handlers to finish".

Here are the uses in 2.6.23-rc6.  There are several that might not
be so easy to replace with RCU.

						Thanx, Paul

arch/i386/oprofile/nmi_timer_int.c timer_stop 55 synchronize_sched();
arch/x86_64/kernel/mce.c mce_read 607 synchronize_sched();
drivers/acpi/processor_idle.c acpi_processor_cst_has_changed 1132 synchronize_sched();
drivers/char/ipmi/ipmi_si_intf.c try_smi_init 2879 synchronize_sched();
drivers/char/sonypi.c sonypi_remove 1435 synchronize_sched();
drivers/input/keyboard/atkbd.c atkbd_disconnect 827 synchronize_sched();
drivers/input/serio/i8042.c i8042_stop 281 synchronize_sched();
drivers/net/r8169.c rtl8169_down 2866 synchronize_sched();
drivers/net/sis190.c sis190_down 1123 synchronize_sched();
drivers/s390/cio/airq.c s390_register_adapter_interrupt 46 synchronize_sched();
drivers/s390/cio/airq.c s390_unregister_adapter_interrupt 66 synchronize_sched();
kernel/kprobes.c check_safety 127 synchronize_sched();
kernel/kprobes.c unregister_kprobe 632 synchronize_sched();
kernel/kprobes.c disable_all_kprobes 970 synchronize_sched();
kernel/module.c sys_init_module 2013 synchronize_sched();
kernel/profile.c sys_init_module 195 synchronize_sched();
kernel/rcutorture.c sched_torture_synchronize 490 synchronize_sched();
kernel/sched.c detach_destroy_domains 6329 synchronize_sched();
kernel/srcu.c synchronize_srcu 176 synchronize_sched();
kernel/srcu.c synchronize_srcu 193 synchronize_sched();
kernel/srcu.c synchronize_srcu 206 synchronize_sched();

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Dmitry Torokhov]

On 9/26/07, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Sep 26, 2007 at 05:07:33PM -0400, Steven Rostedt wrote:
> >
> > --
> > On Wed, 26 Sep 2007, Peter Zijlstra wrote:
> > >
> > > On Wed, 2007-09-26 at 12:55 -0700, Paul E. McKenney wrote:
> > >
> > > > Well, we could make spin_lock_irqsave() invoke rcu_read_lock() and
> > > > spin_lock_irqrestore() invoke rcu_read_unlock(), with similar adjustments
> > > > to the other primitives in this group.  Then RCU priority boosting would
> > > > kick in if configured.
> > >
> > > Might be me, but 'hiding' the RCU scope like that makes my skin crawl.
> > > What is wrong with using rcu_read_lock() explicitly where you depend on
> > > it? It even makes the code cleaner in that it documents the usage.
> > >
> > > These blanket locks like lock_kernel(), preempt_disable(),
> > > local_irq_disable() etc. are very hard to get rid of because they don't
> > > document what is protected.
> > >
> > > Please lets not add to this mess and keep all this explicit.
> > >
> > > Yes, -rt changes the preemption model, and that has concequences. But
> > > IMHO the resulting code is cleaner in most cases.
> > >
> > > I'd go as far as to depricate synchronize_sched() and replace all its
> > > users with proper RCU.
> > >
> > > The more I think of it, the more I dislike this synchronize_all_irqs()
> > > and the 'magic' property of irq handlers.
> >
> > Thinking a little more about this, I fully agree with Peter.
>
> Again, it is not me that you need to convince.  ;-)
>
> > What code needs to know that all spin_locks have released, or you want to
> > know all irq handlers are done?
> >
> > Seems that this is a broad data to ask for and will be a bitch to clean up
> > when we find out that this is not scalable. This all sounds like
> > reinventing the BKL.
>
> It would scale just fine, and inherently does not need priority boosting
> (since preempted/blocked things cannot hold up sychronize_sched()).
> That said, I do agree with you that having things explicit is a good
> thing.
>
> > If you simply want a RCU type of scheme, then simply use RCU.  I second
> > deprecating "synchronize_sched". Everything that does RCU type locking
> > (that is waiting for some kind of grace period to finish) should simply
> > use RCU and not a "wait for all processes to voluntarily schedule" or
> > "wait for all interrupt handlers to finish".
>
> Here are the uses in 2.6.23-rc6.  There are several that might not
> be so easy to replace with RCU.
>

I will look into converting into "proper RCU" but I'd like you to
consider one more thing - proper RCU disables preemption, correct? So
it seems that converting code to "proper RCU" to some degree negates
the benefits of sleeping spinlocks introduced in -rt. If there was a
way to create synchronize_irq_spinlocks() that is separate from RCU
and could be preempted that would give the best of the 2 worlds.

-- 
Dmitry

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Steven Rostedt]

--
On Thu, 27 Sep 2007, Dmitry Torokhov wrote:
> >
> > Here are the uses in 2.6.23-rc6.  There are several that might not
> > be so easy to replace with RCU.
> >
>
> I will look into converting into "proper RCU" but I'd like you to
> consider one more thing - proper RCU disables preemption, correct? So
> it seems that converting code to "proper RCU" to some degree negates
> the benefits of sleeping spinlocks introduced in -rt. If there was a
> way to create synchronize_irq_spinlocks() that is separate from RCU
> and could be preempted that would give the best of the 2 worlds.
>

Not at all. In -rt, we then use preemptible RCU. So read_rcu_lock does not
disable preemption in the -rt kernel (and hopefully soon in mainline as
well - configured not to that is).

-- Steve

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Dmitry Torokhov]

On 9/27/07, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> --
> On Thu, 27 Sep 2007, Dmitry Torokhov wrote:
> > >
> > > Here are the uses in 2.6.23-rc6.  There are several that might not
> > > be so easy to replace with RCU.
> > >
> >
> > I will look into converting into "proper RCU" but I'd like you to
> > consider one more thing - proper RCU disables preemption, correct? So
> > it seems that converting code to "proper RCU" to some degree negates
> > the benefits of sleeping spinlocks introduced in -rt. If there was a
> > way to create synchronize_irq_spinlocks() that is separate from RCU
> > and could be preempted that would give the best of the 2 worlds.
> >
>
> Not at all. In -rt, we then use preemptible RCU. So read_rcu_lock does not
> disable preemption in the -rt kernel (and hopefully soon in mainline as
> well - configured not to that is).
>

Ah, OK then.

-- 
Dmitry

Re: [PATCH RFC -rt] updated synchronize_all_irqs implementation

[Paul E. McKenney]

On Wed, Sep 26, 2007 at 10:54:46PM +0200, Peter Zijlstra wrote:
> 
> On Wed, 2007-09-26 at 12:55 -0700, Paul E. McKenney wrote:
> 
> > Well, we could make spin_lock_irqsave() invoke rcu_read_lock() and
> > spin_lock_irqrestore() invoke rcu_read_unlock(), with similar adjustments
> > to the other primitives in this group.  Then RCU priority boosting would
> > kick in if configured.
> 
> Might be me, but 'hiding' the RCU scope like that makes my skin crawl.
> What is wrong with using rcu_read_lock() explicitly where you depend on
> it? It even makes the code cleaner in that it documents the usage.
> 
> These blanket locks like lock_kernel(), preempt_disable(),
> local_irq_disable() etc. are very hard to get rid of because they don't
> document what is protected.
> 
> Please lets not add to this mess and keep all this explicit.
> 
> Yes, -rt changes the preemption model, and that has concequences. But
> IMHO the resulting code is cleaner in most cases.

I am not going to argue with you on this one!!!

But it is not me that needs convincing.

> I'd go as far as to depricate synchronize_sched() and replace all its
> users with proper RCU.

But this might be going too far, though you might well be able to
convince me.  Reducing RCU API proliferation would be a good thing
in general...

> The more I think of it, the more I dislike this synchronize_all_irqs()
> and the 'magic' property of irq handlers.

So I produced a 'magic' patch?  Cool!!!  ;-)

Seriously, I agree that using explicit rcu_read_lock() where needed would
be much better from a readability and robustness viewpoint.  However,
if the goal is to support the existing code unmodified, then this patch
is one solution.  I would be just as happy to see it become unnecessary
as to see it go in, as long as the problem is solved one way or another.

							Thanx, Paul