Received-SPF: pass (google.com: domain of linux-kernel+bounces-178412-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240513074712.7608-1-ioworker0@gmail.com> <20240513074712.7608-2-ioworker0@gmail.com>
 <2ec286a2-4620-4e6d-ad3d-9b4c0d9e1394@linux.alibaba.com>
In-Reply-To: <2ec286a2-4620-4e6d-ad3d-9b4c0d9e1394@linux.alibaba.com>
From: Lance Yang <ioworker0@gmail.com>
Date: Tue, 14 May 2024 15:43:21 +0800
Message-ID: <CAK1f24maLXzb012QOiVitcb-+7gi=64scCtW4H3LLFvbZs7QGw@mail.gmail.com>
Subject: Re: [PATCH RESEND v5 1/4] mm/rmap: remove duplicated exit code in
 pagewalk loop
To: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: akpm@linux-foundation.org, willy@infradead.org, sj@kernel.org, 
	maskray@google.com, ziy@nvidia.com, ryan.roberts@arm.com, david@redhat.com, 
	21cnbao@gmail.com, mhocko@suse.com, fengwei.yin@intel.com, zokeefe@google.com, 
	shy828301@gmail.com, xiehuan09@gmail.com, libang.li@antgroup.com, 
	wangkefeng.wang@huawei.com, songmuchun@bytedance.com, peterx@redhat.com, 
	minchan@kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Baolin,

Thanks for taking time to review!

Best,
Lance

On Tue, May 14, 2024 at 2:26=E2=80=AFPM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
>
>
> On 2024/5/13 15:47, Lance Yang wrote:
> > Introduce the labels walk_done and walk_done_err as exit points to
> > eliminate duplicated exit code in the pagewalk loop.
> >
> > Reviewed-by: Zi Yan <ziy@nvidia.com>
> > Signed-off-by: Lance Yang <ioworker0@gmail.com>
>
> LGTM.
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>
> > ---
> >   mm/rmap.c | 40 +++++++++++++++-------------------------
> >   1 file changed, 15 insertions(+), 25 deletions(-)
> >
> > diff --git a/mm/rmap.c b/mm/rmap.c
> > index e8fc5ecb59b2..ddffa30c79fb 100644
> > --- a/mm/rmap.c
> > +++ b/mm/rmap.c
> > @@ -1679,9 +1679,7 @@ static bool try_to_unmap_one(struct folio *folio,=
 struct vm_area_struct *vma,
> >                       /* Restore the mlock which got missed */
> >                       if (!folio_test_large(folio))
> >                               mlock_vma_folio(folio, vma);
> > -                     page_vma_mapped_walk_done(&pvmw);
> > -                     ret =3D false;
> > -                     break;
> > +                     goto walk_done_err;
> >               }
> >
> >               pfn =3D pte_pfn(ptep_get(pvmw.pte));
> > @@ -1719,11 +1717,8 @@ static bool try_to_unmap_one(struct folio *folio=
, struct vm_area_struct *vma,
> >                        */
> >                       if (!anon) {
> >                               VM_BUG_ON(!(flags & TTU_RMAP_LOCKED));
> > -                             if (!hugetlb_vma_trylock_write(vma)) {
> > -                                     page_vma_mapped_walk_done(&pvmw);
> > -                                     ret =3D false;
> > -                                     break;
> > -                             }
> > +                             if (!hugetlb_vma_trylock_write(vma))
> > +                                     goto walk_done_err;
> >                               if (huge_pmd_unshare(mm, vma, address, pv=
mw.pte)) {
> >                                       hugetlb_vma_unlock_write(vma);
> >                                       flush_tlb_range(vma,
> > @@ -1738,8 +1733,7 @@ static bool try_to_unmap_one(struct folio *folio,=
 struct vm_area_struct *vma,
> >                                        * actual page and drop map count
> >                                        * to zero.
> >                                        */
> > -                                     page_vma_mapped_walk_done(&pvmw);
> > -                                     break;
> > +                                     goto walk_done;
> >                               }
> >                               hugetlb_vma_unlock_write(vma);
> >                       }
> > @@ -1811,9 +1805,7 @@ static bool try_to_unmap_one(struct folio *folio,=
 struct vm_area_struct *vma,
> >                       if (unlikely(folio_test_swapbacked(folio) !=3D
> >                                       folio_test_swapcache(folio))) {
> >                               WARN_ON_ONCE(1);
> > -                             ret =3D false;
> > -                             page_vma_mapped_walk_done(&pvmw);
> > -                             break;
> > +                             goto walk_done_err;
> >                       }
> >
> >                       /* MADV_FREE page check */
> > @@ -1852,23 +1844,17 @@ static bool try_to_unmap_one(struct folio *foli=
o, struct vm_area_struct *vma,
> >                                */
> >                               set_pte_at(mm, address, pvmw.pte, pteval)=
;
> >                               folio_set_swapbacked(folio);
> > -                             ret =3D false;
> > -                             page_vma_mapped_walk_done(&pvmw);
> > -                             break;
> > +                             goto walk_done_err;
> >                       }
> >
> >                       if (swap_duplicate(entry) < 0) {
> >                               set_pte_at(mm, address, pvmw.pte, pteval)=
;
> > -                             ret =3D false;
> > -                             page_vma_mapped_walk_done(&pvmw);
> > -                             break;
> > +                             goto walk_done_err;
> >                       }
> >                       if (arch_unmap_one(mm, vma, address, pteval) < 0)=
 {
> >                               swap_free(entry);
> >                               set_pte_at(mm, address, pvmw.pte, pteval)=
;
> > -                             ret =3D false;
> > -                             page_vma_mapped_walk_done(&pvmw);
> > -                             break;
> > +                             goto walk_done_err;
> >                       }
> >
> >                       /* See folio_try_share_anon_rmap(): clear PTE fir=
st. */
> > @@ -1876,9 +1862,7 @@ static bool try_to_unmap_one(struct folio *folio,=
 struct vm_area_struct *vma,
> >                           folio_try_share_anon_rmap_pte(folio, subpage)=
) {
> >                               swap_free(entry);
> >                               set_pte_at(mm, address, pvmw.pte, pteval)=
;
> > -                             ret =3D false;
> > -                             page_vma_mapped_walk_done(&pvmw);
> > -                             break;
> > +                             goto walk_done_err;
> >                       }
> >                       if (list_empty(&mm->mmlist)) {
> >                               spin_lock(&mmlist_lock);
> > @@ -1918,6 +1902,12 @@ static bool try_to_unmap_one(struct folio *folio=
, struct vm_area_struct *vma,
> >               if (vma->vm_flags & VM_LOCKED)
> >                       mlock_drain_local();
> >               folio_put(folio);
> > +             continue;
> > +walk_done_err:
> > +             ret =3D false;
> > +walk_done:
> > +             page_vma_mapped_walk_done(&pvmw);
> > +             break;
> >       }
> >
> >       mmu_notifier_invalidate_range_end(&range);