Re: [PATCH] sched,tracing: Convert to sched_set_fifo()

[Steven Rostedt]

On Fri, 24 Jul 2020 23:39:11 +0200
pet...@infradead.org wrote:

> On Mon, Jul 20, 2020 at 11:49:18PM +0200, Peter Zijlstra wrote:
> > Steve, would this work for you, or would you prefer renaming the
> > parameters as well?  
> 
> Steve mentioned he's like to have the parameters changes after all.
> How's this then?
> 
> ---
> Subject: sched,tracing: Convert to sched_set_fifo()
> From: Peter Zijlstra <pet...@infradead.org>
> Date: Mon, 20 Jul 2020 23:49:18 +0200
> 
> One module user of sched_setscheduler() was overlooked and is
> obviously causing build failures.
> 
> Convert ring_buffer_benchmark to use sched_set_fifo_low() when fifo==1
> and sched_set_fifo() when fifo==2. This is a bit of an abuse, but it
> makes the thing 'work' again.
> 
> Specifically, it enables all combinations that were previously
> possible:
> 
>   producer higher than consumer
>   consumer higher than producer
> 
> Fixes: 616d91b68cd5 ("sched: Remove sched_setscheduler*() EXPORTs")
> Reported-by: kernel test robot <l...@intel.com>
> Reported-by: Stephen Rothwell <s...@canb.auug.org.au>
> Signed-off-by: Peter Zijlstra (Intel) <pet...@infradead.org>
> ---
>  kernel/trace/ring_buffer_benchmark.c |   48 
> ++++++++++++++++-------------------
>  1 file changed, 23 insertions(+), 25 deletions(-)
> 
> --- a/kernel/trace/ring_buffer_benchmark.c
> +++ b/kernel/trace/ring_buffer_benchmark.c
> @@ -45,8 +45,8 @@ MODULE_PARM_DESC(write_iteration, "# of
>  static int producer_nice = MAX_NICE;
>  static int consumer_nice = MAX_NICE;
>  
> -static int producer_fifo = -1;
> -static int consumer_fifo = -1;
> +static int producer_fifo = 0;
> +static int consumer_fifo = 0;

The initialization of zero for static variables isn't needed.

>  
>  module_param(producer_nice, int, 0644);
>  MODULE_PARM_DESC(producer_nice, "nice prio for producer");
> @@ -55,10 +55,10 @@ module_param(consumer_nice, int, 0644);
>  MODULE_PARM_DESC(consumer_nice, "nice prio for consumer");
>  
>  module_param(producer_fifo, int, 0644);
> -MODULE_PARM_DESC(producer_fifo, "fifo prio for producer");
> +MODULE_PARM_DESC(producer_fifo, "use fifo for producer: 0 - disabled, 1 - 
> low prio, 2 - fifo");
>  
>  module_param(consumer_fifo, int, 0644);
> -MODULE_PARM_DESC(consumer_fifo, "fifo prio for consumer");
> +MODULE_PARM_DESC(consumer_fifo, "use fifo for consumer: 0 - disabled, 1 - 
> low prio, 2 - fifo");
>  
>  static int read_events;
>  
> @@ -303,22 +303,22 @@ static void ring_buffer_producer(void)
>               trace_printk("ERROR!\n");
>  
>       if (!disable_reader) {
> -             if (consumer_fifo < 0)
> -                     trace_printk("Running Consumer at nice: %d\n",
> -                                  consumer_nice);
> -             else
> +             if (consumer_fifo)
>                       trace_printk("Running Consumer at SCHED_FIFO %d\n",
>                                    consumer_fifo);

Can we change the above to:

                        trace_printk("Running Consumer at SCHED_FIFO %s\n",
                                        consumer_fifo == 1 ? "low" : "high");

> +             else
> +                     trace_printk("Running Consumer at nice: %d\n",
> +                                  consumer_nice);
>       }
> -     if (producer_fifo < 0)
> -             trace_printk("Running Producer at nice: %d\n",
> -                          producer_nice);
> -     else
> +     if (producer_fifo)
>               trace_printk("Running Producer at SCHED_FIFO %d\n",
>                            producer_fifo);

Same here.

> +     else
> +             trace_printk("Running Producer at nice: %d\n",
> +                          producer_nice);
>  
>       /* Let the user know that the test is running at low priority */
> -     if (producer_fifo < 0 && consumer_fifo < 0 &&
> +     if (!producer_fifo && !consumer_fifo &&
>           producer_nice == MAX_NICE && consumer_nice == MAX_NICE)
>               trace_printk("WARNING!!! This test is running at lowest 
> priority.\n");
>  
> @@ -455,21 +455,19 @@ static int __init ring_buffer_benchmark_
>        * Run them as low-prio background tasks by default:
>        */
>       if (!disable_reader) {
> -             if (consumer_fifo >= 0) {
> -                     struct sched_param param = {
> -                             .sched_priority = consumer_fifo
> -                     };
> -                     sched_setscheduler(consumer, SCHED_FIFO, &param);
> -             } else
> +             if (consumer_fifo == 2)

Let's make this be:

                if (consumer_fifo > 1)

Then if someone sends in 3 it doesn't go low again.

> +                     sched_set_fifo(consumer);
> +             else if (consumer_fifo == 1)
> +                     sched_set_fifo_low(consumer);
> +             else
>                       set_user_nice(consumer, consumer_nice);
>       }
>  
> -     if (producer_fifo >= 0) {
> -             struct sched_param param = {
> -                     .sched_priority = producer_fifo
> -             };
> -             sched_setscheduler(producer, SCHED_FIFO, &param);
> -     } else
> +     if (producer_fifo == 2)

Same here.

-- Steve

> +             sched_set_fifo(producer);
> +     else if (producer_fifo == 1)
> +             sched_set_fifo_low(producer);
> +     else
>               set_user_nice(producer, producer_nice);
>  
>       return 0;