Re: [PATCH 4/4] sched/cache: Introduce task_struct->sched_cache_grp

[Peter Zijlstra <peterz@infradead.org>]

On Thu, Sep 10, 2026 at 10:46:12AM -0700, Tim Chen wrote:

> Co-developed-by: Chen Yu <yu.c.chen@intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>

:-(

> ---
>  fs/exec.c                  |  14 ++++
>  include/linux/sched.h      |   3 +
>  kernel/exit.c              |  26 +++++--
>  kernel/fork.c              |  23 ++++++
>  kernel/sched/cache_sched.c |  19 +++++
>  kernel/sched/fair.c        | 142 +++++++++++++++++++++----------------
>  kernel/sched/sched.h       |   3 +
>  7 files changed, 164 insertions(+), 66 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 745f6eb5279e..7a8a9954343e 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -882,6 +882,20 @@ static int exec_mmap(struct linux_binprm *bprm)
>  	active_mm = tsk->active_mm;
>  	tsk->active_mm = mm;
>  	tsk->mm = mm;
> +#ifdef CONFIG_SCHED_CACHE
> +	{
> +		struct sched_cache_group *old_grp, *new_grp;
> +
> +		old_grp = rcu_dereference_protected(tsk->sched_cache_grp, true);
> +
> +		/* Acquire the reference before publishing the pointer. */
> +		new_grp = sched_cache_group_get(mm->sched_cache_grp);
> +
> +		rcu_assign_pointer(tsk->sched_cache_grp, new_grp);
> +		if (old_grp)
> +			sched_cache_group_put(old_grp);
> +	}
> +#endif

Guys no! This is horrific crap. This is not how we do things and I would
have expected you all to know this.

Have you heard of this new fangled thing called a function?

Imagine all of those being just:

	sched_cache_exec_mmap(tsk, mm);


Also: rcu_dereference_protected(.c = true) is another offence, that's
just wrong.


> diff --git a/kernel/exit.c b/kernel/exit.c
> index 006edcc0c2c5..442535778ce1 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -552,23 +552,25 @@ void mm_update_next_owner(struct mm_struct *mm)
>   * Subtract the memory footprint of the current task from
>   * mm.
>   */
> -static void exit_mm_sched_cache(struct mm_struct *mm)
> +static void exit_mm_sched_cache(void)
>  {
> +	struct sched_cache_group *grp =
> +		rcu_dereference_protected(current->sched_cache_grp, true);
>  	unsigned long fp, sub;
>  
> -	if (!current->total_numa_faults)
> +	if (!grp || !current->total_numa_faults)
>  		return;
>  	/*
>  	 * No lock protection due to performance considerations.
>  	 * Make sure the group footprint does not become
>  	 * negative.
>  	 */
> -	fp = READ_ONCE(mm->sched_cache_grp->footprint);
> +	fp = READ_ONCE(grp->footprint);
>  	sub = min(fp, current->total_numa_faults);
> -	WRITE_ONCE(mm->sched_cache_grp->footprint, fp - sub);
> +	WRITE_ONCE(grp->footprint, fp - sub);
>  }
>  #else
> -static inline void exit_mm_sched_cache(struct mm_struct *mm)
> +static inline void exit_mm_sched_cache(void)
>  {
>  }
>  #endif /* CONFIG_SCHED_CACHE CONFIG_NUMA_BALANCING */
> @@ -585,7 +587,19 @@ static void exit_mm(void)
>  	if (!mm)
>  		return;
>  
> -	exit_mm_sched_cache(mm);
> +	exit_mm_sched_cache();
> +
> +#ifdef CONFIG_SCHED_CACHE
> +	{
> +		struct sched_cache_group *grp =
> +			rcu_dereference_protected(current->sched_cache_grp, true);
> +
> +		rcu_assign_pointer(current->sched_cache_grp, NULL);
> +
> +		if (grp)
> +			sched_cache_group_put(grp);
> +	}
> +#endif

Seriously, WTF ?!

>  
>  	mmap_read_lock(mm);
>  	mmgrab_lazy_tlb(mm);
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 416758c8a3d4..2e79548cb7c1 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1599,6 +1599,19 @@ static int copy_mm(u64 clone_flags, struct task_struct *tsk)
>  
>  	tsk->mm = mm;
>  	tsk->active_mm = mm;
> +#ifdef CONFIG_SCHED_CACHE
> +	{
> +		/*
> +		 * A task holds its own reference on the group, separate from
> +		 * the reference held by its mm_struct. Acquire it before
> +		 * publishing the pointer.
> +		 */
> +		struct sched_cache_group *grp =
> +			sched_cache_group_get(mm->sched_cache_grp);
> +
> +		rcu_assign_pointer(tsk->sched_cache_grp, grp);
> +	}
> +#endif

And again.

>  	return 0;
>  }
>  
> @@ -2599,6 +2612,16 @@ __latent_entropy struct task_struct *copy_process(
>  bad_fork_cleanup_namespaces:
>  	exit_nsproxy_namespaces(p);
>  bad_fork_cleanup_mm:
> +#ifdef CONFIG_SCHED_CACHE
> +	/*
> +	 * copy_mm() took a task reference on the cache group; a failed fork
> +	 * never reaches exit_mm(), so release it here to avoid leaking the
> +	 * group and its per-CPU buffer.
> +	 */
> +	sched_cache_group_put(rcu_dereference_protected(p->sched_cache_grp, true));
> +	RCU_INIT_POINTER(p->sched_cache_grp, NULL);
> +#endif
> +
>  	if (p->mm) {
>  		mm_clear_owner(p->mm, p);
>  		mmput(p->mm);

Drugs, it must be drugs and lots of it :-(

Please, try again.