Received-SPF: pass (google.com: domain of linux-kernel+bounces-36400-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: "Huang, Ying" <ying.huang@intel.com>
To: Yosry Ahmed <yosryahmed@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,  Johannes Weiner
 <hannes@cmpxchg.org>,  Nhat Pham <nphamcs@gmail.com>,  Chris Li
 <chrisl@kernel.org>,  Chengming Zhou <zhouchengming@bytedance.com>,
  linux-mm@kvack.org,  linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] mm: swap: update inuse_pages after all cleanups are
 done
In-Reply-To: <CAJD7tkbKxfuy-uWrOMVnOeDpx-TuJwosxk2jG_0Gx4bi1tUBog@mail.gmail.com>
	(Yosry Ahmed's message of "Tue, 23 Jan 2024 19:20:17 -0800")
References: <20240120024007.2850671-1-yosryahmed@google.com>
	<20240120024007.2850671-2-yosryahmed@google.com>
	<87wms0toh4.fsf@yhuang6-desk2.ccr.corp.intel.com>
	<CAJD7tkb=-0mP1CXEmAd4QjMXKgep7myHShiwUSNnY1cjfRqfJA@mail.gmail.com>
	<878r4ftodl.fsf@yhuang6-desk2.ccr.corp.intel.com>
	<CAJD7tkbKxfuy-uWrOMVnOeDpx-TuJwosxk2jG_0Gx4bi1tUBog@mail.gmail.com>
Date: Wed, 24 Jan 2024 11:27:56 +0800
Message-ID: <874jf3tnpf.fsf@yhuang6-desk2.ccr.corp.intel.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=ascii

Yosry Ahmed <yosryahmed@google.com> writes:

>> > In swap_range_free, we want to make sure that the write to
>> > si->inuse_pages in swap_range_free() happens *after* the cleanups
>> > (specifically zswap_invalidate() in this case).
>> > In swap_off, we want to make sure that the cleanups following
>> > try_to_unuse() (e.g. zswap_swapoff) happen *after* reading
>> > si->inuse_pages == 0 in try_to_unuse().
>> >
>> > So I think we want smp_wmb() in swap_range_free() and smp_mb() in
>> > try_to_unuse(). Does the below look correct to you?
>> >
>> > diff --git a/mm/swapfile.c b/mm/swapfile.c
>> > index 2fedb148b9404..a2fa2f65a8ddd 100644
>> > --- a/mm/swapfile.c
>> > +++ b/mm/swapfile.c
>> > @@ -750,6 +750,12 @@ static void swap_range_free(struct
>> > swap_info_struct *si, unsigned long offset,
>> >                 offset++;
>> >         }
>> >         clear_shadow_from_swap_cache(si->type, begin, end);
>> > +
>> > +       /*
>> > +        * Make sure that try_to_unuse() observes si->inuse_pages reaching 0
>> > +        * only after the above cleanups are done.
>> > +        */
>> > +       smp_wmb();
>> >         atomic_long_add(nr_entries, &nr_swap_pages);
>> >         WRITE_ONCE(si->inuse_pages, si->inuse_pages - nr_entries);
>> >  }
>> > @@ -2130,6 +2136,11 @@ static int try_to_unuse(unsigned int type)
>> >                 return -EINTR;
>> >         }
>> >
>> > +       /*
>> > +        * Make sure that further cleanups after try_to_unuse() returns happen
>> > +        * after swap_range_free() reduces si->inuse_pages to 0.
>> > +        */
>> > +       smp_mb();
>> >         return 0;
>> >  }
>>
>> We need to take care of "si->inuse_pages" checking at the beginning of
>> try_to_unuse() too.  Otherwise, it looks good to me.
>
> Hmm, why isn't one barrier at the end of the function enough? I think
> all we need is that before we return from try_to_unuse(), all the
> cleanups in swap_range_free() are taken care of, which the barrier at
> the end should be doing. We just want instructions after
> try_to_unuse() to not get re-ordered before si->inuse_pages is read as
> 0, right?

Because at the begin of try_to_unuse() as below, after reading, function
returns directly without any memory barriers.

  if (!READ_ONCE(si->inuse_pages))
        return 0;

--
Best Regards,
Huang, Ying