Message-ID: <a4a88954-be9a-d953-81ff-a72c7898d5c9@arm.com>
Date:   Mon, 27 Jun 2022 13:57:44 +0530
From:   Anshuman Khandual <anshuman.khandual@....com>
To:     Muchun Song <songmuchun@...edance.com>
Cc:     Qi Zheng <zhengqi.arch@...edance.com>, mike.kravetz@...cle.com,
        akpm@...ux-foundation.org, catalin.marinas@....com,
        will@...nel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH] mm: hugetlb: kill set_huge_swap_pte_at()



On 6/27/22 13:14, Muchun Song wrote:
> 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@...edance.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?

Okay, alright. Lets drop set_huge_swap_pte_at() which helps make
HugeTLB less special.

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

Powered by blists - more mailing lists