Re: [peterz-queue:sched/prep] [sched/fair] 124c8f4374: WARNING:at_kernel/sched/sched.h:#update_load_avg

[K Prateek Nayak <kprateek.nayak@amd.com>]

Hello Peter,

On 7/31/2024 5:24 PM, Peter Zijlstra wrote:
> On Wed, Jul 31, 2024 at 11:46:48AM +0530, K Prateek Nayak wrote:
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index cd4a6bf14828..c437b408d29b 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -13297,10 +13297,34 @@ void unregister_fair_sched_group(struct task_group *tg)
>>   			if (se->sched_delayed) {
>>   				guard(rq_lock_irqsave)(rq);
>>   				if (se->sched_delayed) {
>> +					/*
>> +					 * We can reach here when processing RCU_SOFTIRQ on exit path from
>> +					 * a reschedule IPI. wakeup_preempt() may have set RQCF_REQ_SKIP to
>> +					 * skip a close clock update in schedule(), however, in presence of
>> +					 * a delayed entity, this trips the check in rq_clock_pelt() which
>> +					 * now believes the clock value is stale and needs updating. To
>> +					 * prevent such situation, cancel any pending skip updates, and
>> +					 * update the rq clock.
>> +					 */
>> +					rq_clock_cancel_skipupdate(rq);
>> +
>> +					/*
>> +					 * XXX: Will this trip WARN_DOUBLE_CLOCK? In which case, can
>> +					 * rq_clock_cancel_skipupdate() be made to return a bool if
>> +					 * RQCF_REQ_SKIP is set and we avoid this update?
>> +					 */
>>   					update_rq_clock(rq);
>> +
>>   					dequeue_entities(rq, se, DEQUEUE_SLEEP | DEQUEUE_DELAYED);
>> +
>> +					/* Avoid updating the clock again if a schedule() is pending */
>> +					if (task_on_rq_queued(rq->curr) &&
>> +					    test_tsk_need_resched(rq->curr))
>> +						rq_clock_skip_update(rq);
>>   				}
>>   				list_del_leaf_cfs_rq(cfs_rq);
>> +
>> +
>>   			}
>>   			remove_entity_load_avg(se);
>>   		}
> 
> So I did update this to simply add update_rq_clock() before the
> dequeue_entity(SLEEP|DELAYED). I initially had, these, then confused
> myself between deactivate_task() and dequeue_entity(), where the former
> updates the clock but the latter does not, and removed them. Then Mike
> complained, and I restored it for the regular exit path and forgot the
> cgroup exit path.
> 
> But now they should both be doing update_rq_clock() here.
> 
>    https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=sched/eevdf&id=5b3a132d4dd5c91f26beb3e8973c03cdb77d7873
> 
> Since this is all with our own rq->lock held, I don't think skip would
> be relevant here.

Thank you for clarifying the nuances. I'll continue testing with the
latest branch.
-- 
Thanks and Regards,
Prateek