Re: [PATCH 11/23] sched,spi: Convert to sched_set_fifo*()

[Doug Anderson]

Hi,

On Wed, Apr 22, 2020 at 6:56 AM Mark Brown <broonie@xxxxxxxxxx> wrote:
>
> On Wed, Apr 22, 2020 at 01:27:30PM +0200, Peter Zijlstra wrote:
> > Because SCHED_FIFO is a broken scheduler model (see previous patches)
> > take away the priority field, the kernel can't possibly make an
> > informed decision.
> >
> > No effective change.
>
> Copying Doug who did this change and Guenter who reviewed it.  This
> looks fine to me but I've no particular involvement with the code or
> platforms that are affected here.

Thanks!  Probably the maintainers of cros_ec_spi.c (Benson and Enric)
should be aware of it, too.  CCing them.

>From my point of view, my response is pretty much identical to the one
I wrote when the priority was reduced from "MAX_RT_PRIO - 1" to
"MAX_RT_PRIO / 2" [1].  Basically, any priority that keeps us from
being preempted by tasks that are only high priority for performance
reasons (like dm crypt and loopback did when I last analyzed) is fine.
Our priority needs to be high not for performance reasons but for
correctness reasons (the other side will drop our data if we don't
respond in time).

Thus:

Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>


[1] https://lore.kernel.org/r/CAD=FV=UsYF1R6+XRfFFFm6PfmkTsEOfxxgCw2JxCnpyu1kGVLQ@xxxxxxxxxxxxxx


> > Cc: broonie@xxxxxxxxxx
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> > Reviewed-by: Ingo Molnar <mingo@xxxxxxxxxx>
> > ---
> >  drivers/platform/chrome/cros_ec_spi.c |    6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > --- a/drivers/platform/chrome/cros_ec_spi.c
> > +++ b/drivers/platform/chrome/cros_ec_spi.c
> > @@ -709,9 +709,6 @@ static void cros_ec_spi_high_pri_release
> >  static int cros_ec_spi_devm_high_pri_alloc(struct device *dev,
> >                                          struct cros_ec_spi *ec_spi)
> >  {
> > -     struct sched_param sched_priority = {
> > -             .sched_priority = MAX_RT_PRIO / 2,
> > -     };
> >       int err;
> >
> >       ec_spi->high_pri_worker =
> > @@ -728,8 +725,7 @@ static int cros_ec_spi_devm_high_pri_all
> >       if (err)
> >               return err;
> >
> > -     err = sched_setscheduler_nocheck(ec_spi->high_pri_worker->task,
> > -                                      SCHED_FIFO, &sched_priority);
> > +     err = sched_set_fifo(ec_spi->high_pri_worker->task);
> >       if (err)
> >               dev_err(dev, "Can't set cros_ec high pri priority: %d\n", err);
> >       return err;
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -1589,11 +1589,9 @@ EXPORT_SYMBOL_GPL(spi_take_timestamp_pos
> >   */
> >  static void spi_set_thread_rt(struct spi_controller *ctlr)
> >  {
> > -     struct sched_param param = { .sched_priority = MAX_RT_PRIO / 2 };
> > -
> >       dev_info(&ctlr->dev,
> >               "will run message pump with realtime priority\n");
> > -     sched_setscheduler(ctlr->kworker_task, SCHED_FIFO, &param);
> > +     sched_set_fifo(ctlr->kworker_task);
> >  }
> >
> >  static int spi_init_queue(struct spi_controller *ctlr)
> >
> >