Re: [RFC PATCH 5/5] selftests/sched_ext: Add cpu_avail tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/sched_ext: spinners in test_churn() inherit single-CPU affinity
--

commit 8fdb05fa56aeeeb8544b8552cb1d015e3ebd3eff
Author: Kuba Piecuch <jpiecuch@google.com>

selftests/sched_ext: Add cpu_avail tests

This commit adds the cpu_avail and cpu_avail_core selftests to verify that
ops.cpu_acquire() and ops.cpu_release() accurately track CPU availability
across various scheduling transitions, hotplug events, and core scheduling.

> diff --git a/tools/testing/selftests/sched_ext/cpu_avail.c b/tools/testing/selftests/sched_ext/cpu_avail.c
> new file mode 100644
> index 0000000000000..3796f94b05a93
> --- /dev/null
> +++ b/tools/testing/selftests/sched_ext/cpu_avail.c

[ ... ]

> +/*
> + * Run RT tasks, SCX tasks and sched_setscheduler() class changes on all CPUs
> + * at once. The sched_switch checker must not find any inconsistencies.
> + */
> +static enum scx_test_status test_churn(struct cpu_avail_ctx *ctx)
> +{
> +	struct cpu_avail__bss *bss = ctx->skel->bss;
> +	struct task_spec spinner = { .cpu = -1, .policy = SCHED_OTHER };
> +	struct task_spec rt = {
> +		.policy = SCHED_FIFO, .prio = 20,
> +		.period_us = 1000, .run_us = 200, .duration_ms = 1500,
> +	};
> +	enum scx_test_status ret = SCX_TEST_FAIL;
> +	int nr_spinners = 0, nr_rts = 0, cpu, i;
> +	pid_t *spinners, *rts;
> +	u64 checks = bss->nr_checks;
> +	u64 end;
> +
> +	spinners = calloc(ctx->nr_cpus, sizeof(pid_t));
> +	rts = calloc(ctx->nr_cpus, sizeof(pid_t));
> +	if (!spinners || !rts) {
> +		SCX_ERR("Failed to allocate");
> +		goto out;
> +	}
> +
> +	for (cpu = 0; cpu < ctx->nr_cpus; cpu++) {
> +		if (!cpu_is_online(cpu))
> +			continue;
> +
> +		spinners[nr_spinners] = spawn_task(&spinner);
> +		if (spinners[nr_spinners] < 0) {
> +			SCX_ERR("Failed to fork (%d)", errno);
> +			goto out;
> +		}
> +		nr_spinners++;
> +
> +		rt.cpu = cpu;
> +		rts[nr_rts] = spawn_task(&rt);
> +		if (rts[nr_rts] < 0) {
> +			SCX_ERR("Failed to fork (%d)", errno);
> +			goto out;
> +		}
> +		nr_rts++;
> +	}
> +
> +	/* flip a spinner between SCX and RT while the RT tasks run */
> +	end = now_ns() + 1500 * 1000000ULL;
> +	for (i = 0; now_ns() < end; i++) {
> +		if (set_policy(spinners[0], i % 2 ? SCHED_OTHER : SCHED_FIFO,
> +			       i % 2 ? 0 : 1)) {
> +			SCX_ERR("Failed to set policy (%d)", errno);
> +			goto out;
> +		}
> +		sleep_ms(2);
> +	}

[ ... ]

> +static enum scx_test_status setup(void **ctxp)
> +{
> +	struct cpu_avail_ctx *ctx;
> +	int cpu;

[ ... ]

> +	if (sched_getaffinity(0, sizeof(ctx->orig_mask), &ctx->orig_mask) ||
> +	    pin_to_cpu(0, ctx->ctl_cpu)) {
> +		SCX_ERR("Failed to pin to CPU %d", ctx->ctl_cpu);
> +		free(ctx);
> +		return SCX_TEST_FAIL;
> +	}

[ ... ]

> diff --git a/tools/testing/selftests/sched_ext/cpu_avail.h b/tools/testing/selftests/sched_ext/cpu_avail.h
> new file mode 100644
> index 0000000000000..97659e1061b87
> --- /dev/null
> +++ b/tools/testing/selftests/sched_ext/cpu_avail.h

[ ... ]

> +static inline pid_t fork_task(const struct task_spec *spec, int *go_fd)
> +{
> +	int fds[2];
> +	pid_t pid;
> +	char c;
> +
> +	if (pipe(fds))
> +		return -1;
> +
> +	pid = fork();
> +	if (pid < 0) {
> +		close(fds[0]);
> +		close(fds[1]);
> +		return -1;
> +	}
> +
> +	if (!pid) {
> +		close(fds[1]);
> +		if (read(fds[0], &c, 1) != 1)
> +			exit(EXIT_FAILURE);
> +		close(fds[0]);
> +		if (spec->cpu >= 0 && pin_to_cpu(0, spec->cpu))
> +			exit(EXIT_FAILURE);

[Severity: Medium]
Does setting spec->cpu = -1 in test_churn() leave all spinner tasks pinned
to ctx->ctl_cpu instead of running across all CPUs?

In setup(), the parent test runner pins itself to ctx->ctl_cpu via
pin_to_cpu(0, ctx->ctl_cpu). When test_churn() spawns spinners with
spinner.cpu = -1 via spawn_task() -> fork_task(), fork_task() only changes
affinity when spec->cpu >= 0, so the child inherits the parent's single-CPU
affinity mask on ctx->ctl_cpu.

Because p->nr_cpus_allowed is 1, cpu_avail_enqueue() in
tools/testing/selftests/sched_ext/cpu_avail.bpf.c queues all spinners onto
SCX_DSQ_LOCAL for ctx->ctl_cpu rather than SCX_DSQ_GLOBAL:

	/* queue pinned tasks on the local DSQ of the CPU they're enqueued on */
	if (p->nr_cpus_allowed == 1)
		scx_bpf_dsq_insert(p, SCX_DSQ_LOCAL, slice, enq_flags);
	else
		scx_bpf_dsq_insert(p, SCX_DSQ_GLOBAL, slice, enq_flags);

In addition, when test_churn() switches spinners[0] (an infinite busy-loop
task with period_us == 0) to SCHED_FIFO on iteration i = 0, spinners[0] is
pinned to the same ctx->ctl_cpu as the SCHED_OTHER test runner. This
immediately preempts and starves the test runner until RT throttling kicks
in, where the runner then has to wait behind all the other spinners queued
on ctx->ctl_cpu's local DSQ (which on machines with many CPUs can stall
runnable tasks on ctx->ctl_cpu past the 10-second watchdog timeout).

Should fork_task() restore a full CPU affinity mask (such as ctx->orig_mask)
when spec->cpu < 0?

> +		if (spec->policy != SCHED_OTHER &&
> +		    set_policy(0, spec->policy, spec->prio))
> +			exit(EXIT_FAILURE);
> +		task_body(spec);
> +		exit(EXIT_SUCCESS);
> +	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792248922.git.jpiecuch@google.com?part=5