Re: [LTP] tst_cgroup: add cpu.stat and cpu.weight to cgroup file table

[linuxtestproject.agent@gmail.com]

Hi Shaojie,

On Thu, Jul 30, 2026 at 11:28:48 AM +0800, Shaojie Sun wrote:
> tst_cgroup: add cpu.stat and cpu.weight to cgroup file table

--- [PATCH 1/3] ---

>  	{ "cpu.max", "cpu.cfs_quota_us", CTRL_CPU },
>  	{ "cpu.cfs_period_us", "cpu.cfs_period_us", CTRL_CPU },
> +	{ "cpu.stat", NULL, CTRL_CPU },
> +	{ "cpu.weight", NULL, CTRL_CPU },

A NULL file_name_v1 means "item is V2 only", and cgroup_file_alias()
returns that NULL for TST_CG_V1 roots, so safe_cg_has()/safe_cg_read()
will report the file as absent on a v1 hierarchy.

cpu.stat does exist on v1 though: kernel/sched/core.c has
{ .name = "stat", .seq_show = cpu_cfs_stat_show } in cpu_legacy_files[]
under CONFIG_CFS_BANDWIDTH.

Should this be { "cpu.stat", "cpu.stat", CTRL_CPU }, following the
memory.stat entry?

cpu.weight -> NULL looks right, since the v1 counterpart cpu.shares uses
a different scale and cannot be aliased.

--- [PATCH 2/3] ---

> +	if (!SAFE_FORK()) {
> +		struct cpu_hog_func_param param = {
> +			.nprocs = 1,
> +			.ts = {
> +				.tv_sec = usage_seconds,
> +				.tv_nsec = 0,
> +			},
> +			.clock_type = CPU_HOG_CLOCK_PROCESS,
> +		};
> +
> +		SAFE_CG_PRINTF(cg_test, "cgroup.procs", "%d", getpid());
> +		exit(hog_cpus_timed(&param));
> +	}
> +
> +	SAFE_WAITPID(-1, &status, 0);

The child encodes its outcome in the exit code, but "status" is never
examined here (same pattern in cgroup_cpu03.c, cgroup_cpu04.c
wait_children() and cgroup_cpu05.c).

hog_cpus_timed() returns -1 when clock_gettime() or pthread_create()
fails. With the status ignored, a child that never started a single hog
thread is indistinguishable from a successful run, and the parent then
reads cpu.stat and reports a misleading result. For cgroup_cpu05 this
can even produce a TPASS, since a dead hog and a correctly throttled hog
both yield a near-zero usage_usec.

The LTP way is for the child to report with tst_res()/tst_brk() directly
and exit(0) - the library propagates results from children to the
parent. Could hog_cpus_timed() call tst_brk(TBROK | TERRNO, ...) on
failure instead of returning -1? That also removes the unused "status"
variables and lets the framework reap the children.

> +	SAFE_CG_PRINTF(cg_parent, "cpu.max", "%ld %ld", quota_usec, period_usec);

cpu.max and cpu.max.burst are compiled into cpu_files[] only under
CONFIG_GROUP_SCHED_BANDWIDTH, and CONFIG_CFS_BANDWIDTH which selects it
is "default n". Similarly cpu.weight in cgroup_cpu04.c depends on
CONFIG_GROUP_SCHED_WEIGHT.

On a kernel with the cpu controller but without bandwidth support,
safe_cg_printf() cannot find the file and the test aborts with TBROK,
while a missing feature has to be reported as TCONF.

Would .needs_kconfigs work here, e.g.

	.needs_kconfigs = (const char *[]) {
		"CONFIG_CFS_BANDWIDTH=y",
		NULL
	},

or alternatively a SAFE_CG_HAS() check in setup() followed by
tst_brk(TCONF, ...)?

> +		if (nice(1) == -1 && errno != 0)
> +			exit(1);

nice() may legitimately return -1 as the new nice value, so errno has to
be set to 0 immediately before the call for this check to be meaningful
(see nice(2)). At this point errno can still carry a stale value from
the SAFE_CG_PRINTF() path just above, which would turn a successful
nice(1) returning -1 into a spurious failure.

Adding "errno = 0;" before the call would fix that. The silent exit(1)
is covered by the comment on child result reporting above.

> +/*
> + * Copyright (c) 2025 Richard Palethorpe <rpalethorpe@suse.com>
> + */

All six new files carry only this line while the patch author is someone
else. cgroup_cpu01.c is clearly derived from memcontrol01.c so keeping
the original attribution makes sense there, but the remaining files are
new work - should they carry the author's own copyright line as well,
with the year matching the commit date?

> + * Includes:
> + * - test_cpucg_weight_overprovisioned
> + * - test_cpucg_weight_underprovisioned
> + * - test_cpucg_nested_weight_overprovisioned
> + * - test_cpucg_nested_weight_underprovisioned

The doc block is rendered as-is in the test catalog, and reST requires a
blank line before a bullet list. A " *" line after "Includes:" is needed
here and in cgroup_cpu05.c.

> + * Conversion of kselftest test_cpucg_subtree_control from
> + * tools/testing/selftests/cgroup/test_cpu.c.

The doc has a role for this, which turns the reference into a link in
the catalog. memcontrol01.c uses it as

	:kselftest:`cgroup/test_memcontrol.c`

Could the five doc blocks use :kselftest:`cgroup/test_cpu.c` instead of
the plain path?

> +	.needs_cgroup_ver = TST_CG_V2,
> +	.needs_cgroup_ctrls = (const char *const []){ "cpu", NULL },
> +	.needs_cgroup_nsdelegate = 0,

needs_cgroup_nsdelegate is a bitfield that already defaults to 0, so the
explicit initialiser can be dropped from all five tests. The same
applies to .timeout = 30 in cgroup_cpu05.c, which is the framework
default.

> +#define USEC_PER_SEC 1000000L
> +#define HOG_SECONDS 10

USEC_PER_SEC is not used anywhere in cgroup_cpu04.c.

> +cleanup:
> +	for (i = 2; i >= 0; i--) {
> +		if (cg_leaf[i]) {
> +			SAFE_CG_PRINTF(tst_cg_drain, "cgroup.procs",
> +				       "%d", getpid());
> +			cg_leaf[i] = tst_cg_group_rm(cg_leaf[i]);
> +		}
> +	}

The drain write is inside the loop here, so it runs three times, while
the two nested variants in the same file do it once before the loop.

Beyond the inconsistency, the test process itself is never added to any
cg_leaf group in this file, so is the drain write needed at all here?

> +static void run(unsigned int n)
> +{
> +	switch (n) {
> +	case 0:
> +		test_cpucg_weight_overprovisioned();
> +		break;
...
> +	.test = run,
> +	.tcnt = 4,

LTP dispatches multiple test cases through a table rather than a switch,
either a struct tcase[] or a static array of function pointers, with
.tcnt = ARRAY_SIZE(...). As written, .tcnt = 4 (and .tcnt = 2 in
cgroup_cpu05.c) is a literal that can silently drift out of sync with
the cases.

> +	.forks_child = 1,
> +	.needs_cgroup_ver = TST_CG_V2,

cgroup_cpu02.c does not set .needs_root = 1 while cgroup_cpu03.c,
cgroup_cpu04.c and cgroup_cpu05.c do, even though all four write to
cgroup.procs and cpu.* from the test process. Could this be made
consistent across the series?

> +/*\
> + * Common helpers for cgroup cpu controller tests.
> + *
> + * Converted from kselftest
> + * tools/testing/selftests/cgroup/test_cpu.c
> + */

The "/*\" marker opens a test high-level description that docparse
extracts for the catalog. cgroup_cpu_common.h is a helper header rather
than a test, so a plain "/* */" comment would be more appropriate.

Verdict - Needs revision

---
Note:

The agent can sometimes produce false positives although often its
findings are genuine. If you find issues with the review, please
comment this email or ignore the suggestions.

Regards,
LTP AI Reviewer

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp