On Sat, 11 Jun 2022, Miaohe Lin wrote:
> Use helper transhuge_vma_enabled() to check whether transhuge is enable
> on vma. Minor readability improvement.
>
> Signed-off-by: Miaohe Lin <[email protected]>
No thanks, that's a readability regression, forcing reader
to go and look up what transhuge_vma_enabled() actually means.
What you call a helper, I call an obfuscator - as I implied in
b9e2faaf6fa0 ("huge tmpfs: revert shmem's use of transhuge_vma_enabled()")
Hugh
> ---
> mm/shmem.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 133c67057d41..59cc2e980c95 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -480,8 +480,7 @@ bool shmem_is_huge(struct vm_area_struct *vma,
> return false;
> if (shmem_huge == SHMEM_HUGE_DENY)
> return false;
> - if (vma && ((vma->vm_flags & VM_NOHUGEPAGE) ||
> - test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags)))
> + if (vma && !transhuge_vma_enabled(vma, vma->vm_flags))
> return false;
> if (shmem_huge == SHMEM_HUGE_FORCE)
> return true;
> --
> 2.23.0
On 2022/6/11 11:14, Hugh Dickins wrote:
> On Sat, 11 Jun 2022, Miaohe Lin wrote:
>
>> Use helper transhuge_vma_enabled() to check whether transhuge is enable
>> on vma. Minor readability improvement.
>>
>> Signed-off-by: Miaohe Lin <[email protected]>
>
> No thanks, that's a readability regression, forcing reader
> to go and look up what transhuge_vma_enabled() actually means.
>
> What you call a helper, I call an obfuscator - as I implied in
> b9e2faaf6fa0 ("huge tmpfs: revert shmem's use of transhuge_vma_enabled()")
Oh, that commit also cc to me. But I forgot it... Sorry.
>
> Hugh
>
On 6/11/22 08:44, Hugh Dickins wrote:
> On Sat, 11 Jun 2022, Miaohe Lin wrote:
>
>> Use helper transhuge_vma_enabled() to check whether transhuge is enable
>> on vma. Minor readability improvement.
>>
>> Signed-off-by: Miaohe Lin <[email protected]>
>
> No thanks, that's a readability regression, forcing reader
> to go and look up what transhuge_vma_enabled() actually means.
>
> What you call a helper, I call an obfuscator - as I implied in
> b9e2faaf6fa0 ("huge tmpfs: revert shmem's use of transhuge_vma_enabled()")
The same reasoning should also be applicable for other calls sites
for transhuge_vma_enabled(). Should not they be dropped as well ?
>
> Hugh
>
>> ---
>> mm/shmem.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 133c67057d41..59cc2e980c95 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -480,8 +480,7 @@ bool shmem_is_huge(struct vm_area_struct *vma,
>> return false;
>> if (shmem_huge == SHMEM_HUGE_DENY)
>> return false;
>> - if (vma && ((vma->vm_flags & VM_NOHUGEPAGE) ||
>> - test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags)))
>> + if (vma && !transhuge_vma_enabled(vma, vma->vm_flags))
>> return false;
>> if (shmem_huge == SHMEM_HUGE_FORCE)
>> return true;
>> --
>> 2.23.0
>
On Tue, Jun 14, 2022 at 10:02 PM Anshuman Khandual
<[email protected]> wrote:
>
>
>
> On 6/11/22 08:44, Hugh Dickins wrote:
> > On Sat, 11 Jun 2022, Miaohe Lin wrote:
> >
> >> Use helper transhuge_vma_enabled() to check whether transhuge is enable
> >> on vma. Minor readability improvement.
> >>
> >> Signed-off-by: Miaohe Lin <[email protected]>
> >
> > No thanks, that's a readability regression, forcing reader
> > to go and look up what transhuge_vma_enabled() actually means.
> >
> > What you call a helper, I call an obfuscator - as I implied in
> > b9e2faaf6fa0 ("huge tmpfs: revert shmem's use of transhuge_vma_enabled()")
>
> The same reasoning should also be applicable for other calls sites
> for transhuge_vma_enabled(). Should not they be dropped as well ?
Yes, the only caller was hugepage_vma_check(). It was changed to
open-code and the helper was removed by series:
https://lore.kernel.org/linux-mm/[email protected]/T/#m7610c2a4e5dda753cbc850986d5486f59a821c1a
>
> >
> > Hugh
> >
> >> ---
> >> mm/shmem.c | 3 +--
> >> 1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/mm/shmem.c b/mm/shmem.c
> >> index 133c67057d41..59cc2e980c95 100644
> >> --- a/mm/shmem.c
> >> +++ b/mm/shmem.c
> >> @@ -480,8 +480,7 @@ bool shmem_is_huge(struct vm_area_struct *vma,
> >> return false;
> >> if (shmem_huge == SHMEM_HUGE_DENY)
> >> return false;
> >> - if (vma && ((vma->vm_flags & VM_NOHUGEPAGE) ||
> >> - test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags)))
> >> + if (vma && !transhuge_vma_enabled(vma, vma->vm_flags))
> >> return false;
> >> if (shmem_huge == SHMEM_HUGE_FORCE)
> >> return true;
> >> --
> >> 2.23.0
> >
>