| Message ID | 20231204-slub-cleanup-hooks-v1-2-88b65f7cd9d5@suse.cz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | SLUB: cleanup hook processing | expand |
On 2023/12/5 03:34, Vlastimil Babka wrote: > Currently, when __kmem_cache_alloc_bulk() fails, it frees back the > objects that were allocated before the failure, using > kmem_cache_free_bulk(). Because kmem_cache_free_bulk() calls the free > hooks (KASAN etc.) and those expect objects that were processed by the > post alloc hooks, slab_post_alloc_hook() is called before > kmem_cache_free_bulk(). > > This is wasteful, although not a big concern in practice for the rare > error path. But in order to efficiently handle percpu array batch refill > and free in the near future, we will also need a variant of > kmem_cache_free_bulk() that avoids the free hooks. So introduce it now > and use it for the failure path. > > As a consequence, __kmem_cache_alloc_bulk() no longer needs the objcg > parameter, remove it. The objects may have been charged before, but it seems __kmem_cache_alloc_bulk() forget to uncharge them? I can't find "uncharge" in do_slab_free(), or maybe the bulk interface won't be used on chargeable slab? Thanks. > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > --- > mm/slub.c | 33 ++++++++++++++++++++++++++------- > 1 file changed, 26 insertions(+), 7 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index d7b0ca6012e0..0742564c4538 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -4478,6 +4478,27 @@ int build_detached_freelist(struct kmem_cache *s, size_t size, > return same; > } > > +/* > + * Internal bulk free of objects that were not initialised by the post alloc > + * hooks and thus should not be processed by the free hooks > + */ > +static void __kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p) > +{ > + if (!size) > + return; > + > + do { > + struct detached_freelist df; > + > + size = build_detached_freelist(s, size, p, &df); > + if (!df.slab) > + continue; > + > + do_slab_free(df.s, df.slab, df.freelist, df.tail, df.cnt, > + _RET_IP_); > + } while (likely(size)); > +} > + > /* Note that interrupts must be enabled when calling this function. */ > void kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p) > { > @@ -4499,7 +4520,7 @@ EXPORT_SYMBOL(kmem_cache_free_bulk); > > #ifndef CONFIG_SLUB_TINY > static inline int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, > - size_t size, void **p, struct obj_cgroup *objcg) > + size_t size, void **p) > { > struct kmem_cache_cpu *c; > unsigned long irqflags; > @@ -4563,14 +4584,13 @@ static inline int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, > > error: > slub_put_cpu_ptr(s->cpu_slab); > - slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); > - kmem_cache_free_bulk(s, i, p); > + __kmem_cache_free_bulk(s, i, p); > return 0; > > } > #else /* CONFIG_SLUB_TINY */ > static int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, > - size_t size, void **p, struct obj_cgroup *objcg) > + size_t size, void **p) > { > int i; > > @@ -4593,8 +4613,7 @@ static int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, > return i; > > error: > - slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); > - kmem_cache_free_bulk(s, i, p); > + __kmem_cache_free_bulk(s, i, p); > return 0; > } > #endif /* CONFIG_SLUB_TINY */ > @@ -4614,7 +4633,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, > if (unlikely(!s)) > return 0; > > - i = __kmem_cache_alloc_bulk(s, flags, size, p, objcg); > + i = __kmem_cache_alloc_bulk(s, flags, size, p); > > /* > * memcg and kmem_cache debug support and memory initialization. >
On 12/5/23 09:19, Chengming Zhou wrote: > On 2023/12/5 03:34, Vlastimil Babka wrote: >> Currently, when __kmem_cache_alloc_bulk() fails, it frees back the >> objects that were allocated before the failure, using >> kmem_cache_free_bulk(). Because kmem_cache_free_bulk() calls the free >> hooks (KASAN etc.) and those expect objects that were processed by the >> post alloc hooks, slab_post_alloc_hook() is called before >> kmem_cache_free_bulk(). >> >> This is wasteful, although not a big concern in practice for the rare >> error path. But in order to efficiently handle percpu array batch refill >> and free in the near future, we will also need a variant of >> kmem_cache_free_bulk() that avoids the free hooks. So introduce it now >> and use it for the failure path. >> >> As a consequence, __kmem_cache_alloc_bulk() no longer needs the objcg >> parameter, remove it. > > The objects may have been charged before, but it seems __kmem_cache_alloc_bulk() > forget to uncharge them? I can't find "uncharge" in do_slab_free(), or maybe > the bulk interface won't be used on chargeable slab? You're right! I missed that the memcg_pre_alloc_hook() already does the charging, so we need to uncharge. How does this look? Thanks for noticing! ----8<---- From 52f8e77fdfeabffffdce6b761ba5508e940df3be Mon Sep 17 00:00:00 2001 From: Vlastimil Babka <vbabka@suse.cz> Date: Thu, 2 Nov 2023 16:34:39 +0100 Subject: [PATCH 2/4] mm/slub: introduce __kmem_cache_free_bulk() without free hooks Currently, when __kmem_cache_alloc_bulk() fails, it frees back the objects that were allocated before the failure, using kmem_cache_free_bulk(). Because kmem_cache_free_bulk() calls the free hooks (KASAN etc.) and those expect objects that were processed by the post alloc hooks, slab_post_alloc_hook() is called before kmem_cache_free_bulk(). This is wasteful, although not a big concern in practice for the rare error path. But in order to efficiently handle percpu array batch refill and free in the near future, we will also need a variant of kmem_cache_free_bulk() that avoids the free hooks. So introduce it now and use it for the failure path. In case of failure we however still need to perform memcg uncharge so handle that in a new memcg_slab_alloc_error_hook(). Thanks to Chengming Zhou for noticing the missing uncharge. As a consequence, __kmem_cache_alloc_bulk() no longer needs the objcg parameter, remove it. Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- mm/slub.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 47 insertions(+), 9 deletions(-) diff --git a/mm/slub.c b/mm/slub.c index d7b0ca6012e0..0a9e4bd0dd68 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -2003,6 +2003,14 @@ void memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p, __memcg_slab_free_hook(s, slab, p, objects, objcgs); } + +static inline +void memcg_slab_alloc_error_hook(struct kmem_cache *s, int objects, + struct obj_cgroup *objcg) +{ + if (objcg) + obj_cgroup_uncharge(objcg, objects * obj_full_size(s)); +} #else /* CONFIG_MEMCG_KMEM */ static inline struct mem_cgroup *memcg_from_slab_obj(void *ptr) { @@ -2032,6 +2040,12 @@ static inline void memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p, int objects) { } + +static inline +void memcg_slab_alloc_error_hook(struct kmem_cache *s, int objects, + struct obj_cgroup *objcg) +{ +} #endif /* CONFIG_MEMCG_KMEM */ /* @@ -4478,6 +4492,27 @@ int build_detached_freelist(struct kmem_cache *s, size_t size, return same; } +/* + * Internal bulk free of objects that were not initialised by the post alloc + * hooks and thus should not be processed by the free hooks + */ +static void __kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p) +{ + if (!size) + return; + + do { + struct detached_freelist df; + + size = build_detached_freelist(s, size, p, &df); + if (!df.slab) + continue; + + do_slab_free(df.s, df.slab, df.freelist, df.tail, df.cnt, + _RET_IP_); + } while (likely(size)); +} + /* Note that interrupts must be enabled when calling this function. */ void kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p) { @@ -4498,8 +4533,9 @@ void kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p) EXPORT_SYMBOL(kmem_cache_free_bulk); #ifndef CONFIG_SLUB_TINY -static inline int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, - size_t size, void **p, struct obj_cgroup *objcg) +static inline +int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, + void **p) { struct kmem_cache_cpu *c; unsigned long irqflags; @@ -4563,14 +4599,13 @@ static inline int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, error: slub_put_cpu_ptr(s->cpu_slab); - slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); - kmem_cache_free_bulk(s, i, p); + __kmem_cache_free_bulk(s, i, p); return 0; } #else /* CONFIG_SLUB_TINY */ static int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, - size_t size, void **p, struct obj_cgroup *objcg) + size_t size, void **p) { int i; @@ -4593,8 +4628,7 @@ static int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, return i; error: - slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); - kmem_cache_free_bulk(s, i, p); + __kmem_cache_free_bulk(s, i, p); return 0; } #endif /* CONFIG_SLUB_TINY */ @@ -4614,15 +4648,19 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, if (unlikely(!s)) return 0; - i = __kmem_cache_alloc_bulk(s, flags, size, p, objcg); + i = __kmem_cache_alloc_bulk(s, flags, size, p); /* * memcg and kmem_cache debug support and memory initialization. * Done outside of the IRQ disabled fastpath loop. */ - if (i != 0) + if (likely(i != 0)) { slab_post_alloc_hook(s, objcg, flags, size, p, slab_want_init_on_alloc(flags, s), s->object_size); + } else { + memcg_slab_alloc_error_hook(s, size, objcg); + } + return i; } EXPORT_SYMBOL(kmem_cache_alloc_bulk);
On 2023/12/6 03:57, Vlastimil Babka wrote: > On 12/5/23 09:19, Chengming Zhou wrote: >> On 2023/12/5 03:34, Vlastimil Babka wrote: >>> Currently, when __kmem_cache_alloc_bulk() fails, it frees back the >>> objects that were allocated before the failure, using >>> kmem_cache_free_bulk(). Because kmem_cache_free_bulk() calls the free >>> hooks (KASAN etc.) and those expect objects that were processed by the >>> post alloc hooks, slab_post_alloc_hook() is called before >>> kmem_cache_free_bulk(). >>> >>> This is wasteful, although not a big concern in practice for the rare >>> error path. But in order to efficiently handle percpu array batch refill >>> and free in the near future, we will also need a variant of >>> kmem_cache_free_bulk() that avoids the free hooks. So introduce it now >>> and use it for the failure path. >>> >>> As a consequence, __kmem_cache_alloc_bulk() no longer needs the objcg >>> parameter, remove it. >> >> The objects may have been charged before, but it seems __kmem_cache_alloc_bulk() >> forget to uncharge them? I can't find "uncharge" in do_slab_free(), or maybe >> the bulk interface won't be used on chargeable slab? > > You're right! I missed that the memcg_pre_alloc_hook() already does the > charging, so we need to uncharge. How does this look? Thanks for noticing! > > ----8<---- > From 52f8e77fdfeabffffdce6b761ba5508e940df3be Mon Sep 17 00:00:00 2001 > From: Vlastimil Babka <vbabka@suse.cz> > Date: Thu, 2 Nov 2023 16:34:39 +0100 > Subject: [PATCH 2/4] mm/slub: introduce __kmem_cache_free_bulk() without free > hooks > > Currently, when __kmem_cache_alloc_bulk() fails, it frees back the > objects that were allocated before the failure, using > kmem_cache_free_bulk(). Because kmem_cache_free_bulk() calls the free > hooks (KASAN etc.) and those expect objects that were processed by the > post alloc hooks, slab_post_alloc_hook() is called before > kmem_cache_free_bulk(). > > This is wasteful, although not a big concern in practice for the rare > error path. But in order to efficiently handle percpu array batch refill > and free in the near future, we will also need a variant of > kmem_cache_free_bulk() that avoids the free hooks. So introduce it now > and use it for the failure path. > > In case of failure we however still need to perform memcg uncharge so > handle that in a new memcg_slab_alloc_error_hook(). Thanks to Chengming > Zhou for noticing the missing uncharge. > > As a consequence, __kmem_cache_alloc_bulk() no longer needs the objcg > parameter, remove it. > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> Looks good to me! Reviewed-by: Chengming Zhou <zhouchengming@bytedance.com> Thanks! > --- > mm/slub.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 47 insertions(+), 9 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index d7b0ca6012e0..0a9e4bd0dd68 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -2003,6 +2003,14 @@ void memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p, > > __memcg_slab_free_hook(s, slab, p, objects, objcgs); > } > + > +static inline > +void memcg_slab_alloc_error_hook(struct kmem_cache *s, int objects, > + struct obj_cgroup *objcg) > +{ > + if (objcg) > + obj_cgroup_uncharge(objcg, objects * obj_full_size(s)); > +} > #else /* CONFIG_MEMCG_KMEM */ > static inline struct mem_cgroup *memcg_from_slab_obj(void *ptr) > { > @@ -2032,6 +2040,12 @@ static inline void memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab, > void **p, int objects) > { > } > + > +static inline > +void memcg_slab_alloc_error_hook(struct kmem_cache *s, int objects, > + struct obj_cgroup *objcg) > +{ > +} > #endif /* CONFIG_MEMCG_KMEM */ > > /* > @@ -4478,6 +4492,27 @@ int build_detached_freelist(struct kmem_cache *s, size_t size, > return same; > } > > +/* > + * Internal bulk free of objects that were not initialised by the post alloc > + * hooks and thus should not be processed by the free hooks > + */ > +static void __kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p) > +{ > + if (!size) > + return; > + > + do { > + struct detached_freelist df; > + > + size = build_detached_freelist(s, size, p, &df); > + if (!df.slab) > + continue; > + > + do_slab_free(df.s, df.slab, df.freelist, df.tail, df.cnt, > + _RET_IP_); > + } while (likely(size)); > +} > + > /* Note that interrupts must be enabled when calling this function. */ > void kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p) > { > @@ -4498,8 +4533,9 @@ void kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p) > EXPORT_SYMBOL(kmem_cache_free_bulk); > > #ifndef CONFIG_SLUB_TINY > -static inline int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, > - size_t size, void **p, struct obj_cgroup *objcg) > +static inline > +int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, > + void **p) > { > struct kmem_cache_cpu *c; > unsigned long irqflags; > @@ -4563,14 +4599,13 @@ static inline int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, > > error: > slub_put_cpu_ptr(s->cpu_slab); > - slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); > - kmem_cache_free_bulk(s, i, p); > + __kmem_cache_free_bulk(s, i, p); > return 0; > > } > #else /* CONFIG_SLUB_TINY */ > static int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, > - size_t size, void **p, struct obj_cgroup *objcg) > + size_t size, void **p) > { > int i; > > @@ -4593,8 +4628,7 @@ static int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, > return i; > > error: > - slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); > - kmem_cache_free_bulk(s, i, p); > + __kmem_cache_free_bulk(s, i, p); > return 0; > } > #endif /* CONFIG_SLUB_TINY */ > @@ -4614,15 +4648,19 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, > if (unlikely(!s)) > return 0; > > - i = __kmem_cache_alloc_bulk(s, flags, size, p, objcg); > + i = __kmem_cache_alloc_bulk(s, flags, size, p); > > /* > * memcg and kmem_cache debug support and memory initialization. > * Done outside of the IRQ disabled fastpath loop. > */ > - if (i != 0) > + if (likely(i != 0)) { > slab_post_alloc_hook(s, objcg, flags, size, p, > slab_want_init_on_alloc(flags, s), s->object_size); > + } else { > + memcg_slab_alloc_error_hook(s, size, objcg); > + } > + > return i; > } > EXPORT_SYMBOL(kmem_cache_alloc_bulk);
diff --git a/mm/slub.c b/mm/slub.c index d7b0ca6012e0..0742564c4538 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -4478,6 +4478,27 @@ int build_detached_freelist(struct kmem_cache *s, size_t size, return same; } +/* + * Internal bulk free of objects that were not initialised by the post alloc + * hooks and thus should not be processed by the free hooks + */ +static void __kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p) +{ + if (!size) + return; + + do { + struct detached_freelist df; + + size = build_detached_freelist(s, size, p, &df); + if (!df.slab) + continue; + + do_slab_free(df.s, df.slab, df.freelist, df.tail, df.cnt, + _RET_IP_); + } while (likely(size)); +} + /* Note that interrupts must be enabled when calling this function. */ void kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p) { @@ -4499,7 +4520,7 @@ EXPORT_SYMBOL(kmem_cache_free_bulk); #ifndef CONFIG_SLUB_TINY static inline int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, - size_t size, void **p, struct obj_cgroup *objcg) + size_t size, void **p) { struct kmem_cache_cpu *c; unsigned long irqflags; @@ -4563,14 +4584,13 @@ static inline int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, error: slub_put_cpu_ptr(s->cpu_slab); - slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); - kmem_cache_free_bulk(s, i, p); + __kmem_cache_free_bulk(s, i, p); return 0; } #else /* CONFIG_SLUB_TINY */ static int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, - size_t size, void **p, struct obj_cgroup *objcg) + size_t size, void **p) { int i; @@ -4593,8 +4613,7 @@ static int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, return i; error: - slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); - kmem_cache_free_bulk(s, i, p); + __kmem_cache_free_bulk(s, i, p); return 0; } #endif /* CONFIG_SLUB_TINY */ @@ -4614,7 +4633,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, if (unlikely(!s)) return 0; - i = __kmem_cache_alloc_bulk(s, flags, size, p, objcg); + i = __kmem_cache_alloc_bulk(s, flags, size, p); /* * memcg and kmem_cache debug support and memory initialization.
Currently, when __kmem_cache_alloc_bulk() fails, it frees back the objects that were allocated before the failure, using kmem_cache_free_bulk(). Because kmem_cache_free_bulk() calls the free hooks (KASAN etc.) and those expect objects that were processed by the post alloc hooks, slab_post_alloc_hook() is called before kmem_cache_free_bulk(). This is wasteful, although not a big concern in practice for the rare error path. But in order to efficiently handle percpu array batch refill and free in the near future, we will also need a variant of kmem_cache_free_bulk() that avoids the free hooks. So introduce it now and use it for the failure path. As a consequence, __kmem_cache_alloc_bulk() no longer needs the objcg parameter, remove it. Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- mm/slub.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-)