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

On Thu, Aug 6, 2026 at 9:52 AM Michal Koutný <[email protected]> wrote:
>
> Hi,
> thanks for continuing with this.

Thanks for adding these tests and sorry I was not able to review them
yet. I'll try to review them this weekend and will provide my
feedback.
Thanks,
Suren.

>
> On Tue, Jul 28, 2026 at 04:37:42PM +0800, Tao Cui <[email protected]> wrote:
> > +/* Trigger smoke test: second on same fd -> EBUSY; IRQ rejects "some". */
> > +static int test_proc_triggers(const char *root)
> > +{
> > +     static const char *const resources[] = { "io", "memory", "cpu" };
> > +     int ret = KSFT_FAIL;
> > +     int fd = -1;
> > +     int i;
> > +
> > +     (void)root;
>
> WTH?
>
> > +
> > +     for (i = 0; i < (int)ARRAY_SIZE(resources); i++) {
>
> This (int) cast is quite uncommon, look around.
>
> > +/* 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;
> > +
> > +     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;
> > +     }
>
> When I see this, could you increase the window size to 2000000 (so that
> it has potential to run for unprivileged users)?
>
> > +
> > +     ncpus = sysconf(_SC_NPROCESSORS_ONLN);
> > +     if (ncpus <= 0)
> > +             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);
> > +     }
>
> This part could be replaced with generalized hog_cpus_timed() from
> test_cpu.c (after move to cgroup_util.c).
>
> > +int main(int argc, char **argv)
> > +{
> > +     char root[PATH_MAX];
> > +     int mempress_fd;
> > +     int i;
> > +
> > +     (void)argc;
>
> What's up with this?
>
> > +
> > +     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");
>
> The memory controller is unnecessary now, right?
>
>
> Regards,
> Michal