Re: [PATCH] mm: shmem: fix incorrect vm_flags usage when checking allowable orders

On 8/17/26 5:34 PM, Baolin Wang wrote:
> 
> 
> On 8/17/26 3:40 PM, Lorenzo Stoakes (ARM) wrote:
>> On Mon, Aug 17, 2026 at 03:16:43PM +0800, Baolin Wang wrote:
>>> Lance reported that when MADV_HUGEPAGE is set on a tmpfs file mounted 
>>> with
>>> huge=advise option, khugepaged fails the allowable order check and 
>>> does not
>>> scan the tmpfs file for collapse.
>>
>> Ugh.
>>
>> But really I think this isn't quite accurate - what you mean to say I 
>> think is
>> that when _nothing else_ causes the mm to be considered for khugepaged 
>> collapse,
>> an MADV_HUGEPAGE-advised tmpfs VMA alone does not cause scanning to 
>> commence.
> 
> Yes.
> 
>>> After commit 6beeab870e70 ("mm: shmem: move 
>>> shmem_huge_global_enabled() into
>>> shmem_allowable_huge_orders()"), the shmem/tmpfs allowable order 
>>> check reads
>>> vma->vm_flags directly.  However, when MADV_HUGEPAGE is handled,
>>> khugepaged_enter_vma() is called before the VMA's vm_flags have been 
>>> updated,
>>> so the check uses stale flags and incorrectly rejects the VMA for 
>>> collapse.
>>> As a result, khugepaged does not collapse the tmpfs file into PMD 
>>> order in time.
>>
>> Could we at least refer to the non-deprecated field in the commit 
>> message?
>> i.e. vma->flags.
>>
>> Probably worth mentioning VMA_HUGEPAGE_BIT also.
> 
> Sure.
> 
> 
>>> Fix this by passing vm_flags as a parameter to 
>>> shmem_allowable_huge_orders()
>>> instead of reading it from the vm_area_struct.
>>
>> Ugh this is so disgusting.
>>
>> I understand this is a fix for a bug to be backported but couldn't we 
>> just
>> achieve the same without having to add a deprecated field to be passed 
>> around?
>>
>> As you say the khugepaged_enter_vma() isn't really so helpful in
>> hugepage_madvise().
>>
>> But you could add this to the bottom of madvise_update_vma():
>>
>>     if (vma_flags_test(&new_vma_flags, VMA_HUGEPAGE_BIT))
>>         khugepaged_enter_vma(vma, new_flags);
>>
>> I don't think this is really egregious on this code path and could go 
>> some way
>> towards us eliminating the silly thing of passing around flags-to-be-set.
> 
> This is not the point (maybe I didn't describe it clearly). The point is 
> that the allowable orders check for tmpfs in shmem_huge_global_enabled() 
> (called by shmem_allowable_huge_orders()) uses a stale vma flag.
> 
> static unsigned int shmem_huge_global_enabled(struct inode *inode, 
> pgoff_t index,
>              loff_t write_end, bool shmem_huge_force,
>              struct vm_area_struct *vma,
>              vm_flags_t vm_flags)
> {
>      ......
>      switch (SHMEM_SB(inode->i_sb)->huge) {
> 
>      ......
>      case SHMEM_HUGE_ADVISE:
>          if (vm_flags & VM_HUGEPAGE)
>              return THP_ORDERS_ALL_FILE_DEFAULT;
>          fallthrough;
>      default:
>          return 0;
>      }
> }
> 
> So we should pass the new vma flags for shmem_allowable_huge_orders() to 
> check the allowable orders for tmpfs. Changing madvise_update_vma() 
> doesn't help with the allowable orders check for tmpfs.

Sorry for misreading your code (I need a coffee before reading the email 
:)). Please ignore my reply. After looking at the code again, yes, this 
can work. If nobody rejects, I will follow your suggestion in v2. Thanks.