On 2023/12/5 3:34, Vlastimil Babka wrote:
> Currently we have a single function slab_free() handling both single
> object freeing and bulk freeing with necessary hooks, the latter case
> requiring slab_free_freelist_hook(). It should be however better to
> distinguish the two use cases for the following reasons:
>
> - code simpler to follow for the single object case
>
> - better code generation - although inlining should eliminate the
> slab_free_freelist_hook() for single object freeing in case no
> debugging options are enabled, it seems it's not perfect. When e.g.
> KASAN is enabled, we're imposing additional unnecessary overhead for
> single object freeing.
>
> - preparation to add percpu array caches in near future
>
> Therefore, simplify slab_free() for the single object case by dropping
> unnecessary parameters and calling only slab_free_hook() instead of
> slab_free_freelist_hook(). Rename the bulk variant to slab_free_bulk()
> and adjust callers accordingly.
>
> While at it, flip (and document) slab_free_hook() return value so that
> it returns true when the freeing can proceed, which matches the logic of
> slab_free_freelist_hook() and is not confusingly the opposite.
>
> Additionally we can simplify a bit by changing the tail parameter of
> do_slab_free() when freeing a single object - instead of NULL we can set
> it equal to head.
>
> bloat-o-meter shows small code reduction with a .config that has KASAN
> etc disabled:
>
> add/remove: 0/0 grow/shrink: 0/4 up/down: 0/-118 (-118)
> Function old new delta
> kmem_cache_alloc_bulk 1203 1196 -7
> kmem_cache_free 861 835 -26
> __kmem_cache_free 741 704 -37
> kmem_cache_free_bulk 911 863 -48
>
> Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx>
> ---
> mm/slub.c | 59 +++++++++++++++++++++++++++++++++++------------------------
> 1 file changed, 35 insertions(+), 24 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 0742564c4538..ed2fa92e914c 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2037,9 +2037,12 @@ static inline void memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab,
> /*
> * Hooks for other subsystems that check memory allocations. In a typical
> * production configuration these hooks all should produce no code at all.
> + *
> + * Returns true if freeing of the object can proceed, false if its reuse
> + * was delayed by KASAN quarantine.
> */
> -static __always_inline bool slab_free_hook(struct kmem_cache *s,
> - void *x, bool init)
> +static __always_inline
> +bool slab_free_hook(struct kmem_cache *s, void *x, bool init)
> {
> kmemleak_free_recursive(x, s->flags);
> kmsan_slab_free(s, x);
> @@ -2072,7 +2075,7 @@ static __always_inline bool slab_free_hook(struct kmem_cache *s,
> s->size - s->inuse - rsize);
> }
> /* KASAN might put x into memory quarantine, delaying its reuse. */
> - return kasan_slab_free(s, x, init);
> + return !kasan_slab_free(s, x, init);
> }
>
> static inline bool slab_free_freelist_hook(struct kmem_cache *s,
> @@ -2082,7 +2085,7 @@ static inline bool slab_free_freelist_hook(struct kmem_cache *s,
>
> void *object;
> void *next = *head;
> - void *old_tail = *tail ? *tail : *head;
> + void *old_tail = *tail;
>
> if (is_kfence_address(next)) {
> slab_free_hook(s, next, false);
> @@ -2098,8 +2101,8 @@ static inline bool slab_free_freelist_hook(struct kmem_cache *s,
> next = get_freepointer(s, object);
>
> /* If object's reuse doesn't have to be delayed */
> - if (likely(!slab_free_hook(s, object,
> - slab_want_init_on_free(s)))) {
> + if (likely(slab_free_hook(s, object,
> + slab_want_init_on_free(s)))) {
> /* Move object to the new freelist */
> set_freepointer(s, object, *head);
> *head = object;
> @@ -2114,9 +2117,6 @@ static inline bool slab_free_freelist_hook(struct kmem_cache *s,
> }
> } while (object != old_tail);
>
> - if (*head == *tail)
> - *tail = NULL;
> -
> return *head != NULL;
> }
>
> @@ -4227,7 +4227,6 @@ static __always_inline void do_slab_free(struct kmem_cache *s,
> struct slab *slab, void *head, void *tail,
> int cnt, unsigned long addr)
> {
> - void *tail_obj = tail ? : head;
> struct kmem_cache_cpu *c;
> unsigned long tid;
> void **freelist;
> @@ -4246,14 +4245,14 @@ static __always_inline void do_slab_free(struct kmem_cache *s,
> barrier();
>
> if (unlikely(slab != c->slab)) {
> - __slab_free(s, slab, head, tail_obj, cnt, addr);
> + __slab_free(s, slab, head, tail, cnt, addr);
> return;
> }
>
> if (USE_LOCKLESS_FAST_PATH()) {
> freelist = READ_ONCE(c->freelist);
>
> - set_freepointer(s, tail_obj, freelist);
> + set_freepointer(s, tail, freelist);
>
> if (unlikely(!__update_cpu_freelist_fast(s, freelist, head, tid))) {
> note_cmpxchg_failure("slab_free", s, tid);
> @@ -4270,7 +4269,7 @@ static __always_inline void do_slab_free(struct kmem_cache *s,
> tid = c->tid;
> freelist = c->freelist;
>
> - set_freepointer(s, tail_obj, freelist);
> + set_freepointer(s, tail, freelist);
> c->freelist = head;
> c->tid = next_tid(tid);
>
> @@ -4283,15 +4282,27 @@ static void do_slab_free(struct kmem_cache *s,
> struct slab *slab, void *head, void *tail,
> int cnt, unsigned long addr)
> {
> - void *tail_obj = tail ? : head;
> -
> - __slab_free(s, slab, head, tail_obj, cnt, addr);
> + __slab_free(s, slab, head, tail, cnt, addr);
> }
> #endif /* CONFIG_SLUB_TINY */
>
> -static __fastpath_inline void slab_free(struct kmem_cache *s, struct slab *slab,
> - void *head, void *tail, void **p, int cnt,
> - unsigned long addr)
> +static __fastpath_inline
> +void slab_free(struct kmem_cache *s, struct slab *slab, void *object,
> + unsigned long addr)
> +{
> + bool init;
> +
> + memcg_slab_free_hook(s, slab, &object, 1);
> +
> + init = !is_kfence_address(object) && slab_want_init_on_free(s);
> +
> + if (likely(slab_free_hook(s, object, init)))
> + do_slab_free(s, slab, object, object, 1, addr);
> +}
> +
> +static __fastpath_inline
> +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);
> /*
> @@ -4305,7 +4316,7 @@ static __fastpath_inline void slab_free(struct kmem_cache *s, struct slab *slab,
> #ifdef CONFIG_KASAN_GENERIC
> void ___cache_free(struct kmem_cache *cache, void *x, unsigned long addr)
> {
> - do_slab_free(cache, virt_to_slab(x), x, NULL, 1, addr);
> + do_slab_free(cache, virt_to_slab(x), x, x, 1, addr);
> }
> #endif
>
> @@ -4349,7 +4360,7 @@ void kmem_cache_free(struct kmem_cache *s, void *x)
> if (!s)
> return;
> trace_kmem_cache_free(_RET_IP_, x, s);
> - slab_free(s, virt_to_slab(x), x, NULL, &x, 1, _RET_IP_);
> + slab_free(s, virt_to_slab(x), x, _RET_IP_);
Hi, everyone,
I found many kmemleaks call trace as follow on linux-next, and it occurs
only after Linux 6.11-rc1, and I tried to locate the problem patch, but
it didn't work.
How to reproduce: with following config open
CONFIG_HAVE_DEBUG_KMEMLEAK=y
CONFIG_DEBUG_KMEMLEAK=y
CONFIG_DEBUG_KMEMLEAK_AUTO_SCAN=y
unreferenced object 0xffff4913c0ddb380 (size 32):
comm "kworker/R-acpi_", pid 65, jiffies 4294893729
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 25 94 c1 13 49 ff ff 00 00 00 00 00 00 00 00 .%...I..........
backtrace (crc f9d57280):
[<00000000c0652f06>] kmemleak_alloc+0x34/0x40
[<000000006227a848>] __kmalloc_cache_noprof+0x270/0x2f8
[<000000008acd203e>] kmem_cache_free+0x124/0x410
[<00000000e43531e7>] __cleanup_sighand+0x94/0xc0
[<0000000078680548>] release_task+0x828/0x1070
[<00000000c0f8c5d5>] do_exit+0x111c/0x1edc
[<00000000367ccb22>] kthread_exit+0x58/0x60
[<00000000bc4738b5>] kthread+0x298/0x374
[<00000000a533ef0f>] ret_from_fork+0x10/0x20
unreferenced object 0xffff4913c14d0fc0 (size 32):
comm "softirq", pid 0, jiffies 4294894408
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 c0 a4 c5 13 49 ff ff 00 00 00 00 00 00 00 00 .....I..........
backtrace (crc ad845771):
[<00000000c0652f06>] kmemleak_alloc+0x34/0x40
[<000000006227a848>] __kmalloc_cache_noprof+0x270/0x2f8
[<000000008acd203e>] kmem_cache_free+0x124/0x410
[<00000000e575b5fb>] mempool_free_slab+0x1c/0x28
[<00000000b5df13a3>] mempool_free+0xd0/0x314
[<000000000480d4fb>] bio_free+0x150/0x1bc
[<000000000c247f54>] bio_put+0x3f8/0x950
[<000000003916cfe2>] end_bio_bh_io_sync+0xc0/0x128
[<000000001e99ce6a>] bio_endio+0x4d4/0x6d0
[<0000000085eb0fe3>] blk_update_request+0x41c/0x1064
[<000000007e079da2>] blk_mq_end_request+0x50/0x80
[<0000000067fcfa3f>] virtblk_request_done+0x154/0x2b4
[<00000000c2d49a49>] blk_complete_reqs+0xa8/0xe8
[<000000006559344d>] blk_done_softirq+0x20/0x2c
[<000000001c82c05d>] handle_softirqs+0x2b8/0xbd4
[<00000000924bc46d>] __do_softirq+0x14/0x20
unreferenced object 0xffff4913c573a2c0 (size 32):
comm "kworker/u16:0", pid 67, jiffies 4294894505
hex dump (first 32 bytes):
60 4e 01 c6 13 49 ff ff 00 00 00 00 00 00 00 00 `N...I..........
00 c0 92 c7 13 49 ff ff 00 00 00 00 00 00 00 00 .....I..........
backtrace (crc 8c85f9b3):
[<00000000c0652f06>] kmemleak_alloc+0x34/0x40
[<000000006227a848>] __kmalloc_cache_noprof+0x270/0x2f8
[<000000008acd203e>] kmem_cache_free+0x124/0x410
[<000000006285099d>] fput+0x2a0/0x334
[<00000000e50ddea7>] path_openat+0x120c/0x1f70
[<00000000986ab24f>] do_filp_open+0x154/0x328
[<00000000ea1348f1>] do_open_execat+0xa4/0x224
[<00000000e94e18f5>] alloc_bprm+0x20/0x964
[<00000000b6ff006b>] kernel_execve+0x7c/0x2d4
[<000000007deddead>] call_usermodehelper_exec_async+0x1e0/0x3c8
[<00000000a533ef0f>] ret_from_fork+0x10/0x20
unreferenced object 0xffff4913c2c627c0 (size 32):
comm "softirq", pid 0, jiffies 4294895572
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 1c c5 13 49 ff ff 00 00 00 00 00 00 00 00 .....I..........
backtrace (crc 320469d7):
[<00000000c0652f06>] kmemleak_alloc+0x34/0x40
[<000000006227a848>] __kmalloc_cache_noprof+0x270/0x2f8
[<000000008acd203e>] kmem_cache_free+0x124/0x410
[<00000000e575b5fb>] mempool_free_slab+0x1c/0x28
[<00000000b5df13a3>] mempool_free+0xd0/0x314
[<000000000480d4fb>] bio_free+0x150/0x1bc
[<000000000c247f54>] bio_put+0x3f8/0x950
[<000000003916cfe2>] end_bio_bh_io_sync+0xc0/0x128
[<000000001e99ce6a>] bio_endio+0x4d4/0x6d0
[<0000000085eb0fe3>] blk_update_request+0x41c/0x1064
[<000000007e079da2>] blk_mq_end_request+0x50/0x80
[<0000000067fcfa3f>] virtblk_request_done+0x154/0x2b4
[<00000000c2d49a49>] blk_complete_reqs+0xa8/0xe8
[<000000006559344d>] blk_done_softirq+0x20/0x2c
[<000000001c82c05d>] handle_softirqs+0x2b8/0xbd4
[<00000000924bc46d>] __do_softirq+0x14/0x20
> }
> EXPORT_SYMBOL(kmem_cache_free);
>
> @@ -4395,7 +4406,7 @@ void kfree(const void *object)
>
> slab = folio_slab(folio);
> s = slab->slab_cache;
> - slab_free(s, slab, x, NULL, &x, 1, _RET_IP_);
> + slab_free(s, slab, x, _RET_IP_);
> }
> EXPORT_SYMBOL(kfree);
>
> @@ -4512,8 +4523,8 @@ void kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p)
> if (!df.slab)
> continue;
>
> - slab_free(df.s, df.slab, df.freelist, df.tail, &p[size], df.cnt,
> - _RET_IP_);
> + slab_free_bulk(df.s, df.slab, df.freelist, df.tail, &p[size],
> + df.cnt, _RET_IP_);
> } while (likely(size));
> }
> EXPORT_SYMBOL(kmem_cache_free_bulk);
>
