mm/memcg: alignment memcg_data define condition

PATCH Superseded [email protected] 4 participants 12 messages Jul 10, 2024 – Jul 15, 2024

Subsystems (3)

Memory management MM core Slab allocator

lore

Share & tools

lore

[email protected]

author

2024-07-10 05:38 UTC #0 lore raw

PATCH v3 1/2 mm/memcg: alignment memcg_data define condition

From: "Alex Shi (Tencent)" <[email protected]>

commit 21c690a349ba ("mm: introduce slabobj_ext to support slab object extensions") changed the folio/page->memcg_data define condition from MEMCG to SLAB_OBJ_EXT. And selected SLAB_OBJ_EXT for MEMCG, just for SLAB_MATCH(memcg_data, obj_exts), even no other relationship between them.

Above action make memcg_data exposed and include SLAB_OBJ_EXT for !MEMCG. That's incorrect in logcial and pay on code size.

As Vlastimil Babka suggested, let's add _unused_slab_obj_ext for SLAB_MATCH for slab.obj_exts while !MEMCG. That could resolve the match issue, clean up the feature logical. And decouple the SLAB_OBJ_EXT from MEMCG in next patch.

Signed-off-by Alex Shi (Tencent) <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Yoann Congal <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Petr Mladek <[email protected]>

Cc Suren Baghdasaryan <[email protected]>

Cc Vlastimil Babka <[email protected]>

---
v1->v3: take Vlastimil's suggestion and move SLAB_OBJ_EXT/MEMCG decouple to 2nd patch.

---
 include/linux/mm_types.h | 8 ++++++--
 mm/slab.h                | 4 ++++
 2 files changed, 10 insertions(+), 2 deletions(-)

include/linux/mm_types.h +6 -2

index ef09c4eef6d3..4ac3abc673d3 100644
@@ -180,8 +180,10 @@ struct page {
 	/* Usage count. *DO NOT USE DIRECTLY*. See page_ref.h */
 	atomic_t _refcount;
 
-#ifdef CONFIG_SLAB_OBJ_EXT
+#ifdef CONFIG_MEMCG
 	unsigned long memcg_data;
+#elif defined(CONFIG_SLAB_OBJ_EXT)
+	unsigned long _unused_slab_obj_ext;
 #endif
 
 	/*

@@ -343,8 +345,10 @@ struct folio {
 			};
 			atomic_t _mapcount;
 			atomic_t _refcount;
-#ifdef CONFIG_SLAB_OBJ_EXT
+#ifdef CONFIG_MEMCG
 			unsigned long memcg_data;
+#elif defined(CONFIG_SLAB_OBJ_EXT)
+			unsigned long _unused_slab_obj_ext;
 #endif
 #if defined(WANT_PAGE_VIRTUAL)
 			void *virtual;

mm/slab.h +4 -0

index 3586e6183224..8ffdd4f315f8 100644
@@ -98,7 +98,11 @@ SLAB_MATCH(flags, __page_flags);
 SLAB_MATCH(compound_head, slab_cache);	/* Ensure bit 0 is clear */
 SLAB_MATCH(_refcount, __page_refcount);
 #ifdef CONFIG_SLAB_OBJ_EXT
+#ifdef CONFIG_MEMCG
 SLAB_MATCH(memcg_data, obj_exts);
+#else
+SLAB_MATCH(_unused_slab_obj_ext, obj_exts);
+#endif
 #endif
 #undef SLAB_MATCH
 static_assert(sizeof(struct slab) <= sizeof(struct page));

Signature · 1 line

2.43.0

[email protected]

author

2024-07-10 05:38 UTC #1 lore raw

PATCH v3 2/2 mm/slab: decouple the SLAB_OBJ_EXT from MEMCG

From: "Alex Shi (Tencent)" <[email protected]>

commit 21c690a349ba ("mm: introduce slabobj_ext to support slab object extensions") selected SLAB_OBJ_EXT on MEMCG just for SLAB_MATCH memcg_data, that included SLAB_OBJ_EXT for MEMCG. In fact, I didn't see the necessary to enable SLAB_OBJ_EXT for MEMCG.

Let's decouple the SLAB_OBJ_EXT from MEMCG and move out alloc_slab_obj_exts() definition from SLAB_OBJ_EXT only. To alignment the alloc_slab_obj_exts() return 0 for good. change its return value to '-1' for always failed with !SLAB_OBJ_EXT. Now we could save unnecessary code from MEMCG but !SLAB_OBJ_EXT.

Signed-off-by Alex Shi (Tencent) <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Yoann Congal <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Petr Mladek <[email protected]>

---
 init/Kconfig | 1 -
 mm/slab.h    | 6 +++---
 mm/slub.c    | 6 +++---
 3 files changed, 6 insertions(+), 7 deletions(-)

init/Kconfig +0 -1

index 26bf8bb0a7ce..61e43ac9fe75 100644
@@ -965,7 +965,6 @@ config MEMCG
 	bool "Memory controller"
 	select PAGE_COUNTER
 	select EVENTFD
-	select SLAB_OBJ_EXT
 	help
 	  Provides control over the memory footprint of tasks in a cgroup.

mm/slab.h +3 -3

index 8ffdd4f315f8..6c727ecc1068 100644
@@ -559,9 +559,6 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
 	return (struct slabobj_ext *)(obj_exts & ~OBJEXTS_FLAGS_MASK);
 }
 
-int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
-                        gfp_t gfp, bool new_slab);
-
 #else /* CONFIG_SLAB_OBJ_EXT */
 
 static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)

@@ -571,6 +568,9 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
 
 #endif /* CONFIG_SLAB_OBJ_EXT */
 
+int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
+			gfp_t gfp, bool new_slab);
+
 static inline enum node_stat_item cache_vmstat_idx(struct kmem_cache *s)
 {
 	return (s->flags & SLAB_RECLAIM_ACCOUNT) ?

mm/slub.c +3 -3

index cc11f3869cc6..f531c2d67238 100644
@@ -2075,10 +2075,10 @@ alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
 
 #else /* CONFIG_SLAB_OBJ_EXT */
 
-static int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
-			       gfp_t gfp, bool new_slab)
+int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
+			gfp_t gfp, bool new_slab)
 {
-	return 0;
+	return -1;
 }
 
 static inline void free_slab_obj_exts(struct slab *slab)

Signature · 1 line

2.43.0

Alex Shi

2024-07-10 06:13 UTC #2 lore raw

Re: [REF PATCH v3 2/2] mm/slab: decouple the SLAB_OBJ_EXT from MEMCG

Quoted 7 lines · On 7/10/24 1:43 PM, [email protected] wrote:

From: "Alex Shi (Tencent)" <[email protected]>
commit 21c690a349ba ("mm: introduce slabobj_ext to support slab object extensions") selected SLAB_OBJ_EXT on MEMCG just for SLAB_MATCH memcg_data, that included SLAB_OBJ_EXT for MEMCG. In fact, I didn't see the necessary to enable SLAB_OBJ_EXT for SLAB_OBJ_EXT.

Quoted 5 lines

Let's decouple the SLAB_OBJ_EXT from MEMCG and move out alloc_slab_obj_exts() definition from SLAB_OBJ_EXT only. To alignment the alloc_slab_obj_exts() return 0 for good. change its return value to '-1' for always failed with !SLAB_OBJ_EXT. Now we could save unnecessary code from MEMCG but !SLAB_OBJ_EXT.

remove SLAB_OBJ_EXT from MEMCG, it introduce a new path in __memcg_slab_post_alloc_hook()

        for (i = 0; i < size; i++) {
                slab = virt_to_slab(p[i]);

                if (!slab_obj_exts(slab) &&

Quoted 1 line

                alloc_slab_obj_exts(slab, s, flags, false)) {

                        obj_cgroup_uncharge(objcg, obj_full_size(s));
                        continue;
                }

Here alloc_slab_obj_exts() return 0 for good, that lead to "slab_obj_exts(slab)[off].objcg = objcg;" failed on !SLAB_OBJ_EXT.

so change the return value could fix this. and its the only new path for this function.
Another usage of alloc_slab_obj_exts() in prepare_slab_obj_exts_hook() doesn't exist on !SLAB_OBJ_EXT.

Thanks
Alex

Quoted 66 lines

Signed-off-by Alex Shi (Tencent) <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Yoann Congal <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Petr Mladek <[email protected]>

---
 init/Kconfig | 1 -
 mm/slab.h    | 6 +++---
 mm/slub.c    | 6 +++---
 3 files changed, 6 insertions(+), 7 deletions(-)

init/Kconfig +0 -1

index 26bf8bb0a7ce..61e43ac9fe75 100644
@@ -965,7 +965,6 @@ config MEMCG
 	bool "Memory controller"
 	select PAGE_COUNTER
 	select EVENTFD
-	select SLAB_OBJ_EXT
 	help
 	  Provides control over the memory footprint of tasks in a cgroup.

mm/slab.h +3 -3

index 8ffdd4f315f8..6c727ecc1068 100644
@@ -559,9 +559,6 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
 	return (struct slabobj_ext *)(obj_exts & ~OBJEXTS_FLAGS_MASK);
 }
 
-int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
-                        gfp_t gfp, bool new_slab);
-
 #else /* CONFIG_SLAB_OBJ_EXT */
 
 static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)

@@ -571,6 +568,9 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
 
 #endif /* CONFIG_SLAB_OBJ_EXT */
 
+int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
+			gfp_t gfp, bool new_slab);
+
 static inline enum node_stat_item cache_vmstat_idx(struct kmem_cache *s)
 {
 	return (s->flags & SLAB_RECLAIM_ACCOUNT) ?

mm/slub.c +3 -3

index cc11f3869cc6..f531c2d67238 100644
@@ -2075,10 +2075,10 @@ alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
 
 #else /* CONFIG_SLAB_OBJ_EXT */
 
-static int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
-			       gfp_t gfp, bool new_slab)
+int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
+			gfp_t gfp, bool new_slab)
 {
-	return 0;
+	return -1;
 }
 
 static inline void free_slab_obj_exts(struct slab *slab)

Vlastimil Babka

2024-07-11 08:11 UTC #3 lore raw

Re: [REF PATCH v3 2/2] mm/slab: decouple the SLAB_OBJ_EXT from MEMCG

In reply to [email protected]

Quoted 7 lines · On 7/10/24 7:43 AM, [email protected] wrote:

From: "Alex Shi (Tencent)" <[email protected]>
commit 21c690a349ba ("mm: introduce slabobj_ext to support slab object extensions") selected SLAB_OBJ_EXT on MEMCG just for SLAB_MATCH memcg_data, that included SLAB_OBJ_EXT for MEMCG. In fact, I didn't see the necessary to enable SLAB_OBJ_EXT for MEMCG.

Quoted 7 lines

Let's decouple the SLAB_OBJ_EXT from MEMCG and move out alloc_slab_obj_exts() definition from SLAB_OBJ_EXT only. To alignment the alloc_slab_obj_exts() return 0 for good. change its return value to '-1' for always failed with !SLAB_OBJ_EXT. Now we could save unnecessary code from MEMCG but !SLAB_OBJ_EXT.
Signed-off-by Alex Shi (Tencent) <[email protected]>

This seems just wrong to me. The memcg hooks for slab do use obj_ext. You made alloc_slab_obj_exts() return -1 and that will just fail all memcg charging (unless alloc profiling selects obj_ext). The kernel will appear to work, but memcg charging for slab won't happen at all.

So no, it can't be decoupled for slab, only for pages/folios (patch 1).

Quoted 64 lines

Cc Randy Dunlap <[email protected]>

Cc Yoann Congal <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Petr Mladek <[email protected]>

---
 init/Kconfig | 1 -
 mm/slab.h    | 6 +++---
 mm/slub.c    | 6 +++---
 3 files changed, 6 insertions(+), 7 deletions(-)

init/Kconfig +0 -1

index 26bf8bb0a7ce..61e43ac9fe75 100644
@@ -965,7 +965,6 @@ config MEMCG
 	bool "Memory controller"
 	select PAGE_COUNTER
 	select EVENTFD
-	select SLAB_OBJ_EXT
 	help
 	  Provides control over the memory footprint of tasks in a cgroup.

mm/slab.h +3 -3

index 8ffdd4f315f8..6c727ecc1068 100644
@@ -559,9 +559,6 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
 	return (struct slabobj_ext *)(obj_exts & ~OBJEXTS_FLAGS_MASK);
 }
 
-int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
-                        gfp_t gfp, bool new_slab);
-
 #else /* CONFIG_SLAB_OBJ_EXT */
 
 static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)

@@ -571,6 +568,9 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
 
 #endif /* CONFIG_SLAB_OBJ_EXT */
 
+int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
+			gfp_t gfp, bool new_slab);
+
 static inline enum node_stat_item cache_vmstat_idx(struct kmem_cache *s)
 {
 	return (s->flags & SLAB_RECLAIM_ACCOUNT) ?

mm/slub.c +3 -3

index cc11f3869cc6..f531c2d67238 100644
@@ -2075,10 +2075,10 @@ alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
 
 #else /* CONFIG_SLAB_OBJ_EXT */
 
-static int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
-			       gfp_t gfp, bool new_slab)
+int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
+			gfp_t gfp, bool new_slab)
 {
-	return 0;
+	return -1;
 }
 
 static inline void free_slab_obj_exts(struct slab *slab)

Alex Shi

2024-07-11 11:49 UTC #4 lore raw

Re: [REF PATCH v3 2/2] mm/slab: decouple the SLAB_OBJ_EXT from MEMCG

Quoted 14 lines · Vlastimil Babka

Quoted 13 lines · On 7/10/24 7:43 AM, [email protected] wrote:
From: "Alex Shi (Tencent)" <[email protected]>
commit 21c690a349ba ("mm: introduce slabobj_ext to support slab object extensions") selected SLAB_OBJ_EXT on MEMCG just for SLAB_MATCH memcg_data, that included SLAB_OBJ_EXT for MEMCG. In fact, I didn't see the necessary to enable SLAB_OBJ_EXT for MEMCG.
Let's decouple the SLAB_OBJ_EXT from MEMCG and move out alloc_slab_obj_exts() definition from SLAB_OBJ_EXT only. To alignment the alloc_slab_obj_exts() return 0 for good. change its return value to '-1' for always failed with !SLAB_OBJ_EXT. Now we could save unnecessary code from MEMCG but !SLAB_OBJ_EXT.

Quoted 8 lines

Quoted 1 line
Signed-off-by Alex Shi (Tencent) <[email protected]>
This seems just wrong to me. The memcg hooks for slab do use obj_ext. You made alloc_slab_obj_exts() return -1 and that will just fail all memcg charging (unless alloc profiling selects obj_ext). The kernel will appear to work, but memcg charging for slab won't happen at all.
So no, it can't be decoupled for slab, only for pages/folios (patch 1).

Hi Vlastimil,

Thanks a lot for clarification! Yes, the patch isn't correct.

Just forgive my stupidity, why the memcg needs SLAB_OBJ_EXT?

And why we need to alloc_slab_obj_exts() at line 3019 with !slab_obj_exts?
3015 for (i = 0; i < size; i++) {
3016 slab = virt_to_slab(p[i]);
3017
3018 if (!slab_obj_exts(slab) &&
3019 alloc_slab_obj_exts(slab, s, flags, false)) {
3020 obj_cgroup_uncharge(objcg, obj_full_size(s));
3021 continue;
3022 }

Thanks!
Alex

Quoted 67 lines

Quoted 66 lines

Cc Randy Dunlap <[email protected]>

Cc Yoann Congal <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Petr Mladek <[email protected]>

---
 init/Kconfig | 1 -
 mm/slab.h    | 6 +++---
 mm/slub.c    | 6 +++---
 3 files changed, 6 insertions(+), 7 deletions(-)

init/Kconfig +0 -1

index 26bf8bb0a7ce..61e43ac9fe75 100644
@@ -965,7 +965,6 @@ config MEMCG
 	bool "Memory controller"
 	select PAGE_COUNTER
 	select EVENTFD
-	select SLAB_OBJ_EXT
 	help
 	  Provides control over the memory footprint of tasks in a cgroup.

mm/slab.h +3 -3

index 8ffdd4f315f8..6c727ecc1068 100644
@@ -559,9 +559,6 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
 	return (struct slabobj_ext *)(obj_exts & ~OBJEXTS_FLAGS_MASK);
 }
 
-int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
-                        gfp_t gfp, bool new_slab);
-
 #else /* CONFIG_SLAB_OBJ_EXT */
 
 static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)

@@ -571,6 +568,9 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
 
 #endif /* CONFIG_SLAB_OBJ_EXT */
 
+int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
+			gfp_t gfp, bool new_slab);
+
 static inline enum node_stat_item cache_vmstat_idx(struct kmem_cache *s)
 {
 	return (s->flags & SLAB_RECLAIM_ACCOUNT) ?

mm/slub.c +3 -3

index cc11f3869cc6..f531c2d67238 100644
@@ -2075,10 +2075,10 @@ alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
 
 #else /* CONFIG_SLAB_OBJ_EXT */
 
-static int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
-			       gfp_t gfp, bool new_slab)
+int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
+			gfp_t gfp, bool new_slab)
 {
-	return 0;
+	return -1;
 }
 
 static inline void free_slab_obj_exts(struct slab *slab)

Suren Baghdasaryan

reviewer

2024-07-11 13:55 UTC #5 lore raw

Re: [REF PATCH v3 2/2] mm/slab: decouple the SLAB_OBJ_EXT from MEMCG

Quoted 25 lines · Alex Shi

Quoted 21 lines · Vlastimil Babka
Quoted 7 lines · On 7/10/24 7:43 AM, [email protected] wrote:
From: "Alex Shi (Tencent)" <[email protected]>
commit 21c690a349ba ("mm: introduce slabobj_ext to support slab object extensions") selected SLAB_OBJ_EXT on MEMCG just for SLAB_MATCH memcg_data, that included SLAB_OBJ_EXT for MEMCG. In fact, I didn't see the necessary to enable SLAB_OBJ_EXT for MEMCG.
Quoted 7 lines
Let's decouple the SLAB_OBJ_EXT from MEMCG and move out alloc_slab_obj_exts() definition from SLAB_OBJ_EXT only. To alignment the alloc_slab_obj_exts() return 0 for good. change its return value to '-1' for always failed with !SLAB_OBJ_EXT. Now we could save unnecessary code from MEMCG but !SLAB_OBJ_EXT.
Signed-off-by Alex Shi (Tencent) <[email protected]>
This seems just wrong to me. The memcg hooks for slab do use obj_ext. You made alloc_slab_obj_exts() return -1 and that will just fail all memcg charging (unless alloc profiling selects obj_ext). The kernel will appear to work, but memcg charging for slab won't happen at all.

Quoted 7 lines

Quoted 1 line
So no, it can't be decoupled for slab, only for pages/folios (patch 1).
Hi Vlastimil,
Thanks a lot for clarification! Yes, the patch isn't correct.
Just forgive my stupidity, why the memcg needs SLAB_OBJ_EXT?

Because when CONFIG_MEMCG_KMEM=y, slabobj_ext contains obj_cgroup
(see: https://elixir.bootlin.com/linux/v6.10-rc7/source/include/linux/memcontrol.h#L1593)
and that's used for memcg accounting. Look into this call chain:

kfree
slab_free
memcg_slab_free_hook
__memcg_slab_free_hook
obj_cgroup_uncharge

Quoted 81 lines

Thanks!
Alex

Quoted 67 lines

Quoted 66 lines

Cc Randy Dunlap <[email protected]>

Cc Yoann Congal <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Petr Mladek <[email protected]>

---
 init/Kconfig | 1 -
 mm/slab.h    | 6 +++---
 mm/slub.c    | 6 +++---
 3 files changed, 6 insertions(+), 7 deletions(-)

init/Kconfig +0 -1

index 26bf8bb0a7ce..61e43ac9fe75 100644
@@ -965,7 +965,6 @@ config MEMCG
     bool "Memory controller"
     select PAGE_COUNTER
     select EVENTFD
-    select SLAB_OBJ_EXT
     help
       Provides control over the memory footprint of tasks in a cgroup.

mm/slab.h +3 -3

index 8ffdd4f315f8..6c727ecc1068 100644
@@ -559,9 +559,6 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
     return (struct slabobj_ext *)(obj_exts & ~OBJEXTS_FLAGS_MASK);
 }
 
-int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
-                        gfp_t gfp, bool new_slab);
-
 #else /* CONFIG_SLAB_OBJ_EXT */
 
 static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)

@@ -571,6 +568,9 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
 
 #endif /* CONFIG_SLAB_OBJ_EXT */
 
+int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
+                    gfp_t gfp, bool new_slab);
+
 static inline enum node_stat_item cache_vmstat_idx(struct kmem_cache *s)
 {
     return (s->flags & SLAB_RECLAIM_ACCOUNT) ?

mm/slub.c +3 -3

index cc11f3869cc6..f531c2d67238 100644
@@ -2075,10 +2075,10 @@ alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
 
 #else /* CONFIG_SLAB_OBJ_EXT */
 
-static int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
-                           gfp_t gfp, bool new_slab)
+int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
+                    gfp_t gfp, bool new_slab)
 {
-    return 0;
+    return -1;
 }
 
 static inline void free_slab_obj_exts(struct slab *slab)

Alex Shi

2024-07-12 04:21 UTC #6 lore raw

Re: [REF PATCH v3 2/2] mm/slab: decouple the SLAB_OBJ_EXT from MEMCG

Quoted 28 lines · Suren Baghdasaryan

Quoted 27 lines · Alex Shi
Quoted 22 lines · Vlastimil Babka
Quoted 7 lines · On 7/10/24 7:43 AM, [email protected] wrote:
From: "Alex Shi (Tencent)" <[email protected]>
commit 21c690a349ba ("mm: introduce slabobj_ext to support slab object extensions") selected SLAB_OBJ_EXT on MEMCG just for SLAB_MATCH memcg_data, that included SLAB_OBJ_EXT for MEMCG. In fact, I didn't see the necessary to enable SLAB_OBJ_EXT for MEMCG.
Quoted 7 lines
Let's decouple the SLAB_OBJ_EXT from MEMCG and move out alloc_slab_obj_exts() definition from SLAB_OBJ_EXT only. To alignment the alloc_slab_obj_exts() return 0 for good. change its return value to '-1' for always failed with !SLAB_OBJ_EXT. Now we could save unnecessary code from MEMCG but !SLAB_OBJ_EXT.
Signed-off-by Alex Shi (Tencent) <[email protected]>
This seems just wrong to me. The memcg hooks for slab do use obj_ext. You made alloc_slab_obj_exts() return -1 and that will just fail all memcg charging (unless alloc profiling selects obj_ext). The kernel will appear to work, but memcg charging for slab won't happen at all.
So no, it can't be decoupled for slab, only for pages/folios (patch 1).

Quoted 8 lines

Quoted 5 lines
Hi Vlastimil,
Thanks a lot for clarification! Yes, the patch isn't correct.
Just forgive my stupidity, why the memcg needs SLAB_OBJ_EXT?
Because when CONFIG_MEMCG_KMEM=y, slabobj_ext contains obj_cgroup
(see: https://elixir.bootlin.com/linux/v6.10-rc7/source/include/linux/memcontrol.h#L1593)

Thanks for comments.
Yes, if the obj_cg is sth we must have in MEMCG, then MEMCG should take OBJ_EXT.

Quoted 9 lines

and that's used for memcg accounting. Look into this call chain:
kfree
slab_free
memcg_slab_free_hook
__memcg_slab_free_hook
obj_cgroup_uncharge>
Quoted 2 lines
And why we need to alloc_slab_obj_exts() at line 3019 with !slab_obj_exts?

I checked the history of slab for this part. It introduced
from commit 10befea91b61c ("mm: memcg/slab: use a single set of kmem_caches for all allocations") But still don't know why !page_has_obj_cgroups followed by memcg_alloc_page_obj_cgroups. Anyone like to give a hints?

                        page = virt_to_head_page(p[i]);

+
+ if (!page_has_obj_cgroups(page) &&
+ memcg_alloc_page_obj_cgroups(page, s, flags)) {
+ obj_cgroup_uncharge(objcg, obj_full_size(s));
+ continue;
+ }

Thanks a lot
Alex

Quoted 79 lines

3015 for (i = 0; i < size; i++) {
3016 slab = virt_to_slab(p[i]);
3017
3018 if (!slab_obj_exts(slab) &&
3019 alloc_slab_obj_exts(slab, s, flags, false)) {
3020 obj_cgroup_uncharge(objcg, obj_full_size(s));
3021 continue;
3022 }

Thanks!
Alex

Quoted 67 lines

Quoted 66 lines

Cc Randy Dunlap <[email protected]>

Cc Yoann Congal <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Petr Mladek <[email protected]>

---
 init/Kconfig | 1 -
 mm/slab.h    | 6 +++---
 mm/slub.c    | 6 +++---
 3 files changed, 6 insertions(+), 7 deletions(-)

init/Kconfig +0 -1

index 26bf8bb0a7ce..61e43ac9fe75 100644
@@ -965,7 +965,6 @@ config MEMCG
     bool "Memory controller"
     select PAGE_COUNTER
     select EVENTFD
-    select SLAB_OBJ_EXT
     help
       Provides control over the memory footprint of tasks in a cgroup.

mm/slab.h +3 -3

index 8ffdd4f315f8..6c727ecc1068 100644
@@ -559,9 +559,6 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
     return (struct slabobj_ext *)(obj_exts & ~OBJEXTS_FLAGS_MASK);
 }
 
-int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
-                        gfp_t gfp, bool new_slab);
-
 #else /* CONFIG_SLAB_OBJ_EXT */
 
 static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)

@@ -571,6 +568,9 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
 
 #endif /* CONFIG_SLAB_OBJ_EXT */
 
+int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
+                    gfp_t gfp, bool new_slab);
+
 static inline enum node_stat_item cache_vmstat_idx(struct kmem_cache *s)
 {
     return (s->flags & SLAB_RECLAIM_ACCOUNT) ?

mm/slub.c +3 -3

index cc11f3869cc6..f531c2d67238 100644
@@ -2075,10 +2075,10 @@ alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
 
 #else /* CONFIG_SLAB_OBJ_EXT */
 
-static int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
-                           gfp_t gfp, bool new_slab)
+int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
+                    gfp_t gfp, bool new_slab)
 {
-    return 0;
+    return -1;
 }
 
 static inline void free_slab_obj_exts(struct slab *slab)

Vlastimil Babka

2024-07-12 07:27 UTC #7 lore raw

Re: [REF PATCH v3 2/2] mm/slab: decouple the SLAB_OBJ_EXT from MEMCG

Quoted 59 lines · Alex Shi

Quoted 28 lines · Suren Baghdasaryan
Quoted 27 lines · Alex Shi
Quoted 22 lines · Vlastimil Babka
Quoted 7 lines · On 7/10/24 7:43 AM, [email protected] wrote:
From: "Alex Shi (Tencent)" <[email protected]>
commit 21c690a349ba ("mm: introduce slabobj_ext to support slab object extensions") selected SLAB_OBJ_EXT on MEMCG just for SLAB_MATCH memcg_data, that included SLAB_OBJ_EXT for MEMCG. In fact, I didn't see the necessary to enable SLAB_OBJ_EXT for MEMCG.
Quoted 7 lines
Let's decouple the SLAB_OBJ_EXT from MEMCG and move out alloc_slab_obj_exts() definition from SLAB_OBJ_EXT only. To alignment the alloc_slab_obj_exts() return 0 for good. change its return value to '-1' for always failed with !SLAB_OBJ_EXT. Now we could save unnecessary code from MEMCG but !SLAB_OBJ_EXT.
Signed-off-by Alex Shi (Tencent) <[email protected]>
This seems just wrong to me. The memcg hooks for slab do use obj_ext. You made alloc_slab_obj_exts() return -1 and that will just fail all memcg charging (unless alloc profiling selects obj_ext). The kernel will appear to work, but memcg charging for slab won't happen at all.
So no, it can't be decoupled for slab, only for pages/folios (patch 1).
Quoted 8 lines
Quoted 5 lines
Hi Vlastimil,
Thanks a lot for clarification! Yes, the patch isn't correct.
Just forgive my stupidity, why the memcg needs SLAB_OBJ_EXT?
Because when CONFIG_MEMCG_KMEM=y, slabobj_ext contains obj_cgroup
(see: https://elixir.bootlin.com/linux/v6.10-rc7/source/include/linux/memcontrol.h#L1593)
Thanks for comments.
Yes, if the obj_cg is sth we must have in MEMCG, then MEMCG should take OBJ_EXT.
Quoted 9 lines
and that's used for memcg accounting. Look into this call chain:
kfree
slab_free
memcg_slab_free_hook
__memcg_slab_free_hook
obj_cgroup_uncharge>
Quoted 2 lines
And why we need to alloc_slab_obj_exts() at line 3019 with !slab_obj_exts?
I checked the history of slab for this part. It introduced
from commit 10befea91b61c ("mm: memcg/slab: use a single set of kmem_caches for all allocations") But still don't know why !page_has_obj_cgroups followed by memcg_alloc_page_obj_cgroups. Anyone like to give a hints?
                        page = virt_to_head_page(p[i]);

Review

Quoted 6 lines

(quoted diff) +6 -0

+
+                       if (!page_has_obj_cgroups(page) &&
+                           memcg_alloc_page_obj_cgroups(page, s, flags)) {
+                               obj_cgroup_uncharge(objcg, obj_full_size(s));
+                               continue;
+                       }

I'm not sure I understand your question. The code is trying to charge the allocation to a memcg and use the objext.memcg to associate that memcg to the object so it can be properly uncharged when freeing.
When it's the first object in the particular slab page to be charged, the objext may not be yet allocated, so it has has to be allocated at that point.

Quoted 83 lines

Thanks a lot
Alex

Quoted 79 lines

Thanks!
Alex

Quoted 67 lines

Quoted 66 lines

Cc Randy Dunlap <[email protected]>

Cc Yoann Congal <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Petr Mladek <[email protected]>

---
 init/Kconfig | 1 -
 mm/slab.h    | 6 +++---
 mm/slub.c    | 6 +++---
 3 files changed, 6 insertions(+), 7 deletions(-)

init/Kconfig +0 -1

index 26bf8bb0a7ce..61e43ac9fe75 100644
@@ -965,7 +965,6 @@ config MEMCG
     bool "Memory controller"
     select PAGE_COUNTER
     select EVENTFD
-    select SLAB_OBJ_EXT
     help
       Provides control over the memory footprint of tasks in a cgroup.

mm/slab.h +3 -3

index 8ffdd4f315f8..6c727ecc1068 100644
@@ -559,9 +559,6 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
     return (struct slabobj_ext *)(obj_exts & ~OBJEXTS_FLAGS_MASK);
 }
 
-int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
-                        gfp_t gfp, bool new_slab);
-
 #else /* CONFIG_SLAB_OBJ_EXT */
 
 static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)

@@ -571,6 +568,9 @@ static inline struct slabobj_ext *slab_obj_exts(struct slab *slab)
 
 #endif /* CONFIG_SLAB_OBJ_EXT */
 
+int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
+                    gfp_t gfp, bool new_slab);
+
 static inline enum node_stat_item cache_vmstat_idx(struct kmem_cache *s)
 {
     return (s->flags & SLAB_RECLAIM_ACCOUNT) ?

mm/slub.c +3 -3

index cc11f3869cc6..f531c2d67238 100644
@@ -2075,10 +2075,10 @@ alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
 
 #else /* CONFIG_SLAB_OBJ_EXT */
 
-static int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
-                           gfp_t gfp, bool new_slab)
+int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
+                    gfp_t gfp, bool new_slab)
 {
-    return 0;
+    return -1;
 }
 
 static inline void free_slab_obj_exts(struct slab *slab)

Alex Shi

2024-07-15 01:32 UTC #8 lore raw

Re: [REF PATCH v3 2/2] mm/slab: decouple the SLAB_OBJ_EXT from MEMCG

Quoted 12 lines · Vlastimil Babka

Review
Quoted 12 lines
I checked the history of slab for this part. It introduced
from commit 10befea91b61c ("mm: memcg/slab: use a single set of kmem_caches for all allocations") But still don't know why !page_has_obj_cgroups followed by memcg_alloc_page_obj_cgroups. Anyone like to give a hints?
                        page = virt_to_head_page(p[i]);
(quoted diff) +6 -0
+
+                       if (!page_has_obj_cgroups(page) &&
+                           memcg_alloc_page_obj_cgroups(page, s, flags)) {
+                               obj_cgroup_uncharge(objcg, obj_full_size(s));
+                               continue;
+                       }

Quoted 5 lines

I'm not sure I understand your question. The code is trying to charge the allocation to a memcg and use the objext.memcg to associate that memcg to the object so it can be properly uncharged when freeing.
When it's the first object in the particular slab page to be charged, the objext may not be yet allocated, so it has has to be allocated at that point.

I see. Thanks for explanation!

Vlastimil Babka

2024-07-11 08:13 UTC #9 lore raw

In reply to [email protected]

Quoted 63 lines · On 7/10/24 7:43 AM, [email protected] wrote:

From: "Alex Shi (Tencent)" <[email protected]>
commit 21c690a349ba ("mm: introduce slabobj_ext to support slab object extensions") changed the folio/page->memcg_data define condition from MEMCG to SLAB_OBJ_EXT. And selected SLAB_OBJ_EXT for MEMCG, just for SLAB_MATCH(memcg_data, obj_exts), even no other relationship between them.
Above action make memcg_data exposed and include SLAB_OBJ_EXT for !MEMCG. That's incorrect in logcial and pay on code size.
As Vlastimil Babka suggested, let's add _unused_slab_obj_ext for SLAB_MATCH for slab.obj_exts while !MEMCG. That could resolve the match issue, clean up the feature logical. And decouple the SLAB_OBJ_EXT from MEMCG in next patch.
Signed-off-by Alex Shi (Tencent) <[email protected]>
Cc Randy Dunlap <[email protected]>
Cc Yoann Congal <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Petr Mladek <[email protected]>
Cc Suren Baghdasaryan <[email protected]>
Cc Vlastimil Babka <[email protected]>
---
v1->v3: take Vlastimil's suggestion and move SLAB_OBJ_EXT/MEMCG decouple to 2nd patch.
---
 include/linux/mm_types.h | 8 ++++++--
 mm/slab.h                | 4 ++++
 2 files changed, 10 insertions(+), 2 deletions(-)
include/linux/mm_types.h +6 -2
index ef09c4eef6d3..4ac3abc673d3 100644
@@ -180,8 +180,10 @@ struct page {
 	/* Usage count. *DO NOT USE DIRECTLY*. See page_ref.h */
 	atomic_t _refcount;
 
-#ifdef CONFIG_SLAB_OBJ_EXT
+#ifdef CONFIG_MEMCG
 	unsigned long memcg_data;
+#elif defined(CONFIG_SLAB_OBJ_EXT)
+	unsigned long _unused_slab_obj_ext;
 #endif
 
 	/*
@@ -343,8 +345,10 @@ struct folio {
 			};
 			atomic_t _mapcount;
 			atomic_t _refcount;
-#ifdef CONFIG_SLAB_OBJ_EXT
+#ifdef CONFIG_MEMCG
 			unsigned long memcg_data;
+#elif defined(CONFIG_SLAB_OBJ_EXT)
+			unsigned long _unused_slab_obj_ext;
 #endif
 #if defined(WANT_PAGE_VIRTUAL)
 			void *virtual;
mm/slab.h +0 -0
index 3586e6183224..8ffdd4f315f8 100644

mm/slab.h SLAB_MATCH(flags, __page_flags); · line 106

Quoted 9 lines

mm/slab.h +4 -0

@@ -98,7 +98,11 @@ SLAB_MATCH(flags, __page_flags);
 SLAB_MATCH(compound_head, slab_cache);	/* Ensure bit 0 is clear */
 SLAB_MATCH(_refcount, __page_refcount);
 #ifdef CONFIG_SLAB_OBJ_EXT
+#ifdef CONFIG_MEMCG
 SLAB_MATCH(memcg_data, obj_exts);
+#else
+SLAB_MATCH(_unused_slab_obj_ext, obj_exts);
+#endif
 #endif

Why not also #ifdef / #elif like above, instead of this nesting?

Quoted 2 lines

mm/slab.h +0 -0

 #undef SLAB_MATCH
 static_assert(sizeof(struct slab) <= sizeof(struct page));

Alex Shi

2024-07-11 11:51 UTC #10 lore raw

Quoted 67 lines · Vlastimil Babka

Quoted 66 lines · On 7/10/24 7:43 AM, [email protected] wrote:
From: "Alex Shi (Tencent)" <[email protected]>
commit 21c690a349ba ("mm: introduce slabobj_ext to support slab object extensions") changed the folio/page->memcg_data define condition from MEMCG to SLAB_OBJ_EXT. And selected SLAB_OBJ_EXT for MEMCG, just for SLAB_MATCH(memcg_data, obj_exts), even no other relationship between them.
Above action make memcg_data exposed and include SLAB_OBJ_EXT for !MEMCG. That's incorrect in logcial and pay on code size.
As Vlastimil Babka suggested, let's add _unused_slab_obj_ext for SLAB_MATCH for slab.obj_exts while !MEMCG. That could resolve the match issue, clean up the feature logical. And decouple the SLAB_OBJ_EXT from MEMCG in next patch.
Signed-off-by Alex Shi (Tencent) <[email protected]>
Cc Randy Dunlap <[email protected]>
Cc Yoann Congal <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Petr Mladek <[email protected]>
Cc Suren Baghdasaryan <[email protected]>
Cc Vlastimil Babka <[email protected]>
---
v1->v3: take Vlastimil's suggestion and move SLAB_OBJ_EXT/MEMCG decouple to 2nd patch.
---
 include/linux/mm_types.h | 8 ++++++--
 mm/slab.h                | 4 ++++
 2 files changed, 10 insertions(+), 2 deletions(-)
include/linux/mm_types.h +6 -2
index ef09c4eef6d3..4ac3abc673d3 100644
@@ -180,8 +180,10 @@ struct page {
 	/* Usage count. *DO NOT USE DIRECTLY*. See page_ref.h */
 	atomic_t _refcount;
 
-#ifdef CONFIG_SLAB_OBJ_EXT
+#ifdef CONFIG_MEMCG
 	unsigned long memcg_data;
+#elif defined(CONFIG_SLAB_OBJ_EXT)
+	unsigned long _unused_slab_obj_ext;
 #endif
 
 	/*
@@ -343,8 +345,10 @@ struct folio {
 			};
 			atomic_t _mapcount;
 			atomic_t _refcount;
-#ifdef CONFIG_SLAB_OBJ_EXT
+#ifdef CONFIG_MEMCG
 			unsigned long memcg_data;
+#elif defined(CONFIG_SLAB_OBJ_EXT)
+			unsigned long _unused_slab_obj_ext;
 #endif
 #if defined(WANT_PAGE_VIRTUAL)
 			void *virtual;
mm/slab.h +0 -0
index 3586e6183224..8ffdd4f315f8 100644
@@ -98,7 +98,11 @@ SLAB_MATCH(flags, __page_flags);
 SLAB_MATCH(compound_head, slab_cache);	/* Ensure bit 0 is clear */
 SLAB_MATCH(_refcount, __page_refcount);
 #ifdef CONFIG_SLAB_OBJ_EXT

Quoted 8 lines

mm/slab.h SLAB_MATCH(flags, __page_flags); · line 106
Quoted 6 lines
mm/slab.h +4 -0
+#ifdef CONFIG_MEMCG
 SLAB_MATCH(memcg_data, obj_exts);
+#else
+SLAB_MATCH(_unused_slab_obj_ext, obj_exts);
+#endif
 #endif
Why not also #ifdef / #elif like above, instead of this nesting?

Uh, it works too if MEMCG/SLAB_OBJ_EXT decoupled.
but right, it could be written with #ifdef/#elif.

Thanks
Alex

Quoted 4 lines

Quoted 3 lines

mm/slab.h +0 -0

 #undef SLAB_MATCH
 static_assert(sizeof(struct slab) <= sizeof(struct page));

Suren Baghdasaryan

reviewer

2024-07-11 14:55 UTC #11 lore raw

Quoted 80 lines · Alex Shi

Quoted 67 lines · Vlastimil Babka
Quoted 66 lines · On 7/10/24 7:43 AM, [email protected] wrote:
From: "Alex Shi (Tencent)" <[email protected]>
commit 21c690a349ba ("mm: introduce slabobj_ext to support slab object extensions") changed the folio/page->memcg_data define condition from MEMCG to SLAB_OBJ_EXT. And selected SLAB_OBJ_EXT for MEMCG, just for SLAB_MATCH(memcg_data, obj_exts), even no other relationship between them.
Above action make memcg_data exposed and include SLAB_OBJ_EXT for !MEMCG. That's incorrect in logcial and pay on code size.
As Vlastimil Babka suggested, let's add _unused_slab_obj_ext for SLAB_MATCH for slab.obj_exts while !MEMCG. That could resolve the match issue, clean up the feature logical. And decouple the SLAB_OBJ_EXT from MEMCG in next patch.
Signed-off-by Alex Shi (Tencent) <[email protected]>
Cc Randy Dunlap <[email protected]>
Cc Yoann Congal <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Petr Mladek <[email protected]>
Cc Suren Baghdasaryan <[email protected]>
Cc Vlastimil Babka <[email protected]>
---
v1->v3: take Vlastimil's suggestion and move SLAB_OBJ_EXT/MEMCG decouple to 2nd patch.
---
 include/linux/mm_types.h | 8 ++++++--
 mm/slab.h                | 4 ++++
 2 files changed, 10 insertions(+), 2 deletions(-)
include/linux/mm_types.h +6 -2
index ef09c4eef6d3..4ac3abc673d3 100644
@@ -180,8 +180,10 @@ struct page {
     /* Usage count. *DO NOT USE DIRECTLY*. See page_ref.h */
     atomic_t _refcount;
 
-#ifdef CONFIG_SLAB_OBJ_EXT
+#ifdef CONFIG_MEMCG
     unsigned long memcg_data;
+#elif defined(CONFIG_SLAB_OBJ_EXT)
+    unsigned long _unused_slab_obj_ext;
 #endif
 
     /*
@@ -343,8 +345,10 @@ struct folio {
                     };
                     atomic_t _mapcount;
                     atomic_t _refcount;
-#ifdef CONFIG_SLAB_OBJ_EXT
+#ifdef CONFIG_MEMCG
                     unsigned long memcg_data;
+#elif defined(CONFIG_SLAB_OBJ_EXT)
+                    unsigned long _unused_slab_obj_ext;
 #endif
 #if defined(WANT_PAGE_VIRTUAL)
                     void *virtual;
mm/slab.h +0 -0
index 3586e6183224..8ffdd4f315f8 100644
@@ -98,7 +98,11 @@ SLAB_MATCH(flags, __page_flags);
 SLAB_MATCH(compound_head, slab_cache);      /* Ensure bit 0 is clear */
 SLAB_MATCH(_refcount, __page_refcount);
 #ifdef CONFIG_SLAB_OBJ_EXT
Quoted 8 lines
mm/slab.h SLAB_MATCH(flags, __page_flags); · line 106
Quoted 6 lines
mm/slab.h +4 -0
+#ifdef CONFIG_MEMCG
 SLAB_MATCH(memcg_data, obj_exts);
+#else
+SLAB_MATCH(_unused_slab_obj_ext, obj_exts);
+#endif
 #endif
Why not also #ifdef / #elif like above, instead of this nesting?

Quoted 2 lines

Uh, it works too if MEMCG/SLAB_OBJ_EXT decoupled.
but right, it could be written with #ifdef/#elif.

Yes, please keep the same condition, otherwise it gets confusing.

Quoted 7 lines

Thanks
Alex
Quoted 4 lines
Quoted 3 lines
mm/slab.h +0 -0
 #undef SLAB_MATCH
 static_assert(sizeof(struct slab) <= sizeof(struct page));