Re: [PATCH v5] selftests/cgroup: add PSI pressure trigger and validation tests

["Michal Koutný" <mkoutny@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 3525 bytes --]

Hello Tao.

On Wed, Sep 02, 2026 at 12:07:25PM +0800, Tao Cui <cui.tao@linux.dev> wrote:
> +/* PSI triggers are written with a trailing NUL the kernel parser expects. */
> +static ssize_t write_trigger(int fd, const char *trigger)
> +{
> +	return write(fd, trigger, strlen(trigger) + 1);
> +}

Hyrum's law. It all works for me: NUL, \n or just write(2) the exact
length of the string.
For conventionality, I'd prefer the simple literals and plain strlen() +
0. (I reckon cg_write() cannot be used because of FD access.)

> +
> +static int pressure_open(const char *resource)
> +{
> +	char path[PATH_MAX];
> +	int fd;
> +
> +	snprintf(path, sizeof(path), "/proc/pressure/%s", resource);
> +	fd = open(path, O_RDWR);
> +	if (fd < 0)
> +		ksft_perror(path);

This outputs:
| # /proc/pressure/irq: No such file or directory (2)
| #      SKIP      /proc/pressure/irq unavailable

I.e. similar message is printed twice.
Since strace is a companion of cgroup selftests, I'd keep this helper
silent.

> +	return fd;
> +}
> +
> +FIXTURE(psi)
> +{
> +	char root[PATH_MAX];
> +	char *cg;
> +};
> +
> +FIXTURE_SETUP(psi)
> +{
> +	int psi_fd;
> +
> +	if (cg_find_unified_root(self->root, sizeof(self->root), NULL))
> +		SKIP(return, "cgroup v2 isn't mounted");
> +
> +	/* PSI must be enabled (CONFIG_PSI=y, not disabled on the cmdline). */
> +	psi_fd = open("/proc/pressure/memory", O_RDONLY);
> +	if (psi_fd < 0)
> +		SKIP(return, "PSI unavailable (CONFIG_PSI=n or psi=0)");
> +	close(psi_fd);
> +
> +	self->cg = cg_name(self->root, "psi_trigger_test");
> +	if (!self->cg)
> +		SKIP(return, "failed to allocate cgroup name");
> +	if (cg_create(self->cg))
> +		SKIP(return, "failed to create cgroup: %s", strerror(errno));

Why are these two SKIPs (not failures)?

> +TEST_F(psi, cgroup_trigger_fire)
> +{
> +	char *cpupress;
> +	struct pollfd pfd = { .events = POLLPRI };
> +	long ncpus;
> +	int fd;
> +	int i;
> +
> +	cpupress = cg_control(self->cg, "cpu.pressure");
> +	ASSERT_NE(NULL, cpupress);
> +	fd = open(cpupress, O_RDWR);
> +	free(cpupress);
> +	ASSERT_GE(fd, 0);
> +	pfd.fd = fd;
> +
> +	/*
> +	 * 1usec threshold over a 2s window: any CPU stall fires it. The 2s
> +	 * window is the smallest unprivileged users are allowed to arm.
> +	 */
> +	ASSERT_GT(write_trigger(fd, "some 1 2000000"), 0);

The selftest rarely can be run as unprivileged user (even test cgroup
creation needs privileges), so this comment is irrelevant. (But it's
fine to test with that value.)

On the more abstract level -- I was playing with this and thinking about
a value that'd test both sides, i.e. false triggers as well as false
non-triggers. I'd find that to be the half of the window and the number
of tasks should be then (3*ncpus + 1) / 2.
Or perhaps test two thresholds, one tiny like you did and one maximum
(whole window) with same amount tasks but expect trigger, no trigger
respectively.


> +
> +	ncpus = sysconf(_SC_NPROCESSORS_ONLN);
> +	if (ncpus == -1)
> +		TH_LOG("sysconf(_SC_NPROCESSORS_ONLN): %s", strerror(errno));
> +	ASSERT_NE(-1, ncpus);

Same as messages from pressure_open() above. Simply assert.

> +
> +	/* ncpus+1 hogs guarantee CPU contention inside the cgroup. */
> +	for (i = 0; i < ncpus + 1; i++)
> +		ASSERT_GE(cg_run_nowait(self->cg, hog_cpu, NULL), 0);
> +
> +	ASSERT_EQ(1, poll(&pfd, 1, PSI_POLL_TIMEOUT_MS));
> +	ASSERT_NE(0, pfd.revents & POLLPRI);
> +	close(fd);
> +}
> +
> +TEST_HARNESS_MAIN

All in all, this looks so much better than the initial version, well
done. Just a few polishing touches.

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]