linux-kernel - Re: [PATCH v2 1/2] sched/fair: Decrease cfs bandwidth usage in task

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <xm26le1rzijr.fsf@google.com>
Date: Tue, 23 Jul 2024 18:26:48 -0700
From: Benjamin Segall <bsegall@...gle.com>
To: Chuyi Zhou <zhouchuyi@...edance.com>
Cc: mingo@...hat.com,  peterz@...radead.org,  juri.lelli@...hat.com,
  vincent.guittot@...aro.org,  dietmar.eggemann@....com,
  rostedt@...dmis.org,  mgorman@...e.de,  vschneid@...hat.com,
  chengming.zhou@...ux.dev,  linux-kernel@...r.kernel.org,
  joshdon@...gle.com
Subject: Re: [PATCH v2 1/2] sched/fair: Decrease cfs bandwidth usage in
 task_group destruction

Chuyi Zhou <zhouchuyi@...edance.com> writes:

> The static key __cfs_bandwidth_used is used to indicate whether bandwidth
> control is enabled in the system. Currently, it is only decreased when a
> task group disables bandwidth control. This is incorrect because if there
> was a task group in the past that enabled bandwidth control, the
> __cfs_bandwidth_used will never go to zero, even if there are no task_group
> using bandwidth control now.
>
> This patch tries to fix this issue by decrsasing bandwidth usage in
> destroy_cfs_bandwidth(). cfs_bandwidth_usage_dec() calls
> static_key_slow_dec_cpuslocked which needs to hold hotplug lock, but cfs
> bandwidth destroy maybe run in a rcu callback. Move the call to
> destroy_cfs_bandwidth() from unregister_fair_sched_group() to
> cpu_cgroup_css_free() which runs in process context.
>
> Signed-off-by: Chuyi Zhou <zhouchuyi@...edance.com>

Reviewed-By: Ben Segall <bsegall@...gle.com>

> ---
>  kernel/sched/core.c  |  2 ++
>  kernel/sched/fair.c  | 13 +++++++------
>  kernel/sched/sched.h |  2 ++
>  3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 6d35c48239be..7720d34bd71b 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -12992,8 +12995,6 @@ void unregister_fair_sched_group(struct task_group *tg)
>  	struct rq *rq;
>  	int cpu;
>  
> -	destroy_cfs_bandwidth(tg_cfs_bandwidth(tg));
> -
>  	for_each_possible_cpu(cpu) {
>  		if (tg->se[cpu])
>  			remove_entity_load_avg(tg->se[cpu]);

There is a slightly subtle point here that autogroup cannot have a quota
set. If there's some shenanigans way that that's possible then it would
need a destroy as well. autogroup is already making assumptions anyways
though.