Re: [PATCH v2 3/5] mm/huge_memory.c: add missing read-only THP checking in transparent_hugepage_enabled()

[Miaohe Lin <linmiaohe@huawei.com>]

On 2021/4/30 15:49, David Hildenbrand wrote:
> On 30.04.21 03:57, Miaohe Lin wrote:
>> On 2021/4/29 22:57, David Hildenbrand wrote:
>>> On 29.04.21 15:26, Miaohe Lin wrote:
>>>> Since commit 99cb0dbd47a1 ("mm,thp: add read-only THP support for
>>>> (non-shmem) FS"), read-only THP file mapping is supported. But it
>>>> forgot to add checking for it in transparent_hugepage_enabled().
>>>> To fix it, we add checking for read-only THP file mapping and also
>>>> introduce helper transhuge_vma_enabled() to check whether thp is
>>>> enabled for specified vma to reduce duplicated code.
>>>>
>>>> Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS")
>>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>>> ---
>>>>    include/linux/huge_mm.h | 21 +++++++++++++++++----
>>>>    mm/huge_memory.c        |  6 ++++++
>>>>    mm/khugepaged.c         |  4 +---
>>>>    mm/shmem.c              |  3 +--
>>>>    4 files changed, 25 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>>> index 0a526f211fec..f460b74619fc 100644
>>>> --- a/include/linux/huge_mm.h
>>>> +++ b/include/linux/huge_mm.h
>>>> @@ -115,6 +115,16 @@ extern struct kobj_attribute shmem_enabled_attr;
>>>>      extern unsigned long transparent_hugepage_flags;
>>>>    +static inline bool transhuge_vma_enabled(struct vm_area_struct *vma,
>>>> +                      unsigned long vm_flags)
>>>
>>> You're passing the vma already, why do you pass vma->vm_flags separately? It's sufficient to pass in the vma only.
>>>
>>
>> Many thanks for comment! IMO, vm_flags may not always equal to vma->vm_flags. When hugepage_vma_check()
>> is called from collapse_pte_mapped_thp, vma_flags = vma->vm_flags | VM_HUGEPAGE. So I think we should
>> pass vm_flags here.
> 
> Oh, sorry, I missed the hugepage_vma_check() user. That's unfortunate.

Yes, that's unfortunate.

> 
>>>>    static inline void prep_transhuge_page(struct page *page) {}
>>>>      static inline bool is_transparent_hugepage(struct page *page)
>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>> index 76ca1eb2a223..e24a96de2e37 100644
>>>> --- a/mm/huge_memory.c
>>>> +++ b/mm/huge_memory.c
>>>> @@ -68,12 +68,18 @@ bool transparent_hugepage_enabled(struct vm_area_struct *vma)
>>>>        /* The addr is used to check if the vma size fits */
>>>>        unsigned long addr = (vma->vm_end & HPAGE_PMD_MASK) - HPAGE_PMD_SIZE;
>>>>    +    if (!transhuge_vma_enabled(vma, vma->vm_flags))
>>>> +        return false;
>>>>        if (!transhuge_vma_suitable(vma, addr))
>>>>            return false;
>>>>        if (vma_is_anonymous(vma))
>>>>            return __transparent_hugepage_enabled(vma);
>>>>        if (vma_is_shmem(vma))
>>>>            return shmem_huge_enabled(vma);
>>>> +    if (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) && vma->vm_file &&
>>>> +        !inode_is_open_for_write(vma->vm_file->f_inode) &&
>>>> +        (vma->vm_flags & VM_EXEC))
>>>> +        return true;
>>>
>>> Nit: I'm really wondering why we have 3 different functions that sound like they are doing the same thing
>>>
>>> transparent_hugepage_enabled(vma)
>>> transhuge_vma_enabled()
>>> transhuge_vma_suitable()
>>>
>>> Which check belongs where? Does it really have to be that complicated?
>>>
>>
>> IMO, transhuge_vma_suitable() checks whether pgoff , vm_start and vm_end is possible for thp.
>> transhuge_vma_enabled() checks whether thp is explicitly disabled through madvise.
>> And transparent_hugepage_enabled() use these helpers to get the conclusion whether thp is
>> enabled for specified vma.
>>
>> Any suggestions?
> 
> transparent_hugepage_enabled() vs. transhuge_vma_enabled() is really sub-optimal naming. I guess "transparent_hugepage_active()" would have been clearer (enabled + suitable + applicable). Cannot really give a good suggestion here on how to name transhuge_vma_enabled() differently.
> 

I think transparent_hugepage_active() sounds better too.

> 
> We now have
> 
> transparent_hugepage_enabled()
> -> transhuge_vma_enabled()
> -> __transparent_hugepage_enabled() -> transhuge_vma_enabled()
> -> shmem_huge_enabled() -> transhuge_vma_enabled()
> 
> That looks sub-optimal as well. Maybe we should have a
> 
> static inline bool file_thp_enabled(struct vma *vma)
> {
>     return transhuge_vma_enabled() &&
>            IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) &&
>            !inode_is_open_for_write(vma->vm_file->f_inode) &&
>            (vma->vm_flags & VM_EXEC))
> }
> 
> and in transparent_hugepage_enabled() only do a
> 
> if (vma->vm_file)
>     return file_thp_enabled(vma);
> 

Really good suggestion! I would try to do this one in next version. Many thanks for your time and suggestion! :)

> 
> Or move the transhuge_vma_enabled() check completely to transparent_hugepage_enabled() if possible.
>