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

Hi Suren,

Thanks for the review.

Yeah, you called the "AI glitch" correctly. This is only my second
selftests patch and I ran it through an AI assistant for "polishing"
before sending -- that's where the (void)root, the errno = 0 lines, and
the (int)ARRAY_SIZE cast came from. None of it belongs here and I
should've caught it before sending.

在 2026/8/9 11:44, Suren Baghdasaryan 写道:
> On Tue, Jul 28, 2026 at 1:38 AM Tao Cui <[email protected]> wrote:
>>
>> From: Tao Cui <[email protected]>
>>
>> The cgroup selftests have no PSI coverage. Add test_psi.c: trigger
>> smoke tests (one per fd, IRQ full-only), cgroup.pressure toggle, and a
>> CPU-pressure trigger test using over-subscription. Skips when PSI is
>> disabled or a resource is absent.
>>
>> Signed-off-by: Tao Cui <[email protected]>
>> ---
>> Changes since v1 (Michal Koutny, sashiko review):
>> - Trim errno checks to smoke level; loosen toggle range checks.
>> - Switch trigger test from memory to CPU pressure (deterministic, no SKIP).
>> - churn_memory() removed -- sysconf/shared-helper point is moot.
>> - Runner stays out of the cgroup; hogs reaped via cg_killall().
>> - Add PSI/IRQ skip-guards, zero-init buffers, .gitignore entry.
>> ---
>>  tools/testing/selftests/cgroup/.gitignore |   1 +
>>  tools/testing/selftests/cgroup/Makefile   |   2 +
>>  tools/testing/selftests/cgroup/config     |   1 +
>>  tools/testing/selftests/cgroup/test_psi.c | 280 ++++++++++++++++++++++
>>  4 files changed, 284 insertions(+)
>>  create mode 100644 tools/testing/selftests/cgroup/test_psi.c
>>
>> diff --git a/tools/testing/selftests/cgroup/.gitignore b/tools/testing/selftests/cgroup/.gitignore
>> index 952e4448bf07..ce2b907c57ea 100644
>> --- a/tools/testing/selftests/cgroup/.gitignore
>> +++ b/tools/testing/selftests/cgroup/.gitignore
>> @@ -8,5 +8,6 @@ test_kill
>>  test_kmem
>>  test_memcontrol
>>  test_pids
>> +test_psi
>>  test_zswap
>>  wait_inotify
>> diff --git a/tools/testing/selftests/cgroup/Makefile b/tools/testing/selftests/cgroup/Makefile
>> index e01584c2189a..a8c69e37332a 100644
>> --- a/tools/testing/selftests/cgroup/Makefile
>> +++ b/tools/testing/selftests/cgroup/Makefile
>> @@ -16,6 +16,7 @@ TEST_GEN_PROGS += test_kill
>>  TEST_GEN_PROGS += test_kmem
>>  TEST_GEN_PROGS += test_memcontrol
>>  TEST_GEN_PROGS += test_pids
>> +TEST_GEN_PROGS += test_psi
>>  TEST_GEN_PROGS += test_zswap
>>
>>  LOCAL_HDRS += $(selfdir)/clone3/clone3_selftests.h $(selfdir)/pidfd/pidfd.h
>> @@ -32,4 +33,5 @@ $(OUTPUT)/test_kill: $(LIBCGROUP_O)
>>  $(OUTPUT)/test_kmem: $(LIBCGROUP_O)
>>  $(OUTPUT)/test_memcontrol: $(LIBCGROUP_O)
>>  $(OUTPUT)/test_pids: $(LIBCGROUP_O)
>> +$(OUTPUT)/test_psi: $(LIBCGROUP_O)
>>  $(OUTPUT)/test_zswap: $(LIBCGROUP_O)
>> diff --git a/tools/testing/selftests/cgroup/config b/tools/testing/selftests/cgroup/config
>> index 39f979690dd3..8a3ef479e83d 100644
>> --- a/tools/testing/selftests/cgroup/config
>> +++ b/tools/testing/selftests/cgroup/config
>> @@ -4,3 +4,4 @@ CONFIG_CGROUP_FREEZER=y
>>  CONFIG_CGROUP_SCHED=y
>>  CONFIG_MEMCG=y
>>  CONFIG_PAGE_COUNTER=y
>> +CONFIG_PSI=y
>> diff --git a/tools/testing/selftests/cgroup/test_psi.c b/tools/testing/selftests/cgroup/test_psi.c
>> new file mode 100644
>> index 000000000000..51dc35e26013
>> --- /dev/null
>> +++ b/tools/testing/selftests/cgroup/test_psi.c
>> @@ -0,0 +1,280 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +#define _GNU_SOURCE
>> +#include <errno.h>
>> +#include <fcntl.h>
>> +#include <poll.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <sys/wait.h>
>> +#include <unistd.h>
>> +#include <linux/limits.h>
>> +
>> +#include "kselftest.h"
>> +#include "cgroup_util.h"
>> +
>> +/* How long to wait for the CPU-pressure trigger before giving up. */
>> +#define PSI_POLL_TIMEOUT_MS    5000
>> +
>> +static int pressure_open(const char *resource)
>> +{
>> +       char path[PATH_MAX];
>> +
>> +       snprintf(path, sizeof(path), "/proc/pressure/%s", resource);
>> +       return open(path, O_RDWR);
>> +}
>> +
>> +/* Write a trigger descriptor, including the trailing NUL the parser expects. */
>> +static ssize_t write_trigger(int fd, const char *trigger)
>> +{
>> +       return write(fd, trigger, strlen(trigger) + 1);
>> +}
>> +
>> +/* Trigger smoke test: second on same fd -> EBUSY; IRQ rejects "some". */
> 
> Please write more descriptive comments for your tests. I have to guess
> what you mean by this.
> 
Yeah, those are too cryptic. I'll spell out what each case is actually
exercising instead of just the expected result.
>> +static int test_proc_triggers(const char *root)
> 
> Why do you pass root here if you are not using it? AI glitch?
> 
Dropping the param entirely. The proc-trigger cases don't need a cgroup,
so they shouldn't take one (this also ties into the split below).
>> +{
>> +       static const char *const resources[] = { "io", "memory", "cpu" };
>> +       int ret = KSFT_FAIL;
>> +       int fd = -1;
>> +       int i;
>> +
>> +       (void)root;
>> +
>> +       for (i = 0; i < (int)ARRAY_SIZE(resources); i++) {
>> +               fd = pressure_open(resources[i]);
>> +               if (fd < 0) {
>> +                       ksft_print_msg("open /proc/pressure/%s: %s\n",
>> +                                      resources[i], strerror(errno));
> 
> You could move these ksft_print_msg() into pressure_open() and make
> that function a bit more useful while making the code here simpler.
> 
Will do, folding the error printing in there.
>> +                       goto cleanup;
>> +               }
>> +
>> +               errno = 0;
> 
> Why are you resetting the errno? If the next syscall fails, it will
> reset the errno and if it succeeds, you won't be using errno anyway.
> I'm confused.
> 
No good reason -- removing. I'll only read errno right after a failed
call.
>> +               if (write_trigger(fd, "some 150000 2000000") <= 0) {
>> +                       ksft_print_msg("%s: 'some' trigger rejected: %s\n",
>> +                                      resources[i], strerror(errno));
>> +                       goto cleanup;
>> +               }
>> +               /* A second trigger on the same fd must fail with EBUSY. */
>> +               errno = 0;
>> +               if (write_trigger(fd, "full 150000 2000000") != -1 || errno != EBUSY) {
>> +                       ksft_print_msg("%s: second trigger expected EBUSY, got %s\n",
>> +                                      resources[i], strerror(errno));
>> +                       goto cleanup;
>> +               }
>> +
>> +               close(fd);
>> +               fd = -1;
> 
> When you jump to cleanup label you always need to close the fd. You
> could simplify the flow if you remove all these "fd = -1;" and change
> the end of the function to be:
> 
>         return KSFT_PASS
> cleanup:
>         close(fd);
>         return ret;
> }
> 
Right, that's just noise. Single close(fd) in cleanup.
>> +       }
>> +
>> +       /* IRQ is full-only, and only exists with CONFIG_IRQ_TIME_ACCOUNTING. */
> 
> It would be better if you split this test into separate tests for
> "io", "memory", "cpu" and "irq" and if "irq" is not available you can
> return KSFT_SKIP for that test only. This way when a test fails the
> user will know exactly what failed.
> 
Agreed, splitting them. irq will KSFT_SKIP when /proc/pressure/irq
isn't there.
>> +       fd = pressure_open("irq");
>> +       if (fd >= 0) {
>> +               errno = 0;
>> +               if (write_trigger(fd, "some 150000 1000000") != -1) {
>> +                       ksft_print_msg("irq 'some': expected failure, got %s\n",
>> +                                      strerror(errno));
>> +                       goto cleanup;
>> +               }
>> +               close(fd);
>> +               fd = -1;
>> +       }
>> +
>> +       ret = KSFT_PASS;
>> +cleanup:
>> +       if (fd >= 0)
>> +               close(fd);
>> +       return ret;
>> +}
>> +
>> +/* cgroup.pressure 0/1 hides/shows the *.pressure files and round-trips. */
> 
> The above comment needs to be expanded to explain what you are
> testing. It does not read as a proper English sentence.
> 
Rewriting it as an actual sentence.
>> +static int test_cgroup_pressure_toggle(const char *root)
>> +{
>> +       char buf[BUF_SIZE] = { 0 };
>> +       char *cg = NULL;
>> +       int ret = KSFT_FAIL;
>> +
>> +       cg = cg_name(root, "psi_toggle_test");
>> +       if (!cg)
>> +               goto cleanup;
>> +       if (cg_create(cg))
>> +               goto cleanup;
> 
> Jumping to "cleanup" above results in a call to cg_destroy() while
> cg_create() failed. It will try to rmdir() a directory which we never
> created.
> 
that's a real bug. Adding a "created" flag so we don't tear
down a cgroup that was never set up.
>> +
>> +       if (cg_write(cg, "cgroup.pressure", "0")) {
>> +               ksft_print_msg("failed to disable cgroup.pressure\n");
> 
> Reporting strerror() in these failure logs would be useful.
> 
Adding it to the cg_write/cg_read paths.
>> +               goto cleanup;
>> +       }
>> +       if (cg_read(cg, "cgroup.pressure", buf, sizeof(buf)) < 0 || atoi(buf) != 0) {
> 
> atoi() will return 0 even on error, so if you want to really check the
> value is 0 use a more robust method like strtol() or simply do a
> string comparison.
> 
Right, atoi is wrong here. String compare instead.
>> +               ksft_print_msg("cgroup.pressure=0 readback: '%s'\n", buf);
>> +               goto cleanup;
>> +       }
>> +       if (cg_read(cg, "memory.pressure", buf, sizeof(buf)) >= 0) {
>> +               ksft_print_msg("memory.pressure visible while disabled\n");
>> +               goto cleanup;
>> +       }
>> +
>> +       if (cg_write(cg, "cgroup.pressure", "1")) {
>> +               ksft_print_msg("failed to re-enable cgroup.pressure\n");
>> +               goto cleanup;
>> +       }
>> +       if (cg_read(cg, "cgroup.pressure", buf, sizeof(buf)) < 0 || atoi(buf) != 1) {
>> +               ksft_print_msg("cgroup.pressure=1 readback: '%s'\n", buf);
>> +               goto cleanup;
>> +       }
>> +       if (cg_read(cg, "memory.pressure", buf, sizeof(buf)) < 0) {
>> +               ksft_print_msg("memory.pressure unreadable while enabled\n");
>> +               goto cleanup;
>> +       }
>> +
>> +       ret = KSFT_PASS;
>> +cleanup:
>> +       if (cg)
>> +               cg_destroy(cg);
>> +       free(cg);
> 
> free(NULL) works but would be cleaner to do this instead:
> 
>       if (cg) {
>               cg_destroy(cg);
>               free(cg);
>       }
> 
ok, collapsing those.
>> +       return ret;
>> +}
>> +
>> +/* Induce deterministic CPU pressure (more hogs than CPUs). */
>> +static int test_cgroup_trigger_fire(const char *root)
>> +{
>> +       char *cg = NULL, *cpupress = NULL;
>> +       int fd = -1, ret = KSFT_FAIL;
>> +       struct pollfd pfd;
>> +       long ncpus, i;
>> +       pid_t pid;
>> +
>> +       cg = cg_name(root, "psi_trigger_test");
>> +       if (!cg)
>> +               goto cleanup;
>> +       if (cg_create(cg))
>> +               goto cleanup;
> 
> Same issue with this jump. You will be calling cg_killall(cg) and
> cg_destroy(cg) even though cg_create() did not succeed.
Same created-flag fix.
> 
>> +
>> +       cpupress = cg_control(cg, "cpu.pressure");
>> +       if (!cpupress)
>> +               goto cleanup;
>> +       fd = open(cpupress, O_RDWR);
>> +       if (fd < 0) {
>> +               ksft_print_msg("open cpu.pressure: %s\n", strerror(errno));
>> +               goto cleanup;
>> +       }
>> +
>> +       /* 1us threshold in a 1s window: any cpu stall fires it. */
>> +       errno = 0;
>> +       if (write_trigger(fd, "some 1 1000000") <= 0) {
>> +               ksft_print_msg("arming trigger failed: %s\n", strerror(errno));
>> +               goto cleanup;
>> +       }
>> +
>> +       ncpus = sysconf(_SC_NPROCESSORS_ONLN);
>> +       if (ncpus <= 0)
> 
> Why is this not treated as a test failure?
> 
Yeah, making sysconf() <= 0 a KSFT_FAIL.
>> +               ncpus = 1;
>> +
>> +       pid = fork();
>> +       if (pid < 0) {
>> +               ksft_print_msg("fork: %s\n", strerror(errno));
>> +               goto cleanup;
>> +       }
>> +       if (pid == 0) {
>> +               /* Enter the cgroup, then over-subscribe it with CPU hogs. */
>> +               if (cg_enter_current(cg))
>> +                       _exit(KSFT_FAIL);
>> +               for (i = 0; i < ncpus; i++) {
>> +                       if (fork() == 0) {
>> +                               for (;;)
>> +                                       asm volatile("" ::: "memory");
>> +                               _exit(0);
>> +                       }
>> +               }
>> +               for (;;)
>> +                       asm volatile("" ::: "memory");  /* child is also a hog */
>> +               _exit(0);
>> +       }
>> +
>> +       pfd.fd = fd;
>> +       pfd.events = POLLPRI;
>> +       switch (poll(&pfd, 1, PSI_POLL_TIMEOUT_MS)) {
>> +       case -1:
>> +               ksft_print_msg("poll: %s\n", strerror(errno));
>> +               break;
>> +       case 0:
>> +               ksft_print_msg("no trigger event; could not induce cpu pressure\n");
>> +               ret = KSFT_SKIP;
>> +               break;
>> +       default:
>> +               if (pfd.revents & POLLPRI)
>> +                       ret = KSFT_PASS;
>> +               else
>> +                       ksft_print_msg("poll returned 0x%x\n", pfd.revents);
>> +               break;
>> +       }
>> +
>> +       /* Stop the hogs (bounded, so waitpid can't hang) and reap the child. */
>> +       cg_killall(cg);
>> +       waitpid(pid, NULL, 0);
>> +
>> +cleanup:
>> +       if (fd >= 0)
>> +               close(fd);
>> +       if (cg) {
>> +               cg_killall(cg);
>> +               cg_destroy(cg);
>> +       }
>> +       free(cpupress);
>> +       free(cg);
>> +       return ret;
>> +}
>> +
>> +#define TEST(x) { #x, x }
>> +
>> +struct psi_test {
>> +       const char *name;
>> +       int (*fn)(const char *root);
>> +};
>> +
>> +static struct psi_test tests[] = {
>> +       TEST(test_proc_triggers),
>> +       TEST(test_cgroup_pressure_toggle),
>> +       TEST(test_cgroup_trigger_fire),
> 
> Instead of setting up these tests manually you could use
> TEST_HARNESS_MAIN, TEST_F and other helpers from kselftest_harness.h.
> 
I just went with the manual style to match the rest of the directory --
test_memcontrol.c, test_core.c, test_cpu.c, etc. all use ksft_set_plan()
and none use kselftest_harness.h. Happy to switch to the harness if
you'd prefer.

I'll fold all of this into v3.

Thanks,
Tao
>> +};
>> +
>> +int main(int argc, char **argv)
>> +{
>> +       char root[PATH_MAX];
>> +       int mempress_fd;
>> +       int i;
>> +
>> +       (void)argc;
>> +
>> +       ksft_print_header();
>> +       ksft_set_plan(ARRAY_SIZE(tests));
>> +
>> +       if (cg_find_unified_root(root, sizeof(root), NULL))
>> +               ksft_exit_skip("cgroup v2 isn't mounted\n");
>> +
>> +       /* PSI must be enabled (CONFIG_PSI=y, not default-disabled). */
>> +       mempress_fd = open("/proc/pressure/memory", O_RDONLY);
>> +       if (mempress_fd < 0)
>> +               ksft_exit_skip("PSI unavailable (CONFIG_PSI=n or psi=0)\n");
>> +       close(mempress_fd);
>> +
>> +       if (cg_read_strstr(root, "cgroup.controllers", "memory"))
>> +               ksft_exit_skip("memory controller isn't available\n");
>> +       if (cg_read_strstr(root, "cgroup.subtree_control", "memory"))
>> +               if (cg_write(root, "cgroup.subtree_control", "+memory"))
>> +                       ksft_exit_skip("failed to enable memory controller\n");
>> +
>> +       for (i = 0; i < (int)ARRAY_SIZE(tests); i++) {
>> +               switch (tests[i].fn(root)) {
>> +               case KSFT_PASS:
>> +                       ksft_test_result_pass("%s\n", tests[i].name);
>> +                       break;
>> +               case KSFT_SKIP:
>> +                       ksft_test_result_skip("%s\n", tests[i].name);
>> +                       break;
>> +               default:
>> +                       ksft_test_result_fail("%s\n", tests[i].name);
>> +                       break;
>> +               }
>> +       }
>> +
>> +       ksft_finished();
>> +}
>> --
>> 2.43.0
>>