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 X-Spam-Level: X-Spam-Status: No, score=-12.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DF843C43460 for ; Thu, 13 May 2021 09:55:22 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 37C206143D for ; Thu, 13 May 2021 09:55:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 37C206143D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 585566B0036; Thu, 13 May 2021 05:55:21 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 50E8E6B006E; Thu, 13 May 2021 05:55:21 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3623A6B0070; Thu, 13 May 2021 05:55:21 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0066.hostedemail.com [216.40.44.66]) by kanga.kvack.org (Postfix) with ESMTP id 010356B0036 for ; Thu, 13 May 2021 05:55:20 -0400 (EDT) Received: from smtpin10.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 95A21B9EF for ; Thu, 13 May 2021 09:55:20 +0000 (UTC) X-FDA: 78135750000.10.293277C Received: from mail-io1-f48.google.com (mail-io1-f48.google.com [209.85.166.48]) by imf08.hostedemail.com (Postfix) with ESMTP id 663E780192F3 for ; Thu, 13 May 2021 09:54:52 +0000 (UTC) Received: by mail-io1-f48.google.com with SMTP id k16so10295714ios.10 for ; Thu, 13 May 2021 02:55:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=OQjtHIN8DD1GRyqL6npu5iRZFcZfv7jbqOPudBmj234=; b=KR571OjfjGRU7EPduxHdCzsp8zyXDadZglBKAgsHugklZvh6uUF7ETCczN0zdkkLFb FJtsw+mH+T8jpmA2TqknhDcWZ7+SPVrJWD73+fzpLLWX9xczUI0VlzIcEerqooXWRK0V jtkw0tPtkvr2i6ej6/OdYtMibeM+JcFoX2SgDRW17xL/WNjNFFrdaYe/N/mD7twyZCCX n1ViNvLlMoTu8K3V4a/r44c3DRDruPlw0ont/lm3+CwzgK+nsqABFhF2ESe86ueelit8 nqQ60ovdw7PPXPSYVweTvqacqRxGnO1hjWNEHbg4slc7dx4e1ERA0yrwtEiZIJAyR9w6 7QWw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=OQjtHIN8DD1GRyqL6npu5iRZFcZfv7jbqOPudBmj234=; b=aGXWmb35EOHTxP03x0kCakRw3WeTCRQQ/OU3ng9kd9cKAuOZCBxw3kdwTQ1r8z+80v AJWT5eIJY2Iw4F07RGjaZuYBJP8+zky6Rp8IY2hIs2u9o+TaquCGK4ONGkucXrbtMq/V bnJfcgNkcMCwhFG9o/uuLVgkGhmvS83x+zVE5gVNp0h8c4LI/j3VJOAQVPgkYXwm1ptH mzH166WJH7UBsu0GCV6UMwJ7cjgmMSqaitWqmm9wBIxlrybbMMqmzUkh0LX5AjJ3pzfS BRjoeSo/EaWPyVkeA/RsZ72gnXUCgI5rW2HiEETNdGEfVONn0kzRdXJTjR8o2q6B2HNl 9VMA== X-Gm-Message-State: AOAM533AKsdfR7DGJcY+Z2F7DNf3/2a+o/OTtaLzG/62OQd+AqvWYNKa p1nv2UY+2pPMiXpLqPfu6SA/3TumGHfv2PZyUl0= X-Google-Smtp-Source: ABdhPJytuc/g1Ce6PPopObf0K4vYWWrUyaS/62Xlt/kuREkWqv+7arg44dPMsMv5HP9fCA4606DhLa26FZGnJPQjJs8= X-Received: by 2002:a5d:8506:: with SMTP id q6mr13633247ion.11.1620899719619; Thu, 13 May 2021 02:55:19 -0700 (PDT) MIME-Version: 1.0 References: <20210513064837.3949064-1-ying.huang@intel.com> <176421ba-77d9-8422-21e0-b5e2951533bd@huawei.com> In-Reply-To: <176421ba-77d9-8422-21e0-b5e2951533bd@huawei.com> From: Muchun Song Date: Thu, 13 May 2021 17:54:42 +0800 Message-ID: Subject: Re: [PATCH] mm, swap: Remove unnecessary smp_rmb() in swap_type_to_swap_info() To: Miaohe Lin Cc: Huang Ying , Andrew Morton , linux-mm@kvack.org, linux-kernel , Daniel Jordan , Dan Carpenter , Andrea Parri , Peter Zijlstra , Andi Kleen , Dave Hansen , Omar Sandoval , Paul McKenney , Tejun Heo , Will Deacon Content-Type: text/plain; charset="UTF-8" Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=gmail.com header.s=20161025 header.b=KR571Ojf; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf08.hostedemail.com: domain of smuchun@gmail.com designates 209.85.166.48 as permitted sender) smtp.mailfrom=smuchun@gmail.com X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 663E780192F3 X-Stat-Signature: wc4nty7fty4hbx9hf3kg4deymouzxbzu Received-SPF: none (gmail.com>: No applicable sender policy available) receiver=imf08; identity=mailfrom; envelope-from=""; helo=mail-io1-f48.google.com; client-ip=209.85.166.48 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1620899692-278961 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 Thu, May 13, 2021 at 5:11 PM Miaohe Lin wrote: > > On 2021/5/13 14:48, Huang Ying wrote: > > Before commit c10d38cc8d3e ("mm, swap: bounds check swap_info array > > accesses to avoid NULL derefs"), the typical code to reference the > > swap_info[] is as follows, > > > > type = swp_type(swp_entry); > > if (type >= nr_swapfiles) > > /* handle invalid swp_entry */; > > p = swap_info[type]; > > /* access fields of *p. OOPS! p may be NULL! */ > > > > Because the ordering isn't guaranteed, it's possible that "p" is read > > before checking "type". And that may result in NULL pointer > > dereference. > > > > So in commit c10d38cc8d3e, the code becomes, > > > > struct swap_info_struct *swap_type_to_swap_info(int type) > > { > > if (type >= READ_ONCE(nr_swapfiles)) > > return NULL; > > smp_rmb(); > > return READ_ONCE(swap_info[type]); > > } > > > > /* users */ > > type = swp_type(swp_entry); > > p = swap_type_to_swap_info(type); > > if (!p) > > /* handle invalid swp_entry */; > > /* access fields of *p */ > > > > Because "p" is checked to be non-zero before dereference, smp_rmb() > > isn't needed anymore. > > > > We still need to guarantee swap_info[type] is read before dereference. > > That can be satisfied via the data dependency ordering of > > READ_ONCE(swap_info[type]). The corresponding smp_wmb() is adjusted > > in alloc_swap_info() too. > > > > And, we don't need to read "nr_swapfiles" too. Because if > > "type >= nr_swapfiles", swap_info[type] will be NULL. We just need > > to make sure we will not access out of the boundary of the array. > > With that change, nr_swapfiles will only be accessed with swap_lock > > held, except in swapcache_free_entries(). Where the absolute > > correctness of the value isn't needed, as described in the comments. > > > > Signed-off-by: "Huang, Ying" > > Cc: Daniel Jordan > > Cc: Dan Carpenter > > Cc: Andrea Parri > > Cc: Peter Zijlstra (Intel) > > Cc: Andi Kleen > > Cc: Dave Hansen > > Cc: Omar Sandoval > > Cc: Paul McKenney > > Cc: Tejun Heo > > Cc: Will Deacon > > Cc: Miaohe Lin > > --- > > mm/swapfile.c | 18 +++++++++--------- > > 1 file changed, 9 insertions(+), 9 deletions(-) > > > > diff --git a/mm/swapfile.c b/mm/swapfile.c > > index 2aad85751991..4c1fb28bbe0e 100644 > > --- a/mm/swapfile.c > > +++ b/mm/swapfile.c > > @@ -100,10 +100,14 @@ atomic_t nr_rotate_swap = ATOMIC_INIT(0); > > > > static struct swap_info_struct *swap_type_to_swap_info(int type) > > { > > - if (type >= READ_ONCE(nr_swapfiles)) > > + if (type >= MAX_SWAPFILES) > > return NULL; > > > > - smp_rmb(); /* Pairs with smp_wmb in alloc_swap_info. */ > > + /* > > + * The data dependency ordering from the READ_ONCE() pairs > > + * with smp_wmb() in alloc_swap_info() to guarantee the > > + * swap_info_struct fields are read after swap_info[type]. > > + */ > > return READ_ONCE(swap_info[type]); > > } > > > > @@ -2884,14 +2888,10 @@ static struct swap_info_struct *alloc_swap_info(void) > > } > > if (type >= nr_swapfiles) { > > p->type = type; > > - WRITE_ONCE(swap_info[type], p); > > - /* > > - * Write swap_info[type] before nr_swapfiles, in case a > > - * racing procfs swap_start() or swap_next() is reading them. > > - * (We never shrink nr_swapfiles, we never free this entry.) > > - */ > > + /* Paired with READ_ONCE() in swap_type_to_swap_info() */ > > smp_wmb(); > > Many thank for your patch. The patch looks fine to me. There is one question: > > There is no smp_rmb() paired with above smp_wmb(). What is this smp_wmb() used for ? > Could you please have a explanation ? The comment is very clear, it matches READ_ONCE() which implies a data dependence barrier on some archs. Thanks. > > Thanks again! > > > - WRITE_ONCE(nr_swapfiles, nr_swapfiles + 1); > > + WRITE_ONCE(swap_info[type], p); > > + nr_swapfiles++; > > } else { > > defer = p; > > p = swap_info[type]; > > >