Received-SPF: pass (google.com: domain of linux-kernel+bounces-212753-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: <20240610120209.66311-1-ioworker0@gmail.com> <20240610120209.66311-2-ioworker0@gmail.com>
In-Reply-To: <20240610120209.66311-2-ioworker0@gmail.com>
From: Barry Song <21cnbao@gmail.com>
Date: Thu, 13 Jun 2024 19:52:04 +1200
Message-ID: <CAGsJ_4y4sKXbq4_ra9tTD-LWgB-x-HECyouxm_9r_N7Kw2We4g@mail.gmail.com>
Subject: Re: [PATCH v7 1/4] mm/rmap: remove duplicated exit code in pagewalk loop
To: Lance Yang <ioworker0@gmail.com>
Cc: akpm@linux-foundation.org, willy@infradead.org, sj@kernel.org, 
	baolin.wang@linux.alibaba.com, maskray@google.com, ziy@nvidia.com, 
	ryan.roberts@arm.com, david@redhat.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

On Tue, Jun 11, 2024 at 12:02=E2=80=AFAM Lance Yang <ioworker0@gmail.com> w=
rote:
>
> 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>
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Lance Yang <ioworker0@gmail.com>

I don't think "return false" necessarily indicates an error, so
"walk_done_err" doesn't seem like an appropriate name.
However, this is a minor issue.

Reviewed-by: Barry Song <baohua@kernel.org>

> ---
>  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, s=
truct 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, s=
truct 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, s=
truct 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 *folio,=
 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, s=
truct 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);
> --
> 2.33.1
>