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 AB72DC433F5 for ; Tue, 8 Mar 2022 05:29:40 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0F9278D0002; Tue, 8 Mar 2022 00:29:40 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 0A7668D0001; Tue, 8 Mar 2022 00:29:40 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EB0F28D0002; Tue, 8 Mar 2022 00:29:39 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0164.hostedemail.com [216.40.44.164]) by kanga.kvack.org (Postfix) with ESMTP id D82E48D0001 for ; Tue, 8 Mar 2022 00:29:39 -0500 (EST) Received: from smtpin25.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 79A68AA16E for ; Tue, 8 Mar 2022 05:29:39 +0000 (UTC) X-FDA: 79220091678.25.1D63FCC Received: from mail-pj1-f48.google.com (mail-pj1-f48.google.com [209.85.216.48]) by imf29.hostedemail.com (Postfix) with ESMTP id 161AF12000A for ; Tue, 8 Mar 2022 05:29:38 +0000 (UTC) Received: by mail-pj1-f48.google.com with SMTP id c16-20020a17090aa61000b001befad2bfaaso1388706pjq.1 for ; Mon, 07 Mar 2022 21:29:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=H0vjB4LhDXi7QSy6fRqdPlD7Qk1SfEX7MwnN7FiKGsk=; b=K5xZQW4FSG0AwQk78uctp+7ieDiri1nE0kAIbOd0P4RY/tZvDK0mkLHgXHMNh7+EnH 9+9RpeemFzC5MsSEdgdOsi+4zLr76RHlS2TSlcRvoV4gGFoXwqfJMmwuHv5rENkXlkGz ooYBMNSIwwmrymCi6lsFuf1r1WTpO2yMnzoUDFKTGjNAChCdY2XhhZr56A2G7XsXEZh1 WpEhrinjLF917I7ug+mDDsSWHevuDW0JQl/qtdZRWV70Nq2vxIk6SJQMfXdnNph+HFaj +kkcUhjz4qtELmYhh6kFAH33aXpLp2L4GN3brfAuIpLhlMl5Th8WVImKCT6MIM8CcR1L +rkA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=H0vjB4LhDXi7QSy6fRqdPlD7Qk1SfEX7MwnN7FiKGsk=; b=fIb/2KKU7qDa6rFnDL/hPV3bfdGnucWcTYP1VnIuzDfplcsWkro4S9nPODE7TbXhFu GrFougKeexYIY/hZOrQj50hNNLLAyFJue2j6VQDWxYGAtu0Z/OorU1qzeZHIO5oU0xwd fegrbgTHaNG0U7NjCyatDHN6gOCsIFZswWmn1wJTDV4CgcLBw/yWgqzBx/37MhiKYJ09 iJ9XWb8I3lNYVI05voarnu67cJDxWr425AKiWoMSP8d4vStzIl5NH6CQj4NZkLAXVy0f 0rUZgJnUwWxF51fpBkX1TXnjZp2Fgzcfy11f/ruBgw6fqXeu5YVCqDgYAAWeMp0M5Kia TDEQ== X-Gm-Message-State: AOAM533Y7HJcAzZGx5e1bjLSXDa8+9AfrFoqcxS6wuLUwxbxJYPLzDtM A5bf8u6Vke6K4ll7pTs4YBMdnARc8CztGnyEoK4= X-Google-Smtp-Source: ABdhPJzQCmoHetwvICyKDq+FjNCCmJ0zxHZ97kcLUUPejVKBw7nLSqgJTXlf8uNkmJ1RadVpM5ASOJwUkUdckr7UQOY= X-Received: by 2002:a17:90b:4b4b:b0:1be:fd9a:c279 with SMTP id mi11-20020a17090b4b4b00b001befd9ac279mr2850397pjb.83.1646717378007; Mon, 07 Mar 2022 21:29:38 -0800 (PST) MIME-Version: 1.0 References: <20220307074057.902222-1-42.hyeyoo@gmail.com> <20220307074057.902222-3-42.hyeyoo@gmail.com> In-Reply-To: From: Xiongwei Song Date: Tue, 8 Mar 2022 13:29:11 +0800 Message-ID: Subject: Re: [PATCH v3 2/2] mm/slub: refactor deactivate_slab() To: Hyeonggon Yoo <42.hyeyoo@gmail.com> Cc: linux-mm@kvack.org, Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Andrew Morton , Vlastimil Babka , Marco Elver , Matthew WilCox , Roman Gushchin , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 161AF12000A X-Stat-Signature: nxchdcosgt6igp7k635wj49bubrrgnwe X-Rspam-User: Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=K5xZQW4F; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf29.hostedemail.com: domain of sxwjean@gmail.com designates 209.85.216.48 as permitted sender) smtp.mailfrom=sxwjean@gmail.com X-Rspamd-Server: rspam03 X-HE-Tag: 1646717378-702639 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 Tue, Mar 8, 2022 at 11:50 AM Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: > > On Tue, Mar 08, 2022 at 09:40:07AM +0800, Xiongwei Song wrote: > > Hello, > > > > On Mon, Mar 7, 2022 at 3:41 PM Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: > > > > > > Simplify deactivate_slab() by unlocking n->list_lock and retrying > > > cmpxchg_double() when cmpxchg_double() fails, and perform > > > add_{partial,full} only when it succeed. > > > > > > Releasing and taking n->list_lock again here is not harmful as SLUB > > > avoids deactivating slabs as much as possible. > > > > > > [ vbabka@suse.cz: perform add_{partial,full} when cmpxchg_double() > > > succeed. > > > > > > count deactivating full slabs even if debugging flag is not set. ] > > > > > > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> > > > --- > > > mm/slub.c | 91 +++++++++++++++++++++++-------------------------------- > > > 1 file changed, 38 insertions(+), 53 deletions(-) > > > > > > diff --git a/mm/slub.c b/mm/slub.c > > > index 1ce09b0347ad..f0cb9d0443ac 100644 > > > --- a/mm/slub.c > > > +++ b/mm/slub.c > > > @@ -2348,10 +2348,10 @@ static void init_kmem_cache_cpus(struct kmem_cache *s) > > > static void deactivate_slab(struct kmem_cache *s, struct slab *slab, > > > void *freelist) > > > { > > > - enum slab_modes { M_NONE, M_PARTIAL, M_FULL, M_FREE }; > > > + enum slab_modes { M_NONE, M_PARTIAL, M_FULL, M_FREE, M_FULL_NOLIST }; > > > struct kmem_cache_node *n = get_node(s, slab_nid(slab)); > > > - int lock = 0, free_delta = 0; > > > - enum slab_modes l = M_NONE, m = M_NONE; > > > + int free_delta = 0; > > > + enum slab_modes mode = M_NONE; > > > void *nextfree, *freelist_iter, *freelist_tail; > > > int tail = DEACTIVATE_TO_HEAD; > > > unsigned long flags = 0; > > > @@ -2393,14 +2393,10 @@ static void deactivate_slab(struct kmem_cache *s, struct slab *slab, > > > * Ensure that the slab is unfrozen while the list presence > > > * reflects the actual number of objects during unfreeze. > > > * > > > - * We setup the list membership and then perform a cmpxchg > > > - * with the count. If there is a mismatch then the slab > > > - * is not unfrozen but the slab is on the wrong list. > > > - * > > > - * Then we restart the process which may have to remove > > > - * the slab from the list that we just put it on again > > > - * because the number of objects in the slab may have > > > - * changed. > > > + * We first perform cmpxchg holding lock and insert to list > > > + * when it succeed. If there is mismatch then the slab is not > > > + * unfrozen and number of objects in the slab may have changed. > > > + * Then release lock and retry cmpxchg again. > > > */ > > > redo: > > > > > > @@ -2420,61 +2416,50 @@ static void deactivate_slab(struct kmem_cache *s, struct slab *slab, > > > new.frozen = 0; > > > > > > if (!new.inuse && n->nr_partial >= s->min_partial) > > > - m = M_FREE; > > > + mode = M_FREE; > > > else if (new.freelist) { > > > - m = M_PARTIAL; > > > - if (!lock) { > > > - lock = 1; > > > - /* > > > - * Taking the spinlock removes the possibility that > > > - * acquire_slab() will see a slab that is frozen > > > - */ > > > - spin_lock_irqsave(&n->list_lock, flags); > > > - } > > > - } else { > > > - m = M_FULL; > > > - if (kmem_cache_debug_flags(s, SLAB_STORE_USER) && !lock) { > > > - lock = 1; > > > - /* > > > - * This also ensures that the scanning of full > > > - * slabs from diagnostic functions will not see > > > - * any frozen slabs. > > > - */ > > > - spin_lock_irqsave(&n->list_lock, flags); > > > - } > > > - } > > > - > > > - if (l != m) { > > > - if (l == M_PARTIAL) > > > - remove_partial(n, slab); > > > - else if (l == M_FULL) > > > - remove_full(s, n, slab); > > > + mode = M_PARTIAL; > > > + /* > > > + * Taking the spinlock removes the possibility that > > > + * acquire_slab() will see a slab that is frozen > > > + */ > > > + spin_lock_irqsave(&n->list_lock, flags); > > > + } else if (kmem_cache_debug_flags(s, SLAB_STORE_USER)) { > > > + mode = M_FULL; > > > + /* > > > + * This also ensures that the scanning of full > > > + * slabs from diagnostic functions will not see > > > + * any frozen slabs. > > > + */ > > > + spin_lock_irqsave(&n->list_lock, flags); > > > + } else > > > + mode = M_FULL_NOLIST; > > > > > > - if (m == M_PARTIAL) > > > - add_partial(n, slab, tail); > > > - else if (m == M_FULL) > i> > - add_full(s, n, slab); > > > - } > > > > > > - l = m; > > > if (!cmpxchg_double_slab(s, slab, > > > old.freelist, old.counters, > > > new.freelist, new.counters, > > > - "unfreezing slab")) > > > + "unfreezing slab")) { > > > + if (mode == M_PARTIAL || mode == M_FULL) > > > + spin_unlock_irqrestore(&n->list_lock, flags); > > > > The slab doesn't belong to any node here, should we remove locking/unlocking > > spin for cmpxchg_double_slab() call? Just calling spin_lock_irqsave() before > > add_partial()/add_full call is fine? > > > > I thought about that, and tested, but that is not okay. > > taking spinlock around cmpxchg prevents race between __slab_free() and > deactivate_slab(). list can be corrupted without spinlock. > > > think about case below: (when SLAB_STORE_USER is set) > > __slab_free() deactivate_slab() > ================= ================= > (deactivating full slab) > cmpxchg_double() > > > spin_lock_irqsave() > cmpxchg_double() > > /* not in full list yet */ > remove_full() > add_partial() > spin_unlock_irqrestore() > spin_lock_irqsave() > add_full() > spin_unlock_irqrestore() > Oh... Looks reasonable. Thanks for the detailed explanation. > > > > > goto redo; > > > > How about do {...} while(!cmpxchg_double_slab())? The readability looks better? > > > > This will be: > > do { > if (mode == M_PARTIAL || mode == M_FULL) > spin_unlock_irqrestore(); > > [...] > > } while (!cmpxchg_double_slab()); > I saw __slab_free() is doing so. Not a big deal. Regards, Xiongwei > I think goto version is better for reading? > > Thanks! > > > Regards, > > Xiongwei > > > > > + } > > > > > > - if (lock) > > > - spin_unlock_irqrestore(&n->list_lock, flags); > > > > > > - if (m == M_PARTIAL) > > > + if (mode == M_PARTIAL) { > > > + add_partial(n, slab, tail); > > > + spin_unlock_irqrestore(&n->list_lock, flags); > > > stat(s, tail); > > > - else if (m == M_FULL) > > > - stat(s, DEACTIVATE_FULL); > > > - else if (m == M_FREE) { > > > + } else if (mode == M_FREE) { > > > stat(s, DEACTIVATE_EMPTY); > > > discard_slab(s, slab); > > > stat(s, FREE_SLAB); > > > - } > > > + } else if (mode == M_FULL) { > > > + add_full(s, n, slab); > > > + spin_unlock_irqrestore(&n->list_lock, flags);y > > > + stat(s, DEACTIVATE_FULL); > > > + } else if (mode == M_FULL_NOLIST) > > > + stat(s, DEACTIVATE_FULL); > > > } > > > > > > #ifdef CONFIG_SLUB_CPU_PARTIAL > > > -- > > > 2.33.1 > > > > > > > > -- > Thank you, You are awesome! > Hyeonggon :-)