From: Richard Palethorpe <rpalethorpe@suse.de> To: "Michal Koutný" <mkoutny@suse.com> Cc: Shakeel Butt <shakeelb@google.com>, Linux MM <linux-mm@kvack.org>, LKML <linux-kernel@vger.kernel.org>, LTP List <ltp@lists.linux.it>, Roman Gushchin <guro@fb.com>, Johannes Weiner <hannes@cmpxchg.org>, Andrew Morton <akpm@linux-foundation.org>, Christoph Lameter <cl@linux.com>, Michal Hocko <mhocko@kernel.org>, Tejun Heo <tj@kernel.org>, Vlastimil Babka <vbabka@suse.cz> Subject: Re: [PATCH v3] mm: memcg/slab: Stop reparented obj_cgroups from charging root Date: Thu, 22 Oct 2020 08:04:58 +0100 [thread overview] Message-ID: <87sga6vizp.fsf@suse.de> (raw) In-Reply-To: <20201020172402.GD46039@blackbook> Hello, Michal Koutný <mkoutny@suse.com> writes: > Hi. > > On Tue, Oct 20, 2020 at 06:52:08AM +0100, Richard Palethorpe <rpalethorpe@suse.de> wrote: >> I don't think that is relevant as we get the memcg from objcg->memcg >> which is set during reparenting. I suppose however, we can determine if >> the objcg was reparented by inspecting memcg->objcg. > +1 > >> If we just check use_hierarchy then objects directly charged to the >> memcg where use_hierarchy=0 will not be uncharged. However, maybe it is >> better to check if it was reparented and if use_hierarchy=0. > I think (I had to make a table) the yielded condition would be: > > if ((memcg->use_hierarchy && reparented) || (!mem_cgroup_is_root(memcg) && !reparented)) This looks correct, but I don't think we need to check for reparenting after all. We have the following unique scenarious: use_hierarchy=1, memcg=?, reparented=?: Because use_hierarchy=1 any descendants will have charged the current memcg, including root, so we can simply uncharge regardless of the memcg or objcg. use_hierarchy=0, memcg!=root, reparented=?: When use_hierarchy=0, objcgs are *only* reparented to root, so if we are not root the objcg must belong to us. use_hierarchy=0, memcg=root, reparented=?: We must have been reparented because root is not initialised with an objcg, but use_hierarchy=0 so we can not uncharge. So I believe that the following is sufficient. if (memcg->use_hierarchy || !mem_cgroup_is_root(memcg)) > __memcg_kmem_uncharge(memcg, nr_pages); > > (I admit it's not very readable.) > > > Michal For the record, I did create the following patch which checks for reparenting, but it appears to be unecessary. ---- diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 6877c765b8d0..0285f760f003 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -257,6 +257,14 @@ struct cgroup_subsys_state *vmpressure_to_css(struct vmpressure *vmpr) #ifdef CONFIG_MEMCG_KMEM extern spinlock_t css_set_lock; +/* Assumes objcg originated from a descendant of memcg or is memcg's */ +static bool obj_cgroup_did_charge(const struct obj_cgroup *objcg, + const struct mem_cgroup *memcg) +{ + return memcg->use_hierarchy || + rcu_access_pointer(memcg->objcg) == objcg; +} + static void obj_cgroup_release(struct percpu_ref *ref) { struct obj_cgroup *objcg = container_of(ref, struct obj_cgroup, refcnt); @@ -291,7 +299,7 @@ static void obj_cgroup_release(struct percpu_ref *ref) spin_lock_irqsave(&css_set_lock, flags); memcg = obj_cgroup_memcg(objcg); - if (nr_pages) + if (nr_pages && obj_cgroup_did_charge(objcg, memcg)) __memcg_kmem_uncharge(memcg, nr_pages); list_del(&objcg->list); mem_cgroup_put(memcg); @@ -3100,6 +3108,7 @@ static bool consume_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes) static void drain_obj_stock(struct memcg_stock_pcp *stock) { struct obj_cgroup *old = stock->cached_objcg; + struct mem_cgroup *memcg; if (!old) return; @@ -3110,7 +3119,9 @@ static void drain_obj_stock(struct memcg_stock_pcp *stock) if (nr_pages) { rcu_read_lock(); - __memcg_kmem_uncharge(obj_cgroup_memcg(old), nr_pages); + memcg = obj_cgroup_memcg(old); + if (obj_cgroup_did_charge(old, memcg)) + __memcg_kmem_uncharge(memcg, nr_pages); rcu_read_unlock(); } -- Thank you, Richard.
WARNING: multiple messages have this Message-ID (diff)
From: Richard Palethorpe <rpalethorpe@suse.de> To: ltp@lists.linux.it Subject: [LTP] [PATCH v3] mm: memcg/slab: Stop reparented obj_cgroups from charging root Date: Thu, 22 Oct 2020 08:04:58 +0100 [thread overview] Message-ID: <87sga6vizp.fsf@suse.de> (raw) In-Reply-To: <20201020172402.GD46039@blackbook> Hello, Michal Koutn? <mkoutny@suse.com> writes: > Hi. > > On Tue, Oct 20, 2020 at 06:52:08AM +0100, Richard Palethorpe <rpalethorpe@suse.de> wrote: >> I don't think that is relevant as we get the memcg from objcg->memcg >> which is set during reparenting. I suppose however, we can determine if >> the objcg was reparented by inspecting memcg->objcg. > +1 > >> If we just check use_hierarchy then objects directly charged to the >> memcg where use_hierarchy=0 will not be uncharged. However, maybe it is >> better to check if it was reparented and if use_hierarchy=0. > I think (I had to make a table) the yielded condition would be: > > if ((memcg->use_hierarchy && reparented) || (!mem_cgroup_is_root(memcg) && !reparented)) This looks correct, but I don't think we need to check for reparenting after all. We have the following unique scenarious: use_hierarchy=1, memcg=?, reparented=?: Because use_hierarchy=1 any descendants will have charged the current memcg, including root, so we can simply uncharge regardless of the memcg or objcg. use_hierarchy=0, memcg!=root, reparented=?: When use_hierarchy=0, objcgs are *only* reparented to root, so if we are not root the objcg must belong to us. use_hierarchy=0, memcg=root, reparented=?: We must have been reparented because root is not initialised with an objcg, but use_hierarchy=0 so we can not uncharge. So I believe that the following is sufficient. if (memcg->use_hierarchy || !mem_cgroup_is_root(memcg)) > __memcg_kmem_uncharge(memcg, nr_pages); > > (I admit it's not very readable.) > > > Michal For the record, I did create the following patch which checks for reparenting, but it appears to be unecessary. ---- diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 6877c765b8d0..0285f760f003 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -257,6 +257,14 @@ struct cgroup_subsys_state *vmpressure_to_css(struct vmpressure *vmpr) #ifdef CONFIG_MEMCG_KMEM extern spinlock_t css_set_lock; +/* Assumes objcg originated from a descendant of memcg or is memcg's */ +static bool obj_cgroup_did_charge(const struct obj_cgroup *objcg, + const struct mem_cgroup *memcg) +{ + return memcg->use_hierarchy || + rcu_access_pointer(memcg->objcg) == objcg; +} + static void obj_cgroup_release(struct percpu_ref *ref) { struct obj_cgroup *objcg = container_of(ref, struct obj_cgroup, refcnt); @@ -291,7 +299,7 @@ static void obj_cgroup_release(struct percpu_ref *ref) spin_lock_irqsave(&css_set_lock, flags); memcg = obj_cgroup_memcg(objcg); - if (nr_pages) + if (nr_pages && obj_cgroup_did_charge(objcg, memcg)) __memcg_kmem_uncharge(memcg, nr_pages); list_del(&objcg->list); mem_cgroup_put(memcg); @@ -3100,6 +3108,7 @@ static bool consume_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes) static void drain_obj_stock(struct memcg_stock_pcp *stock) { struct obj_cgroup *old = stock->cached_objcg; + struct mem_cgroup *memcg; if (!old) return; @@ -3110,7 +3119,9 @@ static void drain_obj_stock(struct memcg_stock_pcp *stock) if (nr_pages) { rcu_read_lock(); - __memcg_kmem_uncharge(obj_cgroup_memcg(old), nr_pages); + memcg = obj_cgroup_memcg(old); + if (obj_cgroup_did_charge(old, memcg)) + __memcg_kmem_uncharge(memcg, nr_pages); rcu_read_unlock(); } -- Thank you, Richard.
next prev parent reply other threads:[~2020-10-22 7:05 UTC|newest] Thread overview: 96+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-14 19:07 [RFC PATCH] mm: memcg/slab: Stop reparented obj_cgroups from charging root Richard Palethorpe 2020-10-14 19:07 ` [LTP] " Richard Palethorpe 2020-10-14 20:08 ` Roman Gushchin 2020-10-14 20:08 ` [LTP] " Roman Gushchin 2020-10-16 5:40 ` Richard Palethorpe 2020-10-16 5:40 ` [LTP] " Richard Palethorpe 2020-10-16 6:32 ` [LTP] [PATCH v2] " Richard Palethorpe 2020-10-16 9:47 ` [RFC PATCH] " Michal Koutný 2020-10-16 9:47 ` [LTP] " Michal =?unknown-8bit?q?Koutn=C3=BD?= 2020-10-16 10:41 ` Richard Palethorpe 2020-10-16 10:41 ` [LTP] " Richard Palethorpe 2020-10-16 15:05 ` Richard Palethorpe 2020-10-16 15:05 ` [LTP] " Richard Palethorpe 2020-10-16 17:26 ` Michal Koutný 2020-10-16 17:26 ` [LTP] " Michal =?unknown-8bit?q?Koutn=C3=BD?= 2020-10-16 14:53 ` Johannes Weiner 2020-10-16 14:53 ` [LTP] " Johannes Weiner 2020-10-16 17:02 ` Roman Gushchin 2020-10-16 17:02 ` [LTP] " Roman Gushchin 2020-10-16 17:15 ` Michal Koutný 2020-10-16 17:15 ` [LTP] " Michal =?unknown-8bit?q?Koutn=C3=BD?= 2020-10-19 8:45 ` Richard Palethorpe 2020-10-19 8:45 ` [LTP] " Richard Palethorpe 2020-10-19 9:58 ` [PATCH v3] " Richard Palethorpe 2020-10-19 9:58 ` [LTP] " Richard Palethorpe 2020-10-19 16:58 ` Shakeel Butt 2020-10-19 16:58 ` [LTP] " Shakeel Butt 2020-10-19 16:58 ` Shakeel Butt 2020-10-20 5:52 ` Richard Palethorpe 2020-10-20 5:52 ` [LTP] " Richard Palethorpe 2020-10-20 13:49 ` Richard Palethorpe 2020-10-20 13:49 ` [LTP] " Richard Palethorpe 2020-10-20 16:56 ` Shakeel Butt 2020-10-20 16:56 ` [LTP] " Shakeel Butt 2020-10-20 16:56 ` Shakeel Butt 2020-10-21 20:32 ` Roman Gushchin 2020-10-21 20:32 ` [LTP] " Roman Gushchin 2020-10-20 17:24 ` Michal Koutný 2020-10-20 17:24 ` [LTP] " Michal =?unknown-8bit?q?Koutn=C3=BD?= 2020-10-22 7:04 ` Richard Palethorpe [this message] 2020-10-22 7:04 ` Richard Palethorpe 2020-10-22 12:28 ` [PATCH v4] " Richard Palethorpe 2020-10-22 12:28 ` [LTP] " Richard Palethorpe 2020-10-22 16:37 ` Shakeel Butt 2020-10-22 16:37 ` [LTP] " Shakeel Butt 2020-10-22 16:37 ` Shakeel Butt 2020-10-22 17:25 ` Roman Gushchin 2020-10-22 17:25 ` [LTP] " Roman Gushchin 2020-10-22 23:59 ` Shakeel Butt 2020-10-22 23:59 ` [LTP] " Shakeel Butt 2020-10-22 23:59 ` Shakeel Butt 2020-10-23 0:40 ` Roman Gushchin 2020-10-23 0:40 ` [LTP] " Roman Gushchin 2020-10-23 15:44 ` Johannes Weiner 2020-10-23 15:44 ` [LTP] " Johannes Weiner 2020-10-23 16:41 ` Shakeel Butt 2020-10-23 16:41 ` [LTP] " Shakeel Butt 2020-10-23 16:41 ` Shakeel Butt 2020-10-26 7:32 ` Richard Palethorpe 2020-10-26 7:32 ` [LTP] " Richard Palethorpe 2020-10-26 23:14 ` Roman Gushchin 2020-10-26 23:14 ` [LTP] " Roman Gushchin 2020-10-19 22:28 ` [RFC PATCH] " Roman Gushchin 2020-10-19 22:28 ` [LTP] " Roman Gushchin 2020-10-20 6:04 ` Richard Palethorpe 2020-10-20 6:04 ` [LTP] " Richard Palethorpe 2020-10-20 12:02 ` Richard Palethorpe 2020-10-20 12:02 ` [LTP] " Richard Palethorpe 2020-10-20 14:48 ` Richard Palethorpe 2020-10-20 14:48 ` [LTP] " Richard Palethorpe 2020-10-20 16:27 ` Michal Koutný 2020-10-20 16:27 ` [LTP] " Michal =?unknown-8bit?q?Koutn=C3=BD?= 2020-10-20 17:07 ` Roman Gushchin 2020-10-20 17:07 ` [LTP] " Roman Gushchin 2020-10-20 18:18 ` Johannes Weiner 2020-10-20 18:18 ` [LTP] " Johannes Weiner 2020-10-21 19:33 ` Roman Gushchin 2020-10-21 19:33 ` [LTP] " Roman Gushchin 2020-10-23 16:30 ` Johannes Weiner 2020-10-23 16:30 ` [LTP] " Johannes Weiner 2020-11-10 1:27 ` Roman Gushchin 2020-11-10 1:27 ` [LTP] " Roman Gushchin 2020-11-10 15:11 ` Shakeel Butt 2020-11-10 15:11 ` [LTP] " Shakeel Butt 2020-11-10 19:13 ` Roman Gushchin 2020-11-10 19:13 ` [LTP] " Roman Gushchin 2020-11-20 17:46 ` Michal Koutný 2020-11-20 17:46 ` [LTP] " Michal =?unknown-8bit?q?Koutn=C3=BD?= 2020-11-03 13:22 ` Michal Hocko 2020-11-03 13:22 ` [LTP] " Michal Hocko 2020-11-03 21:30 ` Roman Gushchin 2020-11-03 21:30 ` [LTP] " Roman Gushchin 2020-10-20 16:55 ` Shakeel Butt 2020-10-20 16:55 ` [LTP] " Shakeel Butt 2020-10-20 17:17 ` Roman Gushchin 2020-10-20 17:17 ` [LTP] " Roman Gushchin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87sga6vizp.fsf@suse.de \ --to=rpalethorpe@suse.de \ --cc=akpm@linux-foundation.org \ --cc=cl@linux.com \ --cc=guro@fb.com \ --cc=hannes@cmpxchg.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=ltp@lists.linux.it \ --cc=mhocko@kernel.org \ --cc=mkoutny@suse.com \ --cc=shakeelb@google.com \ --cc=tj@kernel.org \ --cc=vbabka@suse.cz \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.