Re: [PATCH] memcg: optimize memcg_rstat_updated

[Waiman Long <llong@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2159 bytes --]

On 4/9/25 7:49 PM, Shakeel Butt wrote:
> Currently the kernel maintains the stats updates per-memcg which is
> needed to implement stats flushing threshold. On the update side, the
> update is added to the per-cpu per-memcg update of the given memcg and
> all of its ancestors. However when the given memcg has passed the
> flushing threshold, all of its ancestors should have passed the
> threshold as well. There is no need to traverse up the memcg tree to
> maintain the stats updates.
>
> Perf profile collected from our fleet shows that memcg_rstat_updated is
> one of the most expensive memcg function i.e. a lot of cumulative CPU
> is being spent on it. So, even small micro optimizations matter a lot.
>
> Signed-off-by: Shakeel Butt<shakeel.butt@linux.dev>
> ---
>   mm/memcontrol.c | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 421740f1bcdc..ea3e40e589df 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -585,18 +585,20 @@ static inline void memcg_rstat_updated(struct mem_cgroup *memcg, int val)
>   	cgroup_rstat_updated(memcg->css.cgroup, cpu);
>   	statc = this_cpu_ptr(memcg->vmstats_percpu);
>   	for (; statc; statc = statc->parent) {
> +		/*
> +		 * If @memcg is already flushable then all its ancestors are
> +		 * flushable as well and also there is no need to increase
> +		 * stats_updates.
> +		 */
> +		if (!memcg_vmstats_needs_flush(statc->vmstats))
> +			break;
> +

Do you mean "if (memcg_vmstats_needs_flush(statc->vmstats))"?

Cheers, Longman

>   		stats_updates = READ_ONCE(statc->stats_updates) + abs(val);
>   		WRITE_ONCE(statc->stats_updates, stats_updates);
>   		if (stats_updates < MEMCG_CHARGE_BATCH)
>   			continue;
>   
> -		/*
> -		 * If @memcg is already flush-able, increasing stats_updates is
> -		 * redundant. Avoid the overhead of the atomic update.
> -		 */
> -		if (!memcg_vmstats_needs_flush(statc->vmstats))
> -			atomic64_add(stats_updates,
> -				     &statc->vmstats->stats_updates);
> +		atomic64_add(stats_updates, &statc->vmstats->stats_updates);
>   		WRITE_ONCE(statc->stats_updates, 0);
>   	}
>   }

[-- Attachment #2: Type: text/html, Size: 2796 bytes --]