Re: [PATCH v2 1/3] sched/fair: Fix warning if NEXT_BUDDY enabled

[K Prateek Nayak]

Hello Peter,

On 12/3/2024 9:35 PM, Peter Zijlstra wrote:
> On Thu, Nov 28, 2024 at 12:59:54PM +0530, K Prateek Nayak wrote:
>
> > Can you please try the following diff instead of the first two patches
> > and see if you still hit these warnings, stalls, and pick_eevdf()
> > returning NULL?
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index ff7cae9274c5..61e74eb5af22 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -5478,6 +5478,7 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
> >   	bool sleep = flags & DEQUEUE_SLEEP;
> >   	update_curr(cfs_rq);
> > +	clear_buddies(cfs_rq, se);
> >   	if (flags & DEQUEUE_DELAYED) {
> >   		SCHED_WARN_ON(!se->sched_delayed);
> > @@ -5520,8 +5521,6 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
> >   	update_stats_dequeue_fair(cfs_rq, se, flags);
> > -	clear_buddies(cfs_rq, se);
> > -
> >   	update_entity_lag(cfs_rq, se);
> >   	if (sched_feat(PLACE_REL_DEADLINE) && !sleep) {
> >   		se->deadline -= se->vruntime;
> > @@ -8767,7 +8766,7 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
> >   	if (unlikely(throttled_hierarchy(cfs_rq_of(pse))))
> >   		return;
> > -	if (sched_feat(NEXT_BUDDY) && !(wake_flags & WF_FORK)) {
> > +	if (sched_feat(NEXT_BUDDY) && !(wake_flags & WF_FORK) && !pse->sched_delayed) {
> >   		set_next_buddy(pse);
> >   	}
>
>
> Prateek, I've presumed your SoB on this change:
>
>    https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=sched/urgent&id=d1139307fe97ffefcf90212772f7516732a11034

No objections from my side! While at it, perhaps you can also squash in:

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 3ed4af8be76b..eadcd64c03e9 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5519,8 +5519,6 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
 
 		if (sched_feat(DELAY_DEQUEUE) && delay &&
 		    !entity_eligible(cfs_rq, se)) {
-			if (cfs_rq->next == se)
-				cfs_rq->next = NULL;
 			update_load_avg(cfs_rq, se, 0);
 			set_delayed(se);
 			return false;
--

Since we do a clear_buddy() upfront, we no longer need this special case
for delayed entities. Tested it on top of queue:sched/urgent with
hackbench and I didn't run into any problems / splats. Thank you.

> Holler if you want it modified.
>
> Thanks!

--
Thanks and Regards,
Prateek