linux-kernel - Re: [PATCH v2] selftests/cgroup: improve the accuracy of cpu.max tests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aGd5lrUvm9Bhh-b8@JPC00244420>
Date: Fri, 4 Jul 2025 15:49:58 +0900
From: Shashank Balaji <shashank.mahadasyam@...y.com>
To: Michal Koutný <mkoutny@...e.com>
Cc: Tejun Heo <tj@...nel.org>, Johannes Weiner <hannes@...xchg.org>,
	Shuah Khan <shuah@...nel.org>, cgroups@...r.kernel.org,
	linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
	Shinya Takumi <shinya.takumi@...y.com>
Subject: Re: [PATCH v2] selftests/cgroup: improve the accuracy of cpu.max
 tests

On Fri, Jul 04, 2025 at 09:26:56AM +0900, Shashank Balaji wrote:
> > >  tools/testing/selftests/cgroup/test_cpu.c | 63 ++++++++++++++++-------
> > >  1 file changed, 43 insertions(+), 20 deletions(-)
> > 
> > 
> > > -	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;
> > 
> > I think this was a meaningful check. Not sure if dropped accidentally or
> > on purpose w/out explanation.
> > 
> > After that's addressed, feel free to add
> > Acked-by: Michal Koutný <mkoutny@...e.com>
> 
> Sorry about that. I dropped it accidentally. This check should be okay,
> right?
> 
> 	if (usage_usec > expected_usage_usec)
> 		goto cleanup;
> 
> 1. We don't need to separately check user_usec because it'll always be
> less than user_usec, and usage_usec is what's directly affected by
> throttling.
> 2. I changed the >= to > because, not that it'll ever happen, but we can
> let usage_usec = expected_usage_usec pass. Afterall, it's called
> "expected" for a reason.

Hmm, here is something interesting. The following patch adds printfs to the
existing code to see what user_usec, usage_usec, the expected_usage_usec used in
the code, and the theoretical expected_usage_usec are. On running the modified test
a couple of times, here is the output:

	$ sudo ./test_cpu
	user: 10485, usage: 10485, used expected: 1000000, theoretical expected: 10000
	ok 1 test_cpucg_max
	user: 11127, usage: 11127, used expected: 1000000, theoretical expected: 10000
	ok 2 test_cpucg_max_nested
	$ sudo ./test_cpu
	user: 10286, usage: 10286, used expected: 1000000, theoretical expected: 10000
	ok 1 test_cpucg_max
	user: 10404, usage: 11271, used expected: 1000000, theoretical expected: 10000
	ok 2 test_cpucg_max_nested
	$ sudo ./test_cpu
	user: 10490, usage: 10490, used expected: 1000000, theoretical expected: 10000
	ok 1 test_cpucg_max
	user: 9326, usage: 9326, used expected: 1000000, theoretical expected: 10000
	ok 2 test_cpucg_max_nested
	$ sudo ./test_cpu
	user: 10368, usage: 10368, used expected: 1000000, theoretical expected: 10000
	ok 1 test_cpucg_max
	user: 10026, usage: 10026, used expected: 1000000, theoretical expected: 10000
	ok 2 test_cpucg_max_nested
	$ sudo ./test_cpu
	user: 10541, usage: 10541, used expected: 1000000, theoretical expected: 10000
	ok 1 test_cpucg_max
	user: 11040, usage: 11040, used expected: 1000000, theoretical expected: 10000
	ok 2 test_cpucg_max_nested

So, both user_usec and usage_usec exceeding the theoretical expected_usage_usec
is not uncommon. The "fail if usage_usec >= expected_usage_usec" check in the
existing code only really works because of the (wrong) large expected_usage_usec
used.

So I think the best we can do is check if usage_usec is close to expected_usage_usec
or not, and not require usage_usec to be less than expected_usage_usec.

diff --git i/tools/testing/selftests/cgroup/test_cpu.c w/tools/testing/selftests/cgroup/test_cpu.c
index a2b50af8e9ee..14c8c7b49214 100644
--- i/tools/testing/selftests/cgroup/test_cpu.c
+++ w/tools/testing/selftests/cgroup/test_cpu.c
@@ -679,6 +679,9 @@ static int test_cpucg_max(const char *root)
        if (user_usec >= expected_usage_usec)
                goto cleanup;
 
+       printf("user: %ld, usage: %ld, used expected: %ld, theoretical expected: 10000\n",
+               user_usec, usage_usec, expected_usage_usec);
+
        if (values_close(usage_usec, expected_usage_usec, 95))
                goto cleanup;
 
@@ -739,6 +742,9 @@ static int test_cpucg_max_nested(const char *root)
        if (user_usec >= expected_usage_usec)
                goto cleanup;
 
+       printf("user: %ld, usage: %ld, used expected: %ld, theoretical expected: 10000\n",
+               user_usec, usage_usec, expected_usage_usec);
+
        if (values_close(usage_usec, expected_usage_usec, 95))
                goto cleanup;
 
@@ -758,13 +764,13 @@ struct cpucg_test {
        int (*fn)(const char *root);
        const char *name;
 } tests[] = {
-       T(test_cpucg_subtree_control),
-       T(test_cpucg_stats),
-       T(test_cpucg_nice),
-       T(test_cpucg_weight_overprovisioned),
-       T(test_cpucg_weight_underprovisioned),
-       T(test_cpucg_nested_weight_overprovisioned),
-       T(test_cpucg_nested_weight_underprovisioned),
+       // T(test_cpucg_subtree_control),
+       // T(test_cpucg_stats),
+       // T(test_cpucg_nice),
+       // T(test_cpucg_weight_overprovisioned),
+       // T(test_cpucg_weight_underprovisioned),
+       // T(test_cpucg_nested_weight_overprovisioned),
+       // T(test_cpucg_nested_weight_underprovisioned),
        T(test_cpucg_max),
        T(test_cpucg_max_nested),
 };