Re: [PATCH v2 3/2] sched/deadline: Check bandwidth overflow earlier for hotplug

Jon Hunter
2025-02-21T11:56:26Z

On 20/02/2025 15:25, Juri Lelli wrote:
> On 20/02/25 11:40, Juri Lelli wrote:
>> On 19/02/25 19:14, Dietmar Eggemann wrote:
>
> ...
>
>> OK. CPU3 + CPU4 (CPU5 offline).
>>
>>> [ 171.003085] __dl_update() (3) cpu=2 rq->dl.extra_bw=1122848
>>> [ 171.003091] __dl_update() (3) cpu=3 rq->dl.extra_bw=1022361
>>> [ 171.003096] __dl_update() (3) cpu=4 rq->dl.extra_bw=1035468
>>> [ 171.003103] dl_bw_cpus() cpu=2 rd->span=0-2 cpu_active_mask=0-4 cpumask_weight(rd->span)=3 type=DYN
>>> [ 171.003113] __dl_server_attach_root() called cpu=2
>>> [ 171.003118] dl_bw_cpus() cpu=2 rd->span=0-2 cpu_active_mask=0-4 cpumask_weight(rd->span)=3 type=DYN
>>> [ 171.003127] __dl_add() tsk_bw=52428 dl_b->total_bw=157284 type=DYN rd->span=0-2
>>> [ 171.003136] __dl_update() (3) cpu=0 rq->dl.extra_bw=477111
>>> [ 171.003141] __dl_update() (3) cpu=1 rq->dl.extra_bw=851970
>>> [ 171.003147] __dl_update() (3) cpu=2 rq->dl.extra_bw=1105372
>>> [ 171.003188] root domain span: 0-2
>>> [ 171.003194] default domain span: 3-5
>>> [ 171.003220] rd 0-2: Checking EAS, schedutil is mandatory
>>> [ 171.005840] psci: CPU5 killed (polled 0 ms)
>>
>> OK. DYN has (CPU0,1,2) 157284 and DEF (CPU3,4) 104856.
>>
>> CPU4 going offline (it's isolated on DEF).
>>
>>> [ 171.006436] dl_bw_deactivate() called cpu=4
>>> [ 171.006446] __dl_bw_capacity() mask=3-5 cap=892
>>> [ 171.006454] dl_bw_cpus() cpu=4 rd->span=3-5 cpu_active_mask=0-4 cpus=2 type=DEF
>>> [ 171.006464] dl_bw_manage: cpu=4 cap=446 fair_server_bw=52428 total_bw=104856 dl_bw_cpus=2 type=DEF span=3-5
>>> [ 171.006475] dl_bw_cpus() cpu=4 rd->span=3-5 cpu_active_mask=0-4 cpus=2 type=DEF
>>> [ 171.006485] CPU: 4 UID: 0 PID: 36 Comm: cpuhp/4 Not tainted 6.13.0-09343-g9ce523149e08-dirty #172
>>> [ 171.006495] Hardware name: ARM Juno development board (r0) (DT)
>>> [ 171.006499] Call trace:
>>> [ 171.006502] show_stack+0x18/0x24 (C)
>>> [ 171.006514] dump_stack_lvl+0x74/0x8c
>>> [ 171.006528] dump_stack+0x18/0x24
>>> [ 171.006541] dl_bw_manage+0x3a0/0x500
>>> [ 171.006554] dl_bw_deactivate+0x40/0x50
>>> [ 171.006564] sched_cpu_deactivate+0x34/0x24c
>>> [ 171.006579] cpuhp_invoke_callback+0x138/0x694
>>> [ 171.006591] cpuhp_thread_fun+0xb0/0x198
>>> [ 171.006604] smpboot_thread_fn+0x200/0x224
>>> [ 171.006616] kthread+0x12c/0x204
>>> [ 171.006627] ret_from_fork+0x10/0x20
>>> [ 171.006639] __dl_overflow() dl_b->bw=996147 cap=446 cap_scale(dl_b->bw, cap)=433868 dl_b->total_bw=104856 old_bw=52428 new_bw=0 type=DEF rd->span=3-5
>>> [ 171.006652] dl_bw_manage() cpu=4 cap=446 overflow=0 req=0 return=0 type=DEF
>>> [ 171.006706] partition_sched_domains() called
>>> [ 171.006713] CPU: 4 UID: 0 PID: 36 Comm: cpuhp/4 Not tainted 6.13.0-09343-g9ce523149e08-dirty #172
>>> [ 171.006722] Hardware name: ARM Juno development board (r0) (DT)
>>> [ 171.006727] Call trace:
>>> [ 171.006730] show_stack+0x18/0x24 (C)
>>> [ 171.006740] dump_stack_lvl+0x74/0x8c
>>> [ 171.006754] dump_stack+0x18/0x24
>>> [ 171.006767] partition_sched_domains+0x48/0x7c
>>> [ 171.006778] sched_cpu_deactivate+0x1a8/0x24c
>>> [ 171.006792] cpuhp_invoke_callback+0x138/0x694
>>> [ 171.006805] cpuhp_thread_fun+0xb0/0x198
>>> [ 171.006817] smpboot_thread_fn+0x200/0x224
>>> [ 171.006829] kthread+0x12c/0x204
>>> [ 171.006840] ret_from_fork+0x10/0x20
>>> [ 171.006852] partition_sched_domains_locked() ndoms_new=1
>>> [ 171.006861] partition_sched_domains_locked() goto match2
>>> [ 171.006867] rd 0-2: Checking EAS, schedutil is mandatory
>>> [ 171.007774] psci: CPU4 killed (polled 4 ms)
>>
>> As I guess you were saying above, CPU4 contribution is not removed from
>> DEF.
>>
>>> [ 171.007971] dl_bw_deactivate() called cpu=3
>>> [ 171.007981] __dl_bw_capacity() mask=3-5 cap=446
>>> [ 171.007989] dl_bw_cpus() cpu=3 rd->span=3-5 cpu_active_mask=0-3 cpus=1 type=DEF
>>> [ 171.007999] dl_bw_manage: cpu=3 cap=0 fair_server_bw=52428 total_bw=104856 dl_bw_cpus=1 type=DEF span=3-5
>> ^^^^
>> And this is now wrong. :/
>
> So, CPU4 was still on DEF and we don't go through any of the accouting
> functions. I wonder if we could simplify this by always re-doing the
> accounting after root domains are stable (also for partition_
> sched_domain()). So, please take a look at what below. It can definitely
> be better encapsulated (also more cleanups are needed) and maybe it's
> just useless/stupid (hard to say here because I always see 'pass'
> whatever I try to change), but anyway. Also pushed to the usual branch.
>
> ---
> include/linux/sched/deadline.h | 4 ++++
> kernel/cgroup/cpuset.c | 13 ++++++++-----
> kernel/sched/deadline.c | 11 ++++++++---
> kernel/sched/topology.c | 1 +
> 4 files changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/sched/deadline.h b/include/linux/sched/deadline.h
> index 3a912ab42bb5..8fc4918c6f3f 100644
> --- a/include/linux/sched/deadline.h
> +++ b/include/linux/sched/deadline.h
> @@ -34,6 +34,10 @@ static inline bool dl_time_before(u64 a, u64 b)
> struct root_domain;
> extern void dl_add_task_root_domain(struct task_struct *p);
> extern void dl_clear_root_domain(struct root_domain *rd);
> +extern void dl_clear_root_domain_cpu(int cpu);
> +
> +extern u64 dl_generation;
> +extern bool dl_bw_visited(int cpu, u64 gen);
>
> #endif /* CONFIG_SMP */
>
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index 0f910c828973..52243dcc61ab 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -958,6 +958,8 @@ static void dl_rebuild_rd_accounting(void)
> {
> struct cpuset *cs = NULL;
> struct cgroup_subsys_state *pos_css;
> + int cpu;
> + u64 gen = ++dl_generation;
>
> lockdep_assert_held(&cpuset_mutex);
> lockdep_assert_cpus_held();
> @@ -965,11 +967,12 @@ static void dl_rebuild_rd_accounting(void)
>
> rcu_read_lock();
>
> - /*
> - * Clear default root domain DL accounting, it will be computed again
> - * if a task belongs to it.
> - */
> - dl_clear_root_domain(&def_root_domain);
> + for_each_possible_cpu(cpu) {
> + if (dl_bw_visited(cpu, gen))
> + continue;
> +
> + dl_clear_root_domain_cpu(cpu);
> + }
>
> cpuset_for_each_descendant_pre(cs, pos_css, &top_cpuset) {
>
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 8f7420e0c9d6..a6723ed84e68 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -166,7 +166,7 @@ static inline unsigned long dl_bw_capacity(int i)
> }
> }
>
> -static inline bool dl_bw_visited(int cpu, u64 gen)
> +bool dl_bw_visited(int cpu, u64 gen)
> {
> struct root_domain *rd = cpu_rq(cpu)->rd;
>
> @@ -207,7 +207,7 @@ static inline unsigned long dl_bw_capacity(int i)
> return SCHED_CAPACITY_SCALE;
> }
>
> -static inline bool dl_bw_visited(int cpu, u64 gen)
> +bool dl_bw_visited(int cpu, u64 gen)
> {
> return false;
> }
> @@ -3037,6 +3037,11 @@ void dl_clear_root_domain(struct root_domain *rd)
> }
> }
>
> +void dl_clear_root_domain_cpu(int cpu) {
> + printk_deferred("%s: cpu=%d\n", __func__, cpu);
> + dl_clear_root_domain(cpu_rq(cpu)->rd);
> +}
> +
> #endif /* CONFIG_SMP */
>
> static void switched_from_dl(struct rq *rq, struct task_struct *p)
> @@ -3216,7 +3221,7 @@ DEFINE_SCHED_CLASS(dl) = {
> };
>
> /* Used for dl_bw check and update, used under sched_rt_handler()::mutex */
> -static u64 dl_generation;
> +u64 dl_generation;
>
> int sched_dl_global_validate(void)
> {
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index c6a140d8d851..9892e6fa3e57 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -2814,5 +2814,6 @@ void partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
> {
> mutex_lock(&sched_domains_mutex);
> partition_sched_domains_locked(ndoms_new, doms_new, dattr_new);
> + dl_rebuild_rd_accounting();
> mutex_unlock(&sched_domains_mutex);
> }
>

Latest branch is not building for me ...

CC kernel/time/hrtimer.o
In file included from kernel/sched/build_utility.c:88:
kernel/sched/topology.c: In function ‘partition_sched_domains’:
kernel/sched/topology.c:2817:9: error: implicit declaration of function ‘dl_rebuild_rd_accounting’ [-Werror=implicit-function-declaration]
2817 | dl_rebuild_rd_accounting();
| ^~~~~~~~~~~~~~~~~~~~~~~~

Looks like we are missing a prototype.

Jon

--
nvpublic