* Re: [PATCH -next] sched: Dec __cfs_bandwith_used in destroy_cfs_bandwidth()
@ 2021-07-14 11:20 Zhang Qiao
2021-07-14 13:22 ` Daniel Jordan
0 siblings, 1 reply; 5+ messages in thread
From: Zhang Qiao @ 2021-07-14 11:20 UTC (permalink / raw)
To: daniel.m.jordan
Cc: bsegall, dietmar.eggemann, juri.lelli, linux-kernel, mingo,
peterz, pjt, vincent.guittot, zhangqiao22
> 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
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH -next] sched: Dec __cfs_bandwith_used in destroy_cfs_bandwidth()
2021-07-14 11:20 [PATCH -next] sched: Dec __cfs_bandwith_used in destroy_cfs_bandwidth() Zhang Qiao
@ 2021-07-14 13:22 ` Daniel Jordan
2021-07-15 2:07 ` Zhang Qiao
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Jordan @ 2021-07-14 13:22 UTC (permalink / raw)
To: 20210712162655.w3j6uczwbfkzazvt
Cc: bsegall, dietmar.eggemann, juri.lelli, linux-kernel, mingo,
peterz, pjt, vincent.guittot, zhangqiao22
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH -next] sched: Dec __cfs_bandwith_used in destroy_cfs_bandwidth()
2021-07-14 13:22 ` Daniel Jordan
@ 2021-07-15 2:07 ` Zhang Qiao
0 siblings, 0 replies; 5+ messages in thread
From: Zhang Qiao @ 2021-07-15 2:07 UTC (permalink / raw)
To: Daniel Jordan
Cc: mingo, peterz, juri.lelli, vincent.guittot, dietmar.eggemann,
rostedt, bsegall, mgorman, bristot, pjt, linux-kernel
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.
> .
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH -next] sched: Dec __cfs_bandwith_used in destroy_cfs_bandwidth()
2021-07-06 8:38 Zhang Qiao
@ 2021-07-12 16:26 ` Daniel Jordan
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Jordan @ 2021-07-12 16:26 UTC (permalink / raw)
To: Zhang Qiao
Cc: mingo, peterz, juri.lelli, vincent.guittot, pjt, linux-kernel,
Ben Segall, Dietmar Eggemann
[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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH -next] sched: Dec __cfs_bandwith_used in destroy_cfs_bandwidth()
@ 2021-07-06 8:38 Zhang Qiao
2021-07-12 16:26 ` Daniel Jordan
0 siblings, 1 reply; 5+ messages in thread
From: Zhang Qiao @ 2021-07-06 8:38 UTC (permalink / raw)
To: mingo, peterz, juri.lelli, vincent.guittot; +Cc: pjt, linux-kernel
__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
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-07-15 2:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-14 11:20 [PATCH -next] sched: Dec __cfs_bandwith_used in destroy_cfs_bandwidth() Zhang Qiao
2021-07-14 13:22 ` Daniel Jordan
2021-07-15 2:07 ` Zhang Qiao
-- strict thread matches above, loose matches on Subject: below --
2021-07-06 8:38 Zhang Qiao
2021-07-12 16:26 ` Daniel Jordan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.