[PATCH 0/2] cgroup cpu: set burst to zero when cfs bandwidth is

[PATCH 0/2] cgroup cpu: set burst to zero when cfs bandwidth is

[Cheng Yu]

In the cgroup cpu subsystem, when we remove the restriction on cfs
bandwidth, the burst feature is also turned off. At that time, we expect
that the value of burst is zero.

Patch 1 fixes it in cgroup v1 by Zhao Wenhui and patch 2 fixes it in
cgroup v2.

Cheng Yu (1):
  sched/fair: set burst to zero when set max to cpu.max

Zhao Wenhui (1):
  sched/fair: limit burst to zero when cfs bandwidth is turned off

 kernel/sched/core.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH 1/2] sched/fair: limit burst to zero when cfs bandwidth is turned off

[Cheng Yu]

From: Zhao Wenhui <zhaowenhui8@huawei.com>

When the quota value in CFS bandwidth is set to -1, that imples the
cfs bandwidth is turned off. So the burst feature is supposed to
be disable as well.

Currently, when quota is -1, burst will not be checked, so that it can be
set to any value. Examples:
        mkdir /sys/fs/cgroup/cpu/test
        echo -1 > /sys/fs/cgroup/cpu/test/cpu.cfs_quota_us
        echo 10000000000000000 > /sys/fs/cgroup/cpu/test/cpu.cfs_burst_us

Moreover, after the burst modified by this way, quota can't be set
to any value:
        echo 100000 > cpu.cfs_quota_us
        -bash: echo: write error: Invalid argument

This patch can ensure the burst value being zero and unchangeable,
when quota is set to -1.

Fixes: f4183717b370 ("sched/fair: Introduce the burstable CFS controller")
Reported-by: Zhao Gongyi <zhaogongyi@huawei.com>
Signed-off-by: Zhao Wenhui <zhaowenhui8@huawei.com>
Reviewed-by: Tianchen Ding <dtcccc@linux.alibaba.com>
Reviewed-by: Ben Segall <bsegall@google.com>
---
v1: https://lore.kernel.org/all/20220809120320.19496-1-zhaowenhui8@huawei.com/
---
 kernel/sched/core.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index bcf2c4cc0522..e9198e30bb74 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
-- 
2.25.1

[PATCH 2/2] sched/fair: set burst to zero when set max to cpu.max

[Cheng Yu]

In the cgroup v2 cpu subsystem, assuming we have a cgroup named 'test',
and we set cpu.max and cpu.max.burst:
    # echo 1000000 > /sys/fs/cgroup/test/cpu.max
    # echo 1000000 > /sys/fs/cgroup/test/cpu.max.burst

Next we remove the restriction on cfs bandwidth:
    # echo max > /sys/fs/cgroup/test/cpu.max
    # cat /sys/fs/cgroup/test/cpu.max
    max 100000
    # cat /sys/fs/cgroup/test/cpu.max.burst
    1000000

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

Fixes: f4183717b370 ("sched/fair: Introduce the burstable CFS controller")
Reported-by: Qixin Liao <liaoqixin@huawei.com>
Signed-off-by: Cheng Yu <serein.chengyu@huawei.com>
---
 kernel/sched/core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index e9198e30bb74..982d357b3983 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -11414,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 0/2] cgroup cpu: set burst to zero when cfs bandwidth is

[Vishal Chourasia]

On Wed, May 22, 2024 at 11:10:05AM +0800, Cheng Yu wrote:
> In the cgroup cpu subsystem, when we remove the restriction on cfs
> bandwidth, the burst feature is also turned off. At that time, we expect
> that the value of burst is zero.
> 
> Patch 1 fixes it in cgroup v1 by Zhao Wenhui and patch 2 fixes it in
> cgroup v2.
> 
> Cheng Yu (1):
>   sched/fair: set burst to zero when set max to cpu.max
> 
> Zhao Wenhui (1):
>   sched/fair: limit burst to zero when cfs bandwidth is turned off
> 

## Before patch
# uname -r
6.9.0-12124-g6d69b6c12fce-dirty
# mkdir test
# cd test/
# cat cpu.max cpu.max.burst
max 100000
0
# echo 10000000 > cpu.max.burst
# echo 1000000000000 > cpu.max.burst
# cat cpu.max cpu.max.burst
max 100000
1000000000000
# echo "1000 100000" > cpu.max
-bash: echo: write error: Invalid argument
# echo 1000 > cpu.max.burst
# echo "1000 100000" > cpu.max
# cat cpu.max cpu.max.burst
1000 100000
1000

## After patch

# uname -r
6.9.0-12126-g7eb1a247b675-dirty
# mkdir test
# cd test/
# cat cpu.max cpu.max.burst
max 100000
0
# echo 1134535435 > cpu.max.burst
-bash: echo: write error: Invalid argument
# echo 1 > cpu.max.burst
-bash: echo: write error: Invalid argument
# echo -1 > cpu.max.burst
-bash: echo: write error: Invalid argument
# echo "10000 100000" > cpu.max
# echo 1000 > cpu.max.burst
# cat cpu.max cpu.max.burst
10000 100000
1000
# echo "max 100000" > cpu.max
# cat cpu.max cpu.max.burst
max 100000
0

# git log --oneline
7eb1a247b6753 (HEAD) sched/fair: set burst to zero when set max to cpu.max
421647086da9e sched/fair: limit burst to zero when cfs bandwidth is turned off
6d69b6c12fce4 (origin/master, origin/HEAD, master) Merge tag 'nfs-for-6.10-1' o

Now, the burst value can only be set after setting 
the quota. This change also prevents setting excessively
large burst values.

Thank you the fix.

Tested-by: Vishal Chourasia <vishalc@linux.ibm.com>

>  kernel/sched/core.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> -- 
> 2.25.1
> 

Re: [PATCH 2/2] sched/fair: set burst to zero when set max to cpu.max

[Benjamin Segall]

Cheng Yu <serein.chengyu@huawei.com> writes:

> In the cgroup v2 cpu subsystem, assuming we have a cgroup named 'test',
> and we set cpu.max and cpu.max.burst:
>     # echo 1000000 > /sys/fs/cgroup/test/cpu.max
>     # echo 1000000 > /sys/fs/cgroup/test/cpu.max.burst
>
> Next we remove the restriction on cfs bandwidth:
>     # echo max > /sys/fs/cgroup/test/cpu.max
>     # cat /sys/fs/cgroup/test/cpu.max
>     max 100000
>     # cat /sys/fs/cgroup/test/cpu.max.burst
>     1000000
>
> Now we expect that the value of burst should be 0. When the burst is 0,
> it means that the restriction on burst is cancelled.
>
> Fixes: f4183717b370 ("sched/fair: Introduce the burstable CFS controller")
> Reported-by: Qixin Liao <liaoqixin@huawei.com>
> Signed-off-by: Cheng Yu <serein.chengyu@huawei.com>

Yeah, makes sense. My general assumption would be to put these in one
patch, but if there's a convention to separate v1 and v2 that I've
missed, I have no opinion.

Reviewed-by: Ben Segall <bsegall@google.com>

> ---
>  kernel/sched/core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index e9198e30bb74..982d357b3983 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -11414,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

Re: [PATCH 2/2] sched/fair: set burst to zero when set max to cpu.max

[Cheng Yu]

On 2024/5/29 7:10, Benjamin Segall wrote:
> Cheng Yu <serein.chengyu@huawei.com> writes:
> 
>> In the cgroup v2 cpu subsystem, assuming we have a cgroup named 'test',
>> and we set cpu.max and cpu.max.burst:
>>     # echo 1000000 > /sys/fs/cgroup/test/cpu.max
>>     # echo 1000000 > /sys/fs/cgroup/test/cpu.max.burst
>>
>> Next we remove the restriction on cfs bandwidth:
>>     # echo max > /sys/fs/cgroup/test/cpu.max
>>     # cat /sys/fs/cgroup/test/cpu.max
>>     max 100000
>>     # cat /sys/fs/cgroup/test/cpu.max.burst
>>     1000000
>>
>> Now we expect that the value of burst should be 0. When the burst is 0,
>> it means that the restriction on burst is cancelled.
>>
>> Fixes: f4183717b370 ("sched/fair: Introduce the burstable CFS controller")
>> Reported-by: Qixin Liao <liaoqixin@huawei.com>
>> Signed-off-by: Cheng Yu <serein.chengyu@huawei.com>
> 
> Yeah, makes sense. My general assumption would be to put these in one
> patch, but if there's a convention to separate v1 and v2 that I've
> missed, I have no opinion.
> 
> Reviewed-by: Ben Segall <bsegall@google.com>
> 
Thanks for the advice. I will submit a v2 patch that includes both cgroup v1 and v2 modification.

>> ---
>>  kernel/sched/core.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index e9198e30bb74..982d357b3983 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -11414,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