[v2] rcutorture: Create nocb tasks only for CONFIG_RCU_NOCB_CPU=y kernels

  When setting nocbs_nthreads to start rcutorture test with a non-zero value,
the nocb tasks will be created and invoke rcu_nocb_cpu_offload/deoffload()
to toggle CPU's callback-offload state, but for CONFIG_RCU_NOCB_CPU=n
kernel, the rcu_nocb_cpu_offload/deoffload() is a no-op and this is also
meaningless for torture_type is non-rcu.

This commit therefore add member can_nocbs_toggle to rcu_torture_ops
structure to avoid unnecessary nocb tasks creation.

Signed-off-by: Zqiang <qiang1.zhang@intel.com>
---
 kernel/rcu/rcutorture.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
  

> On Feb 2, 2023, at 1:57 AM, Zqiang <qiang1.zhang@intel.com> wrote:
> 
> When setting nocbs_nthreads to start rcutorture test with a non-zero value,
> the nocb tasks will be created and invoke rcu_nocb_cpu_offload/deoffload()
> to toggle CPU's callback-offload state, but for CONFIG_RCU_NOCB_CPU=n
> kernel, the rcu_nocb_cpu_offload/deoffload() is a no-op and this is also
> meaningless for torture_type is non-rcu.
> 
> This commit therefore add member can_nocbs_toggle to rcu_torture_ops
> structure to avoid unnecessary nocb tasks creation.
> 
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> ---
> kernel/rcu/rcutorture.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)

Sorry if I am missing something but what is the point of adding more lines of code and complexity to handle this? Does it improve the test coverage or reduce overhead?

This is test code. I see no problem with cost of an extra unused task with positive trade off of keeping the code simple…

thanks,

 - Joel 


> 
> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> index 297da28ce92d..ced0a8e1d765 100644
> --- a/kernel/rcu/rcutorture.c
> +++ b/kernel/rcu/rcutorture.c
> @@ -383,6 +383,7 @@ struct rcu_torture_ops {
>    long cbflood_max;
>    int irq_capable;
>    int can_boost;
> +    int can_nocbs_toggle;
>    int extendables;
>    int slow_gps;
>    int no_pi_lock;
> @@ -569,6 +570,7 @@ static struct rcu_torture_ops rcu_ops = {
>    .stall_dur        = rcu_jiffies_till_stall_check,
>    .irq_capable        = 1,
>    .can_boost        = IS_ENABLED(CONFIG_RCU_BOOST),
> +    .can_nocbs_toggle    = IS_ENABLED(CONFIG_RCU_NOCB_CPU),
>    .extendables        = RCUTORTURE_MAX_EXTEND,
>    .name            = "rcu"
> };
> @@ -2356,7 +2358,7 @@ rcu_torture_print_module_parms(struct rcu_torture_ops *cur_ops, const char *tag)
>         "n_barrier_cbs=%d "
>         "onoff_interval=%d onoff_holdoff=%d "
>         "read_exit_delay=%d read_exit_burst=%d "
> -         "nocbs_nthreads=%d nocbs_toggle=%d "
> +         "nocbs_nthreads=%d/%d nocbs_toggle=%d "
>         "test_nmis=%d\n",
>         torture_type, tag, nrealreaders, nfakewriters,
>         stat_interval, verbose, test_no_idle_hz, shuffle_interval,
> @@ -2368,7 +2370,7 @@ rcu_torture_print_module_parms(struct rcu_torture_ops *cur_ops, const char *tag)
>         n_barrier_cbs,
>         onoff_interval, onoff_holdoff,
>         read_exit_delay, read_exit_burst,
> -         nocbs_nthreads, nocbs_toggle,
> +         nocbs_nthreads, cur_ops->can_nocbs_toggle, nocbs_toggle,
>         test_nmis);
> }
> 
> @@ -3708,6 +3710,10 @@ rcu_torture_init(void)
>        pr_alert("rcu-torture: ->fqs NULL and non-zero fqs_duration, fqs disabled.\n");
>        fqs_duration = 0;
>    }
> +    if (!cur_ops->can_nocbs_toggle && nocbs_nthreads != 0) {
> +        pr_alert("rcu-torture: ->can_nocbs_toggle false and non-zero nocbs_nthreads, nocbs_toggle disabled.\n");
> +        nocbs_nthreads = 0;
> +    }
>    if (cur_ops->init)
>        cur_ops->init();
> 
> -- 
> 2.25.1
>
  

> On Feb 2, 2023, at 1:57 AM, Zqiang <qiang1.zhang@intel.com> wrote:
> 
> When setting nocbs_nthreads to start rcutorture test with a non-zero value,
> the nocb tasks will be created and invoke rcu_nocb_cpu_offload/deoffload()
> to toggle CPU's callback-offload state, but for CONFIG_RCU_NOCB_CPU=n
> kernel, the rcu_nocb_cpu_offload/deoffload() is a no-op and this is also
> meaningless for torture_type is non-rcu.
> 
> This commit therefore add member can_nocbs_toggle to rcu_torture_ops
> structure to avoid unnecessary nocb tasks creation.
> 
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> ---
> kernel/rcu/rcutorture.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
>Sorry if I am missing something but what is the point of adding more lines of code and complexity to handle this? Does it improve the test coverage or reduce overhead?
>
>This is test code. I see no problem with cost of an extra unused task with positive trade off of keeping the code simple…

For nocbs_nthreads is non-zero and CONFIG_RCU_NOCB_CPU=n kernels, 
the rcu_nocb_cpu_offload/deoffload() is a no-op,  we create nocbs_nthreads 
kthreads and perform nocb toggle tests periodically, which is meaningless and
will take extra cpu time.

For non-rcu tests, it really doesn't make sense for us to turn on nocb toggle test.

Does this make the test a little more rigorous?

Thanks
Zqiang

>
>thanks,
>
> - Joel 


> 
> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> index 297da28ce92d..ced0a8e1d765 100644
> --- a/kernel/rcu/rcutorture.c
> +++ b/kernel/rcu/rcutorture.c
> @@ -383,6 +383,7 @@ struct rcu_torture_ops {
>    long cbflood_max;
>    int irq_capable;
>    int can_boost;
> +    int can_nocbs_toggle;
>    int extendables;
>    int slow_gps;
>    int no_pi_lock;
> @@ -569,6 +570,7 @@ static struct rcu_torture_ops rcu_ops = {
>    .stall_dur        = rcu_jiffies_till_stall_check,
>    .irq_capable        = 1,
>    .can_boost        = IS_ENABLED(CONFIG_RCU_BOOST),
> +    .can_nocbs_toggle    = IS_ENABLED(CONFIG_RCU_NOCB_CPU),
>    .extendables        = RCUTORTURE_MAX_EXTEND,
>    .name            = "rcu"
> };
> @@ -2356,7 +2358,7 @@ rcu_torture_print_module_parms(struct rcu_torture_ops *cur_ops, const char *tag)
>         "n_barrier_cbs=%d "
>         "onoff_interval=%d onoff_holdoff=%d "
>         "read_exit_delay=%d read_exit_burst=%d "
> -         "nocbs_nthreads=%d nocbs_toggle=%d "
> +         "nocbs_nthreads=%d/%d nocbs_toggle=%d "
>         "test_nmis=%d\n",
>         torture_type, tag, nrealreaders, nfakewriters,
>         stat_interval, verbose, test_no_idle_hz, shuffle_interval,
> @@ -2368,7 +2370,7 @@ rcu_torture_print_module_parms(struct rcu_torture_ops *cur_ops, const char *tag)
>         n_barrier_cbs,
>         onoff_interval, onoff_holdoff,
>         read_exit_delay, read_exit_burst,
> -         nocbs_nthreads, nocbs_toggle,
> +         nocbs_nthreads, cur_ops->can_nocbs_toggle, nocbs_toggle,
>         test_nmis);
> }
> 
> @@ -3708,6 +3710,10 @@ rcu_torture_init(void)
>        pr_alert("rcu-torture: ->fqs NULL and non-zero fqs_duration, fqs disabled.\n");
>        fqs_duration = 0;
>    }
> +    if (!cur_ops->can_nocbs_toggle && nocbs_nthreads != 0) {
> +        pr_alert("rcu-torture: ->can_nocbs_toggle false and non-zero nocbs_nthreads, nocbs_toggle disabled.\n");
> +        nocbs_nthreads = 0;
> +    }
>    if (cur_ops->init)
>        cur_ops->init();
> 
> -- 
> 2.25.1
>
  

On Thu, Feb 2, 2023 at 4:25 PM Zhang, Qiang1 <qiang1.zhang@intel.com> wrote:
>
>
> > On Feb 2, 2023, at 1:57 AM, Zqiang <qiang1.zhang@intel.com> wrote:
> >
> > When setting nocbs_nthreads to start rcutorture test with a non-zero value,
> > the nocb tasks will be created and invoke rcu_nocb_cpu_offload/deoffload()
> > to toggle CPU's callback-offload state, but for CONFIG_RCU_NOCB_CPU=n
> > kernel, the rcu_nocb_cpu_offload/deoffload() is a no-op and this is also
> > meaningless for torture_type is non-rcu.
> >
> > This commit therefore add member can_nocbs_toggle to rcu_torture_ops
> > structure to avoid unnecessary nocb tasks creation.
> >
> > Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> > ---
> > kernel/rcu/rcutorture.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> >Sorry if I am missing something but what is the point of adding more lines of code and complexity to handle this? Does it improve the test coverage or reduce overhead?
> >
> >This is test code. I see no problem with cost of an extra unused task with positive trade off of keeping the code simple…
>
> For nocbs_nthreads is non-zero and CONFIG_RCU_NOCB_CPU=n kernels,
> the rcu_nocb_cpu_offload/deoffload() is a no-op,  we create nocbs_nthreads
> kthreads and perform nocb toggle tests periodically, which is meaningless and
> will take extra cpu time.

Ah, ok. I see what you did now, could you add these details to the
changelog. One comment below:

[...]
> > @@ -569,6 +570,7 @@ static struct rcu_torture_ops rcu_ops = {
> >    .stall_dur        = rcu_jiffies_till_stall_check,
> >    .irq_capable        = 1,
> >    .can_boost        = IS_ENABLED(CONFIG_RCU_BOOST),
> > +    .can_nocbs_toggle    = IS_ENABLED(CONFIG_RCU_NOCB_CPU),
> >    .extendables        = RCUTORTURE_MAX_EXTEND,
> >    .name            = "rcu"
> > };
> > @@ -2356,7 +2358,7 @@ rcu_torture_print_module_parms(struct rcu_torture_ops *cur_ops, const char *tag)
> >         "n_barrier_cbs=%d "
> >         "onoff_interval=%d onoff_holdoff=%d "
> >         "read_exit_delay=%d read_exit_burst=%d "
> > -         "nocbs_nthreads=%d nocbs_toggle=%d "
> > +         "nocbs_nthreads=%d/%d nocbs_toggle=%d "
> >         "test_nmis=%d\n",
> >         torture_type, tag, nrealreaders, nfakewriters,
> >         stat_interval, verbose, test_no_idle_hz, shuffle_interval,
> > @@ -2368,7 +2370,7 @@ rcu_torture_print_module_parms(struct rcu_torture_ops *cur_ops, const char *tag)
> >         n_barrier_cbs,
> >         onoff_interval, onoff_holdoff,
> >         read_exit_delay, read_exit_burst,
> > -         nocbs_nthreads, nocbs_toggle,
> > +         nocbs_nthreads, cur_ops->can_nocbs_toggle, nocbs_toggle,
> >         test_nmis);
> > }
> >
> > @@ -3708,6 +3710,10 @@ rcu_torture_init(void)
> >        pr_alert("rcu-torture: ->fqs NULL and non-zero fqs_duration, fqs disabled.\n");
> >        fqs_duration = 0;
> >    }
> > +    if (!cur_ops->can_nocbs_toggle && nocbs_nthreads != 0) {
> > +        pr_alert("rcu-torture: ->can_nocbs_toggle false and non-zero nocbs_nthreads, nocbs_toggle disabled.\n");
> > +        nocbs_nthreads = 0;
> > +    }

Instead of adding a hook, why not check for CONFIG_RCU_NOCB_CPU here?

so like:
 if (cur_ops != &rcu_ops || !IS_ENABLED(CONFIG_RCU_NOCB_CPU))
   nocbs_nthreads = 0;

Or will that not work for some reason? Just 2 line change and no ugly hooks =)

- Joel
  

>
>
> > On Feb 2, 2023, at 1:57 AM, Zqiang <qiang1.zhang@intel.com> wrote:
> >
> > When setting nocbs_nthreads to start rcutorture test with a non-zero value,
> > the nocb tasks will be created and invoke rcu_nocb_cpu_offload/deoffload()
> > to toggle CPU's callback-offload state, but for CONFIG_RCU_NOCB_CPU=n
> > kernel, the rcu_nocb_cpu_offload/deoffload() is a no-op and this is also
> > meaningless for torture_type is non-rcu.
> >
> > This commit therefore add member can_nocbs_toggle to rcu_torture_ops
> > structure to avoid unnecessary nocb tasks creation.
> >
> > Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> > ---
> > kernel/rcu/rcutorture.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> >Sorry if I am missing something but what is the point of adding more lines of code and complexity to handle this? Does it improve the test coverage or reduce overhead?
> >
> >This is test code. I see no problem with cost of an extra unused task with positive trade off of keeping the code simple…
>
> For nocbs_nthreads is non-zero and CONFIG_RCU_NOCB_CPU=n kernels,
> the rcu_nocb_cpu_offload/deoffload() is a no-op,  we create nocbs_nthreads
> kthreads and perform nocb toggle tests periodically, which is meaningless and
> will take extra cpu time.
>
>Ah, ok. I see what you did now, could you add these details to the
>changelog. One comment below:
>
>[...]
> > @@ -569,6 +570,7 @@ static struct rcu_torture_ops rcu_ops = {
> >    .stall_dur        = rcu_jiffies_till_stall_check,
> >    .irq_capable        = 1,
> >    .can_boost        = IS_ENABLED(CONFIG_RCU_BOOST),
> > +    .can_nocbs_toggle    = IS_ENABLED(CONFIG_RCU_NOCB_CPU),
> >    .extendables        = RCUTORTURE_MAX_EXTEND,
> >    .name            = "rcu"
> > };
> > @@ -2356,7 +2358,7 @@ rcu_torture_print_module_parms(struct rcu_torture_ops *cur_ops, const char *tag)
> >         "n_barrier_cbs=%d "
> >         "onoff_interval=%d onoff_holdoff=%d "
> >         "read_exit_delay=%d read_exit_burst=%d "
> > -         "nocbs_nthreads=%d nocbs_toggle=%d "
> > +         "nocbs_nthreads=%d/%d nocbs_toggle=%d "
> >         "test_nmis=%d\n",
> >         torture_type, tag, nrealreaders, nfakewriters,
> >         stat_interval, verbose, test_no_idle_hz, shuffle_interval,
> > @@ -2368,7 +2370,7 @@ rcu_torture_print_module_parms(struct rcu_torture_ops *cur_ops, const char *tag)
> >         n_barrier_cbs,
> >         onoff_interval, onoff_holdoff,
> >         read_exit_delay, read_exit_burst,
> > -         nocbs_nthreads, nocbs_toggle,
> > +         nocbs_nthreads, cur_ops->can_nocbs_toggle, nocbs_toggle,
> >         test_nmis);
> > }
> >
> > @@ -3708,6 +3710,10 @@ rcu_torture_init(void)
> >        pr_alert("rcu-torture: ->fqs NULL and non-zero fqs_duration, fqs disabled.\n");
> >        fqs_duration = 0;
> >    }
> > +    if (!cur_ops->can_nocbs_toggle && nocbs_nthreads != 0) {
> > +        pr_alert("rcu-torture: ->can_nocbs_toggle false and non-zero nocbs_nthreads, nocbs_toggle disabled.\n");
> > +        nocbs_nthreads = 0;
> > +    }

>Instead of adding a hook, why not check for CONFIG_RCU_NOCB_CPU here?
>
>so like:
> if (cur_ops != &rcu_ops || !IS_ENABLED(CONFIG_RCU_NOCB_CPU))
>   nocbs_nthreads = 0;

Concise approach, I will resend.

Thanks
Zqiang

>
>Or will that not work for some reason? Just 2 line change and no ugly hooks =)
>
>- Joel