From: Tejun Heo <tj@kernel.org>
To: Sahaj Chaudhari <sahaj123.sc@gmail.com>
Cc: hannes@cmpxchg.org, mkoutny@suse.com, shuah@kernel.org,
cgroups@vger.kernel.org, linux-kselftest@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] selftests/cgroup: test pids.events notification
Date: Wed, 14 Oct 2026 08:54:18 -1000 [thread overview]
Message-ID: <81a8a0d64d269851ce4b48a240aa673c@kernel.org> (raw)
In-Reply-To: <20261013064812.875787-1-sahaj123.sc@gmail.com>
Hello, Sahaj.
The following is a Claude-generated review.
On Tue, Oct 13, 2026 at 12:18:12PM +0530, Sahaj Chaudhari wrote:
> Add a selftest for pids.events notifications. When pids_localevents is
> enabled, a fork rejected by pids.max must increment pids.events and
> generate an IN_MODIFY inotify event on that file.
Can you mention 1765a153d985 ("cgroup/pids: Restore pids.events
notifications in local mode") here as the fix this test covers?
> Check for a failed read() of the inotify event before comparing its
> signed ssize_t result with sizeof(struct inotify_event). Otherwise -1 is
> converted to a large unsigned length and the uninitialized event buffer
> is inspected.
This describes the v1 to v2 change. As v1 never landed, it belongs only in
the changelog below the --- line.
> +TEST_GEN_PROGS += test_pids_local_events
Can this be a test in test_pids.c instead? Its main() already does the
same setup, so the new test fits as another T() entry next to
test_pids_events() without the Makefile and .gitignore changes. It can
then use cg_run_nowait() with run_success() for the fork and cg_control()
for the pids.events path.
> + if ((size_t)n < sizeof(struct inotify_event) ||
> + !(event_buf.align.mask & IN_MODIFY)) {
> + reason = "received no pids.events modify notification";
> + goto cleanup;
> + }
> + if (cg_read_key_long(cgroup, "pids.events", "max ") != 1) {
> + reason = "pids.events did not count the rejected fork";
> + goto cleanup;
> + }
This repeats the counter check before poll(). Nothing can change the
counter in between, so maybe drop it?
> + if (proc_mount_contains("pids_localevents") <= 0)
> + ksft_exit_skip("mount cgroup2 with pids_localevents to run this test\n");
Is this skip necessary? In the default mode, the rejected fork also
increments the test cgroup's pids.events and notifies it, as the test
cgroup is the one over the limit. The same checks hold, so without the
skip the test covers the notification in both modes and still catches
the regression on a pids_localevents mount.
Thanks.
--
tejun
prev parent reply other threads:[~2026-10-14 18:54 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-13 6:48 [PATCH v3] selftests/cgroup: test pids.events notification Sahaj Chaudhari
2026-10-14 18:54 ` Tejun Heo [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=81a8a0d64d269851ce4b48a240aa673c@kernel.org \
--to=tj@kernel.org \
--cc=cgroups@vger.kernel.org \
--cc=hannes@cmpxchg.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=mkoutny@suse.com \
--cc=sahaj123.sc@gmail.com \
--cc=shuah@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox