Received-SPF: pass (google.com: domain of linux-kernel+bounces-101275-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240304081348.197341-1-21cnbao@gmail.com> <20240304081348.197341-4-21cnbao@gmail.com>
 <e73c12ff-5234-44d5-a2b3-99cdc61a9c37@arm.com> <24dc6251-8582-790f-bbd3-465deed946f5@oppo.com>
 <f880135f-e113-4d42-b3a0-8b0b9eebcbf4@arm.com>
In-Reply-To: <f880135f-e113-4d42-b3a0-8b0b9eebcbf4@arm.com>
From: Chuanhua Han <chuanhuahan@gmail.com>
Date: Wed, 13 Mar 2024 17:24:33 +0800
Message-ID: <CANzGp4+w-e+Xg8B=1pSbdoTuftPiYRvFMKr9GiOXUs44B5SLig@mail.gmail.com>
Subject: Re: [RFC PATCH v3 3/5] mm: swap: make should_try_to_free_swap()
 support large-folio
To: Ryan Roberts <ryan.roberts@arm.com>
Cc: Chuanhua Han <hanchuanhua@oppo.com>, Barry Song <21cnbao@gmail.com>, akpm@linux-foundation.org, 
	linux-mm@kvack.org, chengming.zhou@linux.dev, chrisl@kernel.org, 
	david@redhat.com, hannes@cmpxchg.org, kasong@tencent.com, 
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, 
	mhocko@suse.com, nphamcs@gmail.com, shy828301@gmail.com, steven.price@arm.com, 
	surenb@google.com, wangkefeng.wang@huawei.com, willy@infradead.org, 
	xiang@kernel.org, ying.huang@intel.com, yosryahmed@google.com, 
	yuzhao@google.com, Barry Song <v-songbaohua@oppo.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Ryan,
Ryan Roberts <ryan.roberts@arm.com> =E4=BA=8E2024=E5=B9=B43=E6=9C=8813=E6=
=97=A5=E5=91=A8=E4=B8=89 17:10=E5=86=99=E9=81=93=EF=BC=9A
>
> On 13/03/2024 02:21, Chuanhua Han wrote:
> > hi, Ryan Roberts
> >
> > =E5=9C=A8 2024/3/12 20:34, Ryan Roberts =E5=86=99=E9=81=93:
> >> On 04/03/2024 08:13, Barry Song wrote:
> >>> From: Chuanhua Han <hanchuanhua@oppo.com>
> >>>
> >>> should_try_to_free_swap() works with an assumption that swap-in is al=
ways done
> >>> at normal page granularity, aka, folio_nr_pages =3D 1. To support lar=
ge folio
> >>> swap-in, this patch removes the assumption.
> >>>
> >>> Signed-off-by: Chuanhua Han <hanchuanhua@oppo.com>
> >>> Co-developed-by: Barry Song <v-songbaohua@oppo.com>
> >>> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> >>> Acked-by: Chris Li <chrisl@kernel.org>
> >>> ---
> >>>  mm/memory.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/mm/memory.c b/mm/memory.c
> >>> index abd4f33d62c9..e0d34d705e07 100644
> >>> --- a/mm/memory.c
> >>> +++ b/mm/memory.c
> >>> @@ -3837,7 +3837,7 @@ static inline bool should_try_to_free_swap(stru=
ct folio *folio,
> >>>      * reference only in case it's likely that we'll be the exlusive =
user.
> >>>      */
> >>>     return (fault_flags & FAULT_FLAG_WRITE) && !folio_test_ksm(folio)=
 &&
> >>> -           folio_ref_count(folio) =3D=3D 2;
> >>> +           folio_ref_count(folio) =3D=3D (1 + folio_nr_pages(folio))=
;
> >> I don't think this is correct; one reference has just been added to th=
e folio in
> >> do_swap_page(), either by getting from swapcache (swap_cache_get_folio=
()) or by
> >> allocating. If it came from the swapcache, it could be a large folio, =
because we
> >> swapped out a large folio and never removed it from swapcache. But in =
that case,
> >> others may have partially mapped it, so the refcount could legitimatel=
y equal
> >> the number of pages while still not being exclusively mapped.
> >>
> >> I'm guessing this logic is trying to estimate when we are likely exclu=
sive so
> >> that we remove from swapcache (release ref) and can then reuse rather =
than CoW
> >> the folio? The main CoW path currently CoWs page-by-page even for larg=
e folios,
> >> and with Barry's recent patch, even the last page gets copied. So not =
sure what
> >> this change is really trying to achieve?
> >>
> > First, if it is a large folio in the swap cache, then its refcont is at
> > least folio_nr_pages(folio) :
>
> Ahh! Sorry, I had it backwards - was thinking there would be 1 ref for th=
e swap
> cache, and you were assuming 1 ref per page taken by do_swap_page(). I
> understand now. On this basis:
>
> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>

Thank you for your review!
>
> >
> >
> > For example, in add_to_swap_cache path:
> >
> > int add_to_swap_cache(struct folio *folio, swp_entry_t entry,
> >                         gfp_t gfp, void **shadowp)
> > {
> >         struct address_space *address_space =3D swap_address_space(entr=
y);
> >         pgoff_t idx =3D swp_offset(entry);
> >         XA_STATE_ORDER(xas, &address_space->i_pages, idx,
> > folio_order(folio));
> >         unsigned long i, nr =3D folio_nr_pages(folio); <---
> >         void *old;
> >         ...
> >         folio_ref_add(folio, nr); <---
> >         folio_set_swapcache(folio);
> >         ...
> > }
> >
> >
> >   *
> >
> >     Then in the do_swap_page path:
> >
> >   * if (should_try_to_free_swap(folio, vma, vmf->flags))
> >             folio_free_swap(folio);
> >   *
> >
> >   * It also indicates that only folio in the swap cache will call
> >     folio_free_swap
> >   * to delete it from the swap cache, So I feel like this patch is
> >     necessary!? =F0=9F=98=81
> >
> >>>  }
> >>>
> >>>  static vm_fault_t pte_marker_clear(struct vm_fault *vmf)
> >
> > Thanks,
> >
> > Chuanhua
> >
>
>
Thanks,
Chuanhua