On Tue, Jul 12, 2022 at 10:28:05AM +0800, Rongwei Wang wrote:
> In use cases where allocating and freeing slab frequently, some
> error messages, such as "Left Redzone overwritten", "First byte
> 0xbb instead of 0xcc" would be printed when validating slabs.
> That's because an object has been filled with SLAB_RED_INACTIVE,
> but has not been added to slab's freelist. And between these
> two states, the behaviour of validating slab is likely to occur.
>
> Actually, it doesn't mean the slab can not work stably. But, these
> confusing messages will disturb slab debugging more or less.
>
> Signed-off-by: Rongwei Wang <rongwei.wang@xxxxxxxxxxxxxxxxx>
> ---
> mm/slub.c | 43 +++++++++++++++++++++++++------------------
> 1 file changed, 25 insertions(+), 18 deletions(-)
>
This makes the code more complex.
A part of me says it may be more pleasant to split implementation
allocating from caches for debugging. That would make it simpler.
something like:
__slab_alloc() {
if (kmem_cache_debug(s))
slab_alloc_debug()
else
___slab_alloc()
}
slab_free() {
if (kmem_cache_debug(s))
slab_free_debug()
else
__do_slab_free()
}
See also:
https://lore.kernel.org/lkml/faf416b9-f46c-8534-7fb7-557c046a564d@xxxxxxx/
> diff --git a/mm/slub.c b/mm/slub.c
> index b1281b8654bd..e950d8df8380 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1391,18 +1391,16 @@ static noinline int free_debug_processing(
> void *head, void *tail, int bulk_cnt,
> unsigned long addr)
> {
> - struct kmem_cache_node *n = get_node(s, slab_nid(slab));
> void *object = head;
> int cnt = 0;
> - unsigned long flags, flags2;
> + unsigned long flags;
> int ret = 0;
> depot_stack_handle_t handle = 0;
>
> if (s->flags & SLAB_STORE_USER)
> handle = set_track_prepare();
>
> - spin_lock_irqsave(&n->list_lock, flags);
> - slab_lock(slab, &flags2);
> + slab_lock(slab, &flags);
>
> if (s->flags & SLAB_CONSISTENCY_CHECKS) {
> if (!check_slab(s, slab))
> @@ -1435,8 +1433,7 @@ static noinline int free_debug_processing(
> slab_err(s, slab, "Bulk freelist count(%d) invalid(%d)\n",
> bulk_cnt, cnt);
>
> - slab_unlock(slab, &flags2);
> - spin_unlock_irqrestore(&n->list_lock, flags);
> + slab_unlock(slab, &flags);
> if (!ret)
> slab_fix(s, "Object at 0x%p not freed", object);
> return ret;
> @@ -3330,7 +3327,7 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
>
> {
> void *prior;
> - int was_frozen;
> + int was_frozen, to_take_off = 0;
> struct slab new;
> unsigned long counters;
> struct kmem_cache_node *n = NULL;
> @@ -3341,14 +3338,23 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
> if (kfence_free(head))
> return;
>
> - if (kmem_cache_debug(s) &&
> - !free_debug_processing(s, slab, head, tail, cnt, addr))
> - return;
> + n = get_node(s, slab_nid(slab));
> + if (kmem_cache_debug(s)) {
> + int ret;
>
> - do {
> - if (unlikely(n)) {
> + spin_lock_irqsave(&n->list_lock, flags);
> + ret = free_debug_processing(s, slab, head, tail, cnt, addr);
> + if (!ret) {
> spin_unlock_irqrestore(&n->list_lock, flags);
> - n = NULL;
> + return;
> + }
> + }
> +
> + do {
> + if (unlikely(to_take_off)) {
> + if (!kmem_cache_debug(s))
> + spin_unlock_irqrestore(&n->list_lock, flags);
> + to_take_off = 0;
> }
> prior = slab->freelist;
> counters = slab->counters;
> @@ -3369,8 +3375,6 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
> new.frozen = 1;
>
> } else { /* Needs to be taken off a list */
> -
> - n = get_node(s, slab_nid(slab));
> /*
> * Speculatively acquire the list_lock.
> * If the cmpxchg does not succeed then we may
> @@ -3379,8 +3383,10 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
> * Otherwise the list_lock will synchronize with
> * other processors updating the list of slabs.
> */
> - spin_lock_irqsave(&n->list_lock, flags);
> + if (!kmem_cache_debug(s))
> + spin_lock_irqsave(&n->list_lock, flags);
>
> + to_take_off = 1;
> }
> }
>
> @@ -3389,8 +3395,9 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
> head, new.counters,
> "__slab_free"));
>
> - if (likely(!n)) {
> -
> + if (likely(!to_take_off)) {
> + if (kmem_cache_debug(s))
> + spin_unlock_irqrestore(&n->list_lock, flags);
> if (likely(was_frozen)) {
> /*
> * The list lock was not taken therefore no list
> --
> 2.27.0
>
