Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 13 May 2021 14:34:19 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Muchun Song <smuchun@gmail.com>
Cc:     Miaohe Lin <linmiaohe@huawei.com>,
        Huang Ying <ying.huang@intel.com>,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Daniel Jordan <daniel.m.jordan@oracle.com>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Andrea Parri <andrea.parri@amarulasolutions.com>,
        Andi Kleen <ak@linux.intel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Omar Sandoval <osandov@fb.com>,
        Paul McKenney <paulmck@kernel.org>, Tejun Heo <tj@kernel.org>,
        Will Deacon <will.deacon@arm.com>
Subject: Re: [PATCH] mm, swap: Remove unnecessary smp_rmb() in
 swap_type_to_swap_info()
Message-ID: <YJ0cy00kJdx9MJYJ@hirez.programming.kicks-ass.net>
References: <20210513064837.3949064-1-ying.huang@intel.com>
 <176421ba-77d9-8422-21e0-b5e2951533bd@huawei.com>
 <CAPSr9jEtdSgr5kDX=ESUrVtbuonUNEeFsOHpYYoFxD3PQ20C2A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAPSr9jEtdSgr5kDX=ESUrVtbuonUNEeFsOHpYYoFxD3PQ20C2A@mail.gmail.com>
Precedence: bulk

On Thu, May 13, 2021 at 05:54:42PM +0800, Muchun Song wrote:
> On Thu, May 13, 2021 at 5:11 PM Miaohe Lin <linmiaohe@huawei.com> wrote:
> > On 2021/5/13 14:48, Huang Ying wrote:

> > >  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.

This statement doesn't make sense; this isn't code that needs to be
correct on 'some' archs, it needs to be unconditionally correct.

Also, you cannot pair with a single memop, there is no order in a set of
one element.

And if you depend on a data dependency, you need a store order; but you
just removed the store order. in which case the data dependency is also
moot.

All of this is utter confusion. Possibly correct, but a complete
trainwreck non-the-less.

Either you say ordering is irrelevant, because we only ever increase the
number of swapfiles and therefore any load is either NULL or the correct
pointer, as guaranteed by WRITE_ONCE()/READ_ONCE() avoiding load/store
tearing.

Or you need the data dependency, but then you also need the store order
like:

	CPU0					CPU1

	if (type >= READ_ONCE(nr_swapfiles))	WRITE_ONCE(swap_info[type], p);
		return NULL;
	/* data-dependency on type */		smp_wmb();
	return READ_ONCE(swap_info[type]);	WRITE_ONCE(nr_swapfiles, nr_swapfiles+1);

But you cannot have half of both and expect any of it to make sense.