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

[Jemmy Wong <jemmywong512@gmail.com>]

Hi Chenyu,

I've updated the patch to use atomic_long_t for grp->footprint, with
atomic_long_add() for placement deltas and atomic_long_sub() when a
task leaves the group through exec or exit.

The reason for removing the clamp is that the footprint aggregates the
same contributions tracked by p->total_numa_faults. NUMA placement
already applies each signed memory-fault delta to the per-node counters
and the task total without clamping. A negative delta only removes half
of an existing bucket, so normal decay cannot make the task's
contribution negative.

With atomic updates preventing lost deltas, and each departing task's
contribution subtracted exactly once, the group footprint should remain
nonnegative too, assuming no overflow. Clamping would hide an accounting
mismatch rather than correct it. The non-returning atomic add/sub
operations also need no ordering against unrelated accesses.

I've kept your suggestion to update the footprint once per placement
pass. Instead of maintaining a separate delta accumulator, the patch
snapshots p->total_numa_faults before the loop and applies its net change
afterward. This reuses the existing NUMA accounting and limits shared
cache-line traffic.

There is one exception to the balance argument: if exec fails after
switching groups but before task_numa_free() resets the statistics, exit
can subtract the old total from the new group, where it was never
accounted. That can make the new footprint negative. This group belongs
only to the execing task, which is past the point of no return and will
exit, so the mismatch does not persist in a surviving shared group.
This version leaves the existing NUMA reset ordering unchanged.

Does using atomic_long_t without the clamp look reasonable to you,
including handling that failed-exec case as described?



diff --git a/include/linux/sched.h b/include/linux/sched.h
index d35ae49a991f..4a02a47c988e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2412,7 +2412,8 @@ struct sched_cache_group {
 	unsigned long epoch;
 	u64 nr_running_avg;
 	unsigned long next_scan;
-	unsigned long footprint;
+	/* Sum of the member tasks' accounted footprints, in pages. */
+	atomic_long_t footprint;
 	int cpu;
 	refcount_t refcnt;
 	struct rcu_head rcu;
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 57360f5cdde4..c9aa59148abd 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1483,6 +1483,7 @@ static bool exceed_llc_capacity(struct sched_cache_group *grp, int cpu)
 #ifdef CONFIG_NUMA_BALANCING
 	unsigned long llc, footprint;
 	struct sched_domain *sd;
+	long fp;
 	int scale;

 	guard(rcu)();
@@ -1497,7 +1498,9 @@ static bool exceed_llc_capacity(struct sched_cache_group *grp, int cpu)
 		 * excluded.
 		 */
 		llc = sd->llc_bytes;
-		footprint = READ_ONCE(grp->footprint);
+		fp = atomic_long_read(&grp->footprint);
+		WARN_ON_ONCE(fp < 0);
+		footprint = max(fp, 0L);

 		/*
 		 * Scale the LLC size by 256*llc_aggr_tolerance
@@ -1677,7 +1680,7 @@ int mm_init_sched(struct mm_struct *mm,
 	grp->cpu = -1;
 	grp->next_scan = jiffies;
 	grp->nr_running_avg = 0;
-	grp->footprint = 0;
+	atomic_long_set(&grp->footprint, 0);
 	refcount_set(&grp->refcnt, 1);
 	/*
 	 * The update to grp->pcpu_sched should not be reordered
@@ -1719,12 +1722,24 @@ DEFINE_FREE(sched_cache_group_put, struct sched_cache_group *,
 #define rcu_deref_sched_cache_grp(tsk) \
 	rcu_dereference_check((tsk)->sched_cache_grp, (tsk) == current)

+static inline void sched_cache_footprint_update(struct task_struct *p, long delta)
+{
+	struct sched_cache_group *grp = rcu_deref_sched_cache_grp(p);
+
+	if (grp && delta)
+		atomic_long_add(delta, &grp->footprint);
+}
+
 static struct sched_cache_group *sched_cache_replace_grp(struct task_struct *p,
 							 struct sched_cache_group *new)
 {
 	struct sched_cache_group *old;

 	old = rcu_deref_sched_cache_grp(p);
+#ifdef CONFIG_NUMA_BALANCING
+	if (old && old != new)
+		atomic_long_sub(p->total_numa_faults, &old->footprint);
+#endif
 	rcu_assign_pointer(p->sched_cache_grp, new);

 	return old;
@@ -1787,19 +1802,6 @@ void sched_cache_exit_mm(struct task_struct *p)
 {
 	struct sched_cache_group *grp = sched_cache_replace_grp(p, NULL);

-#ifdef CONFIG_NUMA_BALANCING
-	/*
-	 * Subtract this task's footprint from the group before dropping the
-	 * 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);
-	}
-#endif
 	sched_cache_group_put(grp);
 }

@@ -2189,6 +2191,9 @@ void init_sched_mm(struct task_struct *p)
 static inline void account_mm_sched(struct rq *rq, struct task_struct *p,
 				    s64 delta_exec) { }

+static inline void sched_cache_footprint_update(struct task_struct *p,
+						long delta) { }
+
 void init_sched_mm(struct task_struct *p) { }

 static void task_tick_cache(struct rq *rq, struct task_struct *p) { }
@@ -3883,14 +3888,13 @@ static int preferred_group_nid(struct task_struct *p, int nid)
 static void task_numa_placement(struct task_struct *p)
 	__context_unsafe(/* conditional locking */)
 {
-	struct sched_cache_group __maybe_unused *grp;
 	int seq, nid, max_nid = NUMA_NO_NODE;
 	unsigned long max_faults = 0;
 	unsigned long fault_types[2] = { 0, 0 };
+	unsigned long old_faults = p->total_numa_faults;
 	unsigned long total_faults;
 	u64 runtime, period;
 	spinlock_t *group_lock = NULL;
-	long __maybe_unused new_fp;
 	struct numa_group *ng;

 	/*
@@ -3965,36 +3969,6 @@ static void task_numa_placement(struct task_struct *p)
 				ng->total_faults += diff;
 				group_faults += ng->faults[mem_idx];
 			}
-#ifdef CONFIG_SCHED_CACHE
-			/*
-			 * Per task p->numa_faults[mem_idx] converges,
-			 * so the accumulation of each task's faults
-			 * converges too - Given the number of threads,
-			 * it cannot overflow an unsigned long.
-			 * Racy with concurrent updates from other threads
-			 * sharing this mm. Acceptable since footprint is a
-			 * heuristic and occasional lost updates are tolerable.
-			 *
-			 * If a task exits, its corresponding footprint must
-			 * be subtracted from p->sched_cache_grp->footprint,
-			 * otherwise the footprint will not converge: the
-			 * exiting thread's footprint remains unchanged/undecayed.
-			 * See exit_mm().
-			 *
-			 * Lost updates and unsynchronized subtraction
-			 * in exit_mm() can cause footprint + diff to
-			 * 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));
-				}
-			}
-#endif
 		}

 		if (!ng) {
@@ -4017,6 +3991,8 @@ static void task_numa_placement(struct task_struct *p)
 		max_nid = preferred_group_nid(p, max_nid);
 	}

+	sched_cache_footprint_update(p, p->total_numa_faults - old_faults);
+
 	if (max_faults) {
 		/* Set the new preferred node */
 		if (max_nid != p->numa_preferred_nid)
--
2.25.1


> 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).
> 
> 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):
> 
> 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);
> }
> 
> +#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
> +
> 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);
> + }
> + }
> +#endif
> +
> /* Cannot migrate task to CPU-less node */
> max_nid = numa_nearest_node(max_nid, N_CPU);
> 
> -- 
> 2.25.1
> 
> 
>