Every memcg lruvec (per-node) stat update reads the memcg and lruvec_stats_percpu pointers in the first 64 bytes of struct mem_cgroup_per_node, and the ones that go through mod_lruvec_state() (such as lruvec_stat_mod_folio()) also read lruvec->pgdat. These updates are extremely frequent (eg these updates happen several times for every page that's faulted in, freed, dirtied, or written back). The memcg_pn_read_mostly cacheline group keeps those read-mostly fields on a different cacheline from the lruvec, but on CPUs that do spatial prefetching [1] (which is most Intel CPUs / modern CPUs), cachelines are fetched in 128-byte aligned pairs. Depending on where struct mem_cgroup_per_node lands in its slab, that cacheline can be paired with the cacheline holding the LRU list heads, and the lruvec->pgdat cacheline with the one holding lru_lock. When this happens, every stat update also pulls in a copy of the LRU list heads and lru_lock cachelines, which are written to constantly by other CPUs under the lru_lock. The stat update has to fetch them from another CPU, which is unnecessary overhead since the stat update doesn't use the LRU list heads or lru_lock at all. But even worse, the lru_lock holder then has to invalidate those copies and wait for the acknowledgements to come back before it can do its next write, which makes the lru_lock critical section longer for everyone waiting on the lock. This was found when analyzing the output of perf c2c for a benchmark where many threads fault in pages in one memcg. It showed the first 64 bytes of mem_cgroup_per_node as the most contended cacheline (over a third of all HITMs and 5x more than the lru_lock cacheline) even though there are no writes to it. A quick experiment adding extra padding so that the cacheline no longer shared a pairing with the LRU list heads showed that this removed most of the contention, which confirmed the cause. Fix this by not touching struct mem_cgroup_per_node on the main stat update paths and instead going through the memcg. The memcg keeps an array of pointers to the per-cpu lruvec stats of each node, pointed to from the same cacheline as vmstats_percpu, which the update path already reads. lruvec_stat_mod_folio() and mod_lruvec_kmem_state() pass the memcg and node they already know instead of going through the lruvec and fetching mem_cgroup_per_node->memcg and lruvec->pgdat. Both the memcg and lruvec_stats_percpu reads need to go, since either one alone still pulls in the cacheline. Results from a multi-threaded fault/free benchmark in a single memcg, run on an Intel Skylake machine (80 threads, 2 NUMA nodes) with threads pinned to CPUs showed average pages faulted in (and freed) per second across all threads, over 7 freshly created memcgs per run: before after shmem, 20 threads 4.74M 5.68M (+20%) shmem, 80 threads 7.94M 8.81M (+11%) anon, 20 threads 3.04M 3.13M (+3%) anon, 80 threads 3.23M 3.30M (+2%) perf c2c showed HITMs on the read-mostly line from the stat update paths dropped from ~2350 to ~10. [1] https://www.intel.com/content/dam/doc/manual/64-ia-32-architectures-optimization-manual.pdf (bottom of page 62/800) Link: https://lore.kernel.org/linux-fsdevel/CAJnrk1ZezmmzrYdf8KP6re_a76izMS=qGPeC8ZZXiteHyZZC-Q@mail.gmail.com/ Signed-off-by: Joanne Koong --- An alternative would be to turn memcg->nodeinfo[] into an array of {mem_cgroup_per_node, stats} pointer pairs, which saves one load, but this would require quite a bit of churn (would need to modify every nodeinfo call site). Happy to switch to this for v2 if this is preferred. include/linux/memcontrol.h | 8 +++- mm/memcontrol.c | 88 +++++++++++++++++--------------------- 2 files changed, 47 insertions(+), 49 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 74110a324f9e..dd054c9076d7 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -88,7 +88,6 @@ struct mem_cgroup_per_node { struct mem_cgroup *memcg; /* Back pointer, we cannot */ /* use container_of */ - struct lruvec_stats_percpu __percpu *lruvec_stats_percpu; struct lruvec_stats *lruvec_stats; struct shrinker_info __rcu *shrinker_info; struct obj_cgroup __rcu *objcg; @@ -314,6 +313,13 @@ struct mem_cgroup { __cacheline_group_begin_aligned(memcg_read_mostly); /* Read on every stat update */ struct memcg_vmstats_percpu __percpu *vmstats_percpu; + /* + * Per-CPU lruvec stats of each node, indexed by node id. Points to an + * array after nodeinfo[] in the same allocation. Kept here instead of + * in mem_cgroup_per_node, to avoid sharing a cacheline pair with the + * very frequently written LRU lists. + */ + struct lruvec_stats_percpu __percpu **lruvec_stats_percpu; /* memory.stat */ struct memcg_vmstats *vmstats; diff --git a/mm/memcontrol.c b/mm/memcontrol.c index aad0498a7bd6..203cae528e08 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,9 @@ 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; int i = memcg_stats_index(idx); int cpu; @@ -1006,7 +1000,7 @@ static void __mod_memcg_lruvec_state(struct mem_cgroup_per_node *pn, this_cpu_add(memcg->vmstats_percpu->state[i], val); /* Update lruvec */ - this_cpu_add(pn->lruvec_stats_percpu->state[i], val); + this_cpu_add(memcg->lruvec_stats_percpu[nid]->state[i], val); val = memcg_state_val_in_pages(idx, val); memcg_rstat_updated(memcg, val, cpu); @@ -1015,21 +1009,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 +1054,14 @@ 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, + * 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(); } @@ -4192,7 +4179,6 @@ static void free_mem_cgroup_per_node_info(struct mem_cgroup_per_node *pn) if (!pn) return; - free_percpu(pn->lruvec_stats_percpu); kfree(pn->lruvec_stats); kfree(pn); } @@ -4211,9 +4197,9 @@ static bool alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node) if (!pn->lruvec_stats) goto fail; - pn->lruvec_stats_percpu = alloc_percpu_gfp(struct lruvec_stats_percpu, - GFP_KERNEL_ACCOUNT); - if (!pn->lruvec_stats_percpu) + memcg->lruvec_stats_percpu[node] = + alloc_percpu_gfp(struct lruvec_stats_percpu, GFP_KERNEL_ACCOUNT); + if (!memcg->lruvec_stats_percpu[node]) goto fail; INIT_LIST_HEAD(&pn->objcg_list); @@ -4234,6 +4220,8 @@ static void __mem_cgroup_free(struct mem_cgroup *memcg) for_each_node(node) { struct mem_cgroup_per_node *pn = memcg->nodeinfo[node]; + + free_percpu(memcg->lruvec_stats_percpu[node]); if (!pn) continue; @@ -4265,6 +4253,8 @@ static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent) memcg = kmem_cache_zalloc(memcg_cachep, GFP_KERNEL); if (!memcg) return ERR_PTR(-ENOMEM); + /* see mem_cgroup_init() */ + memcg->lruvec_stats_percpu = (void *)&memcg->nodeinfo[nr_node_ids]; error = xa_alloc(&mem_cgroup_private_ids, &memcg->private_id, NULL, XA_LIMIT(1, MEM_CGROUP_ID_MAX), GFP_KERNEL); @@ -4709,7 +4699,7 @@ static void mem_cgroup_css_rstat_flush(struct cgroup_subsys_state *css, int cpu) if (parent) plstats = parent->nodeinfo[nid]->lruvec_stats; - lstatc = per_cpu_ptr(pn->lruvec_stats_percpu, cpu); + lstatc = per_cpu_ptr(memcg->lruvec_stats_percpu[nid], cpu); ac = (struct aggregate_control) { .aggregate = lstats->state, @@ -5901,6 +5891,8 @@ static void __init memcg_struct_check(void) CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_read_mostly, vmstats_percpu); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_read_mostly, + lruvec_stats_percpu); CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_read_mostly, vmstats); #ifdef CONFIG_ZSWAP @@ -5920,8 +5912,6 @@ static void __init memcg_struct_check(void) CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup_per_node, memcg_pn_read_mostly, memcg); - CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup_per_node, - memcg_pn_read_mostly, lruvec_stats_percpu); CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup_per_node, memcg_pn_read_mostly, lruvec_stats); CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup_per_node, @@ -5977,7 +5967,9 @@ int __init mem_cgroup_init(void) drain_local_obj_stock); } - memcg_size = struct_size_t(struct mem_cgroup, nodeinfo, nr_node_ids); + /* nodeinfo[] followed by the array of per-node lruvec_stats_percpu */ + memcg_size = struct_size_t(struct mem_cgroup, nodeinfo, nr_node_ids) + + nr_node_ids * sizeof(struct lruvec_stats_percpu __percpu *); memcg_cachep = kmem_cache_create("mem_cgroup", memcg_size, 0, SLAB_PANIC | SLAB_HWCACHE_ALIGN, NULL); -- 2.52.0