lkml.org 
[lkml]   [2019]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] mm: memcg/slab: fix percpu slab vmstats flushing
On Thu 19-12-19 20:27:28, Roman Gushchin wrote:
> Currently slab percpu vmstats are flushed twice: during the memcg
> offlining and just before freeing the memcg structure. Each time
> percpu counters are summed, added to the atomic counterparts and
> propagated up by the cgroup tree.
>
> The second flushing is required due to how recursive vmstats are
> implemented: counters are batched in percpu variables on a local
> level, and once a percpu value is crossing some predefined threshold,
> it spills over to atomic values on the local and each ascendant
> levels. It means that without flushing some numbers cached in percpu
> variables will be dropped on floor each time a cgroup is destroyed.
> And with uptime the error on upper levels might become noticeable.
>
> The first flushing aims to make counters on ancestor levels more
> precise. Dying cgroups may resume in the dying state for a long time.
> After kmem_cache reparenting which is performed during the offlining
> slab counters of the dying cgroup don't have any chances to be
> updated, because any slab operations will be performed on the parent
> level. It means that the inaccuracy caused by percpu batching
> will not decrease up to the final destruction of the cgroup.
> By the original idea flushing slab counters during the offlining
> should minimize the visible inaccuracy of slab counters on the parent
> level.
>
> The problem is that percpu counters are not zeroed after the first
> flushing. So every cached percpu value is summed twice. It creates
> a small error (up to 32 pages per cpu, but usually less) which
> accumulates on parent cgroup level. After creating and destroying
> of thousands of child cgroups, slab counter on parent level can
> be way off the real value.
>
> For now, let's just stop flushing slab counters on memcg offlining.
> It can't be done correctly without scheduling a work on each cpu:
> reading and zeroing it during css offlining can race with an
> asynchronous update, which doesn't expect values to be changed
> underneath.
>
> With this change, slab counters on parent level will become eventually
> consistent. Once all dying children are gone, values are correct.
> And if not, the error is capped by 32 * NR_CPUS pages per dying
> cgroup.
>
> It's not perfect, as slab are reparented, so any updates after
> the reparenting will happen on the parent level. It means that
> if a slab page was allocated, a counter on child level was bumped,
> then the page was reparented and freed, the annihilation of positive
> and negative counter values will not happen until the child cgroup is
> released. It makes slab counters different from others, and it might
> want us to implement flushing in a correct form again.
> But it's also a question of performance: scheduling a work on each
> cpu isn't free, and it's an open question if the benefit of having
> more accurate counters is worth it.
>
> We might also consider flushing all counters on offlining, not only
> slab counters.
>
> So let's fix the main problem now: make the slab counters eventually
> consistent, so at least the error won't grow with uptime (or more
> precisely the number of created and destroyed cgroups). And think
> about the accuracy of counters separately.

So this is essentially a revert, right? I have to say I was not a great
fan of bee07b33db78 in the first place.

> v2: added a note to the changelog, asked by Johannes. Thanks!
>
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Fixes: bee07b33db78 ("mm: memcontrol: flush percpu slab vmstats on kmem offlining")
> Cc: stable@vger.kernel.org
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

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

> ---
> include/linux/mmzone.h | 5 ++---
> mm/memcontrol.c | 37 +++++++++----------------------------
> 2 files changed, 11 insertions(+), 31 deletions(-)
>
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 89d8ff06c9ce..5334ad8fc7bd 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -215,9 +215,8 @@ enum node_stat_item {
> NR_INACTIVE_FILE, /* " " " " " */
> NR_ACTIVE_FILE, /* " " " " " */
> NR_UNEVICTABLE, /* " " " " " */
> - NR_SLAB_RECLAIMABLE, /* Please do not reorder this item */
> - NR_SLAB_UNRECLAIMABLE, /* and this one without looking at
> - * memcg_flush_percpu_vmstats() first. */
> + NR_SLAB_RECLAIMABLE,
> + NR_SLAB_UNRECLAIMABLE,
> NR_ISOLATED_ANON, /* Temporary isolated pages from anon lru */
> NR_ISOLATED_FILE, /* Temporary isolated pages from file lru */
> WORKINGSET_NODES,
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 601405b207fb..3165db39827a 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3287,49 +3287,34 @@ static u64 mem_cgroup_read_u64(struct cgroup_subsys_state *css,
> }
> }
>
> -static void memcg_flush_percpu_vmstats(struct mem_cgroup *memcg, bool slab_only)
> +static void memcg_flush_percpu_vmstats(struct mem_cgroup *memcg)
> {
> - unsigned long stat[MEMCG_NR_STAT];
> + unsigned long stat[MEMCG_NR_STAT] = {0};
> struct mem_cgroup *mi;
> int node, cpu, i;
> - int min_idx, max_idx;
> -
> - if (slab_only) {
> - min_idx = NR_SLAB_RECLAIMABLE;
> - max_idx = NR_SLAB_UNRECLAIMABLE;
> - } else {
> - min_idx = 0;
> - max_idx = MEMCG_NR_STAT;
> - }
> -
> - for (i = min_idx; i < max_idx; i++)
> - stat[i] = 0;
>
> for_each_online_cpu(cpu)
> - for (i = min_idx; i < max_idx; i++)
> + for (i = 0; i < MEMCG_NR_STAT; i++)
> stat[i] += per_cpu(memcg->vmstats_percpu->stat[i], cpu);
>
> for (mi = memcg; mi; mi = parent_mem_cgroup(mi))
> - for (i = min_idx; i < max_idx; i++)
> + for (i = 0; i < MEMCG_NR_STAT; i++)
> atomic_long_add(stat[i], &mi->vmstats[i]);
>
> - if (!slab_only)
> - max_idx = NR_VM_NODE_STAT_ITEMS;
> -
> for_each_node(node) {
> struct mem_cgroup_per_node *pn = memcg->nodeinfo[node];
> struct mem_cgroup_per_node *pi;
>
> - for (i = min_idx; i < max_idx; i++)
> + for (i = 0; i < NR_VM_NODE_STAT_ITEMS; i++)
> stat[i] = 0;
>
> for_each_online_cpu(cpu)
> - for (i = min_idx; i < max_idx; i++)
> + for (i = 0; i < NR_VM_NODE_STAT_ITEMS; i++)
> stat[i] += per_cpu(
> pn->lruvec_stat_cpu->count[i], cpu);
>
> for (pi = pn; pi; pi = parent_nodeinfo(pi, node))
> - for (i = min_idx; i < max_idx; i++)
> + for (i = 0; i < NR_VM_NODE_STAT_ITEMS; i++)
> atomic_long_add(stat[i], &pi->lruvec_stat[i]);
> }
> }
> @@ -3403,13 +3388,9 @@ static void memcg_offline_kmem(struct mem_cgroup *memcg)
> parent = root_mem_cgroup;
>
> /*
> - * Deactivate and reparent kmem_caches. Then flush percpu
> - * slab statistics to have precise values at the parent and
> - * all ancestor levels. It's required to keep slab stats
> - * accurate after the reparenting of kmem_caches.
> + * Deactivate and reparent kmem_caches.
> */
> memcg_deactivate_kmem_caches(memcg, parent);
> - memcg_flush_percpu_vmstats(memcg, true);
>
> kmemcg_id = memcg->kmemcg_id;
> BUG_ON(kmemcg_id < 0);
> @@ -4913,7 +4894,7 @@ static void mem_cgroup_free(struct mem_cgroup *memcg)
> * Flush percpu vmstats and vmevents to guarantee the value correctness
> * on parent's and all ancestor levels.
> */
> - memcg_flush_percpu_vmstats(memcg, false);
> + memcg_flush_percpu_vmstats(memcg);
> memcg_flush_percpu_vmevents(memcg);
> __mem_cgroup_free(memcg);
> }
> --
> 2.17.1

--
Michal Hocko
SUSE Labs

\
 
 \ /
  Last update: 2019-12-20 10:33    [W:0.044 / U:0.576 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site