Re: [PATCH 3/4] sched/cache: Decouple sched_cache_group from mm

[Peter Zijlstra]

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

> +static void sched_cache_group_free_rcu(struct rcu_head *rcu)
> +{
> +	struct sched_cache_group *grp =
> +		container_of(rcu, struct sched_cache_group, rcu);
> +
> +	/* free_percpu() may be called from atomic context. */

That comment is misleading at best. This is rcu-free context. And
free_percpu() is not allowed from actual atomic context on RT.

> +	free_percpu(grp->pcpu_sched);
> +	kfree(grp);
> +}
> +
> +void sched_cache_group_put(struct sched_cache_group *grp)
> +{
> +	if (!grp || !refcount_dec_and_test(&grp->refcnt))
> +		return;
> +
> +	call_rcu(&grp->rcu, sched_cache_group_free_rcu);
> +}
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 32213801ea39..b5a823f0a622 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c

> @@ -1669,18 +1677,35 @@ void mm_init_sched(struct mm_struct *mm,
>  		epoch = rq->cpu_epoch;
>  	}
>  
> -	raw_spin_lock_init(&mm->sc_stat.lock);
> -	mm->sc_stat.epoch = epoch;
> -	mm->sc_stat.cpu = -1;
> -	mm->sc_stat.next_scan = jiffies;
> -	mm->sc_stat.nr_running_avg = 0;
> -	mm->sc_stat.footprint = 0;
> +	raw_spin_lock_init(&grp->lock);
> +	grp->epoch = epoch;
> +	grp->cpu = -1;
> +	grp->next_scan = jiffies;
> +	grp->nr_running_avg = 0;
> +	grp->footprint = 0;
> +	refcount_set(&grp->refcnt, 1);
>  	/*
> -	 * The update to mm->sc_stat should not be reordered
> -	 * before initialization to mm's other fields, in case
> +	 * The update to grp->pcpu_sched should not be reordered
> +	 * before initialization to grp's other fields, in case
>  	 * the readers may get invalid mm_sched_epoch, etc.
>  	 */
> -	smp_store_release(&mm->sc_stat.pcpu_sched, _pcpu_sched);
> +	smp_store_release(&grp->pcpu_sched, _pcpu_sched);
> +	/*
> +	 * Publish the group last.  Not every reader qualifies it by
> +	 * grp->pcpu_sched - can_migrate_llc_task() only checks that the
> +	 * pointer is non-NULL before reading grp->footprint and
> +	 * grp->nr_running_avg - so a reachable group must already be
> +	 * fully initialized.
> +	 */
> +	mm->sched_cache_grp = grp;

If it is a publish it needs to be store-release.

> +	return 0;
> +}
> +
> +void mm_destroy_sched(struct mm_struct *mm)
> +{
> +	if (mm->sched_cache_grp)
> +		sched_cache_group_put(mm->sched_cache_grp);
> +	mm->sched_cache_grp = NULL;
>  }
>  
>  /* because why would C be fully specified */
> @@ -1738,7 +1763,7 @@ static int get_pref_llc(struct task_struct *p, struct mm_struct *mm)
>  	if (!mm)
>  		return -1;
>  
> -	mm_sched_cpu = READ_ONCE(mm->sc_stat.cpu);
> +	mm_sched_cpu = READ_ONCE(mm->sched_cache_grp->cpu);
>  	if (mm_sched_cpu != -1) {
>  		mm_sched_llc = llc_id(mm_sched_cpu);
>  
> @@ -1781,11 +1806,15 @@ void account_mm_sched(struct rq *rq, struct task_struct *p, s64 delta_exec)
>  	/*
>  	 * init_task, kthreads and user thread created
>  	 * by user_mode_thread() don't have mm.
> +	 *
> +	 * A kthread can temporarily adopt an mm via kthread_use_mm(),
> +	 * so p->mm alone does not imply a user task.

This seems like a related but distinct fix, no?

>  	 */
> -	if (!mm || !mm->sc_stat.pcpu_sched)
> +	if (!mm || p->flags & PF_KTHREAD || !mm->sched_cache_grp ||
> +	    !mm->sched_cache_grp->pcpu_sched)
>  		return;

Is this susceptible to TOCTOU ?

>  
> -	pcpu_sched = per_cpu_ptr(mm->sc_stat.pcpu_sched, cpu_of(rq));
> +	pcpu_sched = per_cpu_ptr(mm->sched_cache_grp->pcpu_sched, cpu_of(rq));
>  
>  	scoped_guard (raw_spinlock, &rq->cpu_epoch_lock) {
>  		__update_mm_sched(rq, pcpu_sched);
> @@ -1798,11 +1827,11 @@ void account_mm_sched(struct rq *rq, struct task_struct *p, s64 delta_exec)
>  	 * If this process hasn't hit task_cache_work() for a while invalidate
>  	 * its preferred state.
>  	 */
> -	if ((long)(epoch - READ_ONCE(mm->sc_stat.epoch)) > llc_epoch_affinity_timeout ||
> +	if ((long)(epoch - READ_ONCE(mm->sched_cache_grp->epoch)) > llc_epoch_affinity_timeout ||
>  	    invalid_llc_nr(mm, p, cpu_of(rq)) ||
>  	    exceed_llc_capacity(mm, cpu_of(rq))) {
> -		if (READ_ONCE(mm->sc_stat.cpu) != -1)
> -			WRITE_ONCE(mm->sc_stat.cpu, -1);
> +		if (READ_ONCE(mm->sched_cache_grp->cpu) != -1)
> +			WRITE_ONCE(mm->sched_cache_grp->cpu, -1);
>  	}
>  
>  	mm_sched_llc = get_pref_llc(p, mm);

Perhaps it makes sense to have a local:

	struct sched_cache_group *scg = READ_ONCE(mm->sched_cache_grp);

because as is, the compiler is free to keep re-loading that. I mean,
dumb, but allowed. Also the local variable will shorten some of those
long expressions.


Somewhat applicable to the rest of the patch too, where it makes sense
and all that.