Re: [PATCH v3 2/2] memcg: don't go through the lruvec for folio and kmem lruvec stat updates

[Michal Hocko <mhocko@suse.com>]

On Fri 23-10-26 12:54:17, Joanne Koong wrote:
> lruvec_stat_mod_folio() and mod_lruvec_kmem_state() already know the memcg
> and the node, but look up the lruvec with mem_cgroup_lruvec() and update
> the stats through mod_lruvec_state(), which reads lruvec->pgdat and
> mem_cgroup_per_node->memcg to get them back. lruvec->pgdat shares a
> 128-byte cacheline pair with lines that the lru_lock holder writes
> constantly (lru_lock or lru_zone_size[], depending on the alignment of
> struct mem_cgroup_per_node), so on CPUs that do spatial prefetching
> these per-folio stat updates contend with the lru_lock holder.
> 
> Pass the memcg and node they already know to mod_memcg_node_state()
> instead.
> 
> On top of the mem_cgroup_per_node rearrangement, on a 2-socket Skylake
> (80 CPUs) with threads faulting in and freeing shmem in a single memcg,
> this improves throughput by about 3% at 20 threads (5.59M -> 5.77M
> pages/sec) and 2% at 80 threads (8.66M -> 8.82M).
> 
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>

Acked-by: Michal Hocko <mhocko@suse.com>
Thanks!

> ---
>  mm/memcontrol.c | 71 ++++++++++++++++++++-----------------------------
>  1 file changed, 29 insertions(+), 42 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index a05d9a8baa1b..9f33cc5c8293 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -587,7 +587,7 @@ unsigned long lruvec_page_state_local(const struct lruvec *lruvec,
>  	return x;
>  }
>  
> -static void __mod_memcg_lruvec_state(struct mem_cgroup_per_node *pn,
> +static void __mod_memcg_lruvec_state(struct mem_cgroup *memcg, int nid,
>  				     enum node_stat_item idx, long val);
>  
>  void reparent_memcg_lruvec_state_local(struct mem_cgroup *memcg,
> @@ -596,16 +596,11 @@ void reparent_memcg_lruvec_state_local(struct mem_cgroup *memcg,
>  	int nid;
>  
>  	for_each_node(nid) {
> -		struct lruvec *child_lruvec = mem_cgroup_lruvec(memcg, NODE_DATA(nid));
> -		struct lruvec *parent_lruvec = mem_cgroup_lruvec(parent, NODE_DATA(nid));
> -		unsigned long value = lruvec_page_state_local(child_lruvec, idx);
> -		struct mem_cgroup_per_node *child_pn, *parent_pn;
> +		struct lruvec *lruvec = mem_cgroup_lruvec(memcg, NODE_DATA(nid));
> +		unsigned long value = lruvec_page_state_local(lruvec, idx);
>  
> -		child_pn = container_of(child_lruvec, struct mem_cgroup_per_node, lruvec);
> -		parent_pn = container_of(parent_lruvec, struct mem_cgroup_per_node, lruvec);
> -
> -		__mod_memcg_lruvec_state(child_pn, idx, -value);
> -		__mod_memcg_lruvec_state(parent_pn, idx, value);
> +		__mod_memcg_lruvec_state(memcg, nid, idx, -value);
> +		__mod_memcg_lruvec_state(parent, nid, idx, value);
>  	}
>  }
>  
> @@ -990,10 +985,10 @@ void reparent_memcg_state_local(struct mem_cgroup *memcg,
>  }
>  #endif
>  
> -static void __mod_memcg_lruvec_state(struct mem_cgroup_per_node *pn,
> +static void __mod_memcg_lruvec_state(struct mem_cgroup *memcg, int nid,
>  				     enum node_stat_item idx, long val)
>  {
> -	struct mem_cgroup *memcg = pn->memcg;
> +	struct mem_cgroup_per_node *pn = memcg->nodeinfo[nid];
>  	int i = memcg_stats_index(idx);
>  	int cpu;
>  
> @@ -1015,21 +1010,23 @@ static void __mod_memcg_lruvec_state(struct mem_cgroup_per_node *pn,
>  	put_cpu();
>  }
>  
> +static void mod_memcg_node_state(struct mem_cgroup *memcg, int nid,
> +				 enum node_stat_item idx, int val)
> +{
> +	bool rcu_locked = false;
> +
> +	memcg = get_non_dying_memcg_start(memcg, &rcu_locked);
> +	__mod_memcg_lruvec_state(memcg, nid, idx, val);
> +	get_non_dying_memcg_end(rcu_locked);
> +}
> +
>  void mod_memcg_lruvec_state(struct lruvec *lruvec,
>  			    enum node_stat_item idx, int val)
>  {
> -	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>  	struct mem_cgroup_per_node *pn;
> -	struct mem_cgroup *memcg;
> -	bool rcu_locked = false;
>  
>  	pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
> -	memcg = get_non_dying_memcg_start(pn->memcg, &rcu_locked);
> -	pn = memcg->nodeinfo[pgdat->node_id];
> -
> -	__mod_memcg_lruvec_state(pn, idx, val);
> -
> -	get_non_dying_memcg_end(rcu_locked);
> +	mod_memcg_node_state(pn->memcg, lruvec_pgdat(lruvec)->node_id, idx, val);
>  }
>  
>  /**
> @@ -1058,19 +1055,13 @@ void lruvec_stat_mod_folio(struct folio *folio, enum node_stat_item idx,
>  {
>  	struct mem_cgroup *memcg;
>  	pg_data_t *pgdat = folio_pgdat(folio);
> -	struct lruvec *lruvec;
> +
> +	mod_node_page_state(pgdat, idx, val);
>  
>  	rcu_read_lock();
>  	memcg = folio_memcg(folio);
> -	/* Untracked pages have no memcg, no lruvec. Update only the node */
> -	if (!memcg) {
> -		rcu_read_unlock();
> -		mod_node_page_state(pgdat, idx, val);
> -		return;
> -	}
> -
> -	lruvec = mem_cgroup_lruvec(memcg, pgdat);
> -	mod_lruvec_state(lruvec, idx, val);
> +	if (memcg)
> +		mod_memcg_node_state(memcg, pgdat->node_id, idx, val);
>  	rcu_read_unlock();
>  }
>  EXPORT_SYMBOL(lruvec_stat_mod_folio);
> @@ -1079,23 +1070,19 @@ void mod_lruvec_kmem_state(void *p, enum node_stat_item idx, int val)
>  {
>  	pg_data_t *pgdat = page_pgdat(virt_to_page(p));
>  	struct mem_cgroup *memcg;
> -	struct lruvec *lruvec;
> +
> +	mod_node_page_state(pgdat, idx, val);
>  
>  	rcu_read_lock();
>  	memcg = mem_cgroup_from_virt(p);
>  
>  	/*
> -	 * Untracked pages have no memcg, no lruvec. Update only the
> -	 * node. If we reparent the slab objects to the root memcg,
> -	 * when we free the slab object, we need to update the per-memcg
> -	 * vmstats to keep it correct for the root memcg.
> +	 * If we reparent the slab objects to the root memcg, when we free the
> +	 * slab object, we need to update the per-memcg vmstats to keep it
> +	 * correct for the root memcg.
>  	 */
> -	if (!memcg) {
> -		mod_node_page_state(pgdat, idx, val);
> -	} else {
> -		lruvec = mem_cgroup_lruvec(memcg, pgdat);
> -		mod_lruvec_state(lruvec, idx, val);
> -	}
> +	if (memcg)
> +		mod_memcg_node_state(memcg, pgdat->node_id, idx, val);
>  	rcu_read_unlock();
>  }
>  
> -- 
> 2.52.0

-- 
Michal Hocko
SUSE Labs