diff mbox series

[RFC,05/10] memcg: no refilling stock from obj_cgroup_release

Message ID 20250314061511.1308152-6-shakeel.butt@linux.dev (mailing list archive)
State New
Headers show
Series memcg: stock code cleanups | expand

Commit Message

Shakeel Butt March 14, 2025, 6:15 a.m. UTC
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(-)

Comments

Vlastimil Babka March 14, 2025, 10:09 a.m. UTC | #1
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);
Sebastian Andrzej Siewior March 14, 2025, 11:26 a.m. UTC | #2
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
Shakeel Butt March 14, 2025, 3:25 p.m. UTC | #3
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 mbox series

Patch

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);