All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tejun Heo <tj@kernel.org>
To: Andrea Righi <arighi@nvidia.com>
Cc: David Vernet <void@manifault.com>,
	Changwoo Min <changwoo@igalia.com>,
	Emil Tsalapatis <emil@etsalapatis.com>,
	sched-ext@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 sched_ext/for-7.3-fixes] sched_ext: Reenqueue IMMED tasks when their CPU picks idle
Date: Mon, 19 Oct 2026 13:16:12 -1000	[thread overview]
Message-ID: <d9ba33d7358769a98a35eea9d603305d@kernel.org> (raw)
In-Reply-To: <20261017221924.3908922-1-arighi@nvidia.com>

Hello, Andrea.

The following is a Claude-generated review.

On Sun, Oct 18, 2026 at 12:19:24AM +0200, Andrea Righi wrote:
> +	/*
> +	 * Picking idle with IMMED tasks on the local DSQ means that they didn't
> +	 * run immediately, e.g., core scheduling forced the CPU idle. Outside
> +	 * of that, pick_task_scx() would have taken the local DSQ head. As
> +	 * next_class is idle, the rq looks open to rq_is_open(), tell
> +	 * reenq_local() to reenqueue the IMMED tasks anyway.
> +	 */
> +	if (idle && unlikely(rq->scx.nr_immed))
> +		scx_schedule_reenq_local(rq, SCX_REENQ_IDLE_PICK);

The forced-idle CPU is still advertised as idle: update_builtin_idle()
marks it idle right below, and the bounced task's task_cpu() is still
that CPU. A scheduler that trusts the idle state can put the task
straight back, e.g. scx_cosmos for a per-CPU task, which checks
test_cpu_idle(prev_cpu) and inserts into SCX_DSQ_LOCAL_ON | cpu with
SCX_ENQ_IMMED:

1. The IMMED insert calls wakeup_preempt() and reschedules the CPU.
2. The core-wide selection forces it idle again, and pick_task_idle()
   queues another IDLE_PICK reenq.
3. reenq_local() bounces the task again. p->scx.reenq_cnt is reset only
   when the task runs, so after SCX_REENQ_MAX_REPEAT rounds the
   scheduler exits with SCX_EXIT_ERROR_REENQ.

Before this patch, the task waited until the cookie allowed it to run.
With SCX_OPS_ALWAYS_ENQ_IMMED, scx_bpf_dsq_move_to_local() also inserts
with IMMED, so a dispatch can close the same loop.

"Outside of that, pick_task_scx() would have taken the local DSQ head"
doesn't hold for SCX_ENQ_LAST, with or without core scheduling, and the
same assumption was in the v1 suggestion. put_prev_task_scx() runs
ops.enqueue(SCX_ENQ_LAST) before set_next_task_idle(). If that inserts
the task into the local DSQ with IMMED, nr_immed is 1 here and the task
is bounced, although set_tsk_need_resched(next) was about to run it.
ENQ_LAST schedulers using IMMED would take an extra reenq every time a
lone task's slice expires. Would triggering from pick_task_idle(),
regardless of rq->curr, work? It runs before put_prev_task_scx(), and
the core-wide selection calls it for every forced-idle sibling.

Also, next_class isn't idle yet when this runs. On the re-pick after an
insert, it's still ext. It's idle by the time the deferred reenq_local()
runs.

> -	if (rq_is_open(rq, 0))
> +	if (!(reenq_flags & SCX_REENQ_IDLE_PICK) && rq_is_open(rq, 0))
>  		reenq_flags |= SCX_REENQ_TSR_RQ_OPEN;

Maybe update the %SCX_REENQ_TSR_RQ_OPEN description above
local_task_should_reenq() to mention SCX_REENQ_IDLE_PICK?

Separately, a case that neither this patch nor the existing triggers
cover: when the IMMED task at the head of the local DSQ doesn't match
the core cookie but sched_core_find() finds a matching SCX task outside
the local DSQ, the CPU runs that task instead of going idle, and the
IMMED task stays parked.

None of the above has been reproduced. It all comes from reading the
code.

Thanks.

--
tejun

      reply	other threads:[~2026-10-19 23:16 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-17 22:19 [PATCH v2 sched_ext/for-7.3-fixes] sched_ext: Reenqueue IMMED tasks when their CPU picks idle Andrea Righi
2026-10-19 23:16 ` Tejun Heo [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d9ba33d7358769a98a35eea9d603305d@kernel.org \
    --to=tj@kernel.org \
    --cc=arighi@nvidia.com \
    --cc=changwoo@igalia.com \
    --cc=emil@etsalapatis.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sched-ext@lists.linux.dev \
    --cc=void@manifault.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.