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

[Chen Yu <chen.yu@linux.dev>]

Hi Jemmy,

On Wed, Oct 14, 2026 at 05:17:30PM +0800, Jemmy Wong wrote:
> 
> 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,

Yes, your proposal to use atomic_long_add() is more straighforward.

> including handling that failed-exec case as described?
>

The failed-exec case is not a big deal IMO as that over-substract value will
soon disappear after the task exit.

> 
> +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

The minor issue is that the goal of sched_cache_replace_grp() was to serve as a
helper for dealing with sched_cache_group pointer assignment, so we don't want to
interfere with its members' values, such as footprint. If you can reuse
sched_cache_footprint_update() in sched_cache_exit_mm() and sched_cache_exec_mmap(),
that would make the code cleaner for future maintenance. Do you have any reason
to keep the current form?

Could you please send a formal patch? I'll launch a test with your patch applied.
The only concern is that we convert the raw READ/WRITE_ONCE into atomic write,
which might impact performance.

thanks,
Chenyu