Re: [RFC PATCH v7 08/23] sched: Add core wide task selection and scheduling.

[Joel Fernandes]

Hi Vineeth,

On Tue, Sep 01, 2020 at 08:34:23AM -0400, Vineeth Pillai wrote:
> Hi Joel,
> 
> On 9/1/20 1:10 AM, Joel Fernandes wrote:
> > 3. The 'Rescheduling siblings' loop of pick_next_task() is quite fragile. It
> > calls various functions on rq->core_pick which could very well be NULL because:
> > An online sibling might have gone offline before a task could be picked for it,
> > or it might be offline but later happen to come online, but its too late and
> > nothing was picked for it. Just ignore the siblings for which nothing could be
> > picked. This avoids any crashes that may occur in this loop that assume
> > rq->core_pick is not NULL.
> > 
> > Signed-off-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
> I like this idea, its much simpler :-)

Thanks.

> > ---
> >   kernel/sched/core.c | 24 +++++++++++++++++++++---
> >   1 file changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 717122a3dca1..4966e9f14f39 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -4610,13 +4610,24 @@ pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
> >   	if (!sched_core_enabled(rq))
> >   		return __pick_next_task(rq, prev, rf);
> > +	cpu = cpu_of(rq);
> > +
> > +	/* Stopper task is switching into idle, no need core-wide selection. */
>
> I think we can come here when hotplug thread is scheduled during online, but
> mask is not yet updated. Probably can add it with this comment as well.
> 

I don't see how that is possible. Because the cpuhp threads run during the
CPU onlining process, the boot thread for the CPU coming online would have
already updated the mask.

> > +	if (cpu_is_offline(cpu))
> > +		return __pick_next_task(rq, prev, rf);
> > +
> We would need reset core_pick here I think. Something like
>     if (cpu_is_offline(cpu)) {
>         rq->core_pick = NULL;
>         return __pick_next_task(rq, prev, rf);
>     }
> 
> Without this we can end up in a crash like this:
> 1. Sibling of this cpu picks a task (rq_i->core_pick) and this cpu goes
>     offline soon after.
> 2. Before this cpu comes online, sibling goes through another pick loop
>     and before its IPI loop, this cpu comes online and we get an IPI.
> 3. So when this cpu gets into schedule, we have core_pick set and
>     core_pick_seq != core_sched_seq. So we enter the fast path. But
>     core_pick might no longer in this runqueue.
> 
> So, to protect this, we should reset core_pick I think. I have seen this
> crash
> occasionally.

Ok, done.

> >   	/*
> >   	 * If there were no {en,de}queues since we picked (IOW, the task
> >   	 * pointers are all still valid), and we haven't scheduled the last
> >   	 * pick yet, do so now.
> > +	 *
> > +	 * rq->core_pick can be NULL if no selection was made for a CPU because
> > +	 * it was either offline or went offline during a sibling's core-wide
> > +	 * selection. In this case, do a core-wide selection.
> >   	 */
> >   	if (rq->core->core_pick_seq == rq->core->core_task_seq &&
> > -	    rq->core->core_pick_seq != rq->core_sched_seq) {
> > +	    rq->core->core_pick_seq != rq->core_sched_seq &&
> > +	    !rq->core_pick) {
> Should this check be reversed? I mean, we should enter the fastpath if
> we have rq->core_pick is set right?

Done. Sorry my testing did not catch it, but it eventually caused a problem
after several hours of the stress test so I'd have eventually caught it.

> Another unrelated, but related note :-)
> Besides this, I think we need to retain on more change from the previous
> patch. We would need to make core_pick_seq per sibling instead of per
> core. Having it per core might lead to unfairness. For eg: When a cpu
> sees that its sibling's core_pick is the one which is already running, it
> will not send IPI. but core_pick remains set and core->core_pick_seq is
> incremented. Now if the sibling is preempted due to a high priority task

Then don't keep the core_pick set then. If you don't send it IPI and if
core_pick is already running, then NULL it already. I don't know why we add
to more corner cases by making assumptions. We have enough open issues that
are not hotplug related. Here's my suggestion :

1.  Keep the ideas consistent, forget about the exact code currently written
and just understand the pick_seq is for siblings knowing that something was
picked for the whole core.  So if their pick_seq != sched_seq, then they have
to pick what was selected.

2. If core_pick should be NULL, then NULL it in some path. If you keep some
core_pick and you increment pick_seq, then you are automatically asking the
sibling to pick that task up then next time it enters schedule(). See if [1]
will work?

Note that, we have added logic in this patch that does a full selection if
rq->core_pick == NULL.

> or its time slice expired, it enters schedule. But it goes to fast path and
> selects the running task there by starving the high priority task. Having
> the core_pick_seq per sibling will avoid this. It might also help in some
> hotplug corner cases as well.

That can be a separate patch IMHO. It has nothing to do with
stability/crashing of concurrent and rather infrequent CPU hotplug
operations.

Also, Peter said pick_seq is for core-wide picking. If you want to add
another semantic, then maybe add another counter which has a separate
meaning and justify why you are adding it.

thanks,

 - Joel

[1]
---8<-----------------------

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 7728ca7f6bb2..7a03b609e3b7 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4793,6 +4793,8 @@ next_class:;
 
 		if (rq_i->curr != rq_i->core_pick)
 			resched_curr(rq_i);
+		else
+			rq_i->core_pick = NULL;
 
 		/* Did we break L1TF mitigation requirements? */
 		WARN_ON_ONCE(!cookie_match(next, rq_i->core_pick));