linux-kernel - Re: [PATCH -next] sched: Dec __cfs_bandwith_used in destroy_cfs

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-next>] [day] [month] [year] [list]

Date:   Wed, 14 Jul 2021 19:20:39 +0800
From:   Zhang Qiao <zhangqiao22@...wei.com>
To:     <daniel.m.jordan@...cle.com>
CC:     <bsegall@...gle.com>, <dietmar.eggemann@....com>,
        <juri.lelli@...hat.com>, <linux-kernel@...r.kernel.org>,
        <mingo@...hat.com>, <peterz@...radead.org>, <pjt@...gle.com>,
        <vincent.guittot@...aro.org>, <zhangqiao22@...wei.com>
Subject: Re: [PATCH -next] sched: Dec __cfs_bandwith_used in
 destroy_cfs_bandwidth()

> On Tue, Jul 06, 2021 at 04:38:20PM +0800, Zhang Qiao wrote:
>> __cfs_bandwith_uesd is a static_key to control cfs bandwidth
>> feature. When adding a cfs_bandwidth group, we need increase
>> the key, and decrease it when removing. But currently when we
>> remove a cfs_bandwidth group, we don't decrease the key and
>> this switch will always be on even if there is no cfs bandwidth
>> group in the system.
> 
> Yep, that's broken.
> 
>> Therefore, when removing a cfs bandwidth group, we decrease
>> __cfs_bandwith_used by calling cfs_bandwidth_usage_dec().
>> 
>> Fixes: 56f570e512ee ("sched: use jump labels to reduce overhead when bandwidth control is inactive")
>> Signed-off-by: Zhang Qiao <zhangqiao22@...wei.com>
>> ---
>>  kernel/sched/fair.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 103e31e53e2b..857e8908b7f7 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -5344,6 +5344,9 @@ static void destroy_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
>>  	if (!cfs_b->throttled_cfs_rq.next)
>>  		return;
>>  
>> +	if (cfs_b->quota != RUNTIME_INF)
>> +		cfs_bandwidth_usage_dec();
> 
> This calls static_key_slow_dec_cpuslocked, but destroy_cfs_bandwidth
> isn't holding the hotplug lock.
> 
> The other caller of cfs_bandwidth_usage_dec needs to hold it for another
> reason, so what about having both cfs_bandwidth_usage_dec() and
> cfs_bandwidth_usage_dec_cpuslocked()?  In that case, the _inc one could
> be renamed similarly so this isn't a stumbling block later on.
Hi Jordan, thanks for your comments.It is valuable to me.

And i have another thought is that we can hold the
hotplug lock before calling cfs_bandwidth_usage_dec().
This way, fewer modifications are involved.
What do you think about it?

thanks.
> 
>> +
>>  	hrtimer_cancel(&cfs_b->period_timer);
>>  	hrtimer_cancel(&cfs_b->slack_timer);
>>  }
>> -- 
>> 2.18.0.huawei.25
>>