Re: [PATCH] tools/sched_ext: Add scx_priority dual-queue priority CPU scheduler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] scx: RT tasks misclassified as low priority in is_high_prio()
- [Medium] scx: uninitialized stack memory read in main() on allocation failure
- [Medium] scx: missing volatile qualifier on exit_req signal flag
--

commit 909c1bfd71282dc986104d5879f86c906eb61d81
Author: rahadbhuiya <rahadbhuiya2021@gmail.com>

tools/sched_ext: Add scx_priority dual-queue priority CPU scheduler

This commit adds scx_priority, a dual-queue priority CPU scheduler using
sched_ext. It separates latency-sensitive/interactive tasks from normal/batch
tasks, queuing high-priority tasks to a dedicated high-priority DSQ with
boosted time slices.

> diff --git a/tools/sched_ext/scx_priority.bpf.c b/tools/sched_ext/scx_priority.bpf.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/sched_ext/scx_priority.bpf.c

[ ... ]

> +static bool is_high_prio(const struct task_struct *p)
> +{
> +	/*
> +	 * In the Linux kernel, static_prio maps nice -20..19 to 100..139.
> +	 * Default nice 0 corresponds to static_prio 120. Tasks with nice < 0
> +	 * (static_prio < 120) or real-time policies are prioritized.
> +	 */
> +	return p->static_prio < 120;

[Severity: Medium]
Does this accurately capture real-time tasks as intended by the comment?

Real-time tasks typically have a static_prio of 120 (nice 0), while their
effective prio is between 0 and 99. By checking p->static_prio < 120, wouldn't
real-time tasks be mistakenly classified as low priority and placed in the
standard/batch queue? 

Should this evaluate p->prio instead to correctly identify tasks with 
real-time policies?

> +}
> +
> +s32 BPF_STRUCT_OPS(prio_select_cpu, struct task_struct *p, s32 prev_cpu, u64 wake_flags)

[ ... ]

> diff --git a/tools/sched_ext/scx_priority.c b/tools/sched_ext/scx_priority.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/sched_ext/scx_priority.c

[ ... ]

> +static bool verbose;
> +static sig_atomic_t exit_req;

[Severity: Medium]
Can the program fail to exit gracefully on SIGINT or SIGTERM?

Because exit_req is modified asynchronously in a signal handler and checked
in the main loop, lacking the volatile qualifier might allow the compiler to
optimize and cache the flag. Could this result in an infinite loop?

> +
> +static int libbpf_print_fn(enum libbpf_print_level level, const char *format, va_list args)

[ ... ]

> +static void read_stats(struct scx_priority *skel, __u64 *stats)
> +{
> +	int nr_cpus = libbpf_num_possible_cpus();
> +	__u64 *cnts[2];
> +	__u32 idx;
> +
> +	assert(nr_cpus > 0);
> +	cnts[0] = calloc(nr_cpus, sizeof(__u64));
> +	cnts[1] = calloc(nr_cpus, sizeof(__u64));
> +	if (!cnts[0] || !cnts[1]) {
> +		free(cnts[0]);
> +		free(cnts[1]);
> +		return;
> +	}

[Severity: Medium]
Does this early return leave the stats array out-parameter uninitialized?

If calloc fails under memory pressure, this returns before memset is called
to initialize the stats array to zero.

> +
> +	memset(stats, 0, sizeof(stats[0]) * 2);

[ ... ]

> +	while (!exit_req && !UEI_EXITED(skel, uei)) {
> +		__u64 stats[2];
> +
> +		read_stats(skel, stats);
> +		printf("%-15llu %-15llu %-15llu\n",
> +		       stats[0], stats[1], stats[0] + stats[1]);

[Severity: Medium]
When read_stats returns early on allocation failure, won't this printf
read from the uninitialized stats array on the stack?

> +		fflush(stdout);
> +		sleep(interval);
> +	}
> +
> +	bpf_link__destroy(link);
> +	ecode = UEI_REPORT(skel, uei);
> +	scx_priority__destroy(skel);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922084557.532-1-rahadbhuiya2021@gmail.com?part=1