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@xxxxxxxxxx>
>>>> ---
>>>> 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.
>
