| Message ID | 20240306182440.2003814-24-surenb@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Memory allocation profiling | expand |
On 3/6/24 19:24, Suren Baghdasaryan wrote: > Account slab allocations using codetag reference embedded into slabobj_ext. > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > Co-developed-by: Kent Overstreet <kent.overstreet@linux.dev> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > Reviewed-by: Kees Cook <keescook@chromium.org> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> Nit below: > @@ -3833,6 +3913,7 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, > unsigned int orig_size) > { > unsigned int zero_size = s->object_size; > + struct slabobj_ext *obj_exts; > bool kasan_init = init; > size_t i; > gfp_t init_flags = flags & gfp_allowed_mask; > @@ -3875,6 +3956,12 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, > kmemleak_alloc_recursive(p[i], s->object_size, 1, > s->flags, init_flags); > kmsan_slab_alloc(s, p[i], init_flags); > + obj_exts = prepare_slab_obj_exts_hook(s, flags, p[i]); > +#ifdef CONFIG_MEM_ALLOC_PROFILING > + /* obj_exts can be allocated for other reasons */ > + if (likely(obj_exts) && mem_alloc_profiling_enabled()) > + alloc_tag_add(&obj_exts->ref, current->alloc_tag, s->size); > +#endif I think you could still do this a bit better: Check mem_alloc_profiling_enabled() once before the whole block calling prepare_slab_obj_exts_hook() and alloc_tag_add() Remove need_slab_obj_ext() check from prepare_slab_obj_exts_hook() > } > > memcg_slab_post_alloc_hook(s, objcg, flags, size, p); > @@ -4353,6 +4440,7 @@ void slab_free(struct kmem_cache *s, struct slab *slab, void *object, > unsigned long addr) > { > memcg_slab_free_hook(s, slab, &object, 1); > + alloc_tagging_slab_free_hook(s, slab, &object, 1); > > if (likely(slab_free_hook(s, object, slab_want_init_on_free(s)))) > do_slab_free(s, slab, object, object, 1, addr); > @@ -4363,6 +4451,7 @@ void slab_free_bulk(struct kmem_cache *s, struct slab *slab, void *head, > void *tail, void **p, int cnt, unsigned long addr) > { > memcg_slab_free_hook(s, slab, p, cnt); > + alloc_tagging_slab_free_hook(s, slab, p, cnt); > /* > * With KASAN enabled slab_free_freelist_hook modifies the freelist > * to remove objects, whose reuse must be delayed.
On Fri, Mar 15, 2024 at 3:58 AM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 3/6/24 19:24, Suren Baghdasaryan wrote: > > Account slab allocations using codetag reference embedded into slabobj_ext. > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > Co-developed-by: Kent Overstreet <kent.overstreet@linux.dev> > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > > Reviewed-by: Kees Cook <keescook@chromium.org> > > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > > Nit below: > > > @@ -3833,6 +3913,7 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, > > unsigned int orig_size) > > { > > unsigned int zero_size = s->object_size; > > + struct slabobj_ext *obj_exts; > > bool kasan_init = init; > > size_t i; > > gfp_t init_flags = flags & gfp_allowed_mask; > > @@ -3875,6 +3956,12 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, > > kmemleak_alloc_recursive(p[i], s->object_size, 1, > > s->flags, init_flags); > > kmsan_slab_alloc(s, p[i], init_flags); > > + obj_exts = prepare_slab_obj_exts_hook(s, flags, p[i]); > > +#ifdef CONFIG_MEM_ALLOC_PROFILING > > + /* obj_exts can be allocated for other reasons */ > > + if (likely(obj_exts) && mem_alloc_profiling_enabled()) > > + alloc_tag_add(&obj_exts->ref, current->alloc_tag, s->size); > > +#endif > > I think you could still do this a bit better: > > Check mem_alloc_profiling_enabled() once before the whole block calling > prepare_slab_obj_exts_hook() and alloc_tag_add() > Remove need_slab_obj_ext() check from prepare_slab_obj_exts_hook() Agree about checking mem_alloc_profiling_enabled() early and one time, except I would like to use need_slab_obj_ext() instead of mem_alloc_profiling_enabled() for that check. Currently they are equivalent but if there are more slab_obj_ext users in the future then there will be cases when we need to prepare_slab_obj_exts_hook() even when mem_alloc_profiling_enabled()==false. need_slab_obj_ext() will be easy to extend for such cases. Thanks, Suren. > > > } > > > > memcg_slab_post_alloc_hook(s, objcg, flags, size, p); > > @@ -4353,6 +4440,7 @@ void slab_free(struct kmem_cache *s, struct slab *slab, void *object, > > unsigned long addr) > > { > > memcg_slab_free_hook(s, slab, &object, 1); > > + alloc_tagging_slab_free_hook(s, slab, &object, 1); > > > > if (likely(slab_free_hook(s, object, slab_want_init_on_free(s)))) > > do_slab_free(s, slab, object, object, 1, addr); > > @@ -4363,6 +4451,7 @@ void slab_free_bulk(struct kmem_cache *s, struct slab *slab, void *head, > > void *tail, void **p, int cnt, unsigned long addr) > > { > > memcg_slab_free_hook(s, slab, p, cnt); > > + alloc_tagging_slab_free_hook(s, slab, p, cnt); > > /* > > * With KASAN enabled slab_free_freelist_hook modifies the freelist > > * to remove objects, whose reuse must be delayed. >
On 3/15/24 16:43, Suren Baghdasaryan wrote: > On Fri, Mar 15, 2024 at 3:58 AM Vlastimil Babka <vbabka@suse.cz> wrote: >> >> On 3/6/24 19:24, Suren Baghdasaryan wrote: >> > Account slab allocations using codetag reference embedded into slabobj_ext. >> > >> > Signed-off-by: Suren Baghdasaryan <surenb@google.com> >> > Co-developed-by: Kent Overstreet <kent.overstreet@linux.dev> >> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> >> > Reviewed-by: Kees Cook <keescook@chromium.org> >> >> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> >> >> Nit below: >> >> > @@ -3833,6 +3913,7 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, >> > unsigned int orig_size) >> > { >> > unsigned int zero_size = s->object_size; >> > + struct slabobj_ext *obj_exts; >> > bool kasan_init = init; >> > size_t i; >> > gfp_t init_flags = flags & gfp_allowed_mask; >> > @@ -3875,6 +3956,12 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, >> > kmemleak_alloc_recursive(p[i], s->object_size, 1, >> > s->flags, init_flags); >> > kmsan_slab_alloc(s, p[i], init_flags); >> > + obj_exts = prepare_slab_obj_exts_hook(s, flags, p[i]); >> > +#ifdef CONFIG_MEM_ALLOC_PROFILING >> > + /* obj_exts can be allocated for other reasons */ >> > + if (likely(obj_exts) && mem_alloc_profiling_enabled()) Could you at least flip these two checks then so the static key one goes first? >> > + alloc_tag_add(&obj_exts->ref, current->alloc_tag, s->size); >> > +#endif >> >> I think you could still do this a bit better: >> >> Check mem_alloc_profiling_enabled() once before the whole block calling >> prepare_slab_obj_exts_hook() and alloc_tag_add() >> Remove need_slab_obj_ext() check from prepare_slab_obj_exts_hook() > > Agree about checking mem_alloc_profiling_enabled() early and one time, > except I would like to use need_slab_obj_ext() instead of > mem_alloc_profiling_enabled() for that check. Currently they are > equivalent but if there are more slab_obj_ext users in the future then > there will be cases when we need to prepare_slab_obj_exts_hook() even > when mem_alloc_profiling_enabled()==false. need_slab_obj_ext() will be > easy to extend for such cases. I thought we don't generally future-proof internal implementation details like this until it's actually needed. But at least what I suggested above would help, thanks. > Thanks, > Suren. > >> >> > } >> > >> > memcg_slab_post_alloc_hook(s, objcg, flags, size, p); >> > @@ -4353,6 +4440,7 @@ void slab_free(struct kmem_cache *s, struct slab *slab, void *object, >> > unsigned long addr) >> > { >> > memcg_slab_free_hook(s, slab, &object, 1); >> > + alloc_tagging_slab_free_hook(s, slab, &object, 1); >> > >> > if (likely(slab_free_hook(s, object, slab_want_init_on_free(s)))) >> > do_slab_free(s, slab, object, object, 1, addr); >> > @@ -4363,6 +4451,7 @@ void slab_free_bulk(struct kmem_cache *s, struct slab *slab, void *head, >> > void *tail, void **p, int cnt, unsigned long addr) >> > { >> > memcg_slab_free_hook(s, slab, p, cnt); >> > + alloc_tagging_slab_free_hook(s, slab, p, cnt); >> > /* >> > * With KASAN enabled slab_free_freelist_hook modifies the freelist >> > * to remove objects, whose reuse must be delayed. >>
On Fri, Mar 15, 2024 at 4:52 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 3/15/24 16:43, Suren Baghdasaryan wrote: > > On Fri, Mar 15, 2024 at 3:58 AM Vlastimil Babka <vbabka@suse.cz> wrote: > >> > >> On 3/6/24 19:24, Suren Baghdasaryan wrote: > >> > Account slab allocations using codetag reference embedded into slabobj_ext. > >> > > >> > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > >> > Co-developed-by: Kent Overstreet <kent.overstreet@linux.dev> > >> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > >> > Reviewed-by: Kees Cook <keescook@chromium.org> > >> > >> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > >> > >> Nit below: > >> > >> > @@ -3833,6 +3913,7 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, > >> > unsigned int orig_size) > >> > { > >> > unsigned int zero_size = s->object_size; > >> > + struct slabobj_ext *obj_exts; > >> > bool kasan_init = init; > >> > size_t i; > >> > gfp_t init_flags = flags & gfp_allowed_mask; > >> > @@ -3875,6 +3956,12 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, > >> > kmemleak_alloc_recursive(p[i], s->object_size, 1, > >> > s->flags, init_flags); > >> > kmsan_slab_alloc(s, p[i], init_flags); > >> > + obj_exts = prepare_slab_obj_exts_hook(s, flags, p[i]); > >> > +#ifdef CONFIG_MEM_ALLOC_PROFILING > >> > + /* obj_exts can be allocated for other reasons */ > >> > + if (likely(obj_exts) && mem_alloc_profiling_enabled()) > > Could you at least flip these two checks then so the static key one goes first? Yes, definitely. I was thinking about removing need_slab_obj_ext() from prepare_slab_obj_exts_hook() and adding this instead of the above code: + if (need_slab_obj_ext()) { + obj_exts = prepare_slab_obj_exts_hook(s, flags, p[i]); +#ifdef CONFIG_MEM_ALLOC_PROFILING + /* + * Currently obj_exts is used only for allocation profiling. If other users appear + * then mem_alloc_profiling_enabled() check should be added here. + */ + if (likely(obj_exts)) + alloc_tag_add(&obj_exts->ref, current->alloc_tag, s->size); +#endif + } Does that look good? > >> > +#ifdef CONFIG_MEM_ALLOC_PROFILING > >> > + /* obj_exts can be allocated for other reasons */ > >> > + if (likely(obj_exts) && mem_alloc_profiling_enabled()) > > >> > + alloc_tag_add(&obj_exts->ref, current->alloc_tag, s->size); > >> > +#endif > >> > >> I think you could still do this a bit better: > >> > >> Check mem_alloc_profiling_enabled() once before the whole block calling > >> prepare_slab_obj_exts_hook() and alloc_tag_add() > >> Remove need_slab_obj_ext() check from prepare_slab_obj_exts_hook() > > > > Agree about checking mem_alloc_profiling_enabled() early and one time, > > except I would like to use need_slab_obj_ext() instead of > > mem_alloc_profiling_enabled() for that check. Currently they are > > equivalent but if there are more slab_obj_ext users in the future then > > there will be cases when we need to prepare_slab_obj_exts_hook() even > > when mem_alloc_profiling_enabled()==false. need_slab_obj_ext() will be > > easy to extend for such cases. > > I thought we don't generally future-proof internal implementation details > like this until it's actually needed. But at least what I suggested above > would help, thanks. > > > Thanks, > > Suren. > > > >> > >> > } > >> > > >> > memcg_slab_post_alloc_hook(s, objcg, flags, size, p); > >> > @@ -4353,6 +4440,7 @@ void slab_free(struct kmem_cache *s, struct slab *slab, void *object, > >> > unsigned long addr) > >> > { > >> > memcg_slab_free_hook(s, slab, &object, 1); > >> > + alloc_tagging_slab_free_hook(s, slab, &object, 1); > >> > > >> > if (likely(slab_free_hook(s, object, slab_want_init_on_free(s)))) > >> > do_slab_free(s, slab, object, object, 1, addr); > >> > @@ -4363,6 +4451,7 @@ void slab_free_bulk(struct kmem_cache *s, struct slab *slab, void *head, > >> > void *tail, void **p, int cnt, unsigned long addr) > >> > { > >> > memcg_slab_free_hook(s, slab, p, cnt); > >> > + alloc_tagging_slab_free_hook(s, slab, p, cnt); > >> > /* > >> > * With KASAN enabled slab_free_freelist_hook modifies the freelist > >> > * to remove objects, whose reuse must be delayed. > >> > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
diff --git a/mm/slub.c b/mm/slub.c index e94d3cc1b270..ea122aeb89fc 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -1942,7 +1942,69 @@ static inline void free_slab_obj_exts(struct slab *slab) kfree(obj_exts); slab->obj_exts = 0; } + +static inline bool need_slab_obj_ext(void) +{ + if (mem_alloc_profiling_enabled()) + return true; + + /* + * CONFIG_MEMCG_KMEM creates vector of obj_cgroup objects conditionally + * inside memcg_slab_post_alloc_hook. No other users for now. + */ + return false; +} + +static inline struct slabobj_ext * +prepare_slab_obj_exts_hook(struct kmem_cache *s, gfp_t flags, void *p) +{ + struct slab *slab; + + if (!need_slab_obj_ext()) + return NULL; + + if (!p) + return NULL; + + if (s->flags & SLAB_NO_OBJ_EXT) + return NULL; + + if (flags & __GFP_NO_OBJ_EXT) + return NULL; + + slab = virt_to_slab(p); + if (!slab_obj_exts(slab) && + WARN(alloc_slab_obj_exts(slab, s, flags, false), + "%s, %s: Failed to create slab extension vector!\n", + __func__, s->name)) + return NULL; + + return slab_obj_exts(slab) + obj_to_index(s, slab, p); +} + +static inline void +alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p, + int objects) +{ + struct slabobj_ext *obj_exts; + int i; + + if (!mem_alloc_profiling_enabled()) + return; + + obj_exts = slab_obj_exts(slab); + if (!obj_exts) + return; + + for (i = 0; i < objects; i++) { + unsigned int off = obj_to_index(s, slab, p[i]); + + alloc_tag_sub(&obj_exts[off].ref, s->size); + } +} + #else /* CONFIG_SLAB_OBJ_EXT */ + static int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s, gfp_t gfp, bool new_slab) { @@ -1952,6 +2014,24 @@ static int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s, static inline void free_slab_obj_exts(struct slab *slab) { } + +static inline bool need_slab_obj_ext(void) +{ + return false; +} + +static inline struct slabobj_ext * +prepare_slab_obj_exts_hook(struct kmem_cache *s, gfp_t flags, void *p) +{ + return NULL; +} + +static inline void +alloc_tagging_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p, + int objects) +{ +} + #endif /* CONFIG_SLAB_OBJ_EXT */ #ifdef CONFIG_MEMCG_KMEM @@ -2381,7 +2461,7 @@ static __always_inline void account_slab(struct slab *slab, int order, static __always_inline void unaccount_slab(struct slab *slab, int order, struct kmem_cache *s) { - if (memcg_kmem_online()) + if (memcg_kmem_online() || need_slab_obj_ext()) free_slab_obj_exts(slab); mod_node_page_state(slab_pgdat(slab), cache_vmstat_idx(s), @@ -3833,6 +3913,7 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, unsigned int orig_size) { unsigned int zero_size = s->object_size; + struct slabobj_ext *obj_exts; bool kasan_init = init; size_t i; gfp_t init_flags = flags & gfp_allowed_mask; @@ -3875,6 +3956,12 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, kmemleak_alloc_recursive(p[i], s->object_size, 1, s->flags, init_flags); kmsan_slab_alloc(s, p[i], init_flags); + obj_exts = prepare_slab_obj_exts_hook(s, flags, p[i]); +#ifdef CONFIG_MEM_ALLOC_PROFILING + /* obj_exts can be allocated for other reasons */ + if (likely(obj_exts) && mem_alloc_profiling_enabled()) + alloc_tag_add(&obj_exts->ref, current->alloc_tag, s->size); +#endif } memcg_slab_post_alloc_hook(s, objcg, flags, size, p); @@ -4353,6 +4440,7 @@ void slab_free(struct kmem_cache *s, struct slab *slab, void *object, unsigned long addr) { memcg_slab_free_hook(s, slab, &object, 1); + alloc_tagging_slab_free_hook(s, slab, &object, 1); if (likely(slab_free_hook(s, object, slab_want_init_on_free(s)))) do_slab_free(s, slab, object, object, 1, addr); @@ -4363,6 +4451,7 @@ void slab_free_bulk(struct kmem_cache *s, struct slab *slab, void *head, void *tail, void **p, int cnt, unsigned long addr) { memcg_slab_free_hook(s, slab, p, cnt); + alloc_tagging_slab_free_hook(s, slab, p, cnt); /* * With KASAN enabled slab_free_freelist_hook modifies the freelist * to remove objects, whose reuse must be delayed.