linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] rcuperf: Make rcuperf kernel test more robust for !expedited mode
@ 2019-07-04  4:34 Joel Fernandes (Google)
  2019-07-04 17:40 ` Paul E. McKenney
  0 siblings, 1 reply; 7+ messages in thread
From: Joel Fernandes (Google) @ 2019-07-04  4:34 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Davidlohr Bueso, Josh Triplett, Lai Jiangshan, Mathieu Desnoyers,
	Paul E. McKenney, rcu, Steven Rostedt, kernel-team

It is possible that the rcuperf kernel test runs concurrently with init
starting up.  During this time, the system is running all grace periods
as expedited.  However, rcuperf can also be run for normal GP tests.
Right now, it depends on a holdoff time before starting the test to
ensure grace periods start later. This works fine with the default
holdoff time however it is not robust in situations where init takes
greater than the holdoff time to finish running. Or, as in my case:

I modified the rcuperf test locally to also run a thread that did
preempt disable/enable in a loop. This had the effect of slowing down
init. The end result was that the "batches:" counter in rcuperf was 0
causing a division by 0 error in the results. This counter was 0 because
only expedited GPs seem to happen, not normal ones which led to the
rcu_state.gp_seq counter remaining constant across grace periods which
unexpectedly happen to be expedited. The system was running expedited
RCU all the time because rcu_unexpedited_gp() would not have run yet
from init.  In other words, the test would concurrently with init
booting in expedited GP mode.

To fix this properly, let us check if system_state if SYSTEM_RUNNING
is set before starting the test. The system_state approximately aligns
with when rcu_unexpedited_gp() is called and works well in practice.

I also tried late_initcall however it is still too early to be
meaningful for this case.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/rcuperf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c
index 4513807cd4c4..5a879d073c1c 100644
--- a/kernel/rcu/rcuperf.c
+++ b/kernel/rcu/rcuperf.c
@@ -375,6 +375,14 @@ rcu_perf_writer(void *arg)
 	if (holdoff)
 		schedule_timeout_uninterruptible(holdoff * HZ);
 
+	/*
+	 * Wait until rcu_end_inkernel_boot() is called for normal GP tests
+	 * so that RCU is not always expedited for normal GP tests.
+	 * The system_state test is approximate, but works well in practice.
+	 */
+	while (!gp_exp && system_state != SYSTEM_RUNNING)
+		schedule_timeout_uninterruptible(1);
+
 	t = ktime_get_mono_fast_ns();
 	if (atomic_inc_return(&n_rcu_perf_writer_started) >= nrealwriters) {
 		t_rcu_perf_writer_started = t;
-- 
2.22.0.410.gd8fdbe21b5-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] rcuperf: Make rcuperf kernel test more robust for !expedited mode
  2019-07-04  4:34 [PATCH] rcuperf: Make rcuperf kernel test more robust for !expedited mode Joel Fernandes (Google)
@ 2019-07-04 17:40 ` Paul E. McKenney
  2019-07-05  3:52   ` Byungchul Park
  0 siblings, 1 reply; 7+ messages in thread
From: Paul E. McKenney @ 2019-07-04 17:40 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Davidlohr Bueso, Josh Triplett, Lai Jiangshan,
	Mathieu Desnoyers, rcu, Steven Rostedt, kernel-team

On Thu, Jul 04, 2019 at 12:34:30AM -0400, Joel Fernandes (Google) wrote:
> It is possible that the rcuperf kernel test runs concurrently with init
> starting up.  During this time, the system is running all grace periods
> as expedited.  However, rcuperf can also be run for normal GP tests.
> Right now, it depends on a holdoff time before starting the test to
> ensure grace periods start later. This works fine with the default
> holdoff time however it is not robust in situations where init takes
> greater than the holdoff time to finish running. Or, as in my case:
> 
> I modified the rcuperf test locally to also run a thread that did
> preempt disable/enable in a loop. This had the effect of slowing down
> init. The end result was that the "batches:" counter in rcuperf was 0
> causing a division by 0 error in the results. This counter was 0 because
> only expedited GPs seem to happen, not normal ones which led to the
> rcu_state.gp_seq counter remaining constant across grace periods which
> unexpectedly happen to be expedited. The system was running expedited
> RCU all the time because rcu_unexpedited_gp() would not have run yet
> from init.  In other words, the test would concurrently with init
> booting in expedited GP mode.
> 
> To fix this properly, let us check if system_state if SYSTEM_RUNNING
> is set before starting the test. The system_state approximately aligns
> with when rcu_unexpedited_gp() is called and works well in practice.
> 
> I also tried late_initcall however it is still too early to be
> meaningful for this case.
> 
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Good catch, queued, thank you!

							Thanx, Paul

> ---
>  kernel/rcu/rcuperf.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c
> index 4513807cd4c4..5a879d073c1c 100644
> --- a/kernel/rcu/rcuperf.c
> +++ b/kernel/rcu/rcuperf.c
> @@ -375,6 +375,14 @@ rcu_perf_writer(void *arg)
>  	if (holdoff)
>  		schedule_timeout_uninterruptible(holdoff * HZ);
>  
> +	/*
> +	 * Wait until rcu_end_inkernel_boot() is called for normal GP tests
> +	 * so that RCU is not always expedited for normal GP tests.
> +	 * The system_state test is approximate, but works well in practice.
> +	 */
> +	while (!gp_exp && system_state != SYSTEM_RUNNING)
> +		schedule_timeout_uninterruptible(1);
> +
>  	t = ktime_get_mono_fast_ns();
>  	if (atomic_inc_return(&n_rcu_perf_writer_started) >= nrealwriters) {
>  		t_rcu_perf_writer_started = t;
> -- 
> 2.22.0.410.gd8fdbe21b5-goog
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] rcuperf: Make rcuperf kernel test more robust for !expedited mode
  2019-07-04 17:40 ` Paul E. McKenney
@ 2019-07-05  3:52   ` Byungchul Park
  2019-07-05 12:24     ` Joel Fernandes
  0 siblings, 1 reply; 7+ messages in thread
From: Byungchul Park @ 2019-07-05  3:52 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Joel Fernandes (Google),
	linux-kernel, Davidlohr Bueso, Josh Triplett, Lai Jiangshan,
	Mathieu Desnoyers, rcu, Steven Rostedt, kernel-team

On Thu, Jul 04, 2019 at 10:40:44AM -0700, Paul E. McKenney wrote:
> On Thu, Jul 04, 2019 at 12:34:30AM -0400, Joel Fernandes (Google) wrote:
> > It is possible that the rcuperf kernel test runs concurrently with init
> > starting up.  During this time, the system is running all grace periods
> > as expedited.  However, rcuperf can also be run for normal GP tests.
> > Right now, it depends on a holdoff time before starting the test to
> > ensure grace periods start later. This works fine with the default
> > holdoff time however it is not robust in situations where init takes
> > greater than the holdoff time to finish running. Or, as in my case:
> > 
> > I modified the rcuperf test locally to also run a thread that did
> > preempt disable/enable in a loop. This had the effect of slowing down
> > init. The end result was that the "batches:" counter in rcuperf was 0
> > causing a division by 0 error in the results. This counter was 0 because
> > only expedited GPs seem to happen, not normal ones which led to the
> > rcu_state.gp_seq counter remaining constant across grace periods which
> > unexpectedly happen to be expedited. The system was running expedited
> > RCU all the time because rcu_unexpedited_gp() would not have run yet
> > from init.  In other words, the test would concurrently with init
> > booting in expedited GP mode.
> > 
> > To fix this properly, let us check if system_state if SYSTEM_RUNNING
> > is set before starting the test. The system_state approximately aligns

Just minor typo..

To fix this properly, let us check if system_state if SYSTEM_RUNNING
is set before starting the test. ...

Should be

To fix this properly, let us check if system_state is set to
SYSTEM_RUNNING before starting the test. ...

Thanks,
Byungchul

> > with when rcu_unexpedited_gp() is called and works well in practice.
> > 
> > I also tried late_initcall however it is still too early to be
> > meaningful for this case.
> > 
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> 
> Good catch, queued, thank you!
> 
> 							Thanx, Paul
> 
> > ---
> >  kernel/rcu/rcuperf.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/kernel/rcu/rcuperf.c b/kernel/rcu/rcuperf.c
> > index 4513807cd4c4..5a879d073c1c 100644
> > --- a/kernel/rcu/rcuperf.c
> > +++ b/kernel/rcu/rcuperf.c
> > @@ -375,6 +375,14 @@ rcu_perf_writer(void *arg)
> >  	if (holdoff)
> >  		schedule_timeout_uninterruptible(holdoff * HZ);
> >  
> > +	/*
> > +	 * Wait until rcu_end_inkernel_boot() is called for normal GP tests
> > +	 * so that RCU is not always expedited for normal GP tests.
> > +	 * The system_state test is approximate, but works well in practice.
> > +	 */
> > +	while (!gp_exp && system_state != SYSTEM_RUNNING)
> > +		schedule_timeout_uninterruptible(1);
> > +
> >  	t = ktime_get_mono_fast_ns();
> >  	if (atomic_inc_return(&n_rcu_perf_writer_started) >= nrealwriters) {
> >  		t_rcu_perf_writer_started = t;
> > -- 
> > 2.22.0.410.gd8fdbe21b5-goog
> > 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] rcuperf: Make rcuperf kernel test more robust for !expedited mode
  2019-07-05  3:52   ` Byungchul Park
@ 2019-07-05 12:24     ` Joel Fernandes
  2019-07-05 15:09       ` Paul E. McKenney
  0 siblings, 1 reply; 7+ messages in thread
From: Joel Fernandes @ 2019-07-05 12:24 UTC (permalink / raw)
  To: Byungchul Park
  Cc: Paul E. McKenney, linux-kernel, Davidlohr Bueso, Josh Triplett,
	Lai Jiangshan, Mathieu Desnoyers, rcu, Steven Rostedt,
	kernel-team

On Fri, Jul 05, 2019 at 12:52:31PM +0900, Byungchul Park wrote:
> On Thu, Jul 04, 2019 at 10:40:44AM -0700, Paul E. McKenney wrote:
> > On Thu, Jul 04, 2019 at 12:34:30AM -0400, Joel Fernandes (Google) wrote:
> > > It is possible that the rcuperf kernel test runs concurrently with init
> > > starting up.  During this time, the system is running all grace periods
> > > as expedited.  However, rcuperf can also be run for normal GP tests.
> > > Right now, it depends on a holdoff time before starting the test to
> > > ensure grace periods start later. This works fine with the default
> > > holdoff time however it is not robust in situations where init takes
> > > greater than the holdoff time to finish running. Or, as in my case:
> > > 
> > > I modified the rcuperf test locally to also run a thread that did
> > > preempt disable/enable in a loop. This had the effect of slowing down
> > > init. The end result was that the "batches:" counter in rcuperf was 0
> > > causing a division by 0 error in the results. This counter was 0 because
> > > only expedited GPs seem to happen, not normal ones which led to the
> > > rcu_state.gp_seq counter remaining constant across grace periods which
> > > unexpectedly happen to be expedited. The system was running expedited
> > > RCU all the time because rcu_unexpedited_gp() would not have run yet
> > > from init.  In other words, the test would concurrently with init
> > > booting in expedited GP mode.
> > > 
> > > To fix this properly, let us check if system_state if SYSTEM_RUNNING
> > > is set before starting the test. The system_state approximately aligns
> 
> Just minor typo..
> 
> To fix this properly, let us check if system_state if SYSTEM_RUNNING
> is set before starting the test. ...
> 
> Should be
> 
> To fix this properly, let us check if system_state is set to
> SYSTEM_RUNNING before starting the test. ...

That's a fair point. I wonder if Paul already fixed it up in his tree,
however I am happy to resend if he hasn't. Paul, how would you like to handle
this commit log nit?

it is just 'if ..' to 'is SYSTEM_RUNNING'

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] rcuperf: Make rcuperf kernel test more robust for !expedited mode
  2019-07-05 12:24     ` Joel Fernandes
@ 2019-07-05 15:09       ` Paul E. McKenney
  2019-07-05 20:00         ` Joel Fernandes
  0 siblings, 1 reply; 7+ messages in thread
From: Paul E. McKenney @ 2019-07-05 15:09 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: Byungchul Park, linux-kernel, Davidlohr Bueso, Josh Triplett,
	Lai Jiangshan, Mathieu Desnoyers, rcu, Steven Rostedt,
	kernel-team

On Fri, Jul 05, 2019 at 08:24:50AM -0400, Joel Fernandes wrote:
> On Fri, Jul 05, 2019 at 12:52:31PM +0900, Byungchul Park wrote:
> > On Thu, Jul 04, 2019 at 10:40:44AM -0700, Paul E. McKenney wrote:
> > > On Thu, Jul 04, 2019 at 12:34:30AM -0400, Joel Fernandes (Google) wrote:
> > > > It is possible that the rcuperf kernel test runs concurrently with init
> > > > starting up.  During this time, the system is running all grace periods
> > > > as expedited.  However, rcuperf can also be run for normal GP tests.
> > > > Right now, it depends on a holdoff time before starting the test to
> > > > ensure grace periods start later. This works fine with the default
> > > > holdoff time however it is not robust in situations where init takes
> > > > greater than the holdoff time to finish running. Or, as in my case:
> > > > 
> > > > I modified the rcuperf test locally to also run a thread that did
> > > > preempt disable/enable in a loop. This had the effect of slowing down
> > > > init. The end result was that the "batches:" counter in rcuperf was 0
> > > > causing a division by 0 error in the results. This counter was 0 because
> > > > only expedited GPs seem to happen, not normal ones which led to the
> > > > rcu_state.gp_seq counter remaining constant across grace periods which
> > > > unexpectedly happen to be expedited. The system was running expedited
> > > > RCU all the time because rcu_unexpedited_gp() would not have run yet
> > > > from init.  In other words, the test would concurrently with init
> > > > booting in expedited GP mode.
> > > > 
> > > > To fix this properly, let us check if system_state if SYSTEM_RUNNING
> > > > is set before starting the test. The system_state approximately aligns
> > 
> > Just minor typo..
> > 
> > To fix this properly, let us check if system_state if SYSTEM_RUNNING
> > is set before starting the test. ...
> > 
> > Should be
> > 
> > To fix this properly, let us check if system_state is set to
> > SYSTEM_RUNNING before starting the test. ...
> 
> That's a fair point. I wonder if Paul already fixed it up in his tree,
> however I am happy to resend if he hasn't. Paul, how would you like to handle
> this commit log nit?
> 
> it is just 'if ..' to 'is SYSTEM_RUNNING'

It now reads as follows:

	To fix this properly, this commit waits until system_state is
	set to SYSTEM_RUNNING before starting the test.  This change is
	made just before kernel_init() invokes rcu_end_inkernel_boot(),
	and this latter is what turns off boot-time expediting of RCU
	grace periods.

I dropped the last paragraph about late_initcall().  And I suspect that
the last clause from rcu_gp_is_expedited() can be dropped:

bool rcu_gp_is_expedited(void)
{
	return rcu_expedited || atomic_read(&rcu_expedited_nesting) ||
	       rcu_scheduler_active == RCU_SCHEDULER_INIT;
}

This is because rcu_expedited_nesting is initialized to 1, and is
decremented in rcu_end_inkernel_boot(), which is called long after
rcu_scheduler_active has been set to RCU_SCHEDULER_RUNNING, which
happens at core_initcall() time.  So if the last clause says "true",
so does the second-to-last clause.

The similar check in rcu_gp_is_normal() is still need, however, to allow
the power-management subsystem to invoke synchronize_rcu() just after
the scheduler has been initialized, but before RCU is aware of this.

So, how about the commit shown below?

							Thanx, Paul

------------------------------------------------------------------------

commit 1f7e72efe3c761c2b34da7b59e01ad69c657db10
Author: Paul E. McKenney <paulmck@linux.ibm.com>
Date:   Fri Jul 5 08:05:10 2019 -0700

    rcu: Remove redundant "if" condition from rcu_gp_is_expedited()
    
    Because rcu_expedited_nesting is initialized to 1 and not decremented
    until just before init is spawned, rcu_expedited_nesting is guaranteed
    to be non-zero whenever rcu_scheduler_active == RCU_SCHEDULER_INIT.
    This commit therefore removes this redundant "if" equality test.
    
    Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>

diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index 249517058b13..64e9cc8609e7 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -136,8 +136,7 @@ static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
  */
 bool rcu_gp_is_expedited(void)
 {
-	return rcu_expedited || atomic_read(&rcu_expedited_nesting) ||
-	       rcu_scheduler_active == RCU_SCHEDULER_INIT;
+	return rcu_expedited || atomic_read(&rcu_expedited_nesting);
 }
 EXPORT_SYMBOL_GPL(rcu_gp_is_expedited);
 


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] rcuperf: Make rcuperf kernel test more robust for !expedited mode
  2019-07-05 15:09       ` Paul E. McKenney
@ 2019-07-05 20:00         ` Joel Fernandes
  2019-07-13 14:18           ` Paul E. McKenney
  0 siblings, 1 reply; 7+ messages in thread
From: Joel Fernandes @ 2019-07-05 20:00 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Byungchul Park, linux-kernel, Davidlohr Bueso, Josh Triplett,
	Lai Jiangshan, Mathieu Desnoyers, rcu, Steven Rostedt,
	kernel-team

On Fri, Jul 05, 2019 at 08:09:32AM -0700, Paul E. McKenney wrote:
> On Fri, Jul 05, 2019 at 08:24:50AM -0400, Joel Fernandes wrote:
> > On Fri, Jul 05, 2019 at 12:52:31PM +0900, Byungchul Park wrote:
> > > On Thu, Jul 04, 2019 at 10:40:44AM -0700, Paul E. McKenney wrote:
> > > > On Thu, Jul 04, 2019 at 12:34:30AM -0400, Joel Fernandes (Google) wrote:
> > > > > It is possible that the rcuperf kernel test runs concurrently with init
> > > > > starting up.  During this time, the system is running all grace periods
> > > > > as expedited.  However, rcuperf can also be run for normal GP tests.
> > > > > Right now, it depends on a holdoff time before starting the test to
> > > > > ensure grace periods start later. This works fine with the default
> > > > > holdoff time however it is not robust in situations where init takes
> > > > > greater than the holdoff time to finish running. Or, as in my case:
> > > > > 
> > > > > I modified the rcuperf test locally to also run a thread that did
> > > > > preempt disable/enable in a loop. This had the effect of slowing down
> > > > > init. The end result was that the "batches:" counter in rcuperf was 0
> > > > > causing a division by 0 error in the results. This counter was 0 because
> > > > > only expedited GPs seem to happen, not normal ones which led to the
> > > > > rcu_state.gp_seq counter remaining constant across grace periods which
> > > > > unexpectedly happen to be expedited. The system was running expedited
> > > > > RCU all the time because rcu_unexpedited_gp() would not have run yet
> > > > > from init.  In other words, the test would concurrently with init
> > > > > booting in expedited GP mode.
> > > > > 
> > > > > To fix this properly, let us check if system_state if SYSTEM_RUNNING
> > > > > is set before starting the test. The system_state approximately aligns
> > > 
> > > Just minor typo..
> > > 
> > > To fix this properly, let us check if system_state if SYSTEM_RUNNING
> > > is set before starting the test. ...
> > > 
> > > Should be
> > > 
> > > To fix this properly, let us check if system_state is set to
> > > SYSTEM_RUNNING before starting the test. ...
> > 
> > That's a fair point. I wonder if Paul already fixed it up in his tree,
> > however I am happy to resend if he hasn't. Paul, how would you like to handle
> > this commit log nit?
> > 
> > it is just 'if ..' to 'is SYSTEM_RUNNING'
> 
> It now reads as follows:
> 
> 	To fix this properly, this commit waits until system_state is
> 	set to SYSTEM_RUNNING before starting the test.  This change is
> 	made just before kernel_init() invokes rcu_end_inkernel_boot(),
> 	and this latter is what turns off boot-time expediting of RCU
> 	grace periods.

Ok, looks good to me, thanks.

And for below patch,

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>


> I dropped the last paragraph about late_initcall().  And I suspect that
> the last clause from rcu_gp_is_expedited() can be dropped:
> 
> bool rcu_gp_is_expedited(void)
> {
> 	return rcu_expedited || atomic_read(&rcu_expedited_nesting) ||
> 	       rcu_scheduler_active == RCU_SCHEDULER_INIT;
> }
> 
> This is because rcu_expedited_nesting is initialized to 1, and is
> decremented in rcu_end_inkernel_boot(), which is called long after
> rcu_scheduler_active has been set to RCU_SCHEDULER_RUNNING, which
> happens at core_initcall() time.  So if the last clause says "true",
> so does the second-to-last clause.
> 
> The similar check in rcu_gp_is_normal() is still need, however, to allow
> the power-management subsystem to invoke synchronize_rcu() just after
> the scheduler has been initialized, but before RCU is aware of this.
> 
> So, how about the commit shown below?
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit 1f7e72efe3c761c2b34da7b59e01ad69c657db10
> Author: Paul E. McKenney <paulmck@linux.ibm.com>
> Date:   Fri Jul 5 08:05:10 2019 -0700
> 
>     rcu: Remove redundant "if" condition from rcu_gp_is_expedited()
>     
>     Because rcu_expedited_nesting is initialized to 1 and not decremented
>     until just before init is spawned, rcu_expedited_nesting is guaranteed
>     to be non-zero whenever rcu_scheduler_active == RCU_SCHEDULER_INIT.
>     This commit therefore removes this redundant "if" equality test.
>     
>     Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
> 
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index 249517058b13..64e9cc8609e7 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -136,8 +136,7 @@ static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
>   */
>  bool rcu_gp_is_expedited(void)
>  {
> -	return rcu_expedited || atomic_read(&rcu_expedited_nesting) ||
> -	       rcu_scheduler_active == RCU_SCHEDULER_INIT;
> +	return rcu_expedited || atomic_read(&rcu_expedited_nesting);
>  }
>  EXPORT_SYMBOL_GPL(rcu_gp_is_expedited);
>  
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] rcuperf: Make rcuperf kernel test more robust for !expedited mode
  2019-07-05 20:00         ` Joel Fernandes
@ 2019-07-13 14:18           ` Paul E. McKenney
  0 siblings, 0 replies; 7+ messages in thread
From: Paul E. McKenney @ 2019-07-13 14:18 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: Byungchul Park, linux-kernel, Davidlohr Bueso, Josh Triplett,
	Lai Jiangshan, Mathieu Desnoyers, rcu, Steven Rostedt,
	kernel-team

On Fri, Jul 05, 2019 at 04:00:03PM -0400, Joel Fernandes wrote:
> On Fri, Jul 05, 2019 at 08:09:32AM -0700, Paul E. McKenney wrote:
> > On Fri, Jul 05, 2019 at 08:24:50AM -0400, Joel Fernandes wrote:
> > > On Fri, Jul 05, 2019 at 12:52:31PM +0900, Byungchul Park wrote:
> > > > On Thu, Jul 04, 2019 at 10:40:44AM -0700, Paul E. McKenney wrote:
> > > > > On Thu, Jul 04, 2019 at 12:34:30AM -0400, Joel Fernandes (Google) wrote:
> > > > > > It is possible that the rcuperf kernel test runs concurrently with init
> > > > > > starting up.  During this time, the system is running all grace periods
> > > > > > as expedited.  However, rcuperf can also be run for normal GP tests.
> > > > > > Right now, it depends on a holdoff time before starting the test to
> > > > > > ensure grace periods start later. This works fine with the default
> > > > > > holdoff time however it is not robust in situations where init takes
> > > > > > greater than the holdoff time to finish running. Or, as in my case:
> > > > > > 
> > > > > > I modified the rcuperf test locally to also run a thread that did
> > > > > > preempt disable/enable in a loop. This had the effect of slowing down
> > > > > > init. The end result was that the "batches:" counter in rcuperf was 0
> > > > > > causing a division by 0 error in the results. This counter was 0 because
> > > > > > only expedited GPs seem to happen, not normal ones which led to the
> > > > > > rcu_state.gp_seq counter remaining constant across grace periods which
> > > > > > unexpectedly happen to be expedited. The system was running expedited
> > > > > > RCU all the time because rcu_unexpedited_gp() would not have run yet
> > > > > > from init.  In other words, the test would concurrently with init
> > > > > > booting in expedited GP mode.
> > > > > > 
> > > > > > To fix this properly, let us check if system_state if SYSTEM_RUNNING
> > > > > > is set before starting the test. The system_state approximately aligns
> > > > 
> > > > Just minor typo..
> > > > 
> > > > To fix this properly, let us check if system_state if SYSTEM_RUNNING
> > > > is set before starting the test. ...
> > > > 
> > > > Should be
> > > > 
> > > > To fix this properly, let us check if system_state is set to
> > > > SYSTEM_RUNNING before starting the test. ...
> > > 
> > > That's a fair point. I wonder if Paul already fixed it up in his tree,
> > > however I am happy to resend if he hasn't. Paul, how would you like to handle
> > > this commit log nit?
> > > 
> > > it is just 'if ..' to 'is SYSTEM_RUNNING'
> > 
> > It now reads as follows:
> > 
> > 	To fix this properly, this commit waits until system_state is
> > 	set to SYSTEM_RUNNING before starting the test.  This change is
> > 	made just before kernel_init() invokes rcu_end_inkernel_boot(),
> > 	and this latter is what turns off boot-time expediting of RCU
> > 	grace periods.
> 
> Ok, looks good to me, thanks.
> 
> And for below patch,
> 
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Applied, thank you!

							Thnax, Paul

> > I dropped the last paragraph about late_initcall().  And I suspect that
> > the last clause from rcu_gp_is_expedited() can be dropped:
> > 
> > bool rcu_gp_is_expedited(void)
> > {
> > 	return rcu_expedited || atomic_read(&rcu_expedited_nesting) ||
> > 	       rcu_scheduler_active == RCU_SCHEDULER_INIT;
> > }
> > 
> > This is because rcu_expedited_nesting is initialized to 1, and is
> > decremented in rcu_end_inkernel_boot(), which is called long after
> > rcu_scheduler_active has been set to RCU_SCHEDULER_RUNNING, which
> > happens at core_initcall() time.  So if the last clause says "true",
> > so does the second-to-last clause.
> > 
> > The similar check in rcu_gp_is_normal() is still need, however, to allow
> > the power-management subsystem to invoke synchronize_rcu() just after
> > the scheduler has been initialized, but before RCU is aware of this.
> > 
> > So, how about the commit shown below?
> > 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > commit 1f7e72efe3c761c2b34da7b59e01ad69c657db10
> > Author: Paul E. McKenney <paulmck@linux.ibm.com>
> > Date:   Fri Jul 5 08:05:10 2019 -0700
> > 
> >     rcu: Remove redundant "if" condition from rcu_gp_is_expedited()
> >     
> >     Because rcu_expedited_nesting is initialized to 1 and not decremented
> >     until just before init is spawned, rcu_expedited_nesting is guaranteed
> >     to be non-zero whenever rcu_scheduler_active == RCU_SCHEDULER_INIT.
> >     This commit therefore removes this redundant "if" equality test.
> >     
> >     Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com>
> > 
> > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > index 249517058b13..64e9cc8609e7 100644
> > --- a/kernel/rcu/update.c
> > +++ b/kernel/rcu/update.c
> > @@ -136,8 +136,7 @@ static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
> >   */
> >  bool rcu_gp_is_expedited(void)
> >  {
> > -	return rcu_expedited || atomic_read(&rcu_expedited_nesting) ||
> > -	       rcu_scheduler_active == RCU_SCHEDULER_INIT;
> > +	return rcu_expedited || atomic_read(&rcu_expedited_nesting);
> >  }
> >  EXPORT_SYMBOL_GPL(rcu_gp_is_expedited);
> >  
> > 
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-07-13 14:19 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-04  4:34 [PATCH] rcuperf: Make rcuperf kernel test more robust for !expedited mode Joel Fernandes (Google)
2019-07-04 17:40 ` Paul E. McKenney
2019-07-05  3:52   ` Byungchul Park
2019-07-05 12:24     ` Joel Fernandes
2019-07-05 15:09       ` Paul E. McKenney
2019-07-05 20:00         ` Joel Fernandes
2019-07-13 14:18           ` Paul E. McKenney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).