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

[Tim Chen <tim.c.chen@linux.intel.com>]

On Thu, 2026-09-10 at 21:19 +0200, Peter Zijlstra wrote:
> 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 :-(
> 
> 

Sorry for the warts in this version.  Will clean it up and send
an update.

Tim