| Message ID | 20210427133214.2270207-4-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup and fixup for huge_memory | expand |
On Tue, Apr 27, 2021 at 6:32 AM Miaohe Lin <linmiaohe@huawei.com> 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(). > > Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS") > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/huge_memory.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index 76ca1eb2a223..aa22a0ae9894 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -74,6 +74,9 @@ bool transparent_hugepage_enabled(struct vm_area_struct *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 && > + (vma->vm_flags & VM_DENYWRITE)) > + return true; I don't think this change is correct. This function is used to indicate if allocating THP is eligible for the VMAs or not showed by smap. And currently readonly FS THP is collapsed by khugepaged only. So, you need check if the vma is suitable for khugepaged. Take a look at what hugepage_vma_check() does. And, the new patch (https://lore.kernel.org/linux-mm/20210406000930.3455850-1-cfijalkovich@google.com/) relax the constraints for readonly FS THP, it might be already in -mm tree, so you need adopt the new condition as well. > > return false; > } > -- > 2.23.0 > >
On 2021/4/28 5:03, Yang Shi wrote: > On Tue, Apr 27, 2021 at 6:32 AM Miaohe Lin <linmiaohe@huawei.com> 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(). >> >> Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS") >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> mm/huge_memory.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >> index 76ca1eb2a223..aa22a0ae9894 100644 >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -74,6 +74,9 @@ bool transparent_hugepage_enabled(struct vm_area_struct *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 && >> + (vma->vm_flags & VM_DENYWRITE)) >> + return true; > Many thanks for your quick respond and Reviewed-by tag! > I don't think this change is correct. This function is used to > indicate if allocating THP is eligible for the VMAs or not showed by > smap. And currently readonly FS THP is collapsed by khugepaged only. > > So, you need check if the vma is suitable for khugepaged. Take a look > at what hugepage_vma_check() does. > > And, the new patch > (https://lore.kernel.org/linux-mm/20210406000930.3455850-1-cfijalkovich@google.com/) > relax the constraints for readonly FS THP, it might be already in -mm > tree, so you need adopt the new condition as well. > Many thanks for your comment. I referred to what hugepage_vma_check() does about Read-only file mappings when I came up this patch. But it seems I am miss something. Take the new patch into account, the check for READ_ONLY_THP now should be: diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 76ca1eb2a223..a46a558233b4 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -74,6 +74,10 @@ bool transparent_hugepage_enabled(struct vm_area_struct *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; } Am I miss something about checking for READ_ONLY_THP case? Or READ_ONLY_THP case is ok but other case is missed? Could you please explain this more detailed for me? Many thanks! >> >> return false; >> } > >> -- >> 2.23.0 >> >> > > . >
On Tue, Apr 27, 2021 at 7:06 PM Miaohe Lin <linmiaohe@huawei.com> wrote: > > On 2021/4/28 5:03, Yang Shi wrote: > > On Tue, Apr 27, 2021 at 6:32 AM Miaohe Lin <linmiaohe@huawei.com> 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(). > >> > >> Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS") > >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > >> --- > >> mm/huge_memory.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c > >> index 76ca1eb2a223..aa22a0ae9894 100644 > >> --- a/mm/huge_memory.c > >> +++ b/mm/huge_memory.c > >> @@ -74,6 +74,9 @@ bool transparent_hugepage_enabled(struct vm_area_struct *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 && > >> + (vma->vm_flags & VM_DENYWRITE)) > >> + return true; > > > > Many thanks for your quick respond and Reviewed-by tag! > > > I don't think this change is correct. This function is used to > > indicate if allocating THP is eligible for the VMAs or not showed by > > smap. And currently readonly FS THP is collapsed by khugepaged only. > > > > So, you need check if the vma is suitable for khugepaged. Take a look > > at what hugepage_vma_check() does. > > > > And, the new patch > > (https://lore.kernel.org/linux-mm/20210406000930.3455850-1-cfijalkovich@google.com/) > > relax the constraints for readonly FS THP, it might be already in -mm > > tree, so you need adopt the new condition as well. > > > > Many thanks for your comment. I referred to what hugepage_vma_check() does about > Read-only file mappings when I came up this patch. But it seems I am miss something. Yes, you need do the below check for readonly FS THP too: if ((vm_flags & VM_NOHUGEPAGE) || test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags)) return false; This check is done separately for anonymous and shmem. It seems not perfect to do it three times in a row. So I'd suggest extracting the check into a common helper then call it at the top of transparent_hugepage_enabled() . The helper also could replace the same check in __transparent_hugepage_enabled() and shmem_huge_enabled(). > Take the new patch into account, the check for READ_ONLY_THP now should be: > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index 76ca1eb2a223..a46a558233b4 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -74,6 +74,10 @@ bool transparent_hugepage_enabled(struct vm_area_struct *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; > } > > Am I miss something about checking for READ_ONLY_THP case? Or READ_ONLY_THP case is ok > but other case is missed? Could you please explain this more detailed for me? > > Many thanks! > > >> > >> return false; > >> } > > > >> -- > >> 2.23.0 > >> > >> > > > > . > > >
On 2021/4/29 0:21, Yang Shi wrote: > On Tue, Apr 27, 2021 at 7:06 PM Miaohe Lin <linmiaohe@huawei.com> wrote: >> >> On 2021/4/28 5:03, Yang Shi wrote: >>> On Tue, Apr 27, 2021 at 6:32 AM Miaohe Lin <linmiaohe@huawei.com> 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(). >>>> >>>> Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS") >>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >>>> --- >>>> mm/huge_memory.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >>>> index 76ca1eb2a223..aa22a0ae9894 100644 >>>> --- a/mm/huge_memory.c >>>> +++ b/mm/huge_memory.c >>>> @@ -74,6 +74,9 @@ bool transparent_hugepage_enabled(struct vm_area_struct *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 && >>>> + (vma->vm_flags & VM_DENYWRITE)) >>>> + return true; >>> >> >> Many thanks for your quick respond and Reviewed-by tag! >> >>> I don't think this change is correct. This function is used to >>> indicate if allocating THP is eligible for the VMAs or not showed by >>> smap. And currently readonly FS THP is collapsed by khugepaged only. >>> >>> So, you need check if the vma is suitable for khugepaged. Take a look >>> at what hugepage_vma_check() does. >>> >>> And, the new patch >>> (https://lore.kernel.org/linux-mm/20210406000930.3455850-1-cfijalkovich@google.com/) >>> relax the constraints for readonly FS THP, it might be already in -mm >>> tree, so you need adopt the new condition as well. >>> >> >> Many thanks for your comment. I referred to what hugepage_vma_check() does about >> Read-only file mappings when I came up this patch. But it seems I am miss something. > > Yes, you need do the below check for readonly FS THP too: > > if ((vm_flags & VM_NOHUGEPAGE) || > test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags)) > return false; > > This check is done separately for anonymous and shmem. It seems not > perfect to do it three times in a row. So I'd suggest extracting the > check into a common helper then call it at the top of > transparent_hugepage_enabled() . > > The helper also could replace the same check in > __transparent_hugepage_enabled() and shmem_huge_enabled(). > I see. Many thanks for detailed explanation and good suggestion! Will do it in v2. :) >> Take the new patch into account, the check for READ_ONLY_THP now should be: >> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >> index 76ca1eb2a223..a46a558233b4 100644 >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -74,6 +74,10 @@ bool transparent_hugepage_enabled(struct vm_area_struct *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; >> } >> >> Am I miss something about checking for READ_ONLY_THP case? Or READ_ONLY_THP case is ok >> but other case is missed? Could you please explain this more detailed for me? >> >> Many thanks! >> >>>> >>>> return false; >>>> } >>> >>>> -- >>>> 2.23.0 >>>> >>>> >>> >>> . >>> >> > . >
diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 76ca1eb2a223..aa22a0ae9894 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -74,6 +74,9 @@ bool transparent_hugepage_enabled(struct vm_area_struct *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 && + (vma->vm_flags & VM_DENYWRITE)) + return true; return false; }
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(). Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS") Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/huge_memory.c | 3 +++ 1 file changed, 3 insertions(+)