Re: [PATCH v3] sched/deadline: Make dl-server nohz full aware

[Juri Lelli]

Hello,

Thank you for posting this.

On 13/10/26 16:30, Ionut Nechita (Wind River) wrote:
> From: Ionut Nechita <ionut.nechita@xxxxxxxxxxxxx>
> 
> On a nohz_full core, a dl-server has to be armed when an RT task coexists
> with runnable CFS (or SCX) tasks, so the lower-priority class still gets
> its reserved bandwidth (fair_server default 50ms/1s) instead of being
> starved by the RT task.
> 
> Doing that the way v2 did, by arming the server and then returning false

I don't think we want to put this kind of historical discussion
reference into the changelog. I would only document what the current
solution does.

> from sched_can_stop_tick() for as long as a CFS/SCX task is queued, keeps
> the tick running for the whole time that task waits, not just while the
> server actually runs. On an isolated nohz_full core that is the more
> expensive of the two costs: measured with a SCHED_RR hog and a CFS task
> visiting the core about once per second, the tick never stopped again
> (200 irq_vectors:local_timer_entry per second, tick_stop reporting
> dependency=SCHED), against ~19/s with the fair server disabled.
> 
> The deferred server spends most of its period in the zero-laxity wait
> state: throttled, off the dl_rq, waiting for its timer. That wait is
> driven by an hrtimer and needs no tick. Keep the two concerns separate:
> 
>   - sched_can_stop_tick() arms rq->fair_server (and rq->ext_server for
>     SCX) when RT coexists with the respective class, but does not force
>     the tick for a merely deferred server. A non-deferred start enqueues
>     the server immediately; that is rechecked right after via
>     rq->dl.dl_nr_running and does keep the tick.
> 
>   - inc_dl_tasks()/dec_dl_tasks() call sched_update_tick_dependency()
>     for dl_server entities. Servers skip add_nr_running()/
>     sub_nr_running(), so until now nothing re-evaluated the tick when
>     the timer enqueued the server or when it was throttled again. The
>     tick is now started when the server is enqueued, so its runtime is
>     enforced from the tick, and dropped again once it leaves the dl_rq.
> 
> The servers are deliberately never stopped from sched_can_stop_tick().
> dl_server_start()/dl_server_stop() carry the GRUB accounting of
> rq->dl.running_bw through the "active contending" / "active non
> contending" state machine, with an inactive_timer in between. Stopping a
> server from a path that runs on every enqueue and dequeue cycles that
> state machine at a rate it is not meant for: a stop that finds the
> inactive timer still armed subtracts running_bw that the matching start
> did not add back, which underflows running_bw and trips WARN_ON_ONCE()
> in __sub_running_bw() and WARN_ON() in task_non_contending(). Teardown is
> left to the existing lazy path in __pick_task_dl(), which stops a server
> when it has no task of its class to pick. The cost is that a tickless
> core keeps taking the server's defer and inactive timers, which is a few
> interrupts per period, not a tick.

Kind of feel this is not completely ideal. nohz_full configurations
usually don't want to suffer from any kind of interference, even a
"quick" hrtimer expiration is usually too much (also considering it
seems useless).

> All of this is confined to nohz_full CPUs: sched_update_tick_dependency()
> returns early for housekeeping CPUs, so sched_can_stop_tick() (and with
> it the new arming) only runs where the tick can be stopped.
> 
> Fixes: 557a6bfc662c ("sched/fair: Add trivial fair server")
> Reported-by: David Haufe <dhaufe@xxxxxxxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/lkml/CAKJHwtOw_G67edzuHVtL1xC5Vyt6StcZzihtDd0yaKudW=rwVw@xxxxxxxxxxxxxx
> Suggested-by: Juri Lelli <juri.lelli@xxxxxxxxxx>
> Signed-off-by: Ionut Nechita <ionut.nechita@xxxxxxxxxxxxx>
> ---
> Changes from v2 [1]
> 
> - Rework the mechanism following the discussion on v2 [2]. Instead of
>   returning false from sched_can_stop_tick() while a CFS/SCX task is
>   queued (which pins the tick on the isolated core for the whole wait),
>   only arm the server there and let the tick stop while the server is
>   deferred. The tick is kept, and dropped again, from
>   inc_dl_tasks()/dec_dl_tasks() as the server enters and leaves the
>   dl_rq, so it runs only during the server's service window. Measured
>   tick rate on the isolated core drops from CONFIG_HZ to ~the service
>   window (see commit message).
> - Do not stop the servers from sched_can_stop_tick(). Cycling
>   dl_server_start()/stop() at enqueue/dequeue rate underflows
>   rq->dl.running_bw and trips the WARN_ON_ONCE() in __sub_running_bw()
>   and WARN_ON() in task_non_contending(); splats and a reproducer were
>   posted in the v2 thread [3]. Teardown is left to the lazy path in
>   __pick_task_dl(). As a result dl_servers_stop_all() is no longer
>   needed and is dropped.
> - Keep the SCX ext_server handling from v2 (arm rq->ext_server when RT
>   coexists with SCX tasks).
> - Rebased onto tip/sched/core.
> 
> Open question for maintainers: because the servers are no longer stopped
> here, a tickless isolated core keeps taking the server's defer/inactive
> hrtimers (a few interrupts per period, not a tick). Peter's idea in the
> v2 thread of marking the dl-server timers PINNED would keep those on the
> isolated core itself and address the housekeeping-side wakeups the
> original v2 targeted; it was parked pending a remote hrtimer_start_on().
> This patch does not depend on it.
> 
> The running_bw underflow splats and reproducer mentioned above, and
> Peter's PINNED-timer suggestion, are both in the v2 thread [2].
> 
> [1] https://lore.kernel.org/lkml/20260513-upstream-fix-dlserver-nohzfull-b4-v2-1-d3e9cbe5c845@xxxxxxxxxx/
> [2] https://lore.kernel.org/lkml/aszViUxPLBuH0h7L@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> 
>  kernel/sched/core.c     | 32 +++++++++++++++++++++++++++++++-
>  kernel/sched/deadline.c |  4 ++++
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 71d3c948e14ae..0177c950a4885 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1440,10 +1440,40 @@ bool sched_can_stop_tick(struct rq *rq)
>  {
>  	int fifo_nr_running;
>  
> -	/* Deadline tasks, even if single, need the tick */
> +	/*
> +	 * Deadline tasks, even if single, need the tick. This includes a
> +	 * dl-server while it is enqueued and serving its class: its runtime
> +	 * is enforced from the tick.
> +	 */
>  	if (rq->dl.dl_nr_running)
>  		return false;
>  
> +	/*
> +	 * RT coexists with a lower-priority class: make sure the matching
> +	 * dl-server(s) are armed, so CFS/SCX still get their reserved
> +	 * bandwidth even though the tick is about to be stopped. While a
> +	 * server is only deferred (throttled, waiting for its timer) it is
> +	 * not on the dl_rq and needs no tick; once the timer enqueues it,
> +	 * inc_dl_tasks() re-evaluates the dependency.
> +	 *
> +	 * dl_server_start() is a no-op for an already active server (or one
> +	 * with no runtime configured), and the servers are never stopped
> +	 * from here: their start/stop pair carries the GRUB accounting of
> +	 * rq->dl.running_bw, which must not be cycled at enqueue/dequeue
> +	 * rate.
> +	 */
> +	if (rq->rt.rt_nr_running) {
> +		if (rq->cfs.h_nr_queued)
> +			dl_server_start(&rq->fair_server);
> +#ifdef CONFIG_SCHED_CLASS_EXT
> +		if (rq->scx.nr_running)
> +			dl_server_start(&rq->ext_server);
> +#endif
> +		/* A non-deferred start enqueues a server right away. */
> +		if (rq->dl.dl_nr_running)
> +			return false;

Oh, and we can get rid of the bool I used in v2. OK.

> +	}
> +
>  	/*
>  	 * If there are more than one RR tasks, we need the tick to affect the
>  	 * actual RR behaviour.
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index c0ebdcde5fe5d..589c4c881c31b 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -2259,6 +2259,8 @@ void inc_dl_tasks(struct sched_dl_entity *dl_se, struct dl_rq *dl_rq)
>  
>  	if (!dl_server(dl_se))
>  		add_nr_running(rq_of_dl_rq(dl_rq), 1);
> +	else
> +		sched_update_tick_dependency(rq_of_dl_rq(dl_rq));
>  
>  	inc_dl_deadline(dl_rq, deadline);
>  }
> @@ -2271,6 +2273,8 @@ void dec_dl_tasks(struct sched_dl_entity *dl_se, struct dl_rq *dl_rq)
>  
>  	if (!dl_server(dl_se))
>  		sub_nr_running(rq_of_dl_rq(dl_rq), 1);
> +	else
> +		sched_update_tick_dependency(rq_of_dl_rq(dl_rq));

For some reason I don't particularly like calling
sched_update_tick_dependency from deadline code, but after all
add/sub_nr_running ends up doing the same, so maybe it's fine.

Thanks,
Juri