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 <[email protected]>
---
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)
+{
+ /* Explicitly disabled through madvise. */
+ if ((vm_flags & VM_NOHUGEPAGE) ||
+ test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
+ return false;
+ return true;
+}
+
/*
* to be used on vmas which are known to support THP.
* Use transparent_hugepage_enabled otherwise
@@ -128,15 +138,12 @@ static inline bool __transparent_hugepage_enabled(struct vm_area_struct *vma)
if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_NEVER_DAX))
return false;
- if (vma->vm_flags & VM_NOHUGEPAGE)
+ if (!transhuge_vma_enabled(vma, vma->vm_flags))
return false;
if (vma_is_temporary_stack(vma))
return false;
- if (test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
- return false;
-
if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_FLAG))
return true;
@@ -362,6 +369,12 @@ static inline bool transhuge_vma_suitable(struct vm_area_struct *vma,
return false;
}
+static inline bool transhuge_vma_enabled(struct vm_area_struct *vma,
+ unsigned long vm_flags)
+{
+ return false;
+}
+
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;
return false;
}
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 6c0185fdd815..d97b20fad6e8 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -442,9 +442,7 @@ static inline int khugepaged_test_exit(struct mm_struct *mm)
static bool hugepage_vma_check(struct vm_area_struct *vma,
unsigned long vm_flags)
{
- /* Explicitly disabled through madvise. */
- if ((vm_flags & VM_NOHUGEPAGE) ||
- test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
+ if (!transhuge_vma_enabled(vma, vm_flags))
return false;
/* Enabled via shmem mount options or sysfs settings. */
diff --git a/mm/shmem.c b/mm/shmem.c
index a08cedefbfaa..1dcbec313c70 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -4032,8 +4032,7 @@ bool shmem_huge_enabled(struct vm_area_struct *vma)
loff_t i_size;
pgoff_t off;
- if ((vma->vm_flags & VM_NOHUGEPAGE) ||
- test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
+ if (!transhuge_vma_enabled(vma, vma->vm_flags))
return false;
if (shmem_huge == SHMEM_HUGE_FORCE)
return true;
--
2.23.0
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 <[email protected]>
> ---
> 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.
> +{
> + /* Explicitly disabled through madvise. */
> + if ((vm_flags & VM_NOHUGEPAGE) ||
> + test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
> + return false;
> + return true;
> +}
> +
> /*
> * to be used on vmas which are known to support THP.
> * Use transparent_hugepage_enabled otherwise
> @@ -128,15 +138,12 @@ static inline bool __transparent_hugepage_enabled(struct vm_area_struct *vma)
> if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_NEVER_DAX))
> return false;
>
> - if (vma->vm_flags & VM_NOHUGEPAGE)
> + if (!transhuge_vma_enabled(vma, vma->vm_flags))
> return false;
>
> if (vma_is_temporary_stack(vma))
> return false;
>
> - if (test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
> - return false;
> -
> if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_FLAG))
> return true;
>
> @@ -362,6 +369,12 @@ static inline bool transhuge_vma_suitable(struct vm_area_struct *vma,
> return false;
> }
>
> +static inline bool transhuge_vma_enabled(struct vm_area_struct *vma,
> + unsigned long vm_flags)
> +{
> + return false;
> +}
> +
> 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?
>
> return false;
> }
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 6c0185fdd815..d97b20fad6e8 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -442,9 +442,7 @@ static inline int khugepaged_test_exit(struct mm_struct *mm)
> static bool hugepage_vma_check(struct vm_area_struct *vma,
> unsigned long vm_flags)
> {
> - /* Explicitly disabled through madvise. */
> - if ((vm_flags & VM_NOHUGEPAGE) ||
> - test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
> + if (!transhuge_vma_enabled(vma, vm_flags))
> return false;
>
> /* Enabled via shmem mount options or sysfs settings. */
> diff --git a/mm/shmem.c b/mm/shmem.c
> index a08cedefbfaa..1dcbec313c70 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -4032,8 +4032,7 @@ bool shmem_huge_enabled(struct vm_area_struct *vma)
> loff_t i_size;
> pgoff_t off;
>
> - if ((vma->vm_flags & VM_NOHUGEPAGE) ||
> - test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
> + if (!transhuge_vma_enabled(vma, vma->vm_flags))
> return false;
> if (shmem_huge == SHMEM_HUGE_FORCE)
> return true;
>
--
Thanks,
David / dhildenb
On Thu, Apr 29, 2021 at 7:57 AM David Hildenbrand <[email protected]> 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 <[email protected]>
> > ---
> > 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.
I do agree.
>
> > +{
> > + /* Explicitly disabled through madvise. */
> > + if ((vm_flags & VM_NOHUGEPAGE) ||
> > + test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
> > + return false;
> > + return true;
> > +}
> > +
> > /*
> > * to be used on vmas which are known to support THP.
> > * Use transparent_hugepage_enabled otherwise
> > @@ -128,15 +138,12 @@ static inline bool __transparent_hugepage_enabled(struct vm_area_struct *vma)
> > if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_NEVER_DAX))
> > return false;
> >
> > - if (vma->vm_flags & VM_NOHUGEPAGE)
> > + if (!transhuge_vma_enabled(vma, vma->vm_flags))
> > return false;
> >
> > if (vma_is_temporary_stack(vma))
> > return false;
> >
> > - if (test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
> > - return false;
> > -
> > if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_FLAG))
> > return true;
> >
> > @@ -362,6 +369,12 @@ static inline bool transhuge_vma_suitable(struct vm_area_struct *vma,
> > return false;
> > }
> >
> > +static inline bool transhuge_vma_enabled(struct vm_area_struct *vma,
> > + unsigned long vm_flags)
> > +{
> > + return false;
> > +}
> > +
> > 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)
This one is called by smap code only which does check everything (for
both anonymous and shmem) to decide if allocating THP is possible.
Miaohe Lin's patch adds check for readonly FS THP.
> transhuge_vma_enabled()
This is the helper added by Miaohe Lin in this patch. It checks
VM_NOHUGEPAGE and MMF_DISABLE flags. It helps eliminate some duplicate
code. And this check is called at a couple of different places.
> transhuge_vma_suitable()
This one checks if vma is aligned properly. It may be better to rename
it to transhuge_vma_aligned(). It seems this check is just called by
page fault handler.
>
> Which check belongs where? Does it really have to be that complicated?
>
> >
> > return false;
> > }
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index 6c0185fdd815..d97b20fad6e8 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -442,9 +442,7 @@ static inline int khugepaged_test_exit(struct mm_struct *mm)
> > static bool hugepage_vma_check(struct vm_area_struct *vma,
> > unsigned long vm_flags)
> > {
> > - /* Explicitly disabled through madvise. */
> > - if ((vm_flags & VM_NOHUGEPAGE) ||
> > - test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
> > + if (!transhuge_vma_enabled(vma, vm_flags))
> > return false;
> >
> > /* Enabled via shmem mount options or sysfs settings. */
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index a08cedefbfaa..1dcbec313c70 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -4032,8 +4032,7 @@ bool shmem_huge_enabled(struct vm_area_struct *vma)
> > loff_t i_size;
> > pgoff_t off;
> >
> > - if ((vma->vm_flags & VM_NOHUGEPAGE) ||
> > - test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
> > + if (!transhuge_vma_enabled(vma, vma->vm_flags))
> > return false;
> > if (shmem_huge == SHMEM_HUGE_FORCE)
> > return true;
> >
>
>
> --
> Thanks,
>
> David / dhildenb
>
>
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 <[email protected]>
>> ---
>> 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.
>> +{
>> + /* Explicitly disabled through madvise. */
>> + if ((vm_flags & VM_NOHUGEPAGE) ||
>> + test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
>> + return false;
>> + return true;
>> +}
>> +
>> /*
>> * to be used on vmas which are known to support THP.
>> * Use transparent_hugepage_enabled otherwise
>> @@ -128,15 +138,12 @@ static inline bool __transparent_hugepage_enabled(struct vm_area_struct *vma)
>> if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_NEVER_DAX))
>> return false;
>> - if (vma->vm_flags & VM_NOHUGEPAGE)
>> + if (!transhuge_vma_enabled(vma, vma->vm_flags))
>> return false;
>> if (vma_is_temporary_stack(vma))
>> return false;
>> - if (test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
>> - return false;
>> -
>> if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_FLAG))
>> return true;
>> @@ -362,6 +369,12 @@ static inline bool transhuge_vma_suitable(struct vm_area_struct *vma,
>> return false;
>> }
>> +static inline bool transhuge_vma_enabled(struct vm_area_struct *vma,
>> + unsigned long vm_flags)
>> +{
>> + return false;
>> +}
>> +
>> 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?
Thanks again!
>> return false;
>> }
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index 6c0185fdd815..d97b20fad6e8 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -442,9 +442,7 @@ static inline int khugepaged_test_exit(struct mm_struct *mm)
>> static bool hugepage_vma_check(struct vm_area_struct *vma,
>> unsigned long vm_flags)
>> {
>> - /* Explicitly disabled through madvise. */
>> - if ((vm_flags & VM_NOHUGEPAGE) ||
>> - test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
>> + if (!transhuge_vma_enabled(vma, vm_flags))
>> return false;
>> /* Enabled via shmem mount options or sysfs settings. */
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index a08cedefbfaa..1dcbec313c70 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -4032,8 +4032,7 @@ bool shmem_huge_enabled(struct vm_area_struct *vma)
>> loff_t i_size;
>> pgoff_t off;
>> - if ((vma->vm_flags & VM_NOHUGEPAGE) ||
>> - test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))
>> + if (!transhuge_vma_enabled(vma, vma->vm_flags))
>> return false;
>> if (shmem_huge == SHMEM_HUGE_FORCE)
>> return true;
>>
>
>
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 <[email protected]>
>>> ---
>>> 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.
>>> 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.
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);
Or move the transhuge_vma_enabled() check completely to
transparent_hugepage_enabled() if possible.
--
Thanks,
David / dhildenb
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 <[email protected]>
>>>> ---
>>>> 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.
>