Re: [LTP] sched/psi: Add psi_invalid_trigger01 test

[linuxtestproject.agent@gmail.com]

Hi Samir,

On Sat, 24 Oct 2026, Samir Mulani wrote:
> sched/psi: Add psi_invalid_trigger01 test

--- [PATCH 1/4] ---

> +	.needs_kconfigs = (const char *const[]) {
> +		"CONFIG_PSI=y",

Medium: PSI statistics exist in Linux 4.20-5.1, but trigger writes were
introduced in 5.2, so CONFIG_PSI and the setup read do not establish
monitor support. Add .min_kver = "5.2" or equivalent runtime detection
to avoid failing on supported kernels without this interface.

> +		TEST(write(fd, tc->input, len));
> +
> +		if (TST_RET == (long)len)
> +			tst_res(TPASS, "%s: trigger accepted", tc->desc);

Low: Use TST_EXP_VAL(write(fd, tc->input, len), (long)len,
"%s", tc->desc) instead of the manual return-value reporting block.

> +	.needs_root = 1,
> +	.caps = (struct tst_cap []) {
> +		TST_CAP(TST_CAP_REQ, CAP_SYS_RESOURCE),

Low: Document the root/CAP_SYS_RESOURCE requirement for custom trigger
windows in the high-level test description, not only the commit message.

> diff --git a/testcases/kernel/sched/psi/Makefile b/testcases/kernel/sched/psi/Makefile
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +

Low: Add the required copyright line after the SPDX tag.

--- [PATCH 2/4] ---

> +static struct tst_test test = {
> +	.tcnt = ARRAY_SIZE(tests),
> +	.test = run,
> +	.setup = setup,

Medium: This test also needs a monitor-availability gate for PSI-enabled
4.20-5.1 kernels, where setup succeeds but trigger writes are unsupported.
Use .min_kver = "5.2" or equivalent runtime detection.

> +	TST_EXP_FAIL2(WRITE_TRIG(fd, TRIG_B), EBUSY,
> +		      "second write to the same fd");

Medium: This checks the behavior introduced by kernel fix a06247c6804f
("psi: Fix uaf issue when psi trigger is destroyed while being polled").
Add a linux-git regression tag for that fix and explain the relationship
in the commit message; do not skip kernels merely because they lack
the fix.

> +static void (*const tests[])(void) = {
> +	first_write_succeeds,
> +	second_write_ebusy,
> +	dup_fd_shares_trigger,

Low: Use the required struct tcase table and common .test handler,
parameterizing trigger strings, descriptor sharing and expected results
instead of dispatching separate per-case functions.

> +	.needs_root = 1,

Low: Explain the writable PSI interface and custom-window privilege
requirements in the high-level test description.

--- [PATCH 3/4] ---

> +#define PSI_LINE "%7s avg10=%lf avg60=%lf avg300=%lf total=%llu"
> [...]
> +	SAFE_FCLOSE(fp);
> +
> +	if (ret != 10) {

Medium: Supplying total=-1 and an unexpected third line still produces
seven passing results: %llu accepts the sign and the parser ignores
everything after the tenth conversion. Validate unsigned decimal totals,
consume each expected line completely, and reject extra records.

> +	if (n < ARRAY_SIZE(paths))
> +		verify_format(n);
> +	else if (n < 2 * ARRAY_SIZE(paths))
> +		verify_monotonic(n - ARRAY_SIZE(paths));
> +	else
> +		verify_cpu_full_zero();

Low: Replace the index-range dispatch with a struct tcase table describing
each path and check, using the common .test callback.

--- [PATCH 4/4] ---

> +			tst_clock_gettime(CLOCK_MONOTONIC, &start);
> +
> +			if (pwrite(fd, io_buf, IO_SIZE, 0) != IO_SIZE)
> +				tst_brk(TBROK | TERRNO, "pwrite() failed");
> +
> +			tst_clock_gettime(CLOCK_MONOTONIC, &end);
> +			*io_wait_us += tst_timespec_diff_us(end, start);

High: This measures syscall wall time, including CPU work and preemption,
not time blocked on I/O. An O_DIRECT shmem reproduction measured 1995579us
with zero voluntary context switches, yet the test would require 498894us
of I/O pressure; LTP_DEV_FS_TYPE=tmpfs reaches this no-storage path.
Measure actual blocked-I/O time independently or use a controlled
blocking-I/O workload before deriving the expected PSI delta.

> +static char io_buf[IO_SIZE] __attribute__((aligned(4096)));

Medium: A fixed 4KiB alignment is insufficient on supported 64KiB-page
systems with 64KiB-sector Btrfs, which falls back to buffered writes for
this misalignment and invalidates the workload. Allocate a buffer using
the runtime DIO alignment or a suitable runtime page alignment; opening
the file successfully does not validate buffer alignment.

> +	for (i = 0; i < 2; i++) {
> +		if (!SAFE_FORK()) {
> +			join_cgroup();
> +			pin_to_cpu(pin_cpu);
> +
> +			tst_timer_start(CLOCK_MONOTONIC);

Medium: If the parent is delayed between forks for two seconds, the first
worker can finish before the second joins, producing serial workloads
and a false failure on an otherwise idle CPU. Use checkpoints to wait
until both workers have joined and pinned themselves, then release both
before starting their workload timers.

> +			if (pwrite(fd, io_buf, IO_SIZE, 0) != IO_SIZE)
> +				tst_brk(TBROK | TERRNO, "pwrite() failed");

Low: Use SAFE_PWRITE(SAFE_WRITE_ALL, fd, io_buf, IO_SIZE, 0) for this
workload operation; the subject under test is PSI accounting, not pwrite.

> +static void (*const tests[])(void) = {
> +	verify_cpu,
> +	verify_memory,
> +	verify_io,

Low: Use a struct tcase table and common .test handler rather than
dispatching separate per-case functions.

> +	.needs_root = 1,

Low: Document the cgroup/controller and test-filesystem setup as the
reason for requiring root in the high-level description.

> diff --git a/testcases/kernel/sched/psi/.gitignore b/testcases/kernel/sched/psi/.gitignore
>  /psi_proc_format01
> +/psi_accounting01

Low: Keep the entries sorted; /psi_accounting01 belongs first.

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