Re: [PATCH 13/23] sched,ion: Convert to sched_set_normal()

[Peter Zijlstra]

On Wed, Apr 22, 2020 at 03:36:22PM +0200, Vincent Guittot wrote:
> On Wed, 22 Apr 2020 at 15:29, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> >
> > On Wed, Apr 22, 2020 at 03:21:45PM +0200, Vincent Guittot wrote:
> > > On Wed, 22 Apr 2020 at 13:29, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> > > >
> > > > In an attempt to take away sched_setscheduler() from modules, change
> > > > this into sched_set_normal(.nice = 19).
> > > >
> > > > Cc: john.stultz@xxxxxxxxxx
> > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> > > > Reviewed-by: Ingo Molnar <mingo@xxxxxxxxxx>
> > > > ---
> > > >  drivers/staging/android/ion/ion_heap.c |    3 ---
> > > >  1 file changed, 3 deletions(-)
> > > >
> > > > --- a/drivers/staging/android/ion/ion_heap.c
> > > > +++ b/drivers/staging/android/ion/ion_heap.c
> > > > @@ -244,8 +244,6 @@ static int ion_heap_deferred_free(void *
> > > >
> > > >  int ion_heap_init_deferred_free(struct ion_heap *heap)
> > > >  {
> > > > -       struct sched_param param = { .sched_priority = 0 };
> > > > -
> > > >         INIT_LIST_HEAD(&heap->free_list);
> > > >         init_waitqueue_head(&heap->waitqueue);
> > > >         heap->task = kthread_run(ion_heap_deferred_free, heap,
> > > > @@ -255,7 +253,7 @@ int ion_heap_init_deferred_free(struct i
> > > >                        __func__);
> > > >                 return PTR_ERR_OR_ZERO(heap->task);
> > > >         }
> > > > -       sched_setscheduler(heap->task, SCHED_IDLE, &param);
> > > > +       sched_set_normal(heap->task, 19);
> > >
> > > Would it make sense to have a sched_set_idle(task) to enable kernel
> > > setting SCHED_IDLE task ?
> > >
> > > SCHED_NORMAL w/ nice 19 and SCHED_IDLE tasks are not treated in the
> > > same way when checking for preemption at  wakeup
> >
> > Yeah, but does it really matter? I did indeed consider it, but got
> > lazy. Is there a definite need for IDLE?
> 
> John is the best to answer this for this driver but SCHED_IDLE will
> let other tasks which might be involved in end user interaction like
> on Android to run first

So I don't much like SCHED_IDLE because it introduces some pretty
horrible tail latencies. Consider the IDLE task holding a lock, then the
lock waiter will have to wait until the task gets around to running.

It's not unbounded, like a true idle-time scheduler would be, but it can
still be pretty horrible. nice19 has some of that too of course, but
idle has it worse, esp. also because it begs others to preempt it.

I should get back to proxy execution I suppose...