[PATCH v2] sched/fair: set burst to 0 when remove the restriction on cfs bandwidth

[PATCH v2] sched/fair: set burst to 0 when remove the restriction on cfs bandwidth

[Cheng Yu]

From: Zhao Wenhui <zhaowenhui8@huawei.com>

In the cpu subsystem of cgroup v1 and v2, we set the restriction on cfs
bandwidth by setting the quota and burst value. Later, when we remove
the restriction by setting the quota to the default value, the burst
value should also be forced to the its default value of zero.

In the cgroup v1 cpu subsystem, assuming we have a cgroup named 'test',
and we set cpu.cfs_quota_us and cpu.cfs_burst_us:
    # echo 100000 > cpu.cfs_quota_us
    # echo 100000 > cpu.cfs_burst_us

Next we remove the restriction on cfs bandwidth:
    # echo -1 > cpu.cfs_quota_us
    # cat cpu.cfs_quota_us
    -1
    # cat cpu.cfs_burst_us
    100000

Now we expect that the value of burst should be zero. When the burst is
zero, it means that the restriction on burst is removed.

The same situation exists for cgroup v2. The difference is that the
interface definition of the cpu subsystem and the default value of
quota. In v2, we remove the restriction on cfs bandwidth by setting max
to cpu.max.

Fixes: f4183717b370 ("sched/fair: Introduce the burstable CFS controller")
Reported-by: Zhao Gongyi <zhaogongyi@huawei.com>
Reported-by: Qixin Liao <liaoqixin@huawei.com>
Signed-off-by: Zhao Wenhui <zhaowenhui8@huawei.com>
Signed-off-by: Cheng Yu <serein.chengyu@huawei.com>
Tested-by: Vishal Chourasia <vishalc@linux.ibm.com>
Reviewed-by: Tianchen Ding <dtcccc@linux.alibaba.com>
Reviewed-by: Ben Segall <bsegall@google.com>
---
Change log:
----------
v2:
 - Put the modifications to cgroup v1 and v2 in one patch
v1:
 - patch for cgroup v1:
   https://lore.kernel.org/all/20220809120320.19496-1-zhaowenhui8@huawei.com/
 - patchset for cgroup v1 and v2:
   https://lore.kernel.org/all/20240522031007.643498-1-serein.chengyu@huawei.com/
---
 kernel/sched/core.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index bcf2c4cc0522..982d357b3983 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -10840,6 +10840,12 @@ static int tg_set_cfs_bandwidth(struct task_group *tg, u64 period, u64 quota,
 				     burst + quota > max_cfs_runtime))
 		return -EINVAL;
 
+	/*
+	 * Ensure burst equals to zero when quota is -1.
+	 */
+	if (quota == RUNTIME_INF && burst)
+		return -EINVAL;
+
 	/*
 	 * Prevent race between setting of cfs_rq->runtime_enabled and
 	 * unthrottle_offline_cfs_rqs().
@@ -10899,8 +10905,10 @@ static int tg_set_cfs_quota(struct task_group *tg, long cfs_quota_us)
 
 	period = ktime_to_ns(tg->cfs_bandwidth.period);
 	burst = tg->cfs_bandwidth.burst;
-	if (cfs_quota_us < 0)
+	if (cfs_quota_us < 0) {
 		quota = RUNTIME_INF;
+		burst = 0;
+	}
 	else if ((u64)cfs_quota_us <= U64_MAX / NSEC_PER_USEC)
 		quota = (u64)cfs_quota_us * NSEC_PER_USEC;
 	else
@@ -11406,8 +11414,11 @@ static ssize_t cpu_max_write(struct kernfs_open_file *of,
 	int ret;
 
 	ret = cpu_period_quota_parse(buf, &period, &quota);
-	if (!ret)
+	if (!ret) {
+		if (quota == RUNTIME_INF)
+			burst = 0;
 		ret = tg_set_cfs_bandwidth(tg, period, quota, burst);
+	}
 	return ret ?: nbytes;
 }
 #endif
-- 
2.25.1

Re: [PATCH v2] sched/fair: set burst to 0 when remove the restriction on cfs bandwidth

[Tejun Heo]

Hello,

On Mon, Jul 08, 2024 at 08:00:53PM +0800, Cheng Yu wrote:
> From: Zhao Wenhui <zhaowenhui8@huawei.com>
> 
> In the cpu subsystem of cgroup v1 and v2, we set the restriction on cfs
> bandwidth by setting the quota and burst value. Later, when we remove
> the restriction by setting the quota to the default value, the burst
> value should also be forced to the its default value of zero.

Explaining a bit why burst value should become zero can be helpful - ie.
what happens when quota is not set but burst is limited?

...
> @@ -10840,6 +10840,12 @@ static int tg_set_cfs_bandwidth(struct task_group *tg, u64 period, u64 quota,
>  				     burst + quota > max_cfs_runtime))
>  		return -EINVAL;
>  
> +	/*
> +	 * Ensure burst equals to zero when quota is -1.
> +	 */
> +	if (quota == RUNTIME_INF && burst)
> +		return -EINVAL;

This happening would be a bug, right? Would it make more sense to use
WARN_ON_ONCE()?

Thanks.

-- 
tejun

Re: [PATCH v2] sched/fair: set burst to 0 when remove the restriction on cfs bandwidth

[Cheng Yu]

Hi,

On 2024/7/9 2:08, Tejun Heo wrote:
> Hello,
> 
> On Mon, Jul 08, 2024 at 08:00:53PM +0800, Cheng Yu wrote:
>> From: Zhao Wenhui <zhaowenhui8@huawei.com>
>>
>> In the cpu subsystem of cgroup v1 and v2, we set the restriction on cfs
>> bandwidth by setting the quota and burst value. Later, when we remove
>> the restriction by setting the quota to the default value, the burst
>> value should also be forced to the its default value of zero.
> 
> Explaining a bit why burst value should become zero can be helpful - ie.
> what happens when quota is not set but burst is limited?

When the restriction on cfs bandwidth is removed, the purpose of setting
burst value to zero is to avoid ambiguity.

> 
> ...
>> @@ -10840,6 +10840,12 @@ static int tg_set_cfs_bandwidth(struct task_group *tg, u64 period, u64 quota,
>>  				     burst + quota > max_cfs_runtime))
>>  		return -EINVAL;
>>  
>> +	/*
>> +	 * Ensure burst equals to zero when quota is -1.
>> +	 */
>> +	if (quota == RUNTIME_INF && burst)
>> +		return -EINVAL;
> 
> This happening would be a bug, right? Would it make more sense to use
> WARN_ON_ONCE()?

As Vishal Chourasia mentioned, it can prevent setting excessively large
burst value. Using WARN_ON_ONCE() does not have this effect.

> 
> Thanks.
> 

Thanks
--
Cheng Yu

Re: [PATCH v2] sched/fair: set burst to 0 when remove the restriction on cfs bandwidth

[zhaowenhui (A)]

> Hi,
> 
> On 2024/7/9 2:08, Tejun Heo wrote:
>> Hello,
>>
>> On Mon, Jul 08, 2024 at 08:00:53PM +0800, Cheng Yu wrote:
>>> From: Zhao Wenhui <zhaowenhui8@huawei.com>
>>>
>>> In the cpu subsystem of cgroup v1 and v2, we set the restriction on cfs
>>> bandwidth by setting the quota and burst value. Later, when we remove
>>> the restriction by setting the quota to the default value, the burst
>>> value should also be forced to the its default value of zero.
>>
>> Explaining a bit why burst value should become zero can be helpful - ie.
>> what happens when quota is not set but burst is limited?
> 
> When the restriction on cfs bandwidth is removed, the purpose of setting
> burst value to zero is to avoid ambiguity.
> 
>>
>> ...
>>> @@ -10840,6 +10840,12 @@ static int tg_set_cfs_bandwidth(struct task_group *tg, u64 period, u64 quota,
>>>   				     burst + quota > max_cfs_runtime))
>>>   		return -EINVAL;
>>>   
>>> +	/*
>>> +	 * Ensure burst equals to zero when quota is -1.
>>> +	 */
>>> +	if (quota == RUNTIME_INF && burst)
>>> +		return -EINVAL;
>>
>> This happening would be a bug, right? Would it make more sense to use
>> WARN_ON_ONCE()?
> 
> As Vishal Chourasia mentioned, it can prevent setting excessively large
> burst value. Using WARN_ON_ONCE() does not have this effect.
> 
>>
>> Thanks.
>>
> 
> Thanks
> --
> Cheng Yu

Friendly ping. Any more question about this patch?

Regards,
Zhao wenhui