[PATCH 1/2] sched/fair: Reset incompatible burst on quota change

[Zhe Liu]

A burst configured while a cgroup has unlimited CPU bandwidth can prevent
a later finite quota from being installed.  For example, on cgroup v2:

  # echo 100000000 > cpu.max.burst
  # echo "50000 100000" > cpu.max
  sh: write error: Invalid argument

The quota remains unlimited because tg_set_bandwidth() validates the
existing burst against the new quota.  Recovering requires userspace to
know that it must clear the burst before retrying the quota update.  The
same problem affects cpu.cfs_quota_us on cgroup v1.

When changing the quota, reset the existing burst to zero if it is
incompatible with a valid finite quota.  Preserve it when it remains
compatible or when the new quota is unlimited.  This lets a quota update
take effect without depending on the order in which userspace writes the
two files.

Rejecting the quota would retain this ordering dependency.  Clamping the
burst would instead silently choose a different nonzero policy on behalf
of userspace.  Resetting it to zero provides the existing no-burst default
while leaving compatible bursts untouched.  Keeping the burst while the
quota is unlimited also allows userspace to stage a burst before enabling
bandwidth control.

Add a cgroup v2 regression test for the quota update behavior.

Fixes: f4183717b370 ("sched/fair: Introduce the burstable CFS controller")
Signed-off-by: Zhe Liu <liuzhe1@xxxxxxxxxx>
---
 kernel/sched/core.c                       | 17 ++++++-
 tools/testing/selftests/cgroup/test_cpu.c | 62 +++++++++++++++++++++++
 2 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 2e7cde033a31..324a4d22f8d1 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -10081,6 +10081,18 @@ static u64 cpu_period_read_u64(struct cgroup_subsys_state *css,
 	return period_us;
 }
 
+static u64 tg_burst_on_quota_change(u64 quota_us, u64 burst_us)
+{
+	if (quota_us == RUNTIME_INF || quota_us > max_bw_runtime_us)
+		return burst_us;
+
+	if (burst_us > quota_us ||
+	    burst_us > max_bw_runtime_us - quota_us)
+		return 0;
+
+	return burst_us;
+}
+
 static int tg_set_bandwidth(struct task_group *tg,
 			    u64 period_us, u64 quota_us, u64 burst_us)
 {
@@ -10169,6 +10181,7 @@ static int cpu_quota_write_s64(struct cgroup_subsys_state *css,
 		quota_us = RUNTIME_INF;
 
 	tg_bandwidth(tg, &period_us, NULL, &burst_us);
+	burst_us = tg_burst_on_quota_change(quota_us, burst_us);
 	return tg_set_bandwidth(tg, period_us, quota_us, burst_us);
 }
 
@@ -10492,8 +10505,10 @@ static ssize_t cpu_max_write(struct kernfs_open_file *of,
 
 	tg_bandwidth(tg, &period_us, NULL, &burst_us);
 	ret = cpu_period_quota_parse(buf, &period_us, &quota_us);
-	if (!ret)
+	if (!ret) {
+		burst_us = tg_burst_on_quota_change(quota_us, burst_us);
 		ret = tg_set_bandwidth(tg, period_us, quota_us, burst_us);
+	}
 	return ret ?: nbytes;
 }
 #endif /* CONFIG_CFS_BANDWIDTH */
diff --git a/tools/testing/selftests/cgroup/test_cpu.c b/tools/testing/selftests/cgroup/test_cpu.c
index 7a40d76b9548..2686dd79941b 100644
--- a/tools/testing/selftests/cgroup/test_cpu.c
+++ b/tools/testing/selftests/cgroup/test_cpu.c
@@ -703,6 +703,67 @@ static int test_cpucg_max(const char *root)
 	return ret;
 }
 
+/*
+ * This test verifies that writing a finite cpu.max resets an incompatible
+ * cpu.max.burst, while preserving a compatible burst.
+ */
+static int test_cpucg_max_burst_reset(const char *root)
+{
+	char *cpucg = NULL;
+	int ret = KSFT_FAIL;
+
+	cpucg = cg_name(root, "cpucg_max_burst_reset_test");
+	if (!cpucg)
+		goto cleanup;
+
+	if (cg_create(cpucg))
+		goto cleanup;
+
+	/* An unconstrained group may retain a burst for later use. */
+	if (cg_write(cpucg, "cpu.max.burst", "100000000"))
+		goto cleanup;
+	if (cg_read_long(cpucg, "cpu.max.burst") != 100000000)
+		goto cleanup;
+
+	/* A finite quota must not be blocked by the incompatible burst. */
+	if (cg_write(cpucg, "cpu.max", "50000 100000"))
+		goto cleanup;
+	if (cg_read_long(cpucg, "cpu.max.burst") != 0)
+		goto cleanup;
+	if (cg_read_strcmp(cpucg, "cpu.max", "50000 100000\n"))
+		goto cleanup;
+
+	/* Keep a burst which remains valid across a quota update. */
+	if (cg_write(cpucg, "cpu.max", "100000 100000"))
+		goto cleanup;
+	if (cg_write(cpucg, "cpu.max.burst", "50000"))
+		goto cleanup;
+	if (cg_write(cpucg, "cpu.max", "75000 100000"))
+		goto cleanup;
+	if (cg_read_long(cpucg, "cpu.max.burst") != 50000)
+		goto cleanup;
+
+	/* An unlimited quota preserves burst until it becomes incompatible. */
+	if (cg_write(cpucg, "cpu.max", "max 100000"))
+		goto cleanup;
+	if (cg_read_long(cpucg, "cpu.max.burst") != 50000)
+		goto cleanup;
+
+	/* Reset the same burst when a later finite quota conflicts. */
+	if (cg_write(cpucg, "cpu.max", "25000 100000"))
+		goto cleanup;
+	if (cg_read_long(cpucg, "cpu.max.burst") != 0)
+		goto cleanup;
+
+	ret = KSFT_PASS;
+
+cleanup:
+	cg_destroy(cpucg);
+	free(cpucg);
+
+	return ret;
+}
+
 /*
  * This test verifies that a process inside of a nested cgroup whose parent
  * group has a cpu.max value set, is properly throttled.
@@ -789,6 +850,7 @@ struct cpucg_test {
 	T(test_cpucg_nested_weight_overprovisioned),
 	T(test_cpucg_nested_weight_underprovisioned),
 	T(test_cpucg_max),
+	T(test_cpucg_max_burst_reset),
 	T(test_cpucg_max_nested),
 };
 #undef T
-- 
2.25.1