Message ID | 20250314061511.1308152-6-shakeel.butt@linux.dev (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | memcg: stock code cleanups | expand |
On 3/14/25 07:15, Shakeel Butt wrote: > obj_cgroup_release is called when all the references to the objcg has > been released i.e. no more memory objects are pointing to it. Most > probably objcg->memcg will be pointing to some ancestor memcg and at the > moment, in obj_cgroup_release, the kernel call > obj_cgroup_uncharge_pages() to uncharge last remaining memory. > > However obj_cgroup_uncharge_pages() refills the local stock. There is > no need to refill the local stock with some ancestor memcg and flush the > local stock. In addition this removes the requirement to only call > obj_cgroup_put() outside of local_lock. > > Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev> Acked-by: Vlastimil Babka <vbabka@suse.cz> > --- > mm/memcontrol.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 7054b0ebd207..83db180455a1 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -129,8 +129,7 @@ bool mem_cgroup_kmem_disabled(void) > return cgroup_memory_nokmem; > } > > -static void obj_cgroup_uncharge_pages(struct obj_cgroup *objcg, > - unsigned int nr_pages); > +static void memcg_uncharge(struct mem_cgroup *memcg, unsigned int nr_pages); > > static void obj_cgroup_release(struct percpu_ref *ref) > { > @@ -163,8 +162,16 @@ static void obj_cgroup_release(struct percpu_ref *ref) > WARN_ON_ONCE(nr_bytes & (PAGE_SIZE - 1)); > nr_pages = nr_bytes >> PAGE_SHIFT; > > - if (nr_pages) > - obj_cgroup_uncharge_pages(objcg, nr_pages); > + if (nr_pages) { > + struct mem_cgroup *memcg; > + > + memcg = get_mem_cgroup_from_objcg(objcg); > + mod_memcg_state(memcg, MEMCG_KMEM, -nr_pages); > + memcg1_account_kmem(memcg, -nr_pages); > + if (!mem_cgroup_is_root(memcg)) > + memcg_uncharge(memcg, nr_pages); > + css_put(&memcg->css); > + } > > spin_lock_irqsave(&objcg_lock, flags); > list_del(&objcg->list);
On 2025-03-13 23:15:06 [-0700], Shakeel Butt wrote: > obj_cgroup_release is called when all the references to the objcg has "references to the objcg have" > been released i.e. no more memory objects are pointing to it. Most > probably objcg->memcg will be pointing to some ancestor memcg and at the > moment, in obj_cgroup_release, the kernel call > obj_cgroup_uncharge_pages() to uncharge last remaining memory. This sounds somehow funny. I think the point is to uncharge the pages without tampering memcg_stock because it is unnecessary. > However obj_cgroup_uncharge_pages() refills the local stock. There is > no need to refill the local stock with some ancestor memcg and flush the > local stock. In addition this removes the requirement to only call > obj_cgroup_put() outside of local_lock. > > Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev> Sebastian
On Fri, Mar 14, 2025 at 12:26:27PM +0100, Sebastian Andrzej Siewior wrote: > On 2025-03-13 23:15:06 [-0700], Shakeel Butt wrote: > > obj_cgroup_release is called when all the references to the objcg has > > "references to the objcg have" > > > been released i.e. no more memory objects are pointing to it. Most > > probably objcg->memcg will be pointing to some ancestor memcg and at the > > moment, in obj_cgroup_release, the kernel call > > obj_cgroup_uncharge_pages() to uncharge last remaining memory. > > This sounds somehow funny. I think the point is to uncharge the pages > without tampering memcg_stock because it is unnecessary. > Thanks, I will see to make the point more clean in the next version.
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 7054b0ebd207..83db180455a1 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -129,8 +129,7 @@ bool mem_cgroup_kmem_disabled(void) return cgroup_memory_nokmem; } -static void obj_cgroup_uncharge_pages(struct obj_cgroup *objcg, - unsigned int nr_pages); +static void memcg_uncharge(struct mem_cgroup *memcg, unsigned int nr_pages); static void obj_cgroup_release(struct percpu_ref *ref) { @@ -163,8 +162,16 @@ static void obj_cgroup_release(struct percpu_ref *ref) WARN_ON_ONCE(nr_bytes & (PAGE_SIZE - 1)); nr_pages = nr_bytes >> PAGE_SHIFT; - if (nr_pages) - obj_cgroup_uncharge_pages(objcg, nr_pages); + if (nr_pages) { + struct mem_cgroup *memcg; + + memcg = get_mem_cgroup_from_objcg(objcg); + mod_memcg_state(memcg, MEMCG_KMEM, -nr_pages); + memcg1_account_kmem(memcg, -nr_pages); + if (!mem_cgroup_is_root(memcg)) + memcg_uncharge(memcg, nr_pages); + css_put(&memcg->css); + } spin_lock_irqsave(&objcg_lock, flags); list_del(&objcg->list);
obj_cgroup_release is called when all the references to the objcg has been released i.e. no more memory objects are pointing to it. Most probably objcg->memcg will be pointing to some ancestor memcg and at the moment, in obj_cgroup_release, the kernel call obj_cgroup_uncharge_pages() to uncharge last remaining memory. However obj_cgroup_uncharge_pages() refills the local stock. There is no need to refill the local stock with some ancestor memcg and flush the local stock. In addition this removes the requirement to only call obj_cgroup_put() outside of local_lock. Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev> --- mm/memcontrol.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)