| Message ID | 20201026231326.3212225-1-guro@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mm: memcg: link page counters to root if use_hierarchy is false | expand |
Hi. On Mon, Oct 26, 2020 at 04:13:26PM -0700, Roman Gushchin <guro@fb.com> wrote: > Please note, that in the non-hierarchical mode all objcgs are always > reparented to the root memory cgroup, even if the hierarchy has more > than 1 level. This patch doesn't change it. > > The patch also doesn't affect how the hierarchical mode is working, > which is the only sane and truly supported mode now. I agree with the patch and you can add Reviewed-by: Michal Koutný <mkoutny@suse.com> However, it effectively switches any users of root.use_hierarchy=0 (if there are any, watching the counters of root memcg) into root.use_hierarchy=1. So I'd show them the warning even with a single level of cgroups, i.e. add this hunk @@ -5356,12 +5356,11 @@ page_counter_init(&memcg->kmem, &root_mem_cgroup->kmem); page_counter_init(&memcg->tcpmem, &root_mem_cgroup->tcpmem); /* - * Deeper hierachy with use_hierarchy == false doesn't make + * Hierachy with use_hierarchy == false doesn't make * much sense so let cgroup subsystem know about this * unfortunate state in our controller. */ - if (parent != root_mem_cgroup) - memory_cgrp_subsys.broken_hierarchy = true; + memory_cgrp_subsys.broken_hierarchy = true; } /* The following stuff does not apply to the root */ What do you think? Michal
On Thu, Oct 29, 2020 at 04:39:21PM +0100, Michal Koutny wrote: > Hi. > > On Mon, Oct 26, 2020 at 04:13:26PM -0700, Roman Gushchin <guro@fb.com> wrote: > > Please note, that in the non-hierarchical mode all objcgs are always > > reparented to the root memory cgroup, even if the hierarchy has more > > than 1 level. This patch doesn't change it. > > > > The patch also doesn't affect how the hierarchical mode is working, > > which is the only sane and truly supported mode now. > I agree with the patch and you can add > Reviewed-by: Michal Koutný <mkoutny@suse.com> > > However, it effectively switches any users of root.use_hierarchy=0 (if there > are any, watching the counters of root memcg) into root.use_hierarchy=1. > So I'd show them the warning even with a single level of cgroups, i.e. > add this hunk It's only partially true. The main difference between the hierarchical and non-hierarchical mode on the following simple example / | A / \ B C is whether A's memory limits are applied to B, and this is not gonna change. However you're right, it will change some root cgroup's numbers. > > @@ -5356,12 +5356,11 @@ > page_counter_init(&memcg->kmem, &root_mem_cgroup->kmem); > page_counter_init(&memcg->tcpmem, &root_mem_cgroup->tcpmem); > /* > - * Deeper hierachy with use_hierarchy == false doesn't make > + * Hierachy with use_hierarchy == false doesn't make > * much sense so let cgroup subsystem know about this > * unfortunate state in our controller. > */ > - if (parent != root_mem_cgroup) > - memory_cgrp_subsys.broken_hierarchy = true; > + memory_cgrp_subsys.broken_hierarchy = true; > } > > /* The following stuff does not apply to the root */ > > What do you think? I think it's in a good direction of deprecating the non-hierarchical mode. Shakeel did propose it too. I'd also change the displayed message to something similar to we print for kmem.limit_in_bytes: pr_warn_once("kmem.limit_in_bytes is deprecated and will be removed. " "Please report your usecase to linux-mm@kvack.org if you " "depend on this functionality.\n"); Thanks!
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 2636f8bad908..009297017c87 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -5339,17 +5339,22 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css) memcg->swappiness = mem_cgroup_swappiness(parent); memcg->oom_kill_disable = parent->oom_kill_disable; } - if (parent && parent->use_hierarchy) { + if (!parent) { + page_counter_init(&memcg->memory, NULL); + page_counter_init(&memcg->swap, NULL); + page_counter_init(&memcg->kmem, NULL); + page_counter_init(&memcg->tcpmem, NULL); + } else if (parent->use_hierarchy) { memcg->use_hierarchy = true; page_counter_init(&memcg->memory, &parent->memory); page_counter_init(&memcg->swap, &parent->swap); page_counter_init(&memcg->kmem, &parent->kmem); page_counter_init(&memcg->tcpmem, &parent->tcpmem); } else { - page_counter_init(&memcg->memory, NULL); - page_counter_init(&memcg->swap, NULL); - page_counter_init(&memcg->kmem, NULL); - page_counter_init(&memcg->tcpmem, NULL); + page_counter_init(&memcg->memory, &root_mem_cgroup->memory); + page_counter_init(&memcg->swap, &root_mem_cgroup->swap); + page_counter_init(&memcg->kmem, &root_mem_cgroup->kmem); + page_counter_init(&memcg->tcpmem, &root_mem_cgroup->tcpmem); /* * Deeper hierachy with use_hierarchy == false doesn't make * much sense so let cgroup subsystem know about this
Richard reported a warning which can be reproduced by running the LTP madvise6 test (cgroup v1 in the non-hierarchical mode should be used): [ 9.841552] ------------[ cut here ]------------ [ 9.841788] WARNING: CPU: 0 PID: 12 at mm/page_counter.c:57 page_counter_uncharge (mm/page_counter.c:57 mm/page_counter.c:50 mm/page_counter.c:156) [ 9.841982] Modules linked in: [ 9.842072] CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.9.0-rc7-22-default #77 [ 9.842266] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-48-gd9c812d-rebuilt.opensuse.org 04/01/2014 [ 9.842571] Workqueue: events drain_local_stock [ 9.842750] RIP: 0010:page_counter_uncharge (mm/page_counter.c:57 mm/page_counter.c:50 mm/page_counter.c:156) [ 9.842894] Code: 0f c1 45 00 4c 29 e0 48 89 ef 48 89 c3 48 89 c6 e8 2a fe ff ff 48 85 db 78 10 48 8b 6d 28 48 85 ed 75 d8 5b 5d 41 5c 41 5d c3 <0f> 0b eb ec 90 e8 4b f9 88 2a 48 8b 17 48 39 d6 72 41 41 54 49 89 [ 9.843438] RSP: 0018:ffffb1c18006be28 EFLAGS: 00010086 [ 9.843585] RAX: ffffffffffffffff RBX: ffffffffffffffff RCX: ffff94803bc2cae0 [ 9.843806] RDX: 0000000000000001 RSI: ffffffffffffffff RDI: ffff948007d2b248 [ 9.844026] RBP: ffff948007d2b248 R08: ffff948007c58eb0 R09: ffff948007da05ac [ 9.844248] R10: 0000000000000018 R11: 0000000000000018 R12: 0000000000000001 [ 9.844477] R13: ffffffffffffffff R14: 0000000000000000 R15: ffff94803bc2cac0 [ 9.844696] FS: 0000000000000000(0000) GS:ffff94803bc00000(0000) knlGS:0000000000000000 [ 9.844915] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 9.845096] CR2: 00007f0579ee0384 CR3: 000000002cc0a000 CR4: 00000000000006f0 [ 9.845319] Call Trace: [ 9.845429] __memcg_kmem_uncharge (mm/memcontrol.c:3022) [ 9.845582] drain_obj_stock (./include/linux/rcupdate.h:689 mm/memcontrol.c:3114) [ 9.845684] drain_local_stock (mm/memcontrol.c:2255) [ 9.845789] process_one_work (./arch/x86/include/asm/jump_label.h:25 ./include/linux/jump_label.h:200 ./include/trace/events/workqueue.h:108 kernel/workqueue.c:2274) [ 9.845898] worker_thread (./include/linux/list.h:282 kernel/workqueue.c:2416) [ 9.846034] ? process_one_work (kernel/workqueue.c:2358) [ 9.846162] kthread (kernel/kthread.c:292) [ 9.846271] ? __kthread_bind_mask (kernel/kthread.c:245) [ 9.846420] ret_from_fork (arch/x86/entry/entry_64.S:300) [ 9.846531] ---[ end trace 8b5647c1eba9d18a ]--- The problem occurs because in the non-hierarchical mode non-root page counters are not linked to root page counters, so the charge is not propagated to the root memory cgroup. After the removal of the original memory cgroup and reparenting of the object cgroup, the root cgroup might be uncharged by draining a objcg stock, for example. It leads to an eventual underflow of the charge and triggers a warning. Fix it by linking all page counters to corresponding root page counters in the non-hierarchical mode. Please note, that in the non-hierarchical mode all objcgs are always reparented to the root memory cgroup, even if the hierarchy has more than 1 level. This patch doesn't change it. The patch also doesn't affect how the hierarchical mode is working, which is the only sane and truly supported mode now. Thanks to Richard for reporting, debugging and providing an alternative version of the fix! Reported-by: ltp@lists.linux.it Debugged-by: Richard Palethorpe <rpalethorpe@suse.com> Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API") Signed-off-by: Roman Gushchin <guro@fb.com> Reviewed-by: Shakeel Butt <shakeelb@google.com> Acked-by: Johannes Weiner <hannes@cmpxchg.org> Cc: stable@vger.kernel.org --- mm/memcontrol.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)