Making rcu_normal=1 in RT

Making rcu_normal=1 in RT

[Luiz Capitulino]

Hi,

We have the following patch applied to the RT tree:

  commit a9d3cc781a3306bfa332fa7cb6134b70696058d5
  Author: Josh Cartwright <joshc@ni.com>
  Date:   Tue Oct 27 07:31:53 2015 -0500
  
      net: Make synchronize_rcu_expedited() conditional on !RT_FULL

However, as noted by Michael, making rcu_normal=1 default in the
RT kernel should have the same effect (ie.  not calling
synchronize_sched_expedited()).

So, honest question, is there a reason not to:

 1. Drop the patch above
 2. Make rcu_normal=1 default?

Re: Making rcu_normal=1 in RT

[Julia Cartwright]

On Wed, Oct 12, 2016 at 11:12:51AM -0400, Luiz Capitulino wrote:
> Hi,
> 
> We have the following patch applied to the RT tree:
> 
>   commit a9d3cc781a3306bfa332fa7cb6134b70696058d5
>   Author: Josh Cartwright <joshc@ni.com>
>   Date:   Tue Oct 27 07:31:53 2015 -0500
>   
>       net: Make synchronize_rcu_expedited() conditional on !RT_FULL
> 
> However, as noted by Michael, making rcu_normal=1 default in the
> RT kernel should have the same effect (ie.  not calling
> synchronize_sched_expedited()).
> 
> So, honest question, is there a reason not to:
> 
>  1. Drop the patch above
>  2. Make rcu_normal=1 default?

I think this is probably a cleaner way to fix the problems which
motivated this patch in the first place.  In my defense, the above patch
predates rcu_normal :).

Something like this, perhaps?

   Julia

-- 8< --
Subject: [PATCH] rcu: enable rcu_normal_after_boot by default for RT

The forcing of an expedited grace period is an expensive and very
RT-application unfriendly operation, as it forcibly preempts all running
tasks on CPUs which are preventing the gp from expiring.

By default, as a policy decision, disable the expediting of grace
periods (after boot) on configurations which enable PREEMPT_RT_FULL.

Suggested-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Julia Cartwright <julia@ni.com>
---
 kernel/rcu/update.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index b40d346..4975917 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -63,7 +63,7 @@ MODULE_ALIAS("rcupdate");
 #ifndef CONFIG_TINY_RCU
 module_param(rcu_expedited, int, 0);
 module_param(rcu_normal, int, 0);
-static int rcu_normal_after_boot;
+static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT_FULL);
 module_param(rcu_normal_after_boot, int, 0);
 #endif /* #ifndef CONFIG_TINY_RCU */
 
-- 
2.9.3

Re: Making rcu_normal=1 in RT

[Paul E. McKenney]

On Wed, Oct 12, 2016 at 11:21:14AM -0500, Julia Cartwright wrote:
> On Wed, Oct 12, 2016 at 11:12:51AM -0400, Luiz Capitulino wrote:
> > Hi,
> > 
> > We have the following patch applied to the RT tree:
> > 
> >   commit a9d3cc781a3306bfa332fa7cb6134b70696058d5
> >   Author: Josh Cartwright <joshc@ni.com>
> >   Date:   Tue Oct 27 07:31:53 2015 -0500
> >   
> >       net: Make synchronize_rcu_expedited() conditional on !RT_FULL
> > 
> > However, as noted by Michael, making rcu_normal=1 default in the
> > RT kernel should have the same effect (ie.  not calling
> > synchronize_sched_expedited()).
> > 
> > So, honest question, is there a reason not to:
> > 
> >  1. Drop the patch above
> >  2. Make rcu_normal=1 default?
> 
> I think this is probably a cleaner way to fix the problems which
> motivated this patch in the first place.  In my defense, the above patch
> predates rcu_normal :).
> 
> Something like this, perhaps?
> 
>    Julia
> 
> -- 8< --
> Subject: [PATCH] rcu: enable rcu_normal_after_boot by default for RT
> 
> The forcing of an expedited grace period is an expensive and very
> RT-application unfriendly operation, as it forcibly preempts all running
> tasks on CPUs which are preventing the gp from expiring.
> 
> By default, as a policy decision, disable the expediting of grace
> periods (after boot) on configurations which enable PREEMPT_RT_FULL.
> 
> Suggested-by: Luiz Capitulino <lcapitulino@redhat.com>
> Signed-off-by: Julia Cartwright <julia@ni.com>

Looks like a reasonable approach to me.  Objections?

							Thanx, Paul

> ---
>  kernel/rcu/update.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index b40d346..4975917 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -63,7 +63,7 @@ MODULE_ALIAS("rcupdate");
>  #ifndef CONFIG_TINY_RCU
>  module_param(rcu_expedited, int, 0);
>  module_param(rcu_normal, int, 0);
> -static int rcu_normal_after_boot;
> +static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT_FULL);
>  module_param(rcu_normal_after_boot, int, 0);
>  #endif /* #ifndef CONFIG_TINY_RCU */
> 
> -- 
> 2.9.3
> 

Re: Making rcu_normal=1 in RT

[Luiz Capitulino]

On Wed, 12 Oct 2016 11:21:14 -0500
Julia Cartwright <julia@ni.com> wrote:

> On Wed, Oct 12, 2016 at 11:12:51AM -0400, Luiz Capitulino wrote:
> > Hi,
> > 
> > We have the following patch applied to the RT tree:
> > 
> >   commit a9d3cc781a3306bfa332fa7cb6134b70696058d5
> >   Author: Josh Cartwright <joshc@ni.com>
> >   Date:   Tue Oct 27 07:31:53 2015 -0500
> >   
> >       net: Make synchronize_rcu_expedited() conditional on !RT_FULL
> > 
> > However, as noted by Michael, making rcu_normal=1 default in the
> > RT kernel should have the same effect (ie.  not calling
> > synchronize_sched_expedited()).
> > 
> > So, honest question, is there a reason not to:
> > 
> >  1. Drop the patch above
> >  2. Make rcu_normal=1 default?  
> 
> I think this is probably a cleaner way to fix the problems which
> motivated this patch in the first place.  In my defense, the above patch
> predates rcu_normal :).

No need to defend yourself! We debugged this very spike in one of
our kernels that don't have rcu_normal. We decided to do exactly
what you're doing before looking at upstream. Your patch helped
us confirm that we were in the right track.

> Something like this, perhaps?

Looks good, but (honest question) what does it buy us using
rcu_normal_after_boot vs rcu_normal? Is the boot process
improved someway?

As long as we're rcu_normal=1 before launching user-space,
this should be fine.

> 
>    Julia
> 
> -- 8< --
> Subject: [PATCH] rcu: enable rcu_normal_after_boot by default for RT
> 
> The forcing of an expedited grace period is an expensive and very
> RT-application unfriendly operation, as it forcibly preempts all running
> tasks on CPUs which are preventing the gp from expiring.
> 
> By default, as a policy decision, disable the expediting of grace
> periods (after boot) on configurations which enable PREEMPT_RT_FULL.
> 
> Suggested-by: Luiz Capitulino <lcapitulino@redhat.com>
> Signed-off-by: Julia Cartwright <julia@ni.com>
> ---
>  kernel/rcu/update.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index b40d346..4975917 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -63,7 +63,7 @@ MODULE_ALIAS("rcupdate");
>  #ifndef CONFIG_TINY_RCU
>  module_param(rcu_expedited, int, 0);
>  module_param(rcu_normal, int, 0);
> -static int rcu_normal_after_boot;
> +static int rcu_normal_after_boot = IS_ENABLED(CONFIG_PREEMPT_RT_FULL);
>  module_param(rcu_normal_after_boot, int, 0);
>  #endif /* #ifndef CONFIG_TINY_RCU */
>  

Re: Making rcu_normal=1 in RT

[Julia Cartwright]

On Wed, Oct 12, 2016 at 12:49:56PM -0400, Luiz Capitulino wrote:
> On Wed, 12 Oct 2016 11:21:14 -0500
> Julia Cartwright <julia@ni.com> wrote:
> 
> > On Wed, Oct 12, 2016 at 11:12:51AM -0400, Luiz Capitulino wrote:
> > > Hi,
> > > 
> > > We have the following patch applied to the RT tree:
> > > 
> > >   commit a9d3cc781a3306bfa332fa7cb6134b70696058d5
> > >   Author: Josh Cartwright <joshc@ni.com>
> > >   Date:   Tue Oct 27 07:31:53 2015 -0500
> > >   
> > >       net: Make synchronize_rcu_expedited() conditional on !RT_FULL
> > > 
> > > However, as noted by Michael, making rcu_normal=1 default in the
> > > RT kernel should have the same effect (ie.  not calling
> > > synchronize_sched_expedited()).
> > > 
> > > So, honest question, is there a reason not to:
> > > 
> > >  1. Drop the patch above
> > >  2. Make rcu_normal=1 default?  
> > 
> > I think this is probably a cleaner way to fix the problems which
> > motivated this patch in the first place.  In my defense, the above patch
> > predates rcu_normal :).
> 
> No need to defend yourself! We debugged this very spike in one of
> our kernels that don't have rcu_normal. We decided to do exactly
> what you're doing before looking at upstream. Your patch helped
> us confirm that we were in the right track.

Great!  Glad I could help in some way!

> > Something like this, perhaps?
>
> Looks good, but (honest question) what does it buy us using
> rcu_normal_after_boot vs rcu_normal? Is the boot process
> improved someway?

That's the idea, although I don't have data to show much it actually
buys us.

> As long as we're rcu_normal=1 before launching user-space,
> this should be fine.

Agreed.

Thanks,
   Julia

Re: Making rcu_normal=1 in RT

[Paul E. McKenney]

On Wed, Oct 12, 2016 at 12:15:53PM -0500, Julia Cartwright wrote:
> On Wed, Oct 12, 2016 at 12:49:56PM -0400, Luiz Capitulino wrote:
> > On Wed, 12 Oct 2016 11:21:14 -0500
> > Julia Cartwright <julia@ni.com> wrote:
> > 
> > > On Wed, Oct 12, 2016 at 11:12:51AM -0400, Luiz Capitulino wrote:
> > > > Hi,
> > > > 
> > > > We have the following patch applied to the RT tree:
> > > > 
> > > >   commit a9d3cc781a3306bfa332fa7cb6134b70696058d5
> > > >   Author: Josh Cartwright <joshc@ni.com>
> > > >   Date:   Tue Oct 27 07:31:53 2015 -0500
> > > >   
> > > >       net: Make synchronize_rcu_expedited() conditional on !RT_FULL
> > > > 
> > > > However, as noted by Michael, making rcu_normal=1 default in the
> > > > RT kernel should have the same effect (ie.  not calling
> > > > synchronize_sched_expedited()).
> > > > 
> > > > So, honest question, is there a reason not to:
> > > > 
> > > >  1. Drop the patch above
> > > >  2. Make rcu_normal=1 default?  
> > > 
> > > I think this is probably a cleaner way to fix the problems which
> > > motivated this patch in the first place.  In my defense, the above patch
> > > predates rcu_normal :).
> > 
> > No need to defend yourself! We debugged this very spike in one of
> > our kernels that don't have rcu_normal. We decided to do exactly
> > what you're doing before looking at upstream. Your patch helped
> > us confirm that we were in the right track.
> 
> Great!  Glad I could help in some way!
> 
> > > Something like this, perhaps?
> >
> > Looks good, but (honest question) what does it buy us using
> > rcu_normal_after_boot vs rcu_normal? Is the boot process
> > improved someway?
> 
> That's the idea, although I don't have data to show much it actually
> buys us.

It means that grace periods can be expedited during boot.  If you really
care about boot speed, you can also set rcu_expedited=1 and also
rcu_normal_after_boot=1, which will expedite all grace periods during
the boot process, but stop doing so just before spawning init.
After that point, any attempt to do an expedited grace period gets you
a normal grace period instead.

So you get fast boot and then clean realtime.

> > As long as we're rcu_normal=1 before launching user-space,
> > this should be fine.
> 
> Agreed.

Yes, you can also set them manually instead of at boot, if you wish.

							Thanx, Paul

Re: Making rcu_normal=1 in RT

[Michael S. Tsirkin]

On Wed, Oct 12, 2016 at 01:32:23PM -0700, Paul E. McKenney wrote:
> On Wed, Oct 12, 2016 at 12:15:53PM -0500, Julia Cartwright wrote:
> > On Wed, Oct 12, 2016 at 12:49:56PM -0400, Luiz Capitulino wrote:
> > > On Wed, 12 Oct 2016 11:21:14 -0500
> > > Julia Cartwright <julia@ni.com> wrote:
> > > 
> > > > On Wed, Oct 12, 2016 at 11:12:51AM -0400, Luiz Capitulino wrote:
> > > > > Hi,
> > > > > 
> > > > > We have the following patch applied to the RT tree:
> > > > > 
> > > > >   commit a9d3cc781a3306bfa332fa7cb6134b70696058d5
> > > > >   Author: Josh Cartwright <joshc@ni.com>
> > > > >   Date:   Tue Oct 27 07:31:53 2015 -0500
> > > > >   
> > > > >       net: Make synchronize_rcu_expedited() conditional on !RT_FULL
> > > > > 
> > > > > However, as noted by Michael, making rcu_normal=1 default in the
> > > > > RT kernel should have the same effect (ie.  not calling
> > > > > synchronize_sched_expedited()).
> > > > > 
> > > > > So, honest question, is there a reason not to:
> > > > > 
> > > > >  1. Drop the patch above
> > > > >  2. Make rcu_normal=1 default?  
> > > > 
> > > > I think this is probably a cleaner way to fix the problems which
> > > > motivated this patch in the first place.  In my defense, the above patch
> > > > predates rcu_normal :).
> > > 
> > > No need to defend yourself! We debugged this very spike in one of
> > > our kernels that don't have rcu_normal. We decided to do exactly
> > > what you're doing before looking at upstream. Your patch helped
> > > us confirm that we were in the right track.
> > 
> > Great!  Glad I could help in some way!
> > 
> > > > Something like this, perhaps?
> > >
> > > Looks good, but (honest question) what does it buy us using
> > > rcu_normal_after_boot vs rcu_normal? Is the boot process
> > > improved someway?
> > 
> > That's the idea, although I don't have data to show much it actually
> > buys us.
> 
> It means that grace periods can be expedited during boot.  If you really
> care about boot speed, you can also set rcu_expedited=1 and also
> rcu_normal_after_boot=1, which will expedite all grace periods during
> the boot process, but stop doing so just before spawning init.
> After that point, any attempt to do an expedited grace period gets you
> a normal grace period instead.
> 
> So you get fast boot and then clean realtime.
> 
> > > As long as we're rcu_normal=1 before launching user-space,
> > > this should be fine.
> > 
> > Agreed.
> 
> Yes, you can also set them manually instead of at boot, if you wish.
> 
> 							Thanx, Paul

FWIW

Acked-by: Michael S. Tsirkin <mst@redhat.com>

But I have a question - here's the commit that started
it all:


commit be3fc413da9eb17cce0991f214ab019d16c88c41
Author: Eric Dumazet <eric.dumazet@gmail.com>
Date:   Mon May 23 23:07:32 2011 +0000

    net: use synchronize_rcu_expedited()
    
    synchronize_rcu() is very slow in various situations (HZ=100,
    CONFIG_NO_HZ=y, CONFIG_PREEMPT=n)
    
    Extract from my (mostly idle) 8 core machine :
    
     synchronize_rcu() in 99985 us
     synchronize_rcu() in 79982 us
     synchronize_rcu() in 87612 us
     synchronize_rcu() in 79827 us
     synchronize_rcu() in 109860 us
     synchronize_rcu() in 98039 us
     synchronize_rcu() in 89841 us
     synchronize_rcu() in 79842 us
     synchronize_rcu() in 80151 us
     synchronize_rcu() in 119833 us
     synchronize_rcu() in 99858 us
     synchronize_rcu() in 73999 us
     synchronize_rcu() in 79855 us
     synchronize_rcu() in 79853 us
    
    When we hold RTNL mutex, we would like to spend some cpu cycles but not
    block too long other processes waiting for this mutex.
    
    We also want to setup/dismantle network features as fast as possible at
    boot/shutdown time.


To make sure this does not regress for RT,
how about clearing this flag on shutdown as well?



-- 
MST

Re: Making rcu_normal=1 in RT

[Paul E. McKenney]

On Thu, Oct 13, 2016 at 07:25:56PM +0300, Michael S. Tsirkin wrote:
> On Wed, Oct 12, 2016 at 01:32:23PM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 12, 2016 at 12:15:53PM -0500, Julia Cartwright wrote:
> > > On Wed, Oct 12, 2016 at 12:49:56PM -0400, Luiz Capitulino wrote:
> > > > On Wed, 12 Oct 2016 11:21:14 -0500
> > > > Julia Cartwright <julia@ni.com> wrote:
> > > > 
> > > > > On Wed, Oct 12, 2016 at 11:12:51AM -0400, Luiz Capitulino wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > We have the following patch applied to the RT tree:
> > > > > > 
> > > > > >   commit a9d3cc781a3306bfa332fa7cb6134b70696058d5
> > > > > >   Author: Josh Cartwright <joshc@ni.com>
> > > > > >   Date:   Tue Oct 27 07:31:53 2015 -0500
> > > > > >   
> > > > > >       net: Make synchronize_rcu_expedited() conditional on !RT_FULL
> > > > > > 
> > > > > > However, as noted by Michael, making rcu_normal=1 default in the
> > > > > > RT kernel should have the same effect (ie.  not calling
> > > > > > synchronize_sched_expedited()).
> > > > > > 
> > > > > > So, honest question, is there a reason not to:
> > > > > > 
> > > > > >  1. Drop the patch above
> > > > > >  2. Make rcu_normal=1 default?  
> > > > > 
> > > > > I think this is probably a cleaner way to fix the problems which
> > > > > motivated this patch in the first place.  In my defense, the above patch
> > > > > predates rcu_normal :).
> > > > 
> > > > No need to defend yourself! We debugged this very spike in one of
> > > > our kernels that don't have rcu_normal. We decided to do exactly
> > > > what you're doing before looking at upstream. Your patch helped
> > > > us confirm that we were in the right track.
> > > 
> > > Great!  Glad I could help in some way!
> > > 
> > > > > Something like this, perhaps?
> > > >
> > > > Looks good, but (honest question) what does it buy us using
> > > > rcu_normal_after_boot vs rcu_normal? Is the boot process
> > > > improved someway?
> > > 
> > > That's the idea, although I don't have data to show much it actually
> > > buys us.
> > 
> > It means that grace periods can be expedited during boot.  If you really
> > care about boot speed, you can also set rcu_expedited=1 and also
> > rcu_normal_after_boot=1, which will expedite all grace periods during
> > the boot process, but stop doing so just before spawning init.
> > After that point, any attempt to do an expedited grace period gets you
> > a normal grace period instead.
> > 
> > So you get fast boot and then clean realtime.
> > 
> > > > As long as we're rcu_normal=1 before launching user-space,
> > > > this should be fine.
> > > 
> > > Agreed.
> > 
> > Yes, you can also set them manually instead of at boot, if you wish.
> > 
> > 							Thanx, Paul
> 
> FWIW
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> But I have a question - here's the commit that started
> it all:
> 
> 
> commit be3fc413da9eb17cce0991f214ab019d16c88c41
> Author: Eric Dumazet <eric.dumazet@gmail.com>
> Date:   Mon May 23 23:07:32 2011 +0000
> 
>     net: use synchronize_rcu_expedited()
>     
>     synchronize_rcu() is very slow in various situations (HZ=100,
>     CONFIG_NO_HZ=y, CONFIG_PREEMPT=n)
>     
>     Extract from my (mostly idle) 8 core machine :
>     
>      synchronize_rcu() in 99985 us
>      synchronize_rcu() in 79982 us
>      synchronize_rcu() in 87612 us
>      synchronize_rcu() in 79827 us
>      synchronize_rcu() in 109860 us
>      synchronize_rcu() in 98039 us
>      synchronize_rcu() in 89841 us
>      synchronize_rcu() in 79842 us
>      synchronize_rcu() in 80151 us
>      synchronize_rcu() in 119833 us
>      synchronize_rcu() in 99858 us
>      synchronize_rcu() in 73999 us
>      synchronize_rcu() in 79855 us
>      synchronize_rcu() in 79853 us
>     
>     When we hold RTNL mutex, we would like to spend some cpu cycles but not
>     block too long other processes waiting for this mutex.
>     
>     We also want to setup/dismantle network features as fast as possible at
>     boot/shutdown time.
> 
> 
> To make sure this does not regress for RT,
> how about clearing this flag on shutdown as well?

By that, you mean having some way to force all grace periods to be
expedited during shutdown?  Or am I missing your point?

							Thanx, Paul

Re: Making rcu_normal=1 in RT

[Michael S. Tsirkin]

On Fri, Oct 14, 2016 at 02:20:50AM -0700, Paul E. McKenney wrote:
> On Thu, Oct 13, 2016 at 07:25:56PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Oct 12, 2016 at 01:32:23PM -0700, Paul E. McKenney wrote:
> > > On Wed, Oct 12, 2016 at 12:15:53PM -0500, Julia Cartwright wrote:
> > > > On Wed, Oct 12, 2016 at 12:49:56PM -0400, Luiz Capitulino wrote:
> > > > > On Wed, 12 Oct 2016 11:21:14 -0500
> > > > > Julia Cartwright <julia@ni.com> wrote:
> > > > > 
> > > > > > On Wed, Oct 12, 2016 at 11:12:51AM -0400, Luiz Capitulino wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > We have the following patch applied to the RT tree:
> > > > > > > 
> > > > > > >   commit a9d3cc781a3306bfa332fa7cb6134b70696058d5
> > > > > > >   Author: Josh Cartwright <joshc@ni.com>
> > > > > > >   Date:   Tue Oct 27 07:31:53 2015 -0500
> > > > > > >   
> > > > > > >       net: Make synchronize_rcu_expedited() conditional on !RT_FULL
> > > > > > > 
> > > > > > > However, as noted by Michael, making rcu_normal=1 default in the
> > > > > > > RT kernel should have the same effect (ie.  not calling
> > > > > > > synchronize_sched_expedited()).
> > > > > > > 
> > > > > > > So, honest question, is there a reason not to:
> > > > > > > 
> > > > > > >  1. Drop the patch above
> > > > > > >  2. Make rcu_normal=1 default?  
> > > > > > 
> > > > > > I think this is probably a cleaner way to fix the problems which
> > > > > > motivated this patch in the first place.  In my defense, the above patch
> > > > > > predates rcu_normal :).
> > > > > 
> > > > > No need to defend yourself! We debugged this very spike in one of
> > > > > our kernels that don't have rcu_normal. We decided to do exactly
> > > > > what you're doing before looking at upstream. Your patch helped
> > > > > us confirm that we were in the right track.
> > > > 
> > > > Great!  Glad I could help in some way!
> > > > 
> > > > > > Something like this, perhaps?
> > > > >
> > > > > Looks good, but (honest question) what does it buy us using
> > > > > rcu_normal_after_boot vs rcu_normal? Is the boot process
> > > > > improved someway?
> > > > 
> > > > That's the idea, although I don't have data to show much it actually
> > > > buys us.
> > > 
> > > It means that grace periods can be expedited during boot.  If you really
> > > care about boot speed, you can also set rcu_expedited=1 and also
> > > rcu_normal_after_boot=1, which will expedite all grace periods during
> > > the boot process, but stop doing so just before spawning init.
> > > After that point, any attempt to do an expedited grace period gets you
> > > a normal grace period instead.
> > > 
> > > So you get fast boot and then clean realtime.
> > > 
> > > > > As long as we're rcu_normal=1 before launching user-space,
> > > > > this should be fine.
> > > > 
> > > > Agreed.
> > > 
> > > Yes, you can also set them manually instead of at boot, if you wish.
> > > 
> > > 							Thanx, Paul
> > 
> > FWIW
> > 
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > But I have a question - here's the commit that started
> > it all:
> > 
> > 
> > commit be3fc413da9eb17cce0991f214ab019d16c88c41
> > Author: Eric Dumazet <eric.dumazet@gmail.com>
> > Date:   Mon May 23 23:07:32 2011 +0000
> > 
> >     net: use synchronize_rcu_expedited()
> >     
> >     synchronize_rcu() is very slow in various situations (HZ=100,
> >     CONFIG_NO_HZ=y, CONFIG_PREEMPT=n)
> >     
> >     Extract from my (mostly idle) 8 core machine :
> >     
> >      synchronize_rcu() in 99985 us
> >      synchronize_rcu() in 79982 us
> >      synchronize_rcu() in 87612 us
> >      synchronize_rcu() in 79827 us
> >      synchronize_rcu() in 109860 us
> >      synchronize_rcu() in 98039 us
> >      synchronize_rcu() in 89841 us
> >      synchronize_rcu() in 79842 us
> >      synchronize_rcu() in 80151 us
> >      synchronize_rcu() in 119833 us
> >      synchronize_rcu() in 99858 us
> >      synchronize_rcu() in 73999 us
> >      synchronize_rcu() in 79855 us
> >      synchronize_rcu() in 79853 us
> >     
> >     When we hold RTNL mutex, we would like to spend some cpu cycles but not
> >     block too long other processes waiting for this mutex.
> >     
> >     We also want to setup/dismantle network features as fast as possible at
> >     boot/shutdown time.
> > 
> > 
> > To make sure this does not regress for RT,
> > how about clearing this flag on shutdown as well?
> 
> By that, you mean having some way to force all grace periods to be
> expedited during shutdown?  Or am I missing your point?
> 
> 							Thanx, Paul

Exactly. And maybe kexec.

-- 
MST

Re: Making rcu_normal=1 in RT

[Paul E. McKenney]

On Sun, Oct 16, 2016 at 04:45:32AM +0300, Michael S. Tsirkin wrote:
> On Fri, Oct 14, 2016 at 02:20:50AM -0700, Paul E. McKenney wrote:
> > On Thu, Oct 13, 2016 at 07:25:56PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, Oct 12, 2016 at 01:32:23PM -0700, Paul E. McKenney wrote:
> > > > On Wed, Oct 12, 2016 at 12:15:53PM -0500, Julia Cartwright wrote:
> > > > > On Wed, Oct 12, 2016 at 12:49:56PM -0400, Luiz Capitulino wrote:
> > > > > > On Wed, 12 Oct 2016 11:21:14 -0500
> > > > > > Julia Cartwright <julia@ni.com> wrote:
> > > > > > 
> > > > > > > On Wed, Oct 12, 2016 at 11:12:51AM -0400, Luiz Capitulino wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > We have the following patch applied to the RT tree:
> > > > > > > > 
> > > > > > > >   commit a9d3cc781a3306bfa332fa7cb6134b70696058d5
> > > > > > > >   Author: Josh Cartwright <joshc@ni.com>
> > > > > > > >   Date:   Tue Oct 27 07:31:53 2015 -0500
> > > > > > > >   
> > > > > > > >       net: Make synchronize_rcu_expedited() conditional on !RT_FULL
> > > > > > > > 
> > > > > > > > However, as noted by Michael, making rcu_normal=1 default in the
> > > > > > > > RT kernel should have the same effect (ie.  not calling
> > > > > > > > synchronize_sched_expedited()).
> > > > > > > > 
> > > > > > > > So, honest question, is there a reason not to:
> > > > > > > > 
> > > > > > > >  1. Drop the patch above
> > > > > > > >  2. Make rcu_normal=1 default?  
> > > > > > > 
> > > > > > > I think this is probably a cleaner way to fix the problems which
> > > > > > > motivated this patch in the first place.  In my defense, the above patch
> > > > > > > predates rcu_normal :).
> > > > > > 
> > > > > > No need to defend yourself! We debugged this very spike in one of
> > > > > > our kernels that don't have rcu_normal. We decided to do exactly
> > > > > > what you're doing before looking at upstream. Your patch helped
> > > > > > us confirm that we were in the right track.
> > > > > 
> > > > > Great!  Glad I could help in some way!
> > > > > 
> > > > > > > Something like this, perhaps?
> > > > > >
> > > > > > Looks good, but (honest question) what does it buy us using
> > > > > > rcu_normal_after_boot vs rcu_normal? Is the boot process
> > > > > > improved someway?
> > > > > 
> > > > > That's the idea, although I don't have data to show much it actually
> > > > > buys us.
> > > > 
> > > > It means that grace periods can be expedited during boot.  If you really
> > > > care about boot speed, you can also set rcu_expedited=1 and also
> > > > rcu_normal_after_boot=1, which will expedite all grace periods during
> > > > the boot process, but stop doing so just before spawning init.
> > > > After that point, any attempt to do an expedited grace period gets you
> > > > a normal grace period instead.
> > > > 
> > > > So you get fast boot and then clean realtime.
> > > > 
> > > > > > As long as we're rcu_normal=1 before launching user-space,
> > > > > > this should be fine.
> > > > > 
> > > > > Agreed.
> > > > 
> > > > Yes, you can also set them manually instead of at boot, if you wish.
> > > > 
> > > > 							Thanx, Paul
> > > 
> > > FWIW
> > > 
> > > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > > 
> > > But I have a question - here's the commit that started
> > > it all:
> > > 
> > > 
> > > commit be3fc413da9eb17cce0991f214ab019d16c88c41
> > > Author: Eric Dumazet <eric.dumazet@gmail.com>
> > > Date:   Mon May 23 23:07:32 2011 +0000
> > > 
> > >     net: use synchronize_rcu_expedited()
> > >     
> > >     synchronize_rcu() is very slow in various situations (HZ=100,
> > >     CONFIG_NO_HZ=y, CONFIG_PREEMPT=n)
> > >     
> > >     Extract from my (mostly idle) 8 core machine :
> > >     
> > >      synchronize_rcu() in 99985 us
> > >      synchronize_rcu() in 79982 us
> > >      synchronize_rcu() in 87612 us
> > >      synchronize_rcu() in 79827 us
> > >      synchronize_rcu() in 109860 us
> > >      synchronize_rcu() in 98039 us
> > >      synchronize_rcu() in 89841 us
> > >      synchronize_rcu() in 79842 us
> > >      synchronize_rcu() in 80151 us
> > >      synchronize_rcu() in 119833 us
> > >      synchronize_rcu() in 99858 us
> > >      synchronize_rcu() in 73999 us
> > >      synchronize_rcu() in 79855 us
> > >      synchronize_rcu() in 79853 us
> > >     
> > >     When we hold RTNL mutex, we would like to spend some cpu cycles but not
> > >     block too long other processes waiting for this mutex.
> > >     
> > >     We also want to setup/dismantle network features as fast as possible at
> > >     boot/shutdown time.
> > > 
> > > 
> > > To make sure this does not regress for RT,
> > > how about clearing this flag on shutdown as well?
> > 
> > By that, you mean having some way to force all grace periods to be
> > expedited during shutdown?  Or am I missing your point?
> > 
> > 							Thanx, Paul
> 
> Exactly. And maybe kexec.

If the relevant maintainers are OK with that, I am OK with it as long
as it is non-default (at least to begin with) and does not introduce
additional Kconfig questions.  My guess is that a boot parameter would
work best, but something to discuss.

							Thanx, Paul

Re: Making rcu_normal=1 in RT

[Sebastian Andrzej Siewior]

On 2016-10-16 04:28:46 [-0700], Paul E. McKenney wrote:
> If the relevant maintainers are OK with that, I am OK with it as long
> as it is non-default (at least to begin with) and does not introduce
> additional Kconfig questions.  My guess is that a boot parameter would
> work best, but something to discuss.

Okay. For me to summary:
- we want rcu_normal_after_boot=1 on -RT via CONFIG_PREEMPT_RT_FULL

- this makes synchronize_rcu_expedited() behave like
  synchronize_rcu() and therefore I can drop all patches replacing
  synchronize_rcu_expedited() with synchronize_rcu().

- optionally it has been requested to make synchronize_rcu() behave like
  synchronize_rcu_expedited() on shutdown and kexec().

Did I miss understood / forgot something?
> 							Thanx, Paul

Sebastian

Re: Making rcu_normal=1 in RT

[Paul E. McKenney]

On Mon, Oct 31, 2016 at 06:38:52PM +0100, Sebastian Andrzej Siewior wrote:
> On 2016-10-16 04:28:46 [-0700], Paul E. McKenney wrote:
> > If the relevant maintainers are OK with that, I am OK with it as long
> > as it is non-default (at least to begin with) and does not introduce
> > additional Kconfig questions.  My guess is that a boot parameter would
> > work best, but something to discuss.
> 
> Okay. For me to summary:
> - we want rcu_normal_after_boot=1 on -RT via CONFIG_PREEMPT_RT_FULL

That would be good.

> - this makes synchronize_rcu_expedited() behave like
>   synchronize_rcu() and therefore I can drop all patches replacing
>   synchronize_rcu_expedited() with synchronize_rcu().

And this would be a benefit.  ;-)

> - optionally it has been requested to make synchronize_rcu() behave like
>   synchronize_rcu_expedited() on shutdown and kexec().

You can invoke rcu_unexpedite_gp() to force all subsequennt grace periods
to be expedited, but you do need to clear rcu_normal for this to have
effect.  Right now, that means "WRITE_ONCE(rcu_normal, 0)", but I could
easily supply a formal API if you would prefer.  Which you probably
would given  that TINY_RCU doesn't have rcu_normal...

> Did I miss understood / forgot something?

It would not hurt to boot with rcu_expedited if boot speed is critical,
but I don't know whether or not this should be enabled by default.

							Thanx, Paul

> Sebastian
> 

Re: Making rcu_normal=1 in RT

[Michael S. Tsirkin]

On Mon, Oct 31, 2016 at 11:15:43AM -0700, Paul E. McKenney wrote:
> On Mon, Oct 31, 2016 at 06:38:52PM +0100, Sebastian Andrzej Siewior wrote:
> > On 2016-10-16 04:28:46 [-0700], Paul E. McKenney wrote:
> > > If the relevant maintainers are OK with that, I am OK with it as long
> > > as it is non-default (at least to begin with) and does not introduce
> > > additional Kconfig questions.  My guess is that a boot parameter would
> > > work best, but something to discuss.
> > 
> > Okay. For me to summary:
> > - we want rcu_normal_after_boot=1 on -RT via CONFIG_PREEMPT_RT_FULL
> 
> That would be good.
> 
> > - this makes synchronize_rcu_expedited() behave like
> >   synchronize_rcu() and therefore I can drop all patches replacing
> >   synchronize_rcu_expedited() with synchronize_rcu().
> 
> And this would be a benefit.  ;-)
> 
> > - optionally it has been requested to make synchronize_rcu() behave like
> >   synchronize_rcu_expedited() on shutdown and kexec().
> 
> You can invoke rcu_unexpedite_gp() to force all subsequennt grace periods
> to be expedited, but you do need to clear rcu_normal for this to have
> effect.  Right now, that means "WRITE_ONCE(rcu_normal, 0)", but I could
> easily supply a formal API if you would prefer.  Which you probably
> would given  that TINY_RCU doesn't have rcu_normal...
> 
> > Did I miss understood / forgot something?
> 
> It would not hurt to boot with rcu_expedited if boot speed is critical,
> but I don't know whether or not this should be enabled by default.
> 
> 							Thanx, Paul

I don't know either but I'd like to know ATM it's expedited
by default.


> > Sebastian
> > 

Re: Making rcu_normal=1 in RT

[Paul E. McKenney]

On Tue, Nov 01, 2016 at 12:37:04AM +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 31, 2016 at 11:15:43AM -0700, Paul E. McKenney wrote:
> > On Mon, Oct 31, 2016 at 06:38:52PM +0100, Sebastian Andrzej Siewior wrote:
> > > On 2016-10-16 04:28:46 [-0700], Paul E. McKenney wrote:
> > > > If the relevant maintainers are OK with that, I am OK with it as long
> > > > as it is non-default (at least to begin with) and does not introduce
> > > > additional Kconfig questions.  My guess is that a boot parameter would
> > > > work best, but something to discuss.
> > > 
> > > Okay. For me to summary:
> > > - we want rcu_normal_after_boot=1 on -RT via CONFIG_PREEMPT_RT_FULL
> > 
> > That would be good.
> > 
> > > - this makes synchronize_rcu_expedited() behave like
> > >   synchronize_rcu() and therefore I can drop all patches replacing
> > >   synchronize_rcu_expedited() with synchronize_rcu().
> > 
> > And this would be a benefit.  ;-)
> > 
> > > - optionally it has been requested to make synchronize_rcu() behave like
> > >   synchronize_rcu_expedited() on shutdown and kexec().
> > 
> > You can invoke rcu_unexpedite_gp() to force all subsequennt grace periods
> > to be expedited, but you do need to clear rcu_normal for this to have
> > effect.  Right now, that means "WRITE_ONCE(rcu_normal, 0)", but I could
> > easily supply a formal API if you would prefer.  Which you probably
> > would given  that TINY_RCU doesn't have rcu_normal...
> > 
> > > Did I miss understood / forgot something?
> > 
> > It would not hurt to boot with rcu_expedited if boot speed is critical,
> > but I don't know whether or not this should be enabled by default.
> > 
> > 							Thanx, Paul
> 
> I don't know either but I'd like to know ATM it's expedited
> by default.

In current mainline, no.  That is, rcu_expedited=0 by default.

Or am I missing the point of your question?

							Thanx, Paul

Re: Making rcu_normal=1 in RT

[Michael S. Tsirkin]

On Mon, Oct 31, 2016 at 07:19:41PM -0700, Paul E. McKenney wrote:
> On Tue, Nov 01, 2016 at 12:37:04AM +0200, Michael S. Tsirkin wrote:
> > On Mon, Oct 31, 2016 at 11:15:43AM -0700, Paul E. McKenney wrote:
> > > On Mon, Oct 31, 2016 at 06:38:52PM +0100, Sebastian Andrzej Siewior wrote:
> > > > On 2016-10-16 04:28:46 [-0700], Paul E. McKenney wrote:
> > > > > If the relevant maintainers are OK with that, I am OK with it as long
> > > > > as it is non-default (at least to begin with) and does not introduce
> > > > > additional Kconfig questions.  My guess is that a boot parameter would
> > > > > work best, but something to discuss.
> > > > 
> > > > Okay. For me to summary:
> > > > - we want rcu_normal_after_boot=1 on -RT via CONFIG_PREEMPT_RT_FULL
> > > 
> > > That would be good.
> > > 
> > > > - this makes synchronize_rcu_expedited() behave like
> > > >   synchronize_rcu() and therefore I can drop all patches replacing
> > > >   synchronize_rcu_expedited() with synchronize_rcu().
> > > 
> > > And this would be a benefit.  ;-)
> > > 
> > > > - optionally it has been requested to make synchronize_rcu() behave like
> > > >   synchronize_rcu_expedited() on shutdown and kexec().
> > > 
> > > You can invoke rcu_unexpedite_gp() to force all subsequennt grace periods
> > > to be expedited, but you do need to clear rcu_normal for this to have
> > > effect.  Right now, that means "WRITE_ONCE(rcu_normal, 0)", but I could
> > > easily supply a formal API if you would prefer.  Which you probably
> > > would given  that TINY_RCU doesn't have rcu_normal...
> > > 
> > > > Did I miss understood / forgot something?
> > > 
> > > It would not hurt to boot with rcu_expedited if boot speed is critical,
> > > but I don't know whether or not this should be enabled by default.
> > > 
> > > 							Thanx, Paul
> > 
> > I don't know either but I'd like to know ATM it's expedited
> > by default.
> 
> In current mainline, no.  That is, rcu_expedited=0 by default.
> 
> Or am I missing the point of your question?
> 
> 							Thanx, Paul

That's true - what I meant is that adding and removing network devices
happens on boot and calls synchronize_rcu_expedited,
with the commit log explaining that this is done for speeding up boot
and shutdown.


-- 
MST

Re: Making rcu_normal=1 in RT

[Paul E. McKenney]

On Tue, Nov 01, 2016 at 04:36:54AM +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 31, 2016 at 07:19:41PM -0700, Paul E. McKenney wrote:
> > On Tue, Nov 01, 2016 at 12:37:04AM +0200, Michael S. Tsirkin wrote:
> > > On Mon, Oct 31, 2016 at 11:15:43AM -0700, Paul E. McKenney wrote:
> > > > On Mon, Oct 31, 2016 at 06:38:52PM +0100, Sebastian Andrzej Siewior wrote:
> > > > > On 2016-10-16 04:28:46 [-0700], Paul E. McKenney wrote:
> > > > > > If the relevant maintainers are OK with that, I am OK with it as long
> > > > > > as it is non-default (at least to begin with) and does not introduce
> > > > > > additional Kconfig questions.  My guess is that a boot parameter would
> > > > > > work best, but something to discuss.
> > > > > 
> > > > > Okay. For me to summary:
> > > > > - we want rcu_normal_after_boot=1 on -RT via CONFIG_PREEMPT_RT_FULL
> > > > 
> > > > That would be good.
> > > > 
> > > > > - this makes synchronize_rcu_expedited() behave like
> > > > >   synchronize_rcu() and therefore I can drop all patches replacing
> > > > >   synchronize_rcu_expedited() with synchronize_rcu().
> > > > 
> > > > And this would be a benefit.  ;-)
> > > > 
> > > > > - optionally it has been requested to make synchronize_rcu() behave like
> > > > >   synchronize_rcu_expedited() on shutdown and kexec().
> > > > 
> > > > You can invoke rcu_unexpedite_gp() to force all subsequennt grace periods
> > > > to be expedited, but you do need to clear rcu_normal for this to have
> > > > effect.  Right now, that means "WRITE_ONCE(rcu_normal, 0)", but I could
> > > > easily supply a formal API if you would prefer.  Which you probably
> > > > would given  that TINY_RCU doesn't have rcu_normal...
> > > > 
> > > > > Did I miss understood / forgot something?
> > > > 
> > > > It would not hurt to boot with rcu_expedited if boot speed is critical,
> > > > but I don't know whether or not this should be enabled by default.
> > > > 
> > > > 							Thanx, Paul
> > > 
> > > I don't know either but I'd like to know ATM it's expedited
> > > by default.
> > 
> > In current mainline, no.  That is, rcu_expedited=0 by default.
> > 
> > Or am I missing the point of your question?
> > 
> > 							Thanx, Paul
> 
> That's true - what I meant is that adding and removing network devices
> happens on boot and calls synchronize_rcu_expedited,
> with the commit log explaining that this is done for speeding up boot
> and shutdown.

The rcu_normal boot parameter will indeed nullify this optimization.
The boot parameters are normally dumped into dmesg, which should allow
determining when this has happened.  But I do still feel like I am not
fully understanding your concern.

							Thanx, Paul

Re: Making rcu_normal=1 in RT

[Michael S. Tsirkin]

On Mon, Oct 31, 2016 at 07:52:40PM -0700, Paul E. McKenney wrote:
> On Tue, Nov 01, 2016 at 04:36:54AM +0200, Michael S. Tsirkin wrote:
> > On Mon, Oct 31, 2016 at 07:19:41PM -0700, Paul E. McKenney wrote:
> > > On Tue, Nov 01, 2016 at 12:37:04AM +0200, Michael S. Tsirkin wrote:
> > > > On Mon, Oct 31, 2016 at 11:15:43AM -0700, Paul E. McKenney wrote:
> > > > > On Mon, Oct 31, 2016 at 06:38:52PM +0100, Sebastian Andrzej Siewior wrote:
> > > > > > On 2016-10-16 04:28:46 [-0700], Paul E. McKenney wrote:
> > > > > > > If the relevant maintainers are OK with that, I am OK with it as long
> > > > > > > as it is non-default (at least to begin with) and does not introduce
> > > > > > > additional Kconfig questions.  My guess is that a boot parameter would
> > > > > > > work best, but something to discuss.
> > > > > > 
> > > > > > Okay. For me to summary:
> > > > > > - we want rcu_normal_after_boot=1 on -RT via CONFIG_PREEMPT_RT_FULL
> > > > > 
> > > > > That would be good.
> > > > > 
> > > > > > - this makes synchronize_rcu_expedited() behave like
> > > > > >   synchronize_rcu() and therefore I can drop all patches replacing
> > > > > >   synchronize_rcu_expedited() with synchronize_rcu().
> > > > > 
> > > > > And this would be a benefit.  ;-)
> > > > > 
> > > > > > - optionally it has been requested to make synchronize_rcu() behave like
> > > > > >   synchronize_rcu_expedited() on shutdown and kexec().
> > > > > 
> > > > > You can invoke rcu_unexpedite_gp() to force all subsequennt grace periods
> > > > > to be expedited, but you do need to clear rcu_normal for this to have
> > > > > effect.  Right now, that means "WRITE_ONCE(rcu_normal, 0)", but I could
> > > > > easily supply a formal API if you would prefer.  Which you probably
> > > > > would given  that TINY_RCU doesn't have rcu_normal...
> > > > > 
> > > > > > Did I miss understood / forgot something?
> > > > > 
> > > > > It would not hurt to boot with rcu_expedited if boot speed is critical,
> > > > > but I don't know whether or not this should be enabled by default.
> > > > > 
> > > > > 							Thanx, Paul
> > > > 
> > > > I don't know either but I'd like to know ATM it's expedited
> > > > by default.
> > > 
> > > In current mainline, no.  That is, rcu_expedited=0 by default.
> > > 
> > > Or am I missing the point of your question?
> > > 
> > > 							Thanx, Paul
> > 
> > That's true - what I meant is that adding and removing network devices
> > happens on boot and calls synchronize_rcu_expedited,
> > with the commit log explaining that this is done for speeding up boot
> > and shutdown.
> 
> The rcu_normal boot parameter will indeed nullify this optimization.
> The boot parameters are normally dumped into dmesg, which should allow
> determining when this has happened.  But I do still feel like I am not
> fully understanding your concern.
> 
> 							Thanx, Paul

I thought you asked whether people that care about boot time
should just boot with rcu_expedited=1.

I was trying to say that the fact net core calls expedited sync
implies that many people care about boot time.

Should rcu_normal_after_boot override rcu_expedited?
I'm not sure what happens if you specify both ATM.

-- 
MST

Re: Making rcu_normal=1 in RT

[Sebastian Andrzej Siewior]

On 2016-11-01 05:31:56 [+0200], Michael S. Tsirkin wrote:
> > > > > > 
> > > > > > > - optionally it has been requested to make synchronize_rcu() behave like
> > > > > > >   synchronize_rcu_expedited() on shutdown and kexec().
> 
> I was trying to say that the fact net core calls expedited sync
> implies that many people care about boot time.
> 
> Should rcu_normal_after_boot override rcu_expedited?
> I'm not sure what happens if you specify both ATM.

I think what you asked for is something like this:

diff --git a/kernel/reboot.c b/kernel/reboot.c
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -283,6 +283,7 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
 	struct pid_namespace *pid_ns = task_active_pid_ns(current);
 	char buffer[256];
 	int ret = 0;
+	bool force_rcu_exp = false;
 
 	/* We only trust the superuser with rebooting the system. */
 	if (!ns_capable(pid_ns->user_ns, CAP_SYS_BOOT))
@@ -311,7 +312,19 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
 	if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off)
 		cmd = LINUX_REBOOT_CMD_HALT;
 
+	switch (cmd) {
+	case LINUX_REBOOT_CMD_RESTART:
+	case LINUX_REBOOT_CMD_HALT:
+	case LINUX_REBOOT_CMD_POWER_OFF:
+	case LINUX_REBOOT_CMD_RESTART2:
+	case LINUX_REBOOT_CMD_KEXEC:
+		force_rcu_exp = true;
+	}
+
 	mutex_lock(&reboot_mutex);
+	if (force_rcu_exp)
+		rcu_expedite_gp();
+
 	switch (cmd) {
 	case LINUX_REBOOT_CMD_RESTART:
 		kernel_restart(NULL);
@@ -362,6 +375,8 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
 		ret = -EINVAL;
 		break;
 	}
+	if (force_rcu_exp)
+		rcu_unexpedite_gp();
 	mutex_unlock(&reboot_mutex);
 	return ret;
 }

and problem that this patch is pointless because it is too late. This
RESTART, POWER_OFF, KEXEC hooks here called once the `shutdown' binary
is done doing sync & cleanup and invokes the kernel for the final
operation.
So to achieve what you ask for, we would need a
LINUX_REBOOT_CMD_RCU_FAST option which is invoked by the `shutdown'
binary as the first thing - long before it gets to
LINUX_REBOOT_CMD_RESTART (or KEXEC).

Sebastian

Re: Making rcu_normal=1 in RT

[Paul E. McKenney]

On Wed, Nov 02, 2016 at 05:05:47PM +0100, Sebastian Andrzej Siewior wrote:
> On 2016-11-01 05:31:56 [+0200], Michael S. Tsirkin wrote:
> > > > > > > 
> > > > > > > > - optionally it has been requested to make synchronize_rcu() behave like
> > > > > > > >   synchronize_rcu_expedited() on shutdown and kexec().
> > 
> > I was trying to say that the fact net core calls expedited sync
> > implies that many people care about boot time.
> > 
> > Should rcu_normal_after_boot override rcu_expedited?
> > I'm not sure what happens if you specify both ATM.
> 
> I think what you asked for is something like this:
> 
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -283,6 +283,7 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
>  	struct pid_namespace *pid_ns = task_active_pid_ns(current);
>  	char buffer[256];
>  	int ret = 0;
> +	bool force_rcu_exp = false;
> 
>  	/* We only trust the superuser with rebooting the system. */
>  	if (!ns_capable(pid_ns->user_ns, CAP_SYS_BOOT))
> @@ -311,7 +312,19 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
>  	if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off)
>  		cmd = LINUX_REBOOT_CMD_HALT;
> 
> +	switch (cmd) {
> +	case LINUX_REBOOT_CMD_RESTART:
> +	case LINUX_REBOOT_CMD_HALT:
> +	case LINUX_REBOOT_CMD_POWER_OFF:
> +	case LINUX_REBOOT_CMD_RESTART2:
> +	case LINUX_REBOOT_CMD_KEXEC:
> +		force_rcu_exp = true;
> +	}
> +
>  	mutex_lock(&reboot_mutex);
> +	if (force_rcu_exp)
> +		rcu_expedite_gp();
> +
>  	switch (cmd) {
>  	case LINUX_REBOOT_CMD_RESTART:
>  		kernel_restart(NULL);
> @@ -362,6 +375,8 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
>  		ret = -EINVAL;
>  		break;
>  	}
> +	if (force_rcu_exp)
> +		rcu_unexpedite_gp();
>  	mutex_unlock(&reboot_mutex);
>  	return ret;
>  }
> 
> and problem that this patch is pointless because it is too late. This
> RESTART, POWER_OFF, KEXEC hooks here called once the `shutdown' binary
> is done doing sync & cleanup and invokes the kernel for the final
> operation.
> So to achieve what you ask for, we would need a
> LINUX_REBOOT_CMD_RCU_FAST option which is invoked by the `shutdown'
> binary as the first thing - long before it gets to
> LINUX_REBOOT_CMD_RESTART (or KEXEC).

FWIW, this looks sane to me.

							Thanx, Paul

[PATCH] rcu: update: make RCU_EXPEDITE_BOOT default

[Sebastian Andrzej Siewior]

RCU_EXPEDITE_BOOT should speed up the boot process by enforcing
synchronize_rcu_expedited() instead of synchronize_rcu() during the boot
process. There should be no reason why one does not want this and there
is no need worry about real time latency at this point.
Therefore make it default.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
Checked on two boxes and synchronize_rcu() was invoked four times before
it reached rcu_end_inkernel_boot()

 init/Kconfig        | 13 -------------
 kernel/rcu/update.c |  6 ++----
 2 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index b6c9166d878a..fe51bd3bbc61 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -771,19 +771,6 @@ config RCU_NOCB_CPU_ALL
 
 endchoice
 
-config RCU_EXPEDITE_BOOT
-	bool
-	default n
-	help
-	  This option enables expedited grace periods at boot time,
-	  as if rcu_expedite_gp() had been invoked early in boot.
-	  The corresponding rcu_unexpedite_gp() is invoked from
-	  rcu_end_inkernel_boot(), which is intended to be invoked
-	  at the end of the kernel-only boot sequence, just before
-	  init is exec'ed.
-
-	  Accept the default if unsure.
-
 endmenu # "RCU Subsystem"
 
 config BUILD_BIN2C
diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index b40d3468ba4e..419ca811bda9 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -130,8 +130,7 @@ bool rcu_gp_is_normal(void)
 }
 EXPORT_SYMBOL_GPL(rcu_gp_is_normal);
 
-static atomic_t rcu_expedited_nesting =
-	ATOMIC_INIT(IS_ENABLED(CONFIG_RCU_EXPEDITE_BOOT) ? 1 : 0);
+static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
 
 /*
  * Should normal grace-period primitives be expedited?  Intended for
@@ -179,8 +178,7 @@ EXPORT_SYMBOL_GPL(rcu_unexpedite_gp);
  */
 void rcu_end_inkernel_boot(void)
 {
-	if (IS_ENABLED(CONFIG_RCU_EXPEDITE_BOOT))
-		rcu_unexpedite_gp();
+	rcu_unexpedite_gp();
 	if (rcu_normal_after_boot)
 		WRITE_ONCE(rcu_normal, 1);
 }
-- 
2.10.2

Re: [PATCH] rcu: update: make RCU_EXPEDITE_BOOT default

[Paul E. McKenney]

On Wed, Nov 02, 2016 at 05:30:02PM +0100, Sebastian Andrzej Siewior wrote:
> RCU_EXPEDITE_BOOT should speed up the boot process by enforcing
> synchronize_rcu_expedited() instead of synchronize_rcu() during the boot
> process. There should be no reason why one does not want this and there
> is no need worry about real time latency at this point.
> Therefore make it default.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Well, it has been awhile since I removed a Kconfig parameter.

So why could this be a bad thing?

1.	Very large systems might see scalability issues with unconditional
	expediting at boot.  But if we don't try it, we won't know.

2.	People bringing up new hardware might not want quite so many
	IPIs.  But they can just set rcu_normal to prevent that.

I am therefore queuing it for testiong and review.  ;-)

							Thanx, Paul

> ---
> Checked on two boxes and synchronize_rcu() was invoked four times before
> it reached rcu_end_inkernel_boot()
> 
>  init/Kconfig        | 13 -------------
>  kernel/rcu/update.c |  6 ++----
>  2 files changed, 2 insertions(+), 17 deletions(-)
> 
> diff --git a/init/Kconfig b/init/Kconfig
> index b6c9166d878a..fe51bd3bbc61 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -771,19 +771,6 @@ config RCU_NOCB_CPU_ALL
> 
>  endchoice
> 
> -config RCU_EXPEDITE_BOOT
> -	bool
> -	default n
> -	help
> -	  This option enables expedited grace periods at boot time,
> -	  as if rcu_expedite_gp() had been invoked early in boot.
> -	  The corresponding rcu_unexpedite_gp() is invoked from
> -	  rcu_end_inkernel_boot(), which is intended to be invoked
> -	  at the end of the kernel-only boot sequence, just before
> -	  init is exec'ed.
> -
> -	  Accept the default if unsure.
> -
>  endmenu # "RCU Subsystem"
> 
>  config BUILD_BIN2C
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index b40d3468ba4e..419ca811bda9 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -130,8 +130,7 @@ bool rcu_gp_is_normal(void)
>  }
>  EXPORT_SYMBOL_GPL(rcu_gp_is_normal);
> 
> -static atomic_t rcu_expedited_nesting =
> -	ATOMIC_INIT(IS_ENABLED(CONFIG_RCU_EXPEDITE_BOOT) ? 1 : 0);
> +static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
> 
>  /*
>   * Should normal grace-period primitives be expedited?  Intended for
> @@ -179,8 +178,7 @@ EXPORT_SYMBOL_GPL(rcu_unexpedite_gp);
>   */
>  void rcu_end_inkernel_boot(void)
>  {
> -	if (IS_ENABLED(CONFIG_RCU_EXPEDITE_BOOT))
> -		rcu_unexpedite_gp();
> +	rcu_unexpedite_gp();
>  	if (rcu_normal_after_boot)
>  		WRITE_ONCE(rcu_normal, 1);
>  }
> -- 
> 2.10.2
> 

Re: [PATCH] rcu: update: make RCU_EXPEDITE_BOOT default

[Sebastian Andrzej Siewior]

On 2016-11-03 09:22:28 [-0700], Paul E. McKenney wrote:
> On Wed, Nov 02, 2016 at 05:30:02PM +0100, Sebastian Andrzej Siewior wrote:
> > RCU_EXPEDITE_BOOT should speed up the boot process by enforcing
> > synchronize_rcu_expedited() instead of synchronize_rcu() during the boot
> > process. There should be no reason why one does not want this and there
> > is no need worry about real time latency at this point.
> > Therefore make it default.
> > 
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> Well, it has been awhile since I removed a Kconfig parameter.
> 
> So why could this be a bad thing?
> 
> 1.	Very large systems might see scalability issues with unconditional
> 	expediting at boot.  But if we don't try it, we won't know.

You mean we would make the boot process slower for them instead of
faster?

> 2.	People bringing up new hardware might not want quite so many
> 	IPIs.  But they can just set rcu_normal to prevent that.

I wanted to make things simple and not complicated…

> I am therefore queuing it for testiong and review.  ;-)

Okay thanks.

> 							Thanx, Paul

Sebastian

Re: [PATCH] rcu: update: make RCU_EXPEDITE_BOOT default

[Paul E. McKenney]

On Thu, Nov 03, 2016 at 05:33:27PM +0100, Sebastian Andrzej Siewior wrote:
> On 2016-11-03 09:22:28 [-0700], Paul E. McKenney wrote:
> > On Wed, Nov 02, 2016 at 05:30:02PM +0100, Sebastian Andrzej Siewior wrote:
> > > RCU_EXPEDITE_BOOT should speed up the boot process by enforcing
> > > synchronize_rcu_expedited() instead of synchronize_rcu() during the boot
> > > process. There should be no reason why one does not want this and there
> > > is no need worry about real time latency at this point.
> > > Therefore make it default.
> > > 
> > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > 
> > Well, it has been awhile since I removed a Kconfig parameter.
> > 
> > So why could this be a bad thing?
> > 
> > 1.	Very large systems might see scalability issues with unconditional
> > 	expediting at boot.  But if we don't try it, we won't know.
> 
> You mean we would make the boot process slower for them instead of
> faster?

For really bit systems, quite possibly, where "really big" means
many hundreds or (more likely) thousands of CPUs.

But there are things that I can do to fix this when and if.

> > 2.	People bringing up new hardware might not want quite so many
> > 	IPIs.  But they can just set rcu_normal to prevent that.
> 
> I wanted to make things simple and not complicated…

I know that feeling.  ;-)

> > I am therefore queuing it for testiong and review.  ;-)
> 
> Okay thanks.

							Thanx, Paul

> Sebastian
> 

Re: [PATCH] rcu: update: make RCU_EXPEDITE_BOOT default

[Steven Rostedt]

On Thu, 3 Nov 2016 09:59:31 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> On Thu, Nov 03, 2016 at 05:33:27PM +0100, Sebastian Andrzej Siewior wrote:
> > On 2016-11-03 09:22:28 [-0700], Paul E. McKenney wrote:  
> > > On Wed, Nov 02, 2016 at 05:30:02PM +0100, Sebastian Andrzej Siewior wrote:  
> > > > RCU_EXPEDITE_BOOT should speed up the boot process by enforcing
> > > > synchronize_rcu_expedited() instead of synchronize_rcu() during the boot
> > > > process. There should be no reason why one does not want this and there
> > > > is no need worry about real time latency at this point.
> > > > Therefore make it default.
> > > > 
> > > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>  
> > > 
> > > Well, it has been awhile since I removed a Kconfig parameter.
> > > 
> > > So why could this be a bad thing?
> > > 
> > > 1.	Very large systems might see scalability issues with unconditional
> > > 	expediting at boot.  But if we don't try it, we won't know.  
> > 
> > You mean we would make the boot process slower for them instead of
> > faster?  
> 
> For really bit systems, quite possibly, where "really big" means
> many hundreds or (more likely) thousands of CPUs.
> 
> But there are things that I can do to fix this when and if.
> 
> > > 2.	People bringing up new hardware might not want quite so many
> > > 	IPIs.  But they can just set rcu_normal to prevent that.  
> > 
> > I wanted to make things simple and not complicated…  
> 
> I know that feeling.  ;-)
> 

I agree, but if this creates a boot time regression in large machines,
it may not be warranted.

I know Linus usually doesn't like options with default y, but this may
be one of those exceptions. Perhaps we should make it on by default and
say in the config "if you have a machine with 100s or 1000s of CPUs,
you may want to disable this".

-- Steve

Re: [PATCH] rcu: update: make RCU_EXPEDITE_BOOT default

[Sebastian Andrzej Siewior]

On 2016-11-07 12:19:39 [-0500], Steven Rostedt wrote:
> I agree, but if this creates a boot time regression in large machines,
> it may not be warranted.
> 
> I know Linus usually doesn't like options with default y, but this may
> be one of those exceptions. Perhaps we should make it on by default and
> say in the config "if you have a machine with 100s or 1000s of CPUs,
> you may want to disable this".

The default could change if we know where the limit is. I have access to
a box with approx 140 CPUs so I could check there if it is already bad.
But everything above that / in the 1000 range is a different story.

> -- Steve

Sebastian

Re: [PATCH] rcu: update: make RCU_EXPEDITE_BOOT default

[Josh Triplett]

On Mon, Nov 07, 2016 at 06:30:30PM +0100, Sebastian Andrzej Siewior wrote:
> On 2016-11-07 12:19:39 [-0500], Steven Rostedt wrote:
> > I agree, but if this creates a boot time regression in large machines,
> > it may not be warranted.
> > 
> > I know Linus usually doesn't like options with default y, but this may
> > be one of those exceptions. Perhaps we should make it on by default and
> > say in the config "if you have a machine with 100s or 1000s of CPUs,
> > you may want to disable this".
> 
> The default could change if we know where the limit is. I have access to
> a box with approx 140 CPUs so I could check there if it is already bad.
> But everything above that / in the 1000 range is a different story.

Right; if we can characterize what machines it benefits and what
machines it hurts, we can automatically detect and run the appropriate
case with no configuration option needed.

Re: [PATCH] rcu: update: make RCU_EXPEDITE_BOOT default

[Paul E. McKenney]

On Mon, Nov 07, 2016 at 09:35:46AM -0800, Josh Triplett wrote:
> On Mon, Nov 07, 2016 at 06:30:30PM +0100, Sebastian Andrzej Siewior wrote:
> > On 2016-11-07 12:19:39 [-0500], Steven Rostedt wrote:
> > > I agree, but if this creates a boot time regression in large machines,
> > > it may not be warranted.
> > > 
> > > I know Linus usually doesn't like options with default y, but this may
> > > be one of those exceptions. Perhaps we should make it on by default and
> > > say in the config "if you have a machine with 100s or 1000s of CPUs,
> > > you may want to disable this".
> > 
> > The default could change if we know where the limit is. I have access to
> > a box with approx 140 CPUs so I could check there if it is already bad.
> > But everything above that / in the 1000 range is a different story.
> 
> Right; if we can characterize what machines it benefits and what
> machines it hurts, we can automatically detect and run the appropriate
> case with no configuration option needed.

I very much like this approach!  Anyone have access to large systems on
which this experiment could be carried out?  In the absence of new data,
I would just set the cutoff at 256 CPUs, as I have done in the past.

							Thanx, Paul

Re: [PATCH] rcu: update: make RCU_EXPEDITE_BOOT default

[Josh Triplett]

On Mon, Nov 07, 2016 at 10:05:13AM -0800, Paul E. McKenney wrote:
> On Mon, Nov 07, 2016 at 09:35:46AM -0800, Josh Triplett wrote:
> > On Mon, Nov 07, 2016 at 06:30:30PM +0100, Sebastian Andrzej Siewior wrote:
> > > On 2016-11-07 12:19:39 [-0500], Steven Rostedt wrote:
> > > > I agree, but if this creates a boot time regression in large machines,
> > > > it may not be warranted.
> > > > 
> > > > I know Linus usually doesn't like options with default y, but this may
> > > > be one of those exceptions. Perhaps we should make it on by default and
> > > > say in the config "if you have a machine with 100s or 1000s of CPUs,
> > > > you may want to disable this".
> > > 
> > > The default could change if we know where the limit is. I have access to
> > > a box with approx 140 CPUs so I could check there if it is already bad.
> > > But everything above that / in the 1000 range is a different story.
> > 
> > Right; if we can characterize what machines it benefits and what
> > machines it hurts, we can automatically detect and run the appropriate
> > case with no configuration option needed.
> 
> I very much like this approach!  Anyone have access to large systems on
> which this experiment could be carried out?  In the absence of new data,
> I would just set the cutoff at 256 CPUs, as I have done in the past.

One potential issue here: the point where RCU_EXPEDITE_BOOT pessimizes
likely depends on interconnect as much as CPUs.  I'd guess that you may
want to set the cutoff based on number of NUMA nodes, rather than number
of CPUs.

Re: [PATCH] rcu: update: make RCU_EXPEDITE_BOOT default

[Paul E. McKenney]

On Mon, Nov 07, 2016 at 10:08:32AM -0800, Josh Triplett wrote:
> On Mon, Nov 07, 2016 at 10:05:13AM -0800, Paul E. McKenney wrote:
> > On Mon, Nov 07, 2016 at 09:35:46AM -0800, Josh Triplett wrote:
> > > On Mon, Nov 07, 2016 at 06:30:30PM +0100, Sebastian Andrzej Siewior wrote:
> > > > On 2016-11-07 12:19:39 [-0500], Steven Rostedt wrote:
> > > > > I agree, but if this creates a boot time regression in large machines,
> > > > > it may not be warranted.
> > > > > 
> > > > > I know Linus usually doesn't like options with default y, but this may
> > > > > be one of those exceptions. Perhaps we should make it on by default and
> > > > > say in the config "if you have a machine with 100s or 1000s of CPUs,
> > > > > you may want to disable this".
> > > > 
> > > > The default could change if we know where the limit is. I have access to
> > > > a box with approx 140 CPUs so I could check there if it is already bad.
> > > > But everything above that / in the 1000 range is a different story.
> > > 
> > > Right; if we can characterize what machines it benefits and what
> > > machines it hurts, we can automatically detect and run the appropriate
> > > case with no configuration option needed.
> > 
> > I very much like this approach!  Anyone have access to large systems on
> > which this experiment could be carried out?  In the absence of new data,
> > I would just set the cutoff at 256 CPUs, as I have done in the past.
> 
> One potential issue here: the point where RCU_EXPEDITE_BOOT pessimizes
> likely depends on interconnect as much as CPUs.  I'd guess that you may
> want to set the cutoff based on number of NUMA nodes, rather than number
> of CPUs.

Longer term, the solution would be a function that defined the cutoff
point.  If an architecture didn't define the function, they get the
default cutoff of 256.  If they do define the function, they get whatever
they coded.

							Thanx, Paul

Re: Making rcu_normal=1 in RT

[Sebastian Andrzej Siewior]

On 2016-10-12 11:21:14 [-0500], Julia Cartwright wrote:
> Something like this, perhaps?
> 
>    Julia
> 
> -- 8< --
> Subject: [PATCH] rcu: enable rcu_normal_after_boot by default for RT

finally applied and dropped ("net: Make synchronize_rcu_expedited()
conditional on !RT_FULL").

Sebastian

Re: Making rcu_normal=1 in RT

[Luiz Capitulino]

On Wed, 2 Nov 2016 17:51:49 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> On 2016-10-12 11:21:14 [-0500], Julia Cartwright wrote:
> > Something like this, perhaps?
> > 
> >    Julia
> > 
> > -- 8< --
> > Subject: [PATCH] rcu: enable rcu_normal_after_boot by default for RT  
> 
> finally applied and dropped ("net: Make synchronize_rcu_expedited()
> conditional on !RT_FULL").

Cool, thanks for all involved!