From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4B293C433EF for ; Sun, 29 May 2022 11:37:11 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 652BD8D0002; Sun, 29 May 2022 07:37:10 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 605688D0001; Sun, 29 May 2022 07:37:10 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4EFBA8D0002; Sun, 29 May 2022 07:37:10 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 40A0A8D0001 for ; Sun, 29 May 2022 07:37:10 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id F3D9335177 for ; Sun, 29 May 2022 11:37:09 +0000 (UTC) X-FDA: 79518579420.07.DFDB504 Received: from mail-il1-f174.google.com (mail-il1-f174.google.com [209.85.166.174]) by imf04.hostedemail.com (Postfix) with ESMTP id A8DA540055 for ; Sun, 29 May 2022 11:36:51 +0000 (UTC) Received: by mail-il1-f174.google.com with SMTP id b11so5969676ilr.4 for ; Sun, 29 May 2022 04:37:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=f2p+RNA3uW2ooZwCskBW9DfWGMzM7OSZpW6uiTppzOI=; b=e4ZYCc9oVK8MNkLyNA//UfF8oFEpedvj0IZa3bUuNrV66HOKChkPNWU47pOOOjxemh 1dGyXx3X3bH8oBJiaW0F9T0guH902+CB2ax0JxTsTmrd+LSCS12iCZPf8+mkknanlBma sORCIdoBoEr8lCK8+yn7eJnzUENIntoUD/uuqFhq0r9lpV+eL7W9cJliXgw5EA03Shoz tUTTHkeXyvsKQ9OzNUiYuBMtsFDKinbtLFLuHhrajlR0wuwTqTvpS3iID6DVWMKbHUxE ndRb9q4+Yfd29FMEakiw8+dZoclAHSDdIdAVXkazY5CfmJU5nnlwzvwctPpi7DsiC7/Z cCCg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=f2p+RNA3uW2ooZwCskBW9DfWGMzM7OSZpW6uiTppzOI=; b=rZ8OwE/n5pwBw93pqxCkPPF2cictdx0eep59FWaMz9geELSzvBB6iwuCmZJAYLFt5B Be/bXwAtWYPsMCCrUondILNMY3L0g7dt7N0obDnIh6veBKQX3SlIPolQkvKrY+LqFrzx BORvmPwUJE7HXNVK0ooY9ZbF9yTEVJM9BXDC1ba6sB/yTJpm/azyjzoAyjs2DMyM16D8 ttbrEmDth9vWHEXdDq1shWyyjNftjQ09KsElf4fINFFKK2z3qavFDDXvmR2eBS69+H8/ wUp1+/WpGO5fkdDCttJYgISsU+MOngvL8+bsSxlSnUBw/vqJHVZKYnyndQcM0nFNb8h4 RLng== X-Gm-Message-State: AOAM53313wrZrrm1MkQb41/4j219WIkPsa3HpQ4pdN8V3+aZRcYY/r2k cGVFX9Id74JhxRo5a7AJ5IM= X-Google-Smtp-Source: ABdhPJxwX7tSauAwoz+2wEumjBcwtWDJqcKW8H5Ix2Y54w0yVWgPxtM0QNROKlhQaKw35RdTe7TqPw== X-Received: by 2002:a05:6e02:5ab:b0:2d1:9303:1332 with SMTP id k11-20020a056e0205ab00b002d193031332mr18560323ils.168.1653824228821; Sun, 29 May 2022 04:37:08 -0700 (PDT) Received: from n2.us-central1-a.c.spheric-algebra-350919.internal (151.16.70.34.bc.googleusercontent.com. [34.70.16.151]) by smtp.gmail.com with ESMTPSA id q22-20020a0566380d1600b0032eec10773esm1954886jaj.16.2022.05.29.04.37.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 29 May 2022 04:37:07 -0700 (PDT) Date: Sun, 29 May 2022 11:37:06 +0000 From: Hyeonggon Yoo <42.hyeyoo@gmail.com> To: Rongwei Wang 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 Message-ID: References: <20220529081535.69275-1-rongwei.wang@linux.alibaba.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220529081535.69275-1-rongwei.wang@linux.alibaba.com> X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: A8DA540055 X-Stat-Signature: 7thd1bx5dzm5dic9mj7fbcnt7kc9hs7r X-Rspam-User: Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=e4ZYCc9o; spf=pass (imf04.hostedemail.com: domain of 42.hyeyoo@gmail.com designates 209.85.166.174 as permitted sender) smtp.mailfrom=42.hyeyoo@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-HE-Tag: 1653824211-585641 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: 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 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 >