mm, slab: move allocation tagging code in the alloc path into a hook

PATCH Superseded Suren Baghdasaryan 2 participants 3 messages Jul 3, 2024

Subsystems (2)

Memory management Slab allocator

lore

Share & tools

lore

Suren Baghdasaryan

author

2024-07-03 01:53 UTC #0 lore raw

PATCH 1/1 mm, slab: move allocation tagging code in the alloc path into a hook

Move allocation tagging specific code in the allocation path into alloc_tagging_slab_alloc_hook, similar to how freeing path uses alloc_tagging_slab_free_hook. No functional changes, just code cleanup.

Signed-off-by Suren Baghdasaryan <[email protected]>

---
 mm/slub.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

mm/slub.c +29 -5

index 4927edec6a8c..99d53190cfcf 100644
@@ -2033,11 +2033,18 @@ prepare_slab_obj_exts_hook(struct kmem_cache *s, gfp_t flags, void *p)
 	return slab_obj_exts(slab) + obj_to_index(s, slab, p);
 }
 
+#ifdef CONFIG_MEM_ALLOC_PROFILING
+
+static inline void
+alloc_tagging_slab_alloc_hook(struct slabobj_ext *obj_exts, unsigned int size)
+{
+	alloc_tag_add(&obj_exts->ref, current->alloc_tag, size);
+}
+
 static inline void
 alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
 			     int objects)
 {
-#ifdef CONFIG_MEM_ALLOC_PROFILING
 	struct slabobj_ext *obj_exts;
 	int i;

@@ -2053,9 +2060,23 @@ alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
 
 		alloc_tag_sub(&obj_exts[off].ref, s->size);
 	}
-#endif
 }
 
+#else /* CONFIG_MEM_ALLOC_PROFILING */
+
+static inline void
+alloc_tagging_slab_alloc_hook(struct slabobj_ext *obj_exts, unsigned int size)
+{
+}
+
+static inline void
+alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
+			     int objects)
+{
+}
+
+#endif /* CONFIG_MEM_ALLOC_PROFILING*/
+
 #else /* CONFIG_SLAB_OBJ_EXT */
 
 static int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,

@@ -2079,6 +2100,11 @@ prepare_slab_obj_exts_hook(struct kmem_cache *s, gfp_t flags, void *p)
 	return NULL;
 }
 
+static inline void
+alloc_tagging_slab_alloc_hook(struct slabobj_ext *obj_exts, unsigned int size)
+{
+}
+
 static inline void
 alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
 			     int objects)

@@ -3944,7 +3970,6 @@ bool slab_post_alloc_hook(struct kmem_cache *s, struct list_lru *lru,
 		kmemleak_alloc_recursive(p[i], s->object_size, 1,
 					 s->flags, init_flags);
 		kmsan_slab_alloc(s, p[i], init_flags);
-#ifdef CONFIG_MEM_ALLOC_PROFILING
 		if (need_slab_obj_ext()) {
 			struct slabobj_ext *obj_exts;

@@ -3955,9 +3980,8 @@ bool slab_post_alloc_hook(struct kmem_cache *s, struct list_lru *lru,
 			 * check should be added before alloc_tag_add().
 			 */
 			if (likely(obj_exts))
-				alloc_tag_add(&obj_exts->ref, current->alloc_tag, s->size);
+				alloc_tagging_slab_alloc_hook(obj_exts, s->size);
 		}
-#endif
 	}
 
 	return memcg_slab_post_alloc_hook(s, lru, flags, size, p);

base-commit: e9d22f7a6655941fc8b2b942ed354ec780936b3e

Signature · 1 line

2.45.2.803.g4e1b14247a-goog

Vlastimil Babka

2024-07-03 10:47 UTC #1 lore raw

Quoted 16 lines · Suren Baghdasaryan

Move allocation tagging specific code in the allocation path into alloc_tagging_slab_alloc_hook, similar to how freeing path uses alloc_tagging_slab_free_hook. No functional changes, just code cleanup.
Suggested-by Vlastimil Babka <[email protected]>
Signed-off-by Suren Baghdasaryan <[email protected]>
---
 mm/slub.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)
mm/slub.c +0 -0
index 4927edec6a8c..99d53190cfcf 100644

mm/slub.c prepare_slab_obj_exts_hook(struct kmem_cache *s, gfp_t flags, void *p) · line 2036

Quoted 4 lines

mm/slub.c +1 -0

@@ -2033,11 +2033,18 @@ prepare_slab_obj_exts_hook(struct kmem_cache *s, gfp_t flags, void *p)
 	return slab_obj_exts(slab) + obj_to_index(s, slab, p);
 }
 
+#ifdef CONFIG_MEM_ALLOC_PROFILING

I think if you extract this whole #ifdef CONFIG_MEM_ALLOC_PROFILING section to go outside of CONFIG_SLAB_OBJ_EXT sections, i.e. below the final

#endif /* CONFIG_SLAB_OBJ_EXT */

then it wouldn't be necessary to have two instances of the empty hooks?

Quoted 61 lines

mm/slub.c +27 -3

+
+static inline void
+alloc_tagging_slab_alloc_hook(struct slabobj_ext *obj_exts, unsigned int size)
+{
+	alloc_tag_add(&obj_exts->ref, current->alloc_tag, size);
+}
+
 static inline void
 alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
 			     int objects)
 {
-#ifdef CONFIG_MEM_ALLOC_PROFILING
 	struct slabobj_ext *obj_exts;
 	int i;

@@ -2053,9 +2060,23 @@ alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
 
 		alloc_tag_sub(&obj_exts[off].ref, s->size);
 	}
-#endif
 }
 
+#else /* CONFIG_MEM_ALLOC_PROFILING */
+
+static inline void
+alloc_tagging_slab_alloc_hook(struct slabobj_ext *obj_exts, unsigned int size)
+{
+}
+
+static inline void
+alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
+			     int objects)
+{
+}
+
+#endif /* CONFIG_MEM_ALLOC_PROFILING*/
+
 #else /* CONFIG_SLAB_OBJ_EXT */
 
 static int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,

@@ -2079,6 +2100,11 @@ prepare_slab_obj_exts_hook(struct kmem_cache *s, gfp_t flags, void *p)
 	return NULL;
 }
 
+static inline void
+alloc_tagging_slab_alloc_hook(struct slabobj_ext *obj_exts, unsigned int size)
+{
+}
+
 static inline void
 alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
 			     int objects)

@@ -3944,7 +3970,6 @@ bool slab_post_alloc_hook(struct kmem_cache *s, struct list_lru *lru,
 		kmemleak_alloc_recursive(p[i], s->object_size, 1,
 					 s->flags, init_flags);
 		kmsan_slab_alloc(s, p[i], init_flags);
-#ifdef CONFIG_MEM_ALLOC_PROFILING
 		if (need_slab_obj_ext()) {
 			struct slabobj_ext *obj_exts;

mm/slub.c bool slab_post_alloc_hook(struct kmem_cache *s, struct list_lru *lru, · line 3984

Quoted 6 lines

mm/slub.c +1 -1

@@ -3955,9 +3980,8 @@ bool slab_post_alloc_hook(struct kmem_cache *s, struct list_lru *lru,
 			 * check should be added before alloc_tag_add().
 			 */
 			if (likely(obj_exts))
-				alloc_tag_add(&obj_exts->ref, current->alloc_tag, s->size);
+				alloc_tagging_slab_alloc_hook(obj_exts, s->size);
 		}

Could this whole "if (need_slab_obj_ext())" block be part of alloc_tagging_slab_alloc_hook()? That would match __memcg_slab_post_alloc_hook also taking care of calloing alloc_slab_obj_exts on its own. Maybe then we won't even need empty versions of prepare_slab_obj_exts_hook()

Quoted 6 lines

mm/slub.c +0 -1
-#endif
 	}
 
 	return memcg_slab_post_alloc_hook(s, lru, flags, size, p);
base-commit: e9d22f7a6655941fc8b2b942ed354ec780936b3e

Suren Baghdasaryan

author

2024-07-03 16:16 UTC #2 lore raw

Quoted 90 lines · Vlastimil Babka

Quoted 16 lines · Suren Baghdasaryan

Move allocation tagging specific code in the allocation path into alloc_tagging_slab_alloc_hook, similar to how freeing path uses alloc_tagging_slab_free_hook. No functional changes, just code cleanup.
Suggested-by Vlastimil Babka <[email protected]>
Signed-off-by Suren Baghdasaryan <[email protected]>
---
 mm/slub.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)
mm/slub.c +0 -0
index 4927edec6a8c..99d53190cfcf 100644

mm/slub.c prepare_slab_obj_exts_hook(struct kmem_cache *s, gfp_t flags, void *p) · line 2036

Quoted 4 lines

mm/slub.c +1 -0

@@ -2033,11 +2033,18 @@ prepare_slab_obj_exts_hook(struct kmem_cache *s, gfp_t flags, void *p)
      return slab_obj_exts(slab) + obj_to_index(s, slab, p);
 }
 
+#ifdef CONFIG_MEM_ALLOC_PROFILING

I think if you extract this whole #ifdef CONFIG_MEM_ALLOC_PROFILING section to go outside of CONFIG_SLAB_OBJ_EXT sections, i.e. below the final

#endif /* CONFIG_SLAB_OBJ_EXT */

then it wouldn't be necessary to have two instances of the empty hooks?

Quoted 60 lines

mm/slub.c +27 -3

+
+static inline void
+alloc_tagging_slab_alloc_hook(struct slabobj_ext *obj_exts, unsigned int size)
+{
+     alloc_tag_add(&obj_exts->ref, current->alloc_tag, size);
+}
+
 static inline void
 alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
                           int objects)
 {
-#ifdef CONFIG_MEM_ALLOC_PROFILING
      struct slabobj_ext *obj_exts;
      int i;

@@ -2053,9 +2060,23 @@ alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
 
              alloc_tag_sub(&obj_exts[off].ref, s->size);
      }
-#endif
 }
 
+#else /* CONFIG_MEM_ALLOC_PROFILING */
+
+static inline void
+alloc_tagging_slab_alloc_hook(struct slabobj_ext *obj_exts, unsigned int size)
+{
+}
+
+static inline void
+alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
+                          int objects)
+{
+}
+
+#endif /* CONFIG_MEM_ALLOC_PROFILING*/
+
 #else /* CONFIG_SLAB_OBJ_EXT */
 
 static int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,

@@ -2079,6 +2100,11 @@ prepare_slab_obj_exts_hook(struct kmem_cache *s, gfp_t flags, void *p)
      return NULL;
 }
 
+static inline void
+alloc_tagging_slab_alloc_hook(struct slabobj_ext *obj_exts, unsigned int size)
+{
+}
+
 static inline void
 alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
                           int objects)

@@ -3944,7 +3970,6 @@ bool slab_post_alloc_hook(struct kmem_cache *s, struct list_lru *lru,
              kmemleak_alloc_recursive(p[i], s->object_size, 1,
                                       s->flags, init_flags);
              kmsan_slab_alloc(s, p[i], init_flags);
-#ifdef CONFIG_MEM_ALLOC_PROFILING
              if (need_slab_obj_ext()) {
                      struct slabobj_ext *obj_exts;

Quoted 13 lines

mm/slub.c bool slab_post_alloc_hook(struct kmem_cache *s, struct list_lru *lru, · line 3984
Quoted 7 lines
mm/slub.c +1 -1
@@ -3955,9 +3980,8 @@ bool slab_post_alloc_hook(struct kmem_cache *s, struct list_lru *lru,
                       * check should be added before alloc_tag_add().
                       */
                      if (likely(obj_exts))
-                             alloc_tag_add(&obj_exts->ref, current->alloc_tag, s->size);
+                             alloc_tagging_slab_alloc_hook(obj_exts, s->size);
              }
Could this whole "if (need_slab_obj_ext())" block be part of alloc_tagging_slab_alloc_hook()? That would match __memcg_slab_post_alloc_hook also taking care of calloing alloc_slab_obj_exts on its own. Maybe then we won't even need empty versions of prepare_slab_obj_exts_hook()

Yeah, since we don't have other slab_obj_ext users yet, we can simplify the code that way. Originally I wanted to keep it more generic and make it easy to add new slab_obj_ext users, but I'm fine with this simplification. When there are other users we can change the code back to be more generic. I'll post v2 shortly with your suggestions.
Thanks,
Suren.

Quoted 8 lines

Quoted 7 lines
mm/slub.c +0 -1
-#endif
      }
 
      return memcg_slab_post_alloc_hook(s, lru, flags, size, p);
base-commit: e9d22f7a6655941fc8b2b942ed354ec780936b3e