[PATCH] rcu: Remove redundant rcu_is_cpu_rrupt_from_idle() function

[PATCH] rcu: Remove redundant rcu_is_cpu_rrupt_from_idle() function

[Alexander Gordeev]

Function rcu_is_cpu_rrupt_from_idle() is called from scheduling-
clock interrupt handler to check if the current CPU was interrupted
from idle. If true, it results in invocation of RCU callbacks. But
the common hardware interrupt exit path also contains similar check
and therefore the call to rcu_is_cpu_rrupt_from_idle() is redundant.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/rcu/tiny.c | 12 +-----------
 kernel/rcu/tree.c | 14 +-------------
 2 files changed, 2 insertions(+), 24 deletions(-)

diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c
index c0623fc..0bfb3b9 100644
--- a/kernel/rcu/tiny.c
+++ b/kernel/rcu/tiny.c
@@ -186,16 +186,6 @@ EXPORT_SYMBOL(__rcu_is_watching);
 #endif /* defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_RCU_TRACE) */
 
 /*
- * Test whether the current CPU was interrupted from idle.  Nested
- * interrupts don't count, we must be running at the first interrupt
- * level.
- */
-static int rcu_is_cpu_rrupt_from_idle(void)
-{
-	return rcu_dynticks_nesting <= 1;
-}
-
-/*
  * Helper function for rcu_sched_qs() and rcu_bh_qs().
  * Also irqs are disabled to avoid confusion due to interrupt handlers
  * invoking call_rcu().
@@ -250,7 +240,7 @@ void rcu_bh_qs(void)
 void rcu_check_callbacks(int cpu, int user)
 {
 	RCU_TRACE(check_cpu_stalls());
-	if (user || rcu_is_cpu_rrupt_from_idle())
+	if (user)
 		rcu_sched_qs();
 	else if (!in_softirq())
 		rcu_bh_qs();
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 133e472..8e9341c 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -884,18 +884,6 @@ EXPORT_SYMBOL_GPL(rcu_lockdep_current_cpu_online);
 
 #endif /* #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_HOTPLUG_CPU) */
 
-/**
- * rcu_is_cpu_rrupt_from_idle - see if idle or immediately interrupted from idle
- *
- * If the current CPU is idle or running at a first-level (not nested)
- * interrupt from idle, return true.  The caller must have at least
- * disabled preemption.
- */
-static int rcu_is_cpu_rrupt_from_idle(void)
-{
-	return __this_cpu_read(rcu_dynticks.dynticks_nesting) <= 1;
-}
-
 /*
  * Snapshot the specified CPU's dynticks counter so that we can later
  * credit them with an implicit quiescent state.  Return 1 if this CPU
@@ -2391,7 +2379,7 @@ void rcu_check_callbacks(int cpu, int user)
 {
 	trace_rcu_utilization(TPS("Start scheduler-tick"));
 	increment_cpu_stall_ticks();
-	if (user || rcu_is_cpu_rrupt_from_idle()) {
+	if (user) {
 
 		/*
 		 * Get here if this CPU took its interrupt from user
-- 
1.8.3.1

Re: [PATCH] rcu: Remove redundant rcu_is_cpu_rrupt_from_idle() function

[Paul E. McKenney]

On Wed, Oct 29, 2014 at 12:07:07PM +0100, Alexander Gordeev wrote:
> Function rcu_is_cpu_rrupt_from_idle() is called from scheduling-
> clock interrupt handler to check if the current CPU was interrupted
> from idle. If true, it results in invocation of RCU callbacks. But
> the common hardware interrupt exit path also contains similar check
> and therefore the call to rcu_is_cpu_rrupt_from_idle() is redundant.

By common hardware interrupt exit path, you are meaning the calls
to rcu_irq_exit()?  If not, please let me know exactly what you
mean here.

							Thanx, Paul

> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>  kernel/rcu/tiny.c | 12 +-----------
>  kernel/rcu/tree.c | 14 +-------------
>  2 files changed, 2 insertions(+), 24 deletions(-)
> 
> diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c
> index c0623fc..0bfb3b9 100644
> --- a/kernel/rcu/tiny.c
> +++ b/kernel/rcu/tiny.c
> @@ -186,16 +186,6 @@ EXPORT_SYMBOL(__rcu_is_watching);
>  #endif /* defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_RCU_TRACE) */
> 
>  /*
> - * Test whether the current CPU was interrupted from idle.  Nested
> - * interrupts don't count, we must be running at the first interrupt
> - * level.
> - */
> -static int rcu_is_cpu_rrupt_from_idle(void)
> -{
> -	return rcu_dynticks_nesting <= 1;
> -}
> -
> -/*
>   * Helper function for rcu_sched_qs() and rcu_bh_qs().
>   * Also irqs are disabled to avoid confusion due to interrupt handlers
>   * invoking call_rcu().
> @@ -250,7 +240,7 @@ void rcu_bh_qs(void)
>  void rcu_check_callbacks(int cpu, int user)
>  {
>  	RCU_TRACE(check_cpu_stalls());
> -	if (user || rcu_is_cpu_rrupt_from_idle())
> +	if (user)
>  		rcu_sched_qs();
>  	else if (!in_softirq())
>  		rcu_bh_qs();
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 133e472..8e9341c 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -884,18 +884,6 @@ EXPORT_SYMBOL_GPL(rcu_lockdep_current_cpu_online);
> 
>  #endif /* #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_HOTPLUG_CPU) */
> 
> -/**
> - * rcu_is_cpu_rrupt_from_idle - see if idle or immediately interrupted from idle
> - *
> - * If the current CPU is idle or running at a first-level (not nested)
> - * interrupt from idle, return true.  The caller must have at least
> - * disabled preemption.
> - */
> -static int rcu_is_cpu_rrupt_from_idle(void)
> -{
> -	return __this_cpu_read(rcu_dynticks.dynticks_nesting) <= 1;
> -}
> -
>  /*
>   * Snapshot the specified CPU's dynticks counter so that we can later
>   * credit them with an implicit quiescent state.  Return 1 if this CPU
> @@ -2391,7 +2379,7 @@ void rcu_check_callbacks(int cpu, int user)
>  {
>  	trace_rcu_utilization(TPS("Start scheduler-tick"));
>  	increment_cpu_stall_ticks();
> -	if (user || rcu_is_cpu_rrupt_from_idle()) {
> +	if (user) {
> 
>  		/*
>  		 * Get here if this CPU took its interrupt from user
> -- 
> 1.8.3.1
> 

Re: [PATCH] rcu: Remove redundant rcu_is_cpu_rrupt_from_idle() function

[Alexander Gordeev]

On Wed, Oct 29, 2014 at 05:47:29AM -0700, Paul E. McKenney wrote:
> On Wed, Oct 29, 2014 at 12:07:07PM +0100, Alexander Gordeev wrote:
> > Function rcu_is_cpu_rrupt_from_idle() is called from scheduling-
> > clock interrupt handler to check if the current CPU was interrupted
> > from idle. If true, it results in invocation of RCU callbacks. But
> > the common hardware interrupt exit path also contains similar check
> > and therefore the call to rcu_is_cpu_rrupt_from_idle() is redundant.
> 
> By common hardware interrupt exit path, you are meaning the calls
> to rcu_irq_exit()?  If not, please let me know exactly what you
> mean here.

Yes, I mean rcu_irq_exit().

> 							Thanx, Paul

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH] rcu: Remove redundant rcu_is_cpu_rrupt_from_idle() function

[Paul E. McKenney]

On Wed, Oct 29, 2014 at 01:45:19PM +0000, Alexander Gordeev wrote:
> On Wed, Oct 29, 2014 at 05:47:29AM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 29, 2014 at 12:07:07PM +0100, Alexander Gordeev wrote:
> > > Function rcu_is_cpu_rrupt_from_idle() is called from scheduling-
> > > clock interrupt handler to check if the current CPU was interrupted
> > > from idle. If true, it results in invocation of RCU callbacks. But
> > > the common hardware interrupt exit path also contains similar check
> > > and therefore the call to rcu_is_cpu_rrupt_from_idle() is redundant.
> > 
> > By common hardware interrupt exit path, you are meaning the calls
> > to rcu_irq_exit()?  If not, please let me know exactly what you
> > mean here.
> 
> Yes, I mean rcu_irq_exit().

Unless you can get the indication of whether or not the original interrupt
came from userspace execution into rcu_irq_exit(), this will not work.
It will result in grace-period hangs on some configurations.

Now, if you -can- get the userspace-execution indication into
rcu_irq_exit(), this might be of interest.  However, it might be faster
to simply let the scheduling-clock interrupt do the job as it currently
does, especially for workloads with lots of interrupts.

Or did you have something else in mind?

							Thanx, Paul

Re: [PATCH] rcu: Remove redundant rcu_is_cpu_rrupt_from_idle() function

[Alexander Gordeev]

On Wed, Oct 29, 2014 at 07:34:39AM -0700, Paul E. McKenney wrote:
> On Wed, Oct 29, 2014 at 01:45:19PM +0000, Alexander Gordeev wrote:
> > On Wed, Oct 29, 2014 at 05:47:29AM -0700, Paul E. McKenney wrote:
> > > On Wed, Oct 29, 2014 at 12:07:07PM +0100, Alexander Gordeev wrote:
> > > > Function rcu_is_cpu_rrupt_from_idle() is called from scheduling-
> > > > clock interrupt handler to check if the current CPU was interrupted
> > > > from idle. If true, it results in invocation of RCU callbacks. But
> > > > the common hardware interrupt exit path also contains similar check
> > > > and therefore the call to rcu_is_cpu_rrupt_from_idle() is redundant.
> > > 
> > > By common hardware interrupt exit path, you are meaning the calls
> > > to rcu_irq_exit()?  If not, please let me know exactly what you
> > > mean here.
> > 
> > Yes, I mean rcu_irq_exit().
> 
> Unless you can get the indication of whether or not the original interrupt
> came from userspace execution into rcu_irq_exit(), this will not work.
> It will result in grace-period hangs on some configurations.

Okay, that was my concern wrt tree RCU. By contrast, tiny RCU does not seem
able to hang a grace-period, isn't it?

> Now, if you -can- get the userspace-execution indication into
> rcu_irq_exit(), this might be of interest.  However, it might be faster
> to simply let the scheduling-clock interrupt do the job as it currently
> does, especially for workloads with lots of interrupts.
> 
> Or did you have something else in mind?

Nope. I would even leave as is tiny RCU's rcu_is_cpu_rrupt_from_idle()
for clarity then ;)

> 							Thanx, Paul

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH] rcu: Remove redundant rcu_is_cpu_rrupt_from_idle() function

[Paul E. McKenney]

On Wed, Oct 29, 2014 at 03:16:23PM +0000, Alexander Gordeev wrote:
> On Wed, Oct 29, 2014 at 07:34:39AM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 29, 2014 at 01:45:19PM +0000, Alexander Gordeev wrote:
> > > On Wed, Oct 29, 2014 at 05:47:29AM -0700, Paul E. McKenney wrote:
> > > > On Wed, Oct 29, 2014 at 12:07:07PM +0100, Alexander Gordeev wrote:
> > > > > Function rcu_is_cpu_rrupt_from_idle() is called from scheduling-
> > > > > clock interrupt handler to check if the current CPU was interrupted
> > > > > from idle. If true, it results in invocation of RCU callbacks. But
> > > > > the common hardware interrupt exit path also contains similar check
> > > > > and therefore the call to rcu_is_cpu_rrupt_from_idle() is redundant.
> > > > 
> > > > By common hardware interrupt exit path, you are meaning the calls
> > > > to rcu_irq_exit()?  If not, please let me know exactly what you
> > > > mean here.
> > > 
> > > Yes, I mean rcu_irq_exit().
> > 
> > Unless you can get the indication of whether or not the original interrupt
> > came from userspace execution into rcu_irq_exit(), this will not work.
> > It will result in grace-period hangs on some configurations.
> 
> Okay, that was my concern wrt tree RCU. By contrast, tiny RCU does not seem
> able to hang a grace-period, isn't it?

Although it is true that tiny RCU cannot hang a synchronize_rcu()
grace period, it most certainly can hang a call_rcu() grace period
in exactly the same way.

> > Now, if you -can- get the userspace-execution indication into
> > rcu_irq_exit(), this might be of interest.  However, it might be faster
> > to simply let the scheduling-clock interrupt do the job as it currently
> > does, especially for workloads with lots of interrupts.
> > 
> > Or did you have something else in mind?
> 
> Nope. I would even leave as is tiny RCU's rcu_is_cpu_rrupt_from_idle()
> for clarity then ;)

Also to avoid userspace execution from preventing RCU callbacks from
ever being invoked.  ;-)

							Thanx, Paul

Re: [PATCH] rcu: Remove redundant rcu_is_cpu_rrupt_from_idle() function

[Alexander Gordeev]

On Wed, Oct 29, 2014 at 08:46:15AM -0700, Paul E. McKenney wrote:
> On Wed, Oct 29, 2014 at 03:16:23PM +0000, Alexander Gordeev wrote:
> > On Wed, Oct 29, 2014 at 07:34:39AM -0700, Paul E. McKenney wrote:
> > > On Wed, Oct 29, 2014 at 01:45:19PM +0000, Alexander Gordeev wrote:
> > > > On Wed, Oct 29, 2014 at 05:47:29AM -0700, Paul E. McKenney wrote:
> > > > > On Wed, Oct 29, 2014 at 12:07:07PM +0100, Alexander Gordeev wrote:
> > > > > > Function rcu_is_cpu_rrupt_from_idle() is called from scheduling-
> > > > > > clock interrupt handler to check if the current CPU was interrupted
> > > > > > from idle. If true, it results in invocation of RCU callbacks. But
> > > > > > the common hardware interrupt exit path also contains similar check
> > > > > > and therefore the call to rcu_is_cpu_rrupt_from_idle() is redundant.
> > > > > 
> > > > > By common hardware interrupt exit path, you are meaning the calls
> > > > > to rcu_irq_exit()?  If not, please let me know exactly what you
> > > > > mean here.
> > > > 
> > > > Yes, I mean rcu_irq_exit().
> > > 
> > > Unless you can get the indication of whether or not the original interrupt
> > > came from userspace execution into rcu_irq_exit(), this will not work.
> > > It will result in grace-period hangs on some configurations.
> > 
> > Okay, that was my concern wrt tree RCU. By contrast, tiny RCU does not seem
> > able to hang a grace-period, isn't it?
> 
> Although it is true that tiny RCU cannot hang a synchronize_rcu()
> grace period, it most certainly can hang a call_rcu() grace period
> in exactly the same way.

Sorry for being a pain in the neck - just want to make sure I am following.

I only see possibility to cause callbacks not being called for "too long"
in case a system has lots of nested interrupts and rcu_idle_enter_common()
is not being called from hardware interrupt context as result. How could
rcu_is_cpu_rrupt_from_idle() help here?

> > > Now, if you -can- get the userspace-execution indication into
> > > rcu_irq_exit(), this might be of interest.  However, it might be faster
> > > to simply let the scheduling-clock interrupt do the job as it currently
> > > does, especially for workloads with lots of interrupts.
> > > 
> > > Or did you have something else in mind?
> > 
> > Nope. I would even leave as is tiny RCU's rcu_is_cpu_rrupt_from_idle()
> > for clarity then ;)
> 
> Also to avoid userspace execution from preventing RCU callbacks from
> ever being invoked.  ;-)

Hmm.. Am I missing something else? I did not remove the userspace check
from the scheduling-clock interrupt:

@@ -250,7 +240,7 @@ void rcu_bh_qs(void)
 void rcu_check_callbacks(int cpu, int user)
 {
 	RCU_TRACE(check_cpu_stalls());
-	if (user || rcu_is_cpu_rrupt_from_idle())
+	if (user)
 		rcu_sched_qs();
 	else if (!in_softirq())
 		rcu_bh_qs();

Thanks!

> 							Thanx, Paul

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH] rcu: Remove redundant rcu_is_cpu_rrupt_from_idle() function

[Paul E. McKenney]

On Wed, Oct 29, 2014 at 09:09:16PM +0000, Alexander Gordeev wrote:
> On Wed, Oct 29, 2014 at 08:46:15AM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 29, 2014 at 03:16:23PM +0000, Alexander Gordeev wrote:
> > > On Wed, Oct 29, 2014 at 07:34:39AM -0700, Paul E. McKenney wrote:
> > > > On Wed, Oct 29, 2014 at 01:45:19PM +0000, Alexander Gordeev wrote:
> > > > > On Wed, Oct 29, 2014 at 05:47:29AM -0700, Paul E. McKenney wrote:
> > > > > > On Wed, Oct 29, 2014 at 12:07:07PM +0100, Alexander Gordeev wrote:
> > > > > > > Function rcu_is_cpu_rrupt_from_idle() is called from scheduling-
> > > > > > > clock interrupt handler to check if the current CPU was interrupted
> > > > > > > from idle. If true, it results in invocation of RCU callbacks. But
> > > > > > > the common hardware interrupt exit path also contains similar check
> > > > > > > and therefore the call to rcu_is_cpu_rrupt_from_idle() is redundant.
> > > > > > 
> > > > > > By common hardware interrupt exit path, you are meaning the calls
> > > > > > to rcu_irq_exit()?  If not, please let me know exactly what you
> > > > > > mean here.
> > > > > 
> > > > > Yes, I mean rcu_irq_exit().
> > > > 
> > > > Unless you can get the indication of whether or not the original interrupt
> > > > came from userspace execution into rcu_irq_exit(), this will not work.
> > > > It will result in grace-period hangs on some configurations.
> > > 
> > > Okay, that was my concern wrt tree RCU. By contrast, tiny RCU does not seem
> > > able to hang a grace-period, isn't it?
> > 
> > Although it is true that tiny RCU cannot hang a synchronize_rcu()
> > grace period, it most certainly can hang a call_rcu() grace period
> > in exactly the same way.
> 
> Sorry for being a pain in the neck - just want to make sure I am following.

No worries!

> I only see possibility to cause callbacks not being called for "too long"
> in case a system has lots of nested interrupts and rcu_idle_enter_common()
> is not being called from hardware interrupt context as result. How could
> rcu_is_cpu_rrupt_from_idle() help here?

Let's start assuming that something in the idle loop posts a callback,
and then let me see if I understand your reasoning...

1.	The system is idle and stays that way, no runnable tasks.

2.	An interrupt occurs.  Upon return from interrupt, rcu_irq_exit()
	is invoked, which calls rcu_idle_enter_common(), which in turn
	calls rcu_sched_qs(), which does a raise_softirq(RCU_SOFTIRQ).

3.	The softirq happens shortly and invokes rcu_process_callbacks(),
	which invokes __rcu_process_callbacks().

4.	So now callbacks can be invoked.  At least they can be if
	->donetail has been updated.  Which it will have been because
	rcu_sched_qs() invokes rcu_qsctr_help().

So your point that rcu_is_cpu_rrupt_from_idle() might be redundant could
well be valid -- sorry for being so dismissive earlier.

> > > > Now, if you -can- get the userspace-execution indication into
> > > > rcu_irq_exit(), this might be of interest.  However, it might be faster
> > > > to simply let the scheduling-clock interrupt do the job as it currently
> > > > does, especially for workloads with lots of interrupts.
> > > > 
> > > > Or did you have something else in mind?
> > > 
> > > Nope. I would even leave as is tiny RCU's rcu_is_cpu_rrupt_from_idle()
> > > for clarity then ;)
> > 
> > Also to avoid userspace execution from preventing RCU callbacks from
> > ever being invoked.  ;-)
> 
> Hmm.. Am I missing something else? I did not remove the userspace check
> from the scheduling-clock interrupt:
> 
> @@ -250,7 +240,7 @@ void rcu_bh_qs(void)
>  void rcu_check_callbacks(int cpu, int user)
>  {
>  	RCU_TRACE(check_cpu_stalls());
> -	if (user || rcu_is_cpu_rrupt_from_idle())
> +	if (user)
>  		rcu_sched_qs();
>  	else if (!in_softirq())
>  		rcu_bh_qs();

Probably just me being confused.  Hopefully so, as shrinking TINY_RCU
further will probably be welcome.

Have you done any testing of this change?

							Thanx, Paul

Re: [PATCH] rcu: Remove redundant rcu_is_cpu_rrupt_from_idle() function

[Alexander Gordeev]

On Wed, Oct 29, 2014 at 05:05:24PM -0700, Paul E. McKenney wrote:
> > > Although it is true that tiny RCU cannot hang a synchronize_rcu()
> > > grace period, it most certainly can hang a call_rcu() grace period
> > > in exactly the same way.
> > 
> > Sorry for being a pain in the neck - just want to make sure I am following.
> 
> No worries!
> 
> > I only see possibility to cause callbacks not being called for "too long"
> > in case a system has lots of nested interrupts and rcu_idle_enter_common()
> > is not being called from hardware interrupt context as result. How could
> > rcu_is_cpu_rrupt_from_idle() help here?
> 
> Let's start assuming that something in the idle loop posts a callback,
> and then let me see if I understand your reasoning...
> 
> 1.	The system is idle and stays that way, no runnable tasks.
> 
> 2.	An interrupt occurs.  Upon return from interrupt, rcu_irq_exit()
> 	is invoked, which calls rcu_idle_enter_common(), which in turn
> 	calls rcu_sched_qs(), which does a raise_softirq(RCU_SOFTIRQ).
> 
> 3.	The softirq happens shortly and invokes rcu_process_callbacks(),
> 	which invokes __rcu_process_callbacks().
> 
> 4.	So now callbacks can be invoked.  At least they can be if
> 	->donetail has been updated.  Which it will have been because
> 	rcu_sched_qs() invokes rcu_qsctr_help().

Yes, that is exactly my reasoning.

> So your point that rcu_is_cpu_rrupt_from_idle() might be redundant could
> well be valid -- sorry for being so dismissive earlier.
>
> > > > > Now, if you -can- get the userspace-execution indication into
> > > > > rcu_irq_exit(), this might be of interest.  However, it might be faster
> > > > > to simply let the scheduling-clock interrupt do the job as it currently
> > > > > does, especially for workloads with lots of interrupts.
> > > > > 
> > > > > Or did you have something else in mind?
> > > > 
> > > > Nope. I would even leave as is tiny RCU's rcu_is_cpu_rrupt_from_idle()
> > > > for clarity then ;)
> > > 
> > > Also to avoid userspace execution from preventing RCU callbacks from
> > > ever being invoked.  ;-)
> > 
> > Hmm.. Am I missing something else? I did not remove the userspace check
> > from the scheduling-clock interrupt:
> > 
> > @@ -250,7 +240,7 @@ void rcu_bh_qs(void)
> >  void rcu_check_callbacks(int cpu, int user)
> >  {
> >  	RCU_TRACE(check_cpu_stalls());
> > -	if (user || rcu_is_cpu_rrupt_from_idle())
> > +	if (user)
> >  		rcu_sched_qs();
> >  	else if (!in_softirq())
> >  		rcu_bh_qs();
> 
> Probably just me being confused.  Hopefully so, as shrinking TINY_RCU
> further will probably be welcome.

Should I resend tiny-only patch?

> Have you done any testing of this change?

Just booted to a unicore kernel and dd'ed 1G of /dev/sda to /dev/null.

> 							Thanx, Paul
> 

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH] rcu: Remove redundant rcu_is_cpu_rrupt_from_idle() function

[Paul E. McKenney]

On Thu, Oct 30, 2014 at 05:54:35PM +0000, Alexander Gordeev wrote:
> On Wed, Oct 29, 2014 at 05:05:24PM -0700, Paul E. McKenney wrote:
> > > > Although it is true that tiny RCU cannot hang a synchronize_rcu()
> > > > grace period, it most certainly can hang a call_rcu() grace period
> > > > in exactly the same way.
> > > 
> > > Sorry for being a pain in the neck - just want to make sure I am following.
> > 
> > No worries!
> > 
> > > I only see possibility to cause callbacks not being called for "too long"
> > > in case a system has lots of nested interrupts and rcu_idle_enter_common()
> > > is not being called from hardware interrupt context as result. How could
> > > rcu_is_cpu_rrupt_from_idle() help here?
> > 
> > Let's start assuming that something in the idle loop posts a callback,
> > and then let me see if I understand your reasoning...
> > 
> > 1.	The system is idle and stays that way, no runnable tasks.
> > 
> > 2.	An interrupt occurs.  Upon return from interrupt, rcu_irq_exit()
> > 	is invoked, which calls rcu_idle_enter_common(), which in turn
> > 	calls rcu_sched_qs(), which does a raise_softirq(RCU_SOFTIRQ).
> > 
> > 3.	The softirq happens shortly and invokes rcu_process_callbacks(),
> > 	which invokes __rcu_process_callbacks().
> > 
> > 4.	So now callbacks can be invoked.  At least they can be if
> > 	->donetail has been updated.  Which it will have been because
> > 	rcu_sched_qs() invokes rcu_qsctr_help().
> 
> Yes, that is exactly my reasoning.
> 
> > So your point that rcu_is_cpu_rrupt_from_idle() might be redundant could
> > well be valid -- sorry for being so dismissive earlier.
> >
> > > > > > Now, if you -can- get the userspace-execution indication into
> > > > > > rcu_irq_exit(), this might be of interest.  However, it might be faster
> > > > > > to simply let the scheduling-clock interrupt do the job as it currently
> > > > > > does, especially for workloads with lots of interrupts.
> > > > > > 
> > > > > > Or did you have something else in mind?
> > > > > 
> > > > > Nope. I would even leave as is tiny RCU's rcu_is_cpu_rrupt_from_idle()
> > > > > for clarity then ;)
> > > > 
> > > > Also to avoid userspace execution from preventing RCU callbacks from
> > > > ever being invoked.  ;-)
> > > 
> > > Hmm.. Am I missing something else? I did not remove the userspace check
> > > from the scheduling-clock interrupt:
> > > 
> > > @@ -250,7 +240,7 @@ void rcu_bh_qs(void)
> > >  void rcu_check_callbacks(int cpu, int user)
> > >  {
> > >  	RCU_TRACE(check_cpu_stalls());
> > > -	if (user || rcu_is_cpu_rrupt_from_idle())
> > > +	if (user)
> > >  		rcu_sched_qs();
> > >  	else if (!in_softirq())
> > >  		rcu_bh_qs();
> > 
> > Probably just me being confused.  Hopefully so, as shrinking TINY_RCU
> > further will probably be welcome.
> 
> Should I resend tiny-only patch?

Please, but also expand the commit log to contain the above reasoning.

> > Have you done any testing of this change?
> 
> Just booted to a unicore kernel and dd'ed 1G of /dev/sda to /dev/null.

OK, that is a start. Could I convince you to build a kernel?

							Thanx, Paul

Re: [PATCH] rcu: Remove redundant rcu_is_cpu_rrupt_from_idle() function

[Alexander Gordeev]

On Thu, Oct 30, 2014 at 01:43:18PM -0700, Paul E. McKenney wrote:
> > > Have you done any testing of this change?
> > 
> > Just booted to a unicore kernel and dd'ed 1G of /dev/sda to /dev/null.
> 
> OK, that is a start. Could I convince you to build a kernel?

I did build one with the patch I posted. Or you mean something else here?

> 							Thanx, Paul
> 

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH] rcu: Remove redundant rcu_is_cpu_rrupt_from_idle() function

[Paul E. McKenney]

On Thu, Oct 30, 2014 at 09:54:34PM +0000, Alexander Gordeev wrote:
> On Thu, Oct 30, 2014 at 01:43:18PM -0700, Paul E. McKenney wrote:
> > > > Have you done any testing of this change?
> > > 
> > > Just booted to a unicore kernel and dd'ed 1G of /dev/sda to /dev/null.
> > 
> > OK, that is a start. Could I convince you to build a kernel?
> 
> I did build one with the patch I posted. Or you mean something else here?

Something else, I think.

While running a kernel that was built with your patch, do a couple of
kernel builds concurrently.  ;-)

							Thanx, Paul

Re: [PATCH] rcu: Remove redundant rcu_is_cpu_rrupt_from_idle() function

[Alexander Gordeev]

On Thu, Oct 30, 2014 at 08:45:59PM -0700, Paul E. McKenney wrote:
> On Thu, Oct 30, 2014 at 09:54:34PM +0000, Alexander Gordeev wrote:
> > On Thu, Oct 30, 2014 at 01:43:18PM -0700, Paul E. McKenney wrote:
> > > > > Have you done any testing of this change?
> > > > 
> > > > Just booted to a unicore kernel and dd'ed 1G of /dev/sda to /dev/null.
> > > 
> > > OK, that is a start. Could I convince you to build a kernel?
> > 
> > I did build one with the patch I posted. Or you mean something else here?
> 
> Something else, I think.
> 
> While running a kernel that was built with your patch, do a couple of
> kernel builds concurrently.  ;-)

Did it. No issues found. Sending updated patch shortly.

Thanks!

> 							Thanx, Paul
> 

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

[PATCH v2] rcu: Remove redundant rcu_is_cpu_rrupt_from_idle() from tiny RCU

[Alexander Gordeev]

Let's start assuming that something in the idle loop posts a callback,
and scheduling-clock interrupt occurs:

1. The system is idle and stays that way, no runnable tasks.

2. Scheduling-clock interrupt occurs, rcu_check_callbacks() is called
   as result, which in turn calls rcu_is_cpu_rrupt_from_idle().

3. rcu_is_cpu_rrupt_from_idle() reports the CPU was interrupted from
   idle, which results in rcu_sched_qs() call, which does a
   raise_softirq(RCU_SOFTIRQ).

4. Upon return from interrupt, rcu_irq_exit() is invoked, which calls
   rcu_idle_enter_common(), which in turn calls rcu_sched_qs() again,
   which does another raise_softirq(RCU_SOFTIRQ).

5. The softirq happens shortly and invokes rcu_process_callbacks(),
   which invokes __rcu_process_callbacks().

6. So now callbacks can be invoked. At least they can be if
   ->donetail has been updated. Which it will have been because
   rcu_sched_qs() invokes rcu_qsctr_help().

In the described scenario rcu_sched_qs() and raise_softirq(RCU_SOFTIRQ)
get called twice in steps 3 and 4. This redundancy could be eliminated
by removing rcu_is_cpu_rrupt_from_idle() function.

Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/rcu/tiny.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c
index c0623fc..0bfb3b9 100644
--- a/kernel/rcu/tiny.c
+++ b/kernel/rcu/tiny.c
@@ -186,16 +186,6 @@ EXPORT_SYMBOL(__rcu_is_watching);
 #endif /* defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_RCU_TRACE) */
 
 /*
- * Test whether the current CPU was interrupted from idle.  Nested
- * interrupts don't count, we must be running at the first interrupt
- * level.
- */
-static int rcu_is_cpu_rrupt_from_idle(void)
-{
-	return rcu_dynticks_nesting <= 1;
-}
-
-/*
  * Helper function for rcu_sched_qs() and rcu_bh_qs().
  * Also irqs are disabled to avoid confusion due to interrupt handlers
  * invoking call_rcu().
@@ -250,7 +240,7 @@ void rcu_bh_qs(void)
 void rcu_check_callbacks(int cpu, int user)
 {
 	RCU_TRACE(check_cpu_stalls());
-	if (user || rcu_is_cpu_rrupt_from_idle())
+	if (user)
 		rcu_sched_qs();
 	else if (!in_softirq())
 		rcu_bh_qs();
-- 
1.8.3.1

-- 
Regards,
Alexander Gordeev
agordeev@redhat.com

Re: [PATCH v2] rcu: Remove redundant rcu_is_cpu_rrupt_from_idle() from tiny RCU

[Paul E. McKenney]

On Fri, Oct 31, 2014 at 02:55:05PM +0000, Alexander Gordeev wrote:
> Let's start assuming that something in the idle loop posts a callback,
> and scheduling-clock interrupt occurs:
> 
> 1. The system is idle and stays that way, no runnable tasks.
> 
> 2. Scheduling-clock interrupt occurs, rcu_check_callbacks() is called
>    as result, which in turn calls rcu_is_cpu_rrupt_from_idle().
> 
> 3. rcu_is_cpu_rrupt_from_idle() reports the CPU was interrupted from
>    idle, which results in rcu_sched_qs() call, which does a
>    raise_softirq(RCU_SOFTIRQ).
> 
> 4. Upon return from interrupt, rcu_irq_exit() is invoked, which calls
>    rcu_idle_enter_common(), which in turn calls rcu_sched_qs() again,
>    which does another raise_softirq(RCU_SOFTIRQ).
> 
> 5. The softirq happens shortly and invokes rcu_process_callbacks(),
>    which invokes __rcu_process_callbacks().
> 
> 6. So now callbacks can be invoked. At least they can be if
>    ->donetail has been updated. Which it will have been because
>    rcu_sched_qs() invokes rcu_qsctr_help().
> 
> In the described scenario rcu_sched_qs() and raise_softirq(RCU_SOFTIRQ)
> get called twice in steps 3 and 4. This redundancy could be eliminated
> by removing rcu_is_cpu_rrupt_from_idle() function.
> 
> Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Queued for 3.20, thank you!

							Thanx, Paul

> ---
>  kernel/rcu/tiny.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c
> index c0623fc..0bfb3b9 100644
> --- a/kernel/rcu/tiny.c
> +++ b/kernel/rcu/tiny.c
> @@ -186,16 +186,6 @@ EXPORT_SYMBOL(__rcu_is_watching);
>  #endif /* defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_RCU_TRACE) */
> 
>  /*
> - * Test whether the current CPU was interrupted from idle.  Nested
> - * interrupts don't count, we must be running at the first interrupt
> - * level.
> - */
> -static int rcu_is_cpu_rrupt_from_idle(void)
> -{
> -	return rcu_dynticks_nesting <= 1;
> -}
> -
> -/*
>   * Helper function for rcu_sched_qs() and rcu_bh_qs().
>   * Also irqs are disabled to avoid confusion due to interrupt handlers
>   * invoking call_rcu().
> @@ -250,7 +240,7 @@ void rcu_bh_qs(void)
>  void rcu_check_callbacks(int user)
>  {
>  	RCU_TRACE(check_cpu_stalls());
> -	if (user || rcu_is_cpu_rrupt_from_idle())
> +	if (user)
>  		rcu_sched_qs();
>  	else if (!in_softirq())
>  		rcu_bh_qs();
> -- 
> 1.8.3.1
> 
> -- 
> Regards,
> Alexander Gordeev
> agordeev@redhat.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>