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 14025C433F5 for ; Sat, 5 Mar 2022 04:21:47 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6E9D38D0002; Fri, 4 Mar 2022 23:21:47 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 698A98D0001; Fri, 4 Mar 2022 23:21:47 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 512F38D0002; Fri, 4 Mar 2022 23:21:47 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (relay.hostedemail.com [64.99.140.28]) by kanga.kvack.org (Postfix) with ESMTP id 3DFC98D0001 for ; Fri, 4 Mar 2022 23:21:47 -0500 (EST) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 07759516 for ; Sat, 5 Mar 2022 04:21:47 +0000 (UTC) X-FDA: 79209034254.03.D61A751 Received: from mail-pj1-f50.google.com (mail-pj1-f50.google.com [209.85.216.50]) by imf30.hostedemail.com (Postfix) with ESMTP id 80CF680002 for ; Sat, 5 Mar 2022 04:21:46 +0000 (UTC) Received: by mail-pj1-f50.google.com with SMTP id cx5so8923468pjb.1 for ; Fri, 04 Mar 2022 20:21:46 -0800 (PST) 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=+WecTfOPB5EXS9NO8w7OJhZo+wpvPjeoC9Ba/0fEu0s=; b=V8R9hNSnyF2Wgb3QNGFSuVg0OqcmkPUclEYv4iRrpN/LDJjFvWry+/pfFYEAj2TaJm nnTnDiBI6eVBVhRlPMW7vawy7I/SEfbLAtyboRSrOIXkUb3pPx3o06AhJuCbhC4s7LGB MwhRmzeFMxdFqKSrVAxp11RMkjlhqxtLxne91Ldriqjg1AijsXb0qOaA5IkKU41sI9Yq vylMOaMI3e8/Yiaz6Qj1EfDRRZlV7iLG4Ehat0+xSnJ+kCoxMepfPRwZQwakJbltfaju TcgJqz6MN/5tzrtGzpLdqNEu6EgAOInuIpwhoVn7/EhqyFdoel6zho4b+Maty0N9T732 UQ7A== 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=+WecTfOPB5EXS9NO8w7OJhZo+wpvPjeoC9Ba/0fEu0s=; b=c+NOJS1jwBAgOWJo21V5ucX+fCkxT3EOXeHsXCBg0Xw2FRmtqnX/OHOEP8g9lIAY7f p4uhWvR2ObtghtQM+Il29tHew7xgcoCWsTdwCf12h+BaGrwKXcZ97uBHAtOaILrCULoh zW0GwkPNlKAOaUxsb+Uf/FxFqY6btKbxYBiGp7Z7wSly5QChchPrI1MYQ5WJo4tw+jap qVS4ahuVJix6gRH96WSl0hf/k0FtRPNAzeCJreapuSOrSyOZRZbeN67Bom1XmhD97jxm EV4tvEA5nigjk+TiguwjUMXEPlcKPE+aw6soDo4/ze1RO7lifRg1bURMBQZoqyOPekaa 2OwQ== X-Gm-Message-State: AOAM533gYkUZkKG8Y35PjzPXS92omeFTwdqzan7AfojgzRBkflrJGuJ+ 6do7Y4dH0KmhyoAN8I6s1jk= X-Google-Smtp-Source: ABdhPJy0PemOG682V8NIgEbnJiFKcBnPK7RUoXhG3leKLVwaTC3SRCuJ+pZWu6KWHvJrbQ1iVc1sgA== X-Received: by 2002:a17:90a:cf87:b0:1bd:3595:3ee4 with SMTP id i7-20020a17090acf8700b001bd35953ee4mr14182043pju.100.1646454105409; Fri, 04 Mar 2022 20:21:45 -0800 (PST) Received: from ip-172-31-19-208.ap-northeast-1.compute.internal (ec2-18-181-137-102.ap-northeast-1.compute.amazonaws.com. [18.181.137.102]) by smtp.gmail.com with ESMTPSA id j7-20020a056a00130700b004b9f7cd94a4sm7260689pfu.56.2022.03.04.20.21.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 04 Mar 2022 20:21:44 -0800 (PST) Date: Sat, 5 Mar 2022 04:21:39 +0000 From: Hyeonggon Yoo <42.hyeyoo@gmail.com> To: Vlastimil Babka Cc: linux-mm@kvack.org, Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Andrew Morton , Marco Elver , Matthew WilCox , Roman Gushchin , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 5/5] mm/slub: refactor deactivate_slab() Message-ID: References: <20220304063427.372145-1-42.hyeyoo@gmail.com> <20220304063427.372145-6-42.hyeyoo@gmail.com> <6763e97b-88bf-59b0-c80e-26c3846531fc@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6763e97b-88bf-59b0-c80e-26c3846531fc@suse.cz> X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 80CF680002 X-Rspam-User: Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=V8R9hNSn; spf=pass (imf30.hostedemail.com: domain of 42.hyeyoo@gmail.com designates 209.85.216.50 as permitted sender) smtp.mailfrom=42.hyeyoo@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-Stat-Signature: q3hixcjynm7z4wen97sszcog9ukhohu9 X-HE-Tag: 1646454106-204808 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 Fri, Mar 04, 2022 at 08:01:20PM +0100, Vlastimil Babka wrote: > On 3/4/22 07:34, Hyeonggon Yoo 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. ] > > > > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> > > Looks good, just noticed a tiny issue. > > > --- > > mm/slub.c | 81 ++++++++++++++++++++++--------------------------------- > > 1 file changed, 32 insertions(+), 49 deletions(-) > > > > diff --git a/mm/slub.c b/mm/slub.c > > index f9ae983a3dc6..c1a693ec5874 100644 > > --- a/mm/slub.c > > +++ b/mm/slub.c > > @@ -2344,8 +2344,8 @@ static void deactivate_slab(struct kmem_cache *s, struct slab *slab, > > { > > enum slab_modes { M_NONE, M_PARTIAL, M_FULL, M_FREE }; > > 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; > > @@ -2387,14 +2387,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 slub is not > > + * unfrozen and number of objects in the slab may have changed. > > + * Then release lock and retry cmpxchg again. > > */ > > redo: > > > > @@ -2414,57 +2410,44 @@ 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) { > > This used to set m = M_FULL; always. > > > - 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); > > - } > > + 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; > > Now you only set it for SLAB_STORE_USER caches. > > > + /* > > + * 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); > > - > > - if (m == M_PARTIAL) > > - add_partial(n, slab, tail); > > - else if (m == M_FULL) > > - 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); > > goto redo; > > + } > > > > - 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) > > + } else if (mode == M_FULL) { > > + add_full(s, n, slab); > > + spin_unlock_irqrestore(&n->list_lock, flags); > > stat(s, DEACTIVATE_FULL); > > As a result, full slabs without SLAB_STORE_USER will not count > DEACTIVATE_FULL anymore. Oh, thank you for catching this. We usually only deactivate full slabs for debugging to track all list of slabs. But as you pointed, in rare case when pfmemalloc flag does not match, full slabs can be deactivated (even if they are not put on list). > I guess the easiest way to solve it is to e.g. add a M_FULL_NOLIST mode that > only does the DEACTIVATE_FULL counting. That will be enough solution. Will fix this in v3. Thank you! > > - else if (m == M_FREE) { > > + } else if (mode == M_FREE) { > > stat(s, DEACTIVATE_EMPTY); > > discard_slab(s, slab); > > stat(s, FREE_SLAB); > -- Thank you, You are awesome! Hyeonggon :-)