Re: [PATCH v2] sched/cache: Remove the old cache group footprint on exec

[Jemmy Wong <jemmywong512@gmail.com>]

Hi Chenyu,

Thanks for the review and for spotting the race.

> On Oct 10, 2026, at 11:04 PM, Chen Yu <yu.c.chen@intel.com> wrote:
> 
> On Sat, Oct 10, 2026 at 07:16:12PM +0800, Jemmy Wong wrote:
> 
> [ ... ]
> 
>> void sched_cache_exec_mmap(struct task_struct *p, struct mm_struct *mm)
>> {
>> struct sched_cache_group *old;
>> @@ -1778,8 +1797,13 @@ void sched_cache_exec_mmap(struct task_struct *p, struct mm_struct *mm)
>> /*
>> * Acquire the new reference before publishing the pointer, then drop
>> * the old one. @p is current and the only writer of its own pointer.
>> + *
>> + * Footprint subtraction must stay ahead of task_numa_free() in
>> + * bprm_execve(), which clears p->total_numa_faults. Nothing here
>> + * enforces that order.
>> */
>> old = sched_cache_replace_grp(p, sched_cache_group_get(mm->sched_cache_grp));
>> + sched_cache_footprint_sub(old, p);
> 
> 
> Yes, it fixes the stale footprint issue for a vfork(). While looking at this fix I
> realize that maybe even with this patch applied, we might still can not fix the losing
> footprint, due to an existing race condition:
> 
>  CPU0: execve()                    CPU1: CLONE_VM task
>  ------------------------------    -------------------------------
>                                    task_numa_placement()
>                                      read footprint
>  sched_cache_exec_mmap()
>    read footprint
>    write footprint - own faults    <-- the fix
>                                      write footprint + own diff
>                                      <-- overwrites the subtraction
> 
> So the footprint might still be lost because CPU1 overwrites it, and
> there seems to be no lock protected against the race(I did not find one).

Right, and there is no lock. grp->footprint is only ever written by plain
READ_ONCE()/WRITE_ONCE() in task_numa_placement(), exit and (with v2) exec.

> An enhanced way is to turn the raw footprint write into cmpxchg.
> It reduces the race, and since numa balancing scan interval 
> numa_scan_period is not small, the overhead of using
> atomic write is low, I'm thinking of something below(only compile tested):

This looks right to me, including applying the placement diff once.
I've taken it into my local tree with a few tweaks, explained
inline below. Feel free to push back on any of them.

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 57360f5cdde4..4123f0535cd9 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -1771,6 +1771,11 @@ void sched_cache_fork_cleanup(struct task_struct *p)
> RCU_INIT_POINTER(p->sched_cache_grp, NULL);
> }
> 
> +#ifdef CONFIG_NUMA_BALANCING
> +static void sched_cache_footprint_update(struct sched_cache_group *grp,
> + unsigned long delta, bool add);
> +#endif
> +
> void sched_cache_exec_mmap(struct task_struct *p, struct mm_struct *mm)
> {
> struct sched_cache_group *old;
> @@ -1780,9 +1785,28 @@ void sched_cache_exec_mmap(struct task_struct *p, struct mm_struct *mm)
> * the old one. @p is current and the only writer of its own pointer.
> */
> old = sched_cache_replace_grp(p, sched_cache_group_get(mm->sched_cache_grp));
> +#ifdef CONFIG_NUMA_BALANCING
> + if (old && p->total_numa_faults)
> + sched_cache_footprint_update(old, p->total_numa_faults, false);
> +#endif
> sched_cache_group_put(old);
> }

I folded it in without the forward declaration by ordering the helpers ahead
of the callers. The condition lives in sched_cache_footprint_drop(), with an
empty stub when CONFIG_NUMA_BALANCING is off, so exec and exit share one call:

	old = sched_cache_replace_grp(p, sched_cache_group_get(mm->sched_cache_grp));
	sched_cache_footprint_drop(old, p);
	sched_cache_group_put(old);

Moving the check into the helper costs no call: I confirmed in the
generated code that sched_cache_footprint_drop() is inlined into both
exec and exit, so the early-out test still sits in the caller.

The "must stay ahead of task_numa_free()" note moved onto that helper.
Nothing enforces the order; sched_cache_exec_mmap() just remains before
task_numa_free(current, false) in bprm_execve().

> +#ifdef CONFIG_NUMA_BALANCING
> +static void sched_cache_footprint_update(struct sched_cache_group *grp,
> + unsigned long delta, bool add)
> +{
> + unsigned long old = READ_ONCE(grp->footprint), new;
> +
> + do {
> + if (add)
> + new = old + delta;
> + else
> + new = old - min(old, delta);
> + } while (!try_cmpxchg(&grp->footprint, &old, new));
> +}
> +#endif

#ifdef CONFIG_NUMA_BALANCING
static void sched_cache_footprint_update(struct sched_cache_group *grp, long diff)
{
	unsigned long old = READ_ONCE(grp->footprint), new;

	do {
		if (diff < 0)
			new = old - min(old, -(unsigned long)diff);
		else
			new = old + diff;
	} while (!try_cmpxchg_relaxed(&grp->footprint, &old, new));
}

static void sched_cache_footprint_drop(struct sched_cache_group *grp,
				     struct task_struct *p)
{
	if (grp && p->total_numa_faults)
		sched_cache_footprint_update(grp, -(long)p->total_numa_faults);
}
#else
static inline void sched_cache_footprint_drop(struct sched_cache_group *grp,
					    struct task_struct *p) { }
#endif

Two changes in that helper:
- try_cmpxchg_relaxed() rather than try_cmpxchg().
  try_cmpxchg() is fully ordered on success; on arm64 with LSE that
  is casal. Its release half keeps the CAS from being observed before
  any earlier access from this CPU, and its acquire half keeps later
  accesses from being observed before the CAS. In exec_mmap() those
  earlier accesses include e.g. the tsk->mm and tsk->active_mm stores
  just above, which need no ordering against grp->footprint.

- Signed @diff rather than (delta, bool add), so the sign is not split
  across an abs()/">0" pair at the call site. Subtraction still clamps,
  with the negate done in unsigned arithmetic to keep the clamp in one
  type.

One question about the clamp: shouldn't atomic updates balance each
task's placement diffs against its exit/exec subtraction? Assuming no
overflow, old < delta would suggest an accounting bug that clamping
hides. Is there a legitimate path where the contributions do not balance?

If not, we could use atomic_long_t and atomic_long_add(diff,
&grp->footprint) instead of the CAS loop, provided the footprint fits
in a signed long. atomic_long_add() is already unordered; readers would
use atomic_long_read().

> void sched_cache_exit_mm(struct task_struct *p)
> {
> struct sched_cache_group *grp = sched_cache_replace_grp(p, NULL);
> @@ -1793,12 +1817,8 @@ void sched_cache_exit_mm(struct task_struct *p)
> * reference, so the group footprint converges as its threads exit.
> * Unlocked for performance; clamp to avoid underflow.
> */
> - if (grp && p->total_numa_faults) {
> - unsigned long fp = READ_ONCE(grp->footprint);
> - unsigned long sub = min(fp, p->total_numa_faults);
> -
> - WRITE_ONCE(grp->footprint, fp - sub);
> - }
> + if (grp && p->total_numa_faults)
> + sched_cache_footprint_update(grp, p->total_numa_faults, false);
> #endif
> sched_cache_group_put(grp);
> }
> @@ -3890,7 +3910,7 @@ static void task_numa_placement(struct task_struct *p)
> unsigned long total_faults;
> u64 runtime, period;
> spinlock_t *group_lock = NULL;
> - long __maybe_unused new_fp;
> + long __maybe_unused fp_diff = 0;
> struct numa_group *ng;
> 
> /*
> @@ -3986,14 +4006,8 @@ static void task_numa_placement(struct task_struct *p)
> * go negative. Clamp to zero to prevent the
> * unsigned footprint from wrapping.
> */
> - scoped_guard(rcu) {
> - grp = rcu_dereference(p->sched_cache_grp);
> -
> - if (grp) {
> - new_fp = (long)READ_ONCE(grp->footprint) + diff;
> - WRITE_ONCE(grp->footprint, max(new_fp, 0L));
> - }
> - }
> + /* Update grp->footprint once, to limit the c2c cost. */
> + fp_diff += diff;
> #endif
> }
> 
> @@ -4008,6 +4022,18 @@ static void task_numa_placement(struct task_struct *p)
> }
> }
> 
> +#ifdef CONFIG_SCHED_CACHE
> + if (fp_diff) {
> + scoped_guard(rcu) {
> + grp = rcu_dereference(p->sched_cache_grp);
> +
> + if (grp)
> + sched_cache_footprint_update(grp, abs(fp_diff),
> +     fp_diff > 0);
> + }

grp is declared inside the guard, and the signed diff is passed through:

	if (fp_diff) {
		scoped_guard(rcu) {
			struct sched_cache_group *grp;

			grp = rcu_dereference(p->sched_cache_grp);
			if (grp)
				sched_cache_footprint_update(grp, fp_diff);
		}
	}

> + }
> +#endif
> +
> /* Cannot migrate task to CPU-less node */
> max_nid = numa_nearest_node(max_nid, N_CPU);
> 
> -- 
> 2.25.1

Thanks,
Jemmy