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

["David Hildenbrand (Arm)" <david@kernel.org>]

On 10/12/26 14:04, Yafang Shao wrote:
> 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.
> 

I'll only comment on the new exposed helpers. Some of these might make sense
even without the bigger series.

(obviously, minus the task change)

> No functional change.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  include/linux/sched/numa_balancing.h | 31 ++++++++++++++++++++++++++++
>  kernel/sched/fair.c                  | 16 +++++++-------
>  kernel/sched/sched.h                 |  1 -
>  mm/memory-tiers.c                    |  3 ++-
>  mm/migrate.c                         |  7 ++++---
>  mm/vmscan.c                          |  6 +++---
>  6 files changed, 48 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/sched/numa_balancing.h b/include/linux/sched/numa_balancing.h
> index 52b22c5c396d..353772b49c5f 100644
> --- a/include/linux/sched/numa_balancing.h
> +++ b/include/linux/sched/numa_balancing.h
> @@ -8,6 +8,7 @@
>   */
>  
>  #include <linux/sched.h>
> +#include <linux/sched/sysctl.h>
>  
>  #define TNF_MIGRATED	0x01
>  #define TNF_NO_GROUP	0x02
> @@ -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.

Talking about later patches in a function doc is ... wrong.

> + */
> +static inline bool task_numab_enabled(struct task_struct *p)
> +{
> +	return static_branch_unlikely(&sched_numa_balancing);
> +}
> +
> +/*
> + * The global sysctl state, for the system-wide tiering machinery: this
> + * reflects kernel.numa_balancing_mode only and takes no task.

The latter part doesn't make a lot of sense in this documentation.

> + */
> +static inline bool numab_mode_tiering(void)

The function name isn't really intuitive. Should this be

	numab_in_tiering_mode

	
	numab_tiering_enabled

or something like this.

> +{
> +	if (sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING)
> +		return true;
> +	return false;


return sysctl_numa_balancing_mode & NUMA_BALANCING_MEMORY_TIERING;

?

Both functions could directly have proper kerneldoc if we are already
documenting these.

> +}
>  #else
>  static inline void task_numa_fault(int last_node, int node, int pages,
>  				   int flags)
> @@ -52,6 +75,14 @@ static inline bool should_numa_migrate_memory(struct task_struct *p,
>  {
>  	return true;
>  }
> +static inline bool task_numab_enabled(struct task_struct *p)
> +{
> +	return false;
> +}
> +static inline bool numab_mode_tiering(void)
> +{
> +	return false;
> +}


-- 
Cheers,

David