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

[Chen Yu]

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