Re: [PATCH v2 2/4] sched/fair: refill bandwidth for the periods the timer slept through

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] sched/fair: period_timer forwarded with old period in
  tg_set_cfs_bandwidth()
--

commit 6dcac4d8cedf5c5fafb47bd8f0ac725a2be649bc
Author: Fernand Sieber <sieberf@amazon.com>

sched/fair: refill bandwidth for the periods the timer slept through

This commit refills a CFS group's accumulated bandwidth for the period
boundaries missed while its period timer was stopped, capped at quota +
burst, and updates bandwidth configuration and cpu.max.runtime reads and
writes to account for those idle periods.

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 76fcd70ff207a..75a5f5d1b8f54 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -9856,6 +9856,12 @@ static int tg_set_cfs_bandwidth(struct task_group *tg,
>  		cfs_bandwidth_usage_inc();
>  
>  	scoped_guard (raw_spinlock_irq, &cfs_b->lock) {
> +		/*
> +		 * The new bandwidth applies from now on. The group keeps what
> +		 * it earned while idle under the old one.
> +		 */
> +		cfs_bandwidth_refill_idle_periods(cfs_b);
> +
>  		cfs_b->period = ns_to_ktime(period);
>  		cfs_b->quota = quota;
>  		cfs_b->burst = burst;

[Severity: Medium]
When cfs_b->period_active is 0, does calling
cfs_bandwidth_refill_idle_periods() before updating cfs_b->period arm
cfs_b->period_timer with the old period's expiry instead of the new one?

cfs_bandwidth_refill_idle_periods() calls hrtimer_forward_now() with the
old cfs_b->period, pushing period_timer's expiry into (now, now +
old_period].  When tg_set_cfs_bandwidth() then calls
start_cfs_bandwidth() after updating cfs_b->period:

kernel/sched/core.c:tg_set_cfs_bandwidth() {
	...
		__refill_cfs_bandwidth_runtime(cfs_b);

		/*
		 * Restart the period timer (if active) to handle new
		 * period expiry:
		 */
		if (runtime_enabled)
			start_cfs_bandwidth(cfs_b);
	}
}

the second cfs_bandwidth_refill_idle_periods() call inside
start_cfs_bandwidth() sees an expiry already in the future, so
hrtimer_forward_now() returns 0 without adjusting it to the new period.
When a newly created cgroup (initialized with a 100ms period) or an idle
cgroup is configured with a shorter period such as 1ms,
hrtimer_start_expires() arms period_timer up to old_period in the future,
starving the group of refills for that interval after its initial quota is
spent.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794303670.git.sieberf@amazon.com?part=2