Re: [RFC PATCH v2 1/3] sched: add helpers for numa balancing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] sched: Incomplete refactoring leaves direct sched_numa_balancing accesses
- [Low] sched: performance regression in task_numa_fault() due to incorrect static branch hint
--

commit 6333e53c9f6ee67bda34cc209d9f45e01a0af5fd
Author: Yafang Shao <laoar.shao@gmail.com>

    sched: add helpers for numa balancing
    
    Two new helpers task_numab_enabled() and numab_mode_tiering() are
    introduced to replace the scattered direct accesses to the
    sched_numa_balancing static key and the sysctl_numa_balancing_mode
    bitmask.

> diff --git a/include/linux/sched/numa_balancing.h b/include/linux/sched/numa_balancing.h
> index 52b22c5c396db..353772b49c5f9 100644
> --- a/include/linux/sched/numa_balancing.h
> +++ b/include/linux/sched/numa_balancing.h
> @@ -32,6 +33,28 @@ extern void set_numabalancing_state(bool enabled);
>  extern void task_numa_free(struct task_struct *p, bool final);
>  bool should_numa_migrate_memory(struct task_struct *p, struct folio *folio,
>  				int src_nid, int dst_cpu);
> +
> +extern struct static_key_false sched_numa_balancing;
> +
> +/*
> + * Whether NUMA balancing applies to @p.  The @p argument is unused for
> + * now: a later patch will consult a per-cgroup BPF policy with it.
> + */
> +static inline bool task_numab_enabled(struct task_struct *p)
> +{
> +	return static_branch_unlikely(&sched_numa_balancing);
> +}

[Severity: Low]
Does this wrapper unintentionally flip the branch prediction hint from
likely to unlikely on hot paths?

For instance, task_numa_fault() is almost exclusively called from NUMA
hinting fault handlers, which only trigger when NUMA balancing is active.
Before this patch, it used static_branch_likely():

-	if (!static_branch_likely(&sched_numa_balancing))
+	if (!task_numab_enabled(p))

Because the new task_numab_enabled() wrapper hardcodes
static_branch_unlikely(), could the compiler now outline the body of
task_numa_fault() and other functions that expect the condition to usually
be true?

[ ... ]

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index c52c29dcb6a2e..451e26da5af24 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4199,7 +4200,7 @@ void task_numa_fault(int last_cpupid, int mem_node, int pages, int flags)
>  	struct numa_group *ng;
>  	int priv;
>  
> -	if (!static_branch_likely(&sched_numa_balancing))
> +	if (!task_numab_enabled(p))
>  		return;

[Severity: Medium]
Are there a few direct accesses to the sched_numa_balancing static key in
kernel/sched/fair.c that might have been missed by this refactoring?

For example:

kernel/sched/fair.c:exceed_llc_capacity() {
    ...
	if (static_branch_likely(&sched_numa_balancing)) {
    ...
}

kernel/sched/fair.c:get_pref_llc() {
    ...
		if (static_branch_likely(&sched_numa_balancing) &&
		    p->numa_preferred_nid >= 0 &&
    ...
}

kernel/sched/fair.c:get_scan_cpumasks() {
    ...
	if (!static_branch_likely(&sched_numa_balancing))
		goto out;
    ...
}

Since get_pref_llc() and get_scan_cpumasks() already take a task_struct
pointer 'p', shouldn't they also be updated to use the new
task_numab_enabled(p) wrapper?

If these direct checks remain, might they bypass the per-cgroup BPF policy
that the commit message notes will be consulted in a later patch?

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