Re: [PATCH -next] sched: Dec __cfs_bandwith_used in destroy_cfs_bandwidth()

Re: [PATCH -next] sched: Dec __cfs_bandwith_used in destroy_cfs_bandwidth()

[Zhang Qiao]

> 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@huawei.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
>> 

Re: [PATCH -next] sched: Dec __cfs_bandwith_used in destroy_cfs_bandwidth()

[Daniel Jordan]

On Wed, Jul 14, 2021 at 07:20:39PM +0800, Zhang Qiao wrote:
> 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?

It is fewer lines, but then hotplug lock is taken pointlessly for
!JUMP_LABEL.  Not a huge deal in a slow path like this, just a bit lame.
Adding a new variant seems cleaner if more verbose.

Re: [PATCH -next] sched: Dec __cfs_bandwith_used in destroy_cfs_bandwidth()

[Zhang Qiao]

On 2021/7/14 21:22, Daniel Jordan wrote:
> On Wed, Jul 14, 2021 at 07:20:39PM +0800, Zhang Qiao wrote:
>> 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?
> 
> It is fewer lines, but then hotplug lock is taken pointlessly for
> !JUMP_LABEL.  Not a huge deal in a slow path like this, just a bit lame.
> Adding a new variant seems cleaner if more verbose.
make sense. And I will modify in next version.
thanks.
> .
> 

[PATCH -next] sched: Dec __cfs_bandwith_used in destroy_cfs_bandwidth()

[Zhang Qiao]

__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.
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@huawei.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();
+
 	hrtimer_cancel(&cfs_b->period_timer);
 	hrtimer_cancel(&cfs_b->slack_timer);
 }
-- 
2.18.0.huawei.25

Re: [PATCH -next] sched: Dec __cfs_bandwith_used in destroy_cfs_bandwidth()

[Daniel Jordan]

[cc more, leaving full context]

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@huawei.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.

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