__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 <[email protected]>
---
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
[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 <[email protected]>
> ---
> 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
>
> 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 <[email protected]>
>> ---
>> 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
>>
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.
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.
> .
>
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.
Fix the problem as two steps:
1.Rename cfs_bandwidth_usage_{dec, inc}() to
cfs_bandwidth_usage_{dec,inc}_cpuslocked() and its caller need to
hold the hotplug lock.
2.Add cfs_bandwidth_usage_{dec,inc}() and its caller don't need
to hold the hotplug lock. And 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 <[email protected]>
---
Changes since v1:
- Add suffix _cpuslocked to cfs_bandwidth_usage_{dec,inc}.
- Use static_key_slow_{dec,inc}_cpuslocked() in origin
cfs_bandwidth_usage_{dec,inc}().
---
kernel/sched/core.c | 4 ++--
kernel/sched/fair.c | 20 ++++++++++++++++++--
kernel/sched/sched.h | 3 +++
3 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index b726ea4ac341..76f661f3f12b 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -9854,7 +9854,7 @@ static int tg_set_cfs_bandwidth(struct task_group *tg, u64 period, u64 quota,
* before making related changes, and on->off must occur afterwards
*/
if (runtime_enabled && !runtime_was_enabled)
- cfs_bandwidth_usage_inc();
+ cfs_bandwidth_usage_inc_cpuslocked();
raw_spin_lock_irq(&cfs_b->lock);
cfs_b->period = ns_to_ktime(period);
cfs_b->quota = quota;
@@ -9882,7 +9882,7 @@ static int tg_set_cfs_bandwidth(struct task_group *tg, u64 period, u64 quota,
rq_unlock_irq(rq, &rf);
}
if (runtime_was_enabled && !runtime_enabled)
- cfs_bandwidth_usage_dec();
+ cfs_bandwidth_usage_dec_cpuslocked();
out_unlock:
mutex_unlock(&cfs_constraints_mutex);
put_online_cpus();
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 44e44c235f1f..e6d66a62c960 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4613,21 +4613,34 @@ static inline bool cfs_bandwidth_used(void)
return static_key_false(&__cfs_bandwidth_used);
}
-void cfs_bandwidth_usage_inc(void)
+void cfs_bandwidth_usage_inc_cpuslocked(void)
{
static_key_slow_inc_cpuslocked(&__cfs_bandwidth_used);
}
-void cfs_bandwidth_usage_dec(void)
+void cfs_bandwidth_usage_dec_cpuslocked(void)
{
static_key_slow_dec_cpuslocked(&__cfs_bandwidth_used);
}
+
+void cfs_bandwidth_usage_inc(void)
+{
+ static_key_slow_inc(&__cfs_bandwidth_used);
+}
+
+void cfs_bandwidth_usage_dec(void)
+{
+ static_key_slow_dec(&__cfs_bandwidth_used);
+}
#else /* CONFIG_JUMP_LABEL */
static bool cfs_bandwidth_used(void)
{
return true;
}
+void cfs_bandwidth_usage_inc_cpuslocked(void) {}
+void cfs_bandwidth_usage_dec_cpuslocked(void) {}
+
void cfs_bandwidth_usage_inc(void) {}
void cfs_bandwidth_usage_dec(void) {}
#endif /* CONFIG_JUMP_LABEL */
@@ -5345,6 +5358,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);
}
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 0fa583db5c4a..4baba3473b0b 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -2693,6 +2693,9 @@ extern void init_dl_rq(struct dl_rq *dl_rq);
extern void cfs_bandwidth_usage_inc(void);
extern void cfs_bandwidth_usage_dec(void);
+extern void cfs_bandwidth_usage_inc_cpuslocked(void);
+extern void cfs_bandwidth_usage_dec_cpuslocked(void);
+
#ifdef CONFIG_NO_HZ_COMMON
#define NOHZ_BALANCE_KICK_BIT 0
#define NOHZ_STATS_KICK_BIT 1
--
2.18.0.huawei.25
Only nits left from my side anyway. Looks fine otherwise!
There are a couple typos in the subject and changelog, cfs_bandwith_used
and cfs_bandwith_uesd.
On Thu, Jul 15, 2021 at 07:24:33PM +0800, Zhang Qiao wrote:
> +
> +void cfs_bandwidth_usage_inc(void)
> +{
> + static_key_slow_inc(&__cfs_bandwidth_used);
> +}
Nothing calls this, and getting rid of it would potentially avoid a
janitorial patch later.
> +void cfs_bandwidth_usage_dec(void)
static void cfs_bandwidth_usage_dec(void)
> void cfs_bandwidth_usage_inc(void) {}
> void cfs_bandwidth_usage_dec(void) {}
static void cfs_bandwidth_usage_dec(void) {}
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> extern void cfs_bandwidth_usage_inc(void);
> extern void cfs_bandwidth_usage_dec(void);
These would then go away.
cfs_bandwidth_used 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.
Fix the problem as two steps:
1.Rename cfs_bandwidth_usage_{dec, inc}() to
cfs_bandwidth_usage_{dec,inc}_cpuslocked() and its caller need to
hold the hotplug lock.
2.Add cfs_bandwidth_usage_dec() and its caller don't need
to hold the hotplug lock. And when removing a cfs bandwidth group,
we decrease cfs_bandwidth_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 <[email protected]>
---
Changes since v2:
- Remove cfs_bandwidth_usage_inc()
- Make cfs_bandwidth_usage_dec() static
Changes since v1:
- Add suffix _cpuslocked to cfs_bandwidth_usage_{dec,inc}.
- Use static_key_slow_{dec,inc}_cpuslocked() in origin
cfs_bandwidth_usage_{dec,inc}().
---
kernel/sched/core.c | 4 ++--
kernel/sched/fair.c | 18 ++++++++++++++----
kernel/sched/sched.h | 4 ++--
3 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 014ce8b385c7..d407aab4ea38 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -9865,7 +9865,7 @@ static int tg_set_cfs_bandwidth(struct task_group *tg, u64 period, u64 quota,
* before making related changes, and on->off must occur afterwards
*/
if (runtime_enabled && !runtime_was_enabled)
- cfs_bandwidth_usage_inc();
+ cfs_bandwidth_usage_inc_cpuslocked();
raw_spin_lock_irq(&cfs_b->lock);
cfs_b->period = ns_to_ktime(period);
cfs_b->quota = quota;
@@ -9893,7 +9893,7 @@ static int tg_set_cfs_bandwidth(struct task_group *tg, u64 period, u64 quota,
rq_unlock_irq(rq, &rf);
}
if (runtime_was_enabled && !runtime_enabled)
- cfs_bandwidth_usage_dec();
+ cfs_bandwidth_usage_dec_cpuslocked();
out_unlock:
mutex_unlock(&cfs_constraints_mutex);
put_online_cpus();
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 44e44c235f1f..03a1e98cbe96 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4613,23 +4613,30 @@ static inline bool cfs_bandwidth_used(void)
return static_key_false(&__cfs_bandwidth_used);
}
-void cfs_bandwidth_usage_inc(void)
+void cfs_bandwidth_usage_inc_cpuslocked(void)
{
static_key_slow_inc_cpuslocked(&__cfs_bandwidth_used);
}
-void cfs_bandwidth_usage_dec(void)
+void cfs_bandwidth_usage_dec_cpuslocked(void)
{
static_key_slow_dec_cpuslocked(&__cfs_bandwidth_used);
}
+
+static void cfs_bandwidth_usage_dec(void)
+{
+ static_key_slow_dec(&__cfs_bandwidth_used);
+}
#else /* CONFIG_JUMP_LABEL */
static bool cfs_bandwidth_used(void)
{
return true;
}
-void cfs_bandwidth_usage_inc(void) {}
-void cfs_bandwidth_usage_dec(void) {}
+void cfs_bandwidth_usage_inc_cpuslocked(void) {}
+void cfs_bandwidth_usage_dec_cpuslocked(void) {}
+
+static void cfs_bandwidth_usage_dec(void) {}
#endif /* CONFIG_JUMP_LABEL */
/*
@@ -5345,6 +5352,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);
}
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 0fa583db5c4a..6b2f58a99fa2 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -2690,8 +2690,8 @@ extern void init_cfs_rq(struct cfs_rq *cfs_rq);
extern void init_rt_rq(struct rt_rq *rt_rq);
extern void init_dl_rq(struct dl_rq *dl_rq);
-extern void cfs_bandwidth_usage_inc(void);
-extern void cfs_bandwidth_usage_dec(void);
+extern void cfs_bandwidth_usage_inc_cpuslocked(void);
+extern void cfs_bandwidth_usage_dec_cpuslocked(void);
#ifdef CONFIG_NO_HZ_COMMON
#define NOHZ_BALANCE_KICK_BIT 0
--
2.18.0.huawei.25
On Fri, Jul 16, 2021 at 10:27:56AM +0800, Zhang Qiao wrote:
> cfs_bandwidth_used 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.
>
> Fix the problem as two steps:
> 1.Rename cfs_bandwidth_usage_{dec, inc}() to
> cfs_bandwidth_usage_{dec,inc}_cpuslocked() and its caller need to
> hold the hotplug lock.
> 2.Add cfs_bandwidth_usage_dec() and its caller don't need
> to hold the hotplug lock. And when removing a cfs bandwidth group,
> we decrease cfs_bandwidth_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 <[email protected]>
Reviewed-by: Daniel Jordan <[email protected]>
Just a ping...
thanks,
Qiao Zhang.