Hi Tejun,
Could you please take a look at this patch? After some back-and-forth
with Michal, this is the v3 with his Acked-by.
Thanks,
Shashank
On Fri, Jul 04, 2025 at 08:08:41PM +0900, Shashank Balaji wrote:
> Current cpu.max tests (both the normal one and the nested one) are broken.
>
> They setup cpu.max with 1000 us quota and the default period (100,000 us).
> A cpu hog is run for a duration of 1s as per wall clock time. This corresponds
> to 10 periods, hence an expected usage of 10,000 us. We want the measured
> usage (as per cpu.stat) to be close to 10,000 us.
>
> Previously, this approximate equality test was done by
> `!values_close(usage_usec, expected_usage_usec, 95)`: if the absolute
> difference between usage_usec and expected_usage_usec is greater than 95% of
> their sum, then we pass. And expected_usage_usec was set to 1,000,000 us.
> Mathematically, this translates to the following being true for pass:
>
> |usage - expected_usage| > (usage + expected_usage)*0.95
>
> If usage > expected_usage:
> usage - expected_usage > (usage + expected_usage)*0.95
> 0.05*usage > 1.95*expected_usage
> usage > 39*expected_usage = 39s
>
> If usage < expected_usage:
> expected_usage - usage > (usage + expected_usage)*0.95
> 0.05*expected_usage > 1.95*usage
> usage < 0.0256*expected_usage = 25,600 us
>
> Combined,
>
> Pass if usage < 25,600 us or > 39 s,
>
> which makes no sense given that all we need is for usage_usec to be close to
> 10,000 us.
>
> Fix this by explicitly calcuating the expected usage duration based on the
> configured quota, default period, and the duration, and compare usage_usec
> and expected_usage_usec using values_close() with a 10% error margin.
>
> Also, use snprintf to get the quota string to write to cpu.max instead of
> hardcoding the quota, ensuring a single source of truth.
>
> Remove the check comparing user_usec and expected_usage_usec, since on running
> this test modified with printfs, it's seen that user_usec and usage_usec can
> regularly exceed the theoretical expected_usage_usec:
>
> $ sudo ./test_cpu
> user: 10485, usage: 10485, expected: 10000
> ok 1 test_cpucg_max
> user: 11127, usage: 11127, expected: 10000
> ok 2 test_cpucg_max_nested
> $ sudo ./test_cpu
> user: 10286, usage: 10286, expected: 10000
> ok 1 test_cpucg_max
> user: 10404, usage: 11271, expected: 10000
> ok 2 test_cpucg_max_nested
>
> Hence, a values_close() check of usage_usec and expected_usage_usec is
> sufficient.
>
> Fixes: a79906570f9646ae17 ("cgroup: Add test_cpucg_max_nested() testcase")
> Fixes: 889ab8113ef1386c57 ("cgroup: Add test_cpucg_max() testcase")
> Acked-by: Michal Koutný <[email protected]>
> Signed-off-by: Shashank Balaji <[email protected]>
>
> ---
>
> Changes in v3:
> - Simplified commit message
> - Explained why the "user_usec >= expected_usage_usec" check is removed
> - Added fixes tags and Michal's Acked-by
> - No code changes
> - v2:
> https://lore.kernel.org/all/[email protected]/
>
> Changes in v2:
> - Incorporate Michal's suggestions:
> - Merge two patches into one
> - Generate the quota string from the variable instead of hardcoding it
> - Use values_close() instead of labs()
> - Explicitly calculate expected_usage_usec
> - v1:
> https://lore.kernel.org/all/[email protected]/
> ---
> tools/testing/selftests/cgroup/test_cpu.c | 63 ++++++++++++++++-------
> 1 file changed, 43 insertions(+), 20 deletions(-)
>
> diff --git a/tools/testing/selftests/cgroup/test_cpu.c
> b/tools/testing/selftests/cgroup/test_cpu.c
> index a2b50af8e9ee..2a60e6c41940 100644
> --- a/tools/testing/selftests/cgroup/test_cpu.c
> +++ b/tools/testing/selftests/cgroup/test_cpu.c
> @@ -2,6 +2,7 @@
>
> #define _GNU_SOURCE
> #include <linux/limits.h>
> +#include <sys/param.h>
> #include <sys/sysinfo.h>
> #include <sys/wait.h>
> #include <errno.h>
> @@ -645,10 +646,16 @@ test_cpucg_nested_weight_underprovisioned(const char
> *root)
> static int test_cpucg_max(const char *root)
> {
> int ret = KSFT_FAIL;
> - long usage_usec, user_usec;
> - long usage_seconds = 1;
> - long expected_usage_usec = usage_seconds * USEC_PER_SEC;
> + long quota_usec = 1000;
> + long default_period_usec = 100000; /* cpu.max's default period */
> + long duration_seconds = 1;
> +
> + long duration_usec = duration_seconds * USEC_PER_SEC;
> + long usage_usec, n_periods, remainder_usec, expected_usage_usec;
> char *cpucg;
> + char quota_buf[32];
> +
> + snprintf(quota_buf, sizeof(quota_buf), "%ld", quota_usec);
>
> cpucg = cg_name(root, "cpucg_test");
> if (!cpucg)
> @@ -657,13 +664,13 @@ static int test_cpucg_max(const char *root)
> if (cg_create(cpucg))
> goto cleanup;
>
> - if (cg_write(cpucg, "cpu.max", "1000"))
> + if (cg_write(cpucg, "cpu.max", quota_buf))
> goto cleanup;
>
> struct cpu_hog_func_param param = {
> .nprocs = 1,
> .ts = {
> - .tv_sec = usage_seconds,
> + .tv_sec = duration_seconds,
> .tv_nsec = 0,
> },
> .clock_type = CPU_HOG_CLOCK_WALL,
> @@ -672,14 +679,19 @@ static int test_cpucg_max(const char *root)
> goto cleanup;
>
> usage_usec = cg_read_key_long(cpucg, "cpu.stat", "usage_usec");
> - user_usec = cg_read_key_long(cpucg, "cpu.stat", "user_usec");
> - if (user_usec <= 0)
> + if (usage_usec <= 0)
> goto cleanup;
>
> - if (user_usec >= expected_usage_usec)
> - goto cleanup;
> + /*
> + * The following calculation applies only since
> + * the cpu hog is set to run as per wall-clock time
> + */
> + n_periods = duration_usec / default_period_usec;
> + remainder_usec = duration_usec - n_periods * default_period_usec;
> + expected_usage_usec
> + = n_periods * quota_usec + MIN(remainder_usec, quota_usec);
>
> - if (values_close(usage_usec, expected_usage_usec, 95))
> + if (!values_close(usage_usec, expected_usage_usec, 10))
> goto cleanup;
>
> ret = KSFT_PASS;
> @@ -698,10 +710,16 @@ static int test_cpucg_max(const char *root)
> static int test_cpucg_max_nested(const char *root)
> {
> int ret = KSFT_FAIL;
> - long usage_usec, user_usec;
> - long usage_seconds = 1;
> - long expected_usage_usec = usage_seconds * USEC_PER_SEC;
> + long quota_usec = 1000;
> + long default_period_usec = 100000; /* cpu.max's default period */
> + long duration_seconds = 1;
> +
> + long duration_usec = duration_seconds * USEC_PER_SEC;
> + long usage_usec, n_periods, remainder_usec, expected_usage_usec;
> char *parent, *child;
> + char quota_buf[32];
> +
> + snprintf(quota_buf, sizeof(quota_buf), "%ld", quota_usec);
>
> parent = cg_name(root, "cpucg_parent");
> child = cg_name(parent, "cpucg_child");
> @@ -717,13 +735,13 @@ static int test_cpucg_max_nested(const char *root)
> if (cg_create(child))
> goto cleanup;
>
> - if (cg_write(parent, "cpu.max", "1000"))
> + if (cg_write(parent, "cpu.max", quota_buf))
> goto cleanup;
>
> struct cpu_hog_func_param param = {
> .nprocs = 1,
> .ts = {
> - .tv_sec = usage_seconds,
> + .tv_sec = duration_seconds,
> .tv_nsec = 0,
> },
> .clock_type = CPU_HOG_CLOCK_WALL,
> @@ -732,14 +750,19 @@ static int test_cpucg_max_nested(const char *root)
> goto cleanup;
>
> usage_usec = cg_read_key_long(child, "cpu.stat", "usage_usec");
> - user_usec = cg_read_key_long(child, "cpu.stat", "user_usec");
> - if (user_usec <= 0)
> + if (usage_usec <= 0)
> goto cleanup;
>
> - if (user_usec >= expected_usage_usec)
> - goto cleanup;
> + /*
> + * The following calculation applies only since
> + * the cpu hog is set to run as per wall-clock time
> + */
> + n_periods = duration_usec / default_period_usec;
> + remainder_usec = duration_usec - n_periods * default_period_usec;
> + expected_usage_usec
> + = n_periods * quota_usec + MIN(remainder_usec, quota_usec);
>
> - if (values_close(usage_usec, expected_usage_usec, 95))
> + if (!values_close(usage_usec, expected_usage_usec, 10))
> goto cleanup;
>
> ret = KSFT_PASS;
> --
> 2.43.0
>
