From: Hyeonggon Yoo <42.hyeyoo@gmail.com>
To: Rongwei Wang <rongwei.wang@linux.alibaba.com>
Cc: akpm@linux-foundation.org, vbabka@suse.cz,
roman.gushchin@linux.dev, iamjoonsoo.kim@lge.com,
rientjes@google.com, penberg@kernel.org, cl@linux.com,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] mm/slub: fix the race between validate_slab and slab_free
Date: Sun, 29 May 2022 11:37:06 +0000 [thread overview]
Message-ID: <YpNa4tB/jfW3MDyi@n2.us-central1-a.c.spheric-algebra-350919.internal> (raw)
In-Reply-To: <20220529081535.69275-1-rongwei.wang@linux.alibaba.com>
On Sun, May 29, 2022 at 04:15:33PM +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@linux.alibaba.com>
Have you observed it or it's from code inspection?
> ---
> mm/slub.c | 40 +++++++++++++++++-----------------------
> 1 file changed, 17 insertions(+), 23 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index ed5c2c03a47a..310e56d99116 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1374,15 +1374,12 @@ 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;
>
> - 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))
> goto out;
> @@ -1414,8 +1411,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;
> @@ -3304,7 +3300,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;
> @@ -3315,15 +3311,19 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
> if (kfence_free(head))
> return;
>
> + n = get_node(s, slab_nid(slab));
> + spin_lock_irqsave(&n->list_lock, flags);
> +
Oh please don't do this.
SLUB free slowpath can be hit a lot depending on workload.
__slab_free() try its best not to take n->list_lock. currently takes n->list_lock
only when the slab need to be taken from list.
Unconditionally taking n->list_lock will degrade performance.
> if (kmem_cache_debug(s) &&
> - !free_debug_processing(s, slab, head, tail, cnt, addr))
> + !free_debug_processing(s, slab, head, tail, cnt, addr)) {
> +
> + spin_unlock_irqrestore(&n->list_lock, flags);
> return;
> + }
>
> do {
> - if (unlikely(n)) {
> - spin_unlock_irqrestore(&n->list_lock, flags);
> - n = NULL;
> - }
> + if (unlikely(to_take_off))
> + to_take_off = 0;
> prior = slab->freelist;
> counters = slab->counters;
> set_freepointer(s, tail, prior);
> @@ -3343,18 +3343,11 @@ 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
> - * drop the list_lock without any processing.
> - *
> - * Otherwise the list_lock will synchronize with
> - * other processors updating the list of slabs.
> + * drop this behavior without any processing.
> */
> - spin_lock_irqsave(&n->list_lock, flags);
> -
> + to_take_off = 1;
> }
> }
>
> @@ -3363,8 +3356,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)) {
>
> + spin_unlock_irqrestore(&n->list_lock, flags);
> if (likely(was_frozen)) {
> /*
> * The list lock was not taken therefore no list
>
> --
> 2.27.0
>
next prev parent reply other threads:[~2022-05-29 11:37 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-29 8:15 [PATCH 1/3] mm/slub: fix the race between validate_slab and slab_free Rongwei Wang
2022-05-29 8:15 ` [PATCH 2/3] mm/slub: improve consistency of nr_slabs count Rongwei Wang
2022-05-29 12:26 ` Hyeonggon Yoo
2022-05-29 8:15 ` [PATCH 3/3] mm/slub: add nr_full count for debugging slub Rongwei Wang
2022-05-29 11:37 ` Hyeonggon Yoo [this message]
2022-05-30 21:14 ` [PATCH 1/3] mm/slub: fix the race between validate_slab and slab_free David Rientjes
2022-06-02 15:14 ` Christoph Lameter
2022-06-03 3:35 ` Rongwei Wang
2022-06-07 12:14 ` Christoph Lameter
2022-06-08 3:04 ` Rongwei Wang
2022-06-08 12:23 ` Christoph Lameter
2022-06-11 4:04 ` Rongwei Wang
2022-06-13 13:50 ` Christoph Lameter
2022-06-14 2:38 ` Rongwei Wang
2022-06-17 7:55 ` Rongwei Wang
2022-06-17 14:19 ` Christoph Lameter
2022-06-18 2:33 ` Rongwei Wang
2022-06-20 11:57 ` Christoph Lameter
2022-06-26 16:48 ` Rongwei Wang
2022-06-17 9:40 ` Vlastimil Babka
2022-07-15 8:05 ` Rongwei Wang
2022-07-15 10:33 ` Vlastimil Babka
2022-07-15 10:51 ` Rongwei Wang
2022-05-31 3:47 ` Muchun Song
2022-06-04 11:05 ` Hyeonggon Yoo
2022-05-31 8:50 ` Rongwei Wang
2022-07-18 11:09 ` Vlastimil Babka
2022-07-19 14:15 ` Rongwei Wang
2022-07-19 14:21 ` Vlastimil Babka
2022-07-19 14:43 ` Rongwei Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YpNa4tB/jfW3MDyi@n2.us-central1-a.c.spheric-algebra-350919.internal \
--to=42.hyeyoo@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=cl@linux.com \
--cc=iamjoonsoo.kim@lge.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=penberg@kernel.org \
--cc=rientjes@google.com \
--cc=roman.gushchin@linux.dev \
--cc=rongwei.wang@linux.alibaba.com \
--cc=vbabka@suse.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).