Re: [PATCH v2] sched/fair: limit sched slice duration

[Peter Zijlstra <peterz@infradead.org>]

On Thu, Sep 29, 2022 at 07:15:42PM +0200, Vincent Guittot wrote:
> On Thu, 29 Sept 2022 at 18:14, Peter Zijlstra <peterz@infradead.org> wrote:

> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 5ffec4370602..2b218167fadf 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -4575,17 +4575,33 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
> >  }
> >
> >  /*
> > - * Preempt the current task with a newly woken task if needed:
> > + * Tick driven preemption; preempt the task if it has ran long enough.
> > + * Allows other tasks to have a go.
> >   */
> >  static void
> >  check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
> >  {
> > -       unsigned long ideal_runtime, delta_exec;
> >         struct sched_entity *se;
> > -       s64 delta;
> > +       s64 delta, delta_exec;
> > +       u64 ideal_runtime;
> >
> > -       ideal_runtime = sched_slice(cfs_rq, curr);
> > +       /* How long has this task been on the CPU for [walltime]. */
> >         delta_exec = curr->sum_exec_runtime - curr->prev_sum_exec_runtime;
> > +
> > +       /*
> > +        * Ensure that a task that missed wakeup preemption by a
> > +        * narrow margin doesn't have to wait for a full slice.
> > +        * This also mitigates buddy induced latencies under load.
> > +        */
> > +       if (delta_exec < sysctl_sched_min_granularity)
> > +               return;
> 
> ideal_runtime can be lower than sysctl_sched_min_granularity. It can
> be as low as sysctl_sched_idle_min_granularity for idle task. In this
> case, we want to resched even if(delta_exec <
> sysctl_sched_min_granularity). That's why the 1st test was still done
> before

Duh, indeed.

> > +
> > +       /*
> > +        * When many tasks blow up the sched_period; it is possible that
> > +        * sched_slice() reports unusually large results (when many tasks are
> > +        * very light for example). Therefore impose a maximum.
> > +        */
> > +       ideal_runtime = min_t(u64, sched_slice(cfs_rq, curr), sysctl_sched_latency);
> 
> I didn't cap ideal_runtime before this test because we have situations
> where large ideal_runtime is ok: If there is only one normal thread
> with hundreds of idle threads as an example: Is it acceptable to
> trigger a useless resched in this case ? That's why I have computed
> the virtual time generated by the capped version of ideal_runtime.

Yeah; I think that should be fine. It's an edge case, and sched_latency
is fairly large already.

> >         se = __pick_first_entity(cfs_rq);
> >         delta = curr->vruntime - se->vruntime;
> > -
> >         if (delta < 0)
> >                 return;
> >
> > +       /*
> > +        * Compare @delta [vtime] to @ideal_runtime [walltime]. This means that
> > +        * heavy tasks (for which vtime goes slower) get relatively more time
> > +        * before preemption, while light tasks (for which vtime goes faster)
> > +        * get relatively less time.  IOW, heavy task get to run longer.
> > +        */
> 
> After your comment on v1, I looked more deeply on this and the
> comparison of [vtime] with [walltime] can create a large unfairness.
> vtime of nice-20 increases by ~250us for 24ms of walltime which means
> that the nice-20 will have to run for a long time before reaching this
> walltime delta (assuming the vruntime were similar at the beg)

As I wrote, strictly speaking we should do without this. The entire
vtime thing is a band-aid.

I'm sure I've tried taking it out at least once; but sadly I seem to
have forgotten everything relevant :-( That is, I can't tell you why
this code exists.