Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 27 Jun 2022 15:44:22 +0800
From:   Muchun Song <songmuchun@bytedance.com>
To:     Anshuman Khandual <anshuman.khandual@arm.com>
Cc:     Qi Zheng <zhengqi.arch@bytedance.com>, mike.kravetz@oracle.com,
        akpm@linux-foundation.org, catalin.marinas@arm.com,
        will@kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH] mm: hugetlb: kill set_huge_swap_pte_at()
Message-ID: <Yrlf1iej4/BL2znu@FVFYT0MHHV2J.usts.net>
References: <20220626145717.53572-1-zhengqi.arch@bytedance.com>
 <f0cfe169-44fa-5653-d454-149ef286d3bb@arm.com>
 <037fc8c3-9b71-cb83-8882-95d5459a494f@bytedance.com>
 <b46f5390-830d-08fd-0d6d-3fda7d31c36a@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <b46f5390-830d-08fd-0d6d-3fda7d31c36a@arm.com>
Precedence: bulk

On Mon, Jun 27, 2022 at 12:44:19PM +0530, Anshuman Khandual wrote:
> 
> 
> On 6/27/22 12:25, Qi Zheng wrote:
> > 
> > 
> > On 2022/6/27 14:18, Anshuman Khandual wrote:
> >>
> >>
> >> On 6/26/22 20:27, Qi Zheng wrote:
> >>> The commit e5251fd43007 ("mm/hugetlb: introduce set_huge_swap_pte_at()
> >>> helper") add set_huge_swap_pte_at() to handle swap entries on
> >>> architectures that support hugepages consisting of contiguous ptes.
> >>> And currently the set_huge_swap_pte_at() is only overridden by arm64.
> >>>
> >>> The set_huge_swap_pte_at() provide a sz parameter to help determine
> >>> the number of entries to be updated. But in fact, all hugetlb swap
> >>> entries contain pfn information, so we can find the corresponding
> >>> folio through the pfn recorded in the swap entry, then the folio_size()
> >>> is the number of entries that need to be updated.
> >>>
> >>> And considering that users will easily cause bugs by ignoring the
> >>> difference between set_huge_swap_pte_at() and set_huge_pte_at().
> >>> Let's handle swap entries in set_huge_pte_at() and remove the
> >>> set_huge_swap_pte_at(), then we can call set_huge_pte_at()
> >>> anywhere, which simplifies our coding.
> >>>
> >>> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
> >>> ---
> >>> ? arch/arm64/include/asm/hugetlb.h |? 3 ---
> >>> ? arch/arm64/mm/hugetlbpage.c????? | 34 ++++++++++++++++----------------
> >>> ? include/linux/hugetlb.h????????? | 13 ------------
> >>> ? mm/hugetlb.c???????????????????? |? 8 +++-----
> >>> ? mm/rmap.c??????????????????????? | 11 +++--------
> >>> ? 5 files changed, 23 insertions(+), 46 deletions(-)
> >>>
> >>> diff --git a/arch/arm64/include/asm/hugetlb.h b/arch/arm64/include/asm/hugetlb.h
> >>> index 1fd2846dbefe..d20f5da2d76f 100644
> >>> --- a/arch/arm64/include/asm/hugetlb.h
> >>> +++ b/arch/arm64/include/asm/hugetlb.h
> >>> @@ -46,9 +46,6 @@ extern void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
> >>> ???????????????? pte_t *ptep, unsigned long sz);
> >>> ? #define __HAVE_ARCH_HUGE_PTEP_GET
> >>> ? extern pte_t huge_ptep_get(pte_t *ptep);
> >>> -extern void set_huge_swap_pte_at(struct mm_struct *mm, unsigned long addr,
> >>> -???????????????? pte_t *ptep, pte_t pte, unsigned long sz);
> >>> -#define set_huge_swap_pte_at set_huge_swap_pte_at
> >>> ? ? void __init arm64_hugetlb_cma_reserve(void);
> >>> ? diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
> >>> index c9e076683e5d..58b89b9d13e0 100644
> >>> --- a/arch/arm64/mm/hugetlbpage.c
> >>> +++ b/arch/arm64/mm/hugetlbpage.c
> >>> @@ -238,6 +238,13 @@ static void clear_flush(struct mm_struct *mm,
> >>> ????? flush_tlb_range(&vma, saddr, addr);
> >>> ? }
> >>> ? +static inline struct folio *hugetlb_swap_entry_to_folio(swp_entry_t entry)
> >>> +{
> >>> +??? VM_BUG_ON(!is_migration_entry(entry) && !is_hwpoison_entry(entry));
> >>> +
> >>> +??? return page_folio(pfn_to_page(swp_offset(entry)));
> >>> +}
> >>
> >> Extracting this huge page size from swap entry is an additional operation which
> >> will increase the over all cost for set_huge_swap_pte_at(). At present the size
> > 
> > Hmm, I think this cost is very small. And replacing
> > set_huge_swap_pte_at() by transparently handling swap entries helps
> > reduce possible bugs, which is worthwhile.
> 
> Possible bugs ? There are just six call sites for this function.

I think it is easy to make mistakes (see commit 5d4af6195c87).
I usually think of why the swap entry is special for HugeTLB pages
compared to normal pages (why we do not have set_swap_pte_at?).
set_huge_swap_pte_at() make HugeTLB more special, killing it
can make HugeTLB more consistent with normal page. From the point
of view of code maintenance, I think it is better to kill it. What
do you think?

Thanks.

> Although this proposed patch is functionally correct, I dont see
> a valid enough reason to increase the overall cost in the path.
>