Re: [PATCH 1/6] sched/core: Handle pick_task() releasing the rq lock

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Stale runqueue clock and WARN_DOUBLE_CLOCK splats upon pick_task() retry
--

--- Patch [1]: [PATCH 1/6] sched/core: Handle pick_task() releasing the rq lock ---
commit e3861f6642d2aed51cd1900fde0081c42124a072
Author: Tejun Heo <[email protected]>

sched/core: Handle pick_task() releasing the rq lock

Core scheduling's pick_next_task() breaks when a ->pick_task() implementation
can release the rq lock. This patch handles this by restarting the whole
selection when a pick returns RETRY_TASK after releasing the lock.

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 96226707c2f61..136246846e792 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
[ ... ]
> @@ -6301,10 +6303,15 @@ pick_next_task(struct rq *rq, struct rq_flags *rf)
>  	 * and there are no cookied tasks running on siblings.
>  	 */
>  	if (!need_sync) {
>  		next = pick_task(rq, rf);
> -		if (unlikely(next == RETRY_TASK))
> -			goto restart_single;
> +		if (unlikely(next == RETRY_TASK)) {
> +			/* rq lock may have been dropped, clocks invalidated */
> +			core_clock_updated = false;
> +			if (!(rq->clock_update_flags & RQCF_UPDATED))
> +				update_rq_clock(rq);

[Severity: High]
Does this bypass the clock update entirely? When pick_task() drops the lock
and repins it, the rq->clock_update_flags are preserved and restored. This
means RQCF_UPDATED would still be set, making this condition false and leaving
the runqueue clock stale despite the lock having been dropped.

> +			goto restart;
> +		}
> +
>  		if (!next->core_cookie) {
>  			rq->core_pick = NULL;
>  			rq->core_dl_server = NULL;
> @@ -6324,7 +6331,6 @@ pick_next_task(struct rq *rq, struct rq_flags *rf)
>  	 *
>  	 * Tie-break prio towards the current CPU
>  	 */
>  	max = NULL;
>  	for_each_cpu_wrap(i, smt_mask, cpu) {
>  		rq_i = cpu_rq(i);
> @@ -6338,8 +6344,13 @@ pick_next_task(struct rq *rq, struct rq_flags *rf)
>  			update_rq_clock(rq_i);

[Severity: High]
Will this trigger a WARN_DOUBLE_CLOCK splat for sibling runqueues if the
loop is restarted? 

If a previous pass updated the clock for rq_i, the RQCF_UPDATED flag is set.
When we hit goto restart, we loop again and call update_rq_clock(rq_i)
unconditionally without clearing the flag first.

>  
>  		p = pick_task(rq_i, rf);
> -		if (unlikely(p == RETRY_TASK))
> -			goto restart_multi;
> +		if (unlikely(p == RETRY_TASK)) {
> +			/* rq lock may have been dropped, clocks invalidated */
> +			core_clock_updated = false;
> +			if (!(rq->clock_update_flags & RQCF_UPDATED))
> +				update_rq_clock(rq);

[Severity: High]
Similarly, does this bypass the clock update on the local runqueue because
RQCF_UPDATED is still set after restoring the pin context?

> +			goto restart;
> +		}
>  
>  		rq_i->core_pick = p;
>  		rq_i->core_dl_server = rq_i->dl_server;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1