Re: [PATCH v2] mm: shmem: remove __shmem_huge_global_enabled()

[David Hildenbrand <david@redhat.com>]

On 01.11.24 02:07, Kefeng Wang wrote:
> 
> 
> On 2024/10/30 21:09, David Hildenbrand wrote:
>> On 17.10.24 16:14, Kefeng Wang wrote:
>>> Remove __shmem_huge_global_enabled() since only one caller,
>>> and remove repeated check of VM_NOHUGEPAGE/MMF_DISABLE_THP
>>> as they are checked in shmem_allowable_huge_orders(), also
>>> remove unnecessary vma parameter.
>>>
>>> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>> ---
>>> v2:
>>> - Add RB and send separately
>>>
>>>    mm/shmem.c | 33 ++++++++++-----------------------
>>>    1 file changed, 10 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>> index 247c0403af83..e933327d8dac 100644
>>> --- a/mm/shmem.c
>>> +++ b/mm/shmem.c
>>> @@ -548,17 +548,15 @@ static bool shmem_confirm_swap(struct
>>> address_space *mapping,
>>>    static int shmem_huge __read_mostly = SHMEM_HUGE_NEVER;
>>> -static bool __shmem_huge_global_enabled(struct inode *inode, pgoff_t
>>> index,
>>> -                    loff_t write_end, bool shmem_huge_force,
>>> -                    struct vm_area_struct *vma,
>>> -                    unsigned long vm_flags)
>>> +static bool shmem_huge_global_enabled(struct inode *inode, pgoff_t
>>> index,
>>> +                      loff_t write_end, bool shmem_huge_force,
>>> +                      unsigned long vm_flags)
>>
>> Please use double tabs on the second+ parameter list.
> 
> Ok, I will send a tab fix patch.
> 
>>
>>>    {
>>> -    struct mm_struct *mm = vma ? vma->vm_mm : NULL;
>>>        loff_t i_size;
>>> -    if (!S_ISREG(inode->i_mode))
>>> +    if (HPAGE_PMD_ORDER > MAX_PAGECACHE_ORDER)
>>>            return false;
>>> -    if (mm && ((vm_flags & VM_NOHUGEPAGE) ||
>>> test_bit(MMF_DISABLE_THP, &mm->flags)))
>>
>> But where is this check for shmem_getattr()?
> 
> 
> In shmem_getattr(), the vma=NULL/vm_flags=0, so only vm_flags check is
> enough.
> 
> -	if (shmem_huge_global_enabled(inode, 0, 0, false, NULL, 0))
> +	if (shmem_huge_global_enabled(inode, 0, 0, false, 0))
> 

Ah, missed that, makes perfect sense!

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb