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) <j...@joelfernandes.org>
> 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));
