linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm: huge_memory: fix the check for allowed huge orders in shmem
@ 2025-06-13  9:12 Baolin Wang
  2025-06-13 11:16 ` Lorenzo Stoakes
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Baolin Wang @ 2025-06-13  9:12 UTC (permalink / raw)
  To: akpm, david
  Cc: ziy, lorenzo.stoakes, Liam.Howlett, npache, ryan.roberts,
	dev.jain, baohua, baolin.wang, linux-mm, linux-kernel

Shmem already supports mTHP, and shmem_allowable_huge_orders() will return
the huge orders allowed by shmem. However, there is no check against the
'orders' parameter passed by __thp_vma_allowable_orders(), which can lead
to incorrect check results for __thp_vma_allowable_orders().

For example, when a user wants to check if shmem supports PMD-sized THP
by thp_vma_allowable_order(), if shmem only enables 64K mTHP, the current
logic would cause thp_vma_allowable_order() to return true, implying that
shmem allows PMD-sized THP allocation, which it actually does not.

I don't think this will cause a significant impact on users, and this will
only have some impact on the shmem THP collapse. That is to say, even though
the shmem sysfs setting does not enable the PMD-sized THP, the
thp_vma_allowable_order() still indicates that shmem allows PMD-sized collapse,
meaning it might successfully collapse into THP, or it might not (for example,
thp_vma_suitable_order() check failed in the collapse process). However, this
still does not align with the shmem sysfs configuration, fix it.

Fixes: 26c7d8413aaf ("mm: thp: support "THPeligible" semantics for mTHP with anonymous shmem")
Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
Note: this general change is suitable to be split out as a bugfix patch
based on the discussions in the previous thread[1].

[1] https://lore.kernel.org/all/86bf2dcd-4be9-4fd9-98cc-da55aea52be0@lucifer.local/
---
 mm/huge_memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index d3e66136e41a..a8cfa37cae72 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -166,7 +166,7 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma,
 	 * own flags.
 	 */
 	if (!in_pf && shmem_file(vma->vm_file))
-		return shmem_allowable_huge_orders(file_inode(vma->vm_file),
+		return orders & shmem_allowable_huge_orders(file_inode(vma->vm_file),
 						   vma, vma->vm_pgoff, 0,
 						   !enforce_sysfs);
 
-- 
2.43.5



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] mm: huge_memory: fix the check for allowed huge orders in shmem
  2025-06-13  9:12 [PATCH] mm: huge_memory: fix the check for allowed huge orders in shmem Baolin Wang
@ 2025-06-13 11:16 ` Lorenzo Stoakes
  2025-06-15 11:14   ` Baolin Wang
  2025-06-13 13:06 ` Zi Yan
  2025-06-13 13:38 ` David Hildenbrand
  2 siblings, 1 reply; 5+ messages in thread
From: Lorenzo Stoakes @ 2025-06-13 11:16 UTC (permalink / raw)
  To: Baolin Wang
  Cc: akpm, david, ziy, Liam.Howlett, npache, ryan.roberts, dev.jain,
	baohua, linux-mm, linux-kernel

On Fri, Jun 13, 2025 at 05:12:19PM +0800, Baolin Wang wrote:
> Shmem already supports mTHP, and shmem_allowable_huge_orders() will return
> the huge orders allowed by shmem. However, there is no check against the
> 'orders' parameter passed by __thp_vma_allowable_orders(), which can lead
> to incorrect check results for __thp_vma_allowable_orders().
>
> For example, when a user wants to check if shmem supports PMD-sized THP
> by thp_vma_allowable_order(), if shmem only enables 64K mTHP, the current
> logic would cause thp_vma_allowable_order() to return true, implying that
> shmem allows PMD-sized THP allocation, which it actually does not.
>
> I don't think this will cause a significant impact on users, and this will
> only have some impact on the shmem THP collapse. That is to say, even though
> the shmem sysfs setting does not enable the PMD-sized THP, the
> thp_vma_allowable_order() still indicates that shmem allows PMD-sized collapse,
> meaning it might successfully collapse into THP, or it might not (for example,
> thp_vma_suitable_order() check failed in the collapse process). However, this
> still does not align with the shmem sysfs configuration, fix it.

Can you explain why?

It's a bit painful to trace through the code paths, but why do you think only
MADV_COLLAPSE will be impacted? Surely everywhere that checks this is?

>
> Fixes: 26c7d8413aaf ("mm: thp: support "THPeligible" semantics for mTHP with anonymous shmem")
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>

I can't see how this can be incorrect, as we really should be restricting
ourselves to the orders requested.

So:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

> ---
> Note: this general change is suitable to be split out as a bugfix patch
> based on the discussions in the previous thread[1].
>
> [1] https://lore.kernel.org/all/86bf2dcd-4be9-4fd9-98cc-da55aea52be0@lucifer.local/
> ---
>  mm/huge_memory.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index d3e66136e41a..a8cfa37cae72 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -166,7 +166,7 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma,
>  	 * own flags.
>  	 */
>  	if (!in_pf && shmem_file(vma->vm_file))
> -		return shmem_allowable_huge_orders(file_inode(vma->vm_file),
> +		return orders & shmem_allowable_huge_orders(file_inode(vma->vm_file),
>  						   vma, vma->vm_pgoff, 0,

I mean this seems correct to me, but what a massive oversight.

I wish we had a sensible way of testing this...

>  						   !enforce_sysfs);

This whole code path is entirely indicative of what a complete mess this whole
thing is.

The fact shmem separately calls this function is just ugh. I'm talking myself
into some mega refactoring here :)

>
> --
> 2.43.5
>


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] mm: huge_memory: fix the check for allowed huge orders in shmem
  2025-06-13  9:12 [PATCH] mm: huge_memory: fix the check for allowed huge orders in shmem Baolin Wang
  2025-06-13 11:16 ` Lorenzo Stoakes
@ 2025-06-13 13:06 ` Zi Yan
  2025-06-13 13:38 ` David Hildenbrand
  2 siblings, 0 replies; 5+ messages in thread
From: Zi Yan @ 2025-06-13 13:06 UTC (permalink / raw)
  To: Baolin Wang
  Cc: akpm, david, lorenzo.stoakes, Liam.Howlett, npache, ryan.roberts,
	dev.jain, baohua, linux-mm, linux-kernel

On 13 Jun 2025, at 5:12, Baolin Wang wrote:

> Shmem already supports mTHP, and shmem_allowable_huge_orders() will return
> the huge orders allowed by shmem. However, there is no check against the
> 'orders' parameter passed by __thp_vma_allowable_orders(), which can lead
> to incorrect check results for __thp_vma_allowable_orders().
>
> For example, when a user wants to check if shmem supports PMD-sized THP
> by thp_vma_allowable_order(), if shmem only enables 64K mTHP, the current
> logic would cause thp_vma_allowable_order() to return true, implying that
> shmem allows PMD-sized THP allocation, which it actually does not.
>
> I don't think this will cause a significant impact on users, and this will
> only have some impact on the shmem THP collapse. That is to say, even though
> the shmem sysfs setting does not enable the PMD-sized THP, the
> thp_vma_allowable_order() still indicates that shmem allows PMD-sized collapse,
> meaning it might successfully collapse into THP, or it might not (for example,
> thp_vma_suitable_order() check failed in the collapse process). However, this
> still does not align with the shmem sysfs configuration, fix it.
>
> Fixes: 26c7d8413aaf ("mm: thp: support "THPeligible" semantics for mTHP with anonymous shmem")
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
> Note: this general change is suitable to be split out as a bugfix patch
> based on the discussions in the previous thread[1].
>
> [1] https://lore.kernel.org/all/86bf2dcd-4be9-4fd9-98cc-da55aea52be0@lucifer.local/
> ---
>  mm/huge_memory.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
Acked-by: Zi Yan <ziy@nvidia.com>


Best Regards,
Yan, Zi


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] mm: huge_memory: fix the check for allowed huge orders in shmem
  2025-06-13  9:12 [PATCH] mm: huge_memory: fix the check for allowed huge orders in shmem Baolin Wang
  2025-06-13 11:16 ` Lorenzo Stoakes
  2025-06-13 13:06 ` Zi Yan
@ 2025-06-13 13:38 ` David Hildenbrand
  2 siblings, 0 replies; 5+ messages in thread
From: David Hildenbrand @ 2025-06-13 13:38 UTC (permalink / raw)
  To: Baolin Wang, akpm
  Cc: ziy, lorenzo.stoakes, Liam.Howlett, npache, ryan.roberts,
	dev.jain, baohua, linux-mm, linux-kernel

On 13.06.25 11:12, Baolin Wang wrote:
> Shmem already supports mTHP, and shmem_allowable_huge_orders() will return
> the huge orders allowed by shmem. However, there is no check against the
> 'orders' parameter passed by __thp_vma_allowable_orders(), which can lead
> to incorrect check results for __thp_vma_allowable_orders().
> 
> For example, when a user wants to check if shmem supports PMD-sized THP
> by thp_vma_allowable_order(), if shmem only enables 64K mTHP, the current
> logic would cause thp_vma_allowable_order() to return true, implying that
> shmem allows PMD-sized THP allocation, which it actually does not.
> 
> I don't think this will cause a significant impact on users, and this will
> only have some impact on the shmem THP collapse. That is to say, even though
> the shmem sysfs setting does not enable the PMD-sized THP, the
> thp_vma_allowable_order() still indicates that shmem allows PMD-sized collapse,
> meaning it might successfully collapse into THP, or it might not (for example,
> thp_vma_suitable_order() check failed in the collapse process). However, this
> still does not align with the shmem sysfs configuration, fix it.
> 
> Fixes: 26c7d8413aaf ("mm: thp: support "THPeligible" semantics for mTHP with anonymous shmem")
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
> Note: this general change is suitable to be split out as a bugfix patch
> based on the discussions in the previous thread[1].
> 
> [1] https://lore.kernel.org/all/86bf2dcd-4be9-4fd9-98cc-da55aea52be0@lucifer.local/
> ---
>   mm/huge_memory.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index d3e66136e41a..a8cfa37cae72 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -166,7 +166,7 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma,
>   	 * own flags.
>   	 */
>   	if (!in_pf && shmem_file(vma->vm_file))
> -		return shmem_allowable_huge_orders(file_inode(vma->vm_file),
> +		return orders & shmem_allowable_huge_orders(file_inode(vma->vm_file),
>   						   vma, vma->vm_pgoff, 0,
>   						   !enforce_sysfs);
>   

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

-- 
Cheers,

David / dhildenb



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] mm: huge_memory: fix the check for allowed huge orders in shmem
  2025-06-13 11:16 ` Lorenzo Stoakes
@ 2025-06-15 11:14   ` Baolin Wang
  0 siblings, 0 replies; 5+ messages in thread
From: Baolin Wang @ 2025-06-15 11:14 UTC (permalink / raw)
  To: Lorenzo Stoakes
  Cc: akpm, david, ziy, Liam.Howlett, npache, ryan.roberts, dev.jain,
	baohua, linux-mm, linux-kernel



On 2025/6/13 19:16, Lorenzo Stoakes wrote:
> On Fri, Jun 13, 2025 at 05:12:19PM +0800, Baolin Wang wrote:
>> Shmem already supports mTHP, and shmem_allowable_huge_orders() will return
>> the huge orders allowed by shmem. However, there is no check against the
>> 'orders' parameter passed by __thp_vma_allowable_orders(), which can lead
>> to incorrect check results for __thp_vma_allowable_orders().
>>
>> For example, when a user wants to check if shmem supports PMD-sized THP
>> by thp_vma_allowable_order(), if shmem only enables 64K mTHP, the current
>> logic would cause thp_vma_allowable_order() to return true, implying that
>> shmem allows PMD-sized THP allocation, which it actually does not.
>>
>> I don't think this will cause a significant impact on users, and this will
>> only have some impact on the shmem THP collapse. That is to say, even though
>> the shmem sysfs setting does not enable the PMD-sized THP, the
>> thp_vma_allowable_order() still indicates that shmem allows PMD-sized collapse,
>> meaning it might successfully collapse into THP, or it might not (for example,
>> thp_vma_suitable_order() check failed in the collapse process). However, this
>> still does not align with the shmem sysfs configuration, fix it.
> 
> Can you explain why?
> 
> It's a bit painful to trace through the code paths, but why do you think only
> MADV_COLLAPSE will be impacted? Surely everywhere that checks this is?

For shmem, thp_vma_allowable_order() and its wrapper are only used in 
show_smap() and shmem collapse (which includes khugepaged and 
madvise_collapse()). For shmem collapse, as I mentioned, the impact 
might not be very significant. For show_smap(), since it will use the 
'THP_ORDERS_ALL', it will not affect the results of show_smap().

>> Fixes: 26c7d8413aaf ("mm: thp: support "THPeligible" semantics for mTHP with anonymous shmem")
>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> 
> I can't see how this can be incorrect, as we really should be restricting
> ourselves to the orders requested.
> 
> So:
> 
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

Thanks.

> 
>> ---
>> Note: this general change is suitable to be split out as a bugfix patch
>> based on the discussions in the previous thread[1].
>>
>> [1] https://lore.kernel.org/all/86bf2dcd-4be9-4fd9-98cc-da55aea52be0@lucifer.local/
>> ---
>>   mm/huge_memory.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index d3e66136e41a..a8cfa37cae72 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -166,7 +166,7 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma,
>>   	 * own flags.
>>   	 */
>>   	if (!in_pf && shmem_file(vma->vm_file))
>> -		return shmem_allowable_huge_orders(file_inode(vma->vm_file),
>> +		return orders & shmem_allowable_huge_orders(file_inode(vma->vm_file),
>>   						   vma, vma->vm_pgoff, 0,
> 
> I mean this seems correct to me, but what a massive oversight.
> 
> I wish we had a sensible way of testing this...

It might not be easy to write test cases because it requires dynamically 
toggling the mTHP sysfs setting for shmem. However, as khugepaged 
supports mTHP collapse in the future, we can try to add more tests.

>>   						   !enforce_sysfs
> This whole code path is entirely indicative of what a complete mess this whole
> thing is.
> 
> The fact shmem separately calls this function is just ugh. I'm talking myself
> into some mega refactoring here :)

Yes, Shmem has its own separate mTHP sysfs interfaces, with more complex 
logic :)


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2025-06-15 11:14 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-13  9:12 [PATCH] mm: huge_memory: fix the check for allowed huge orders in shmem Baolin Wang
2025-06-13 11:16 ` Lorenzo Stoakes
2025-06-15 11:14   ` Baolin Wang
2025-06-13 13:06 ` Zi Yan
2025-06-13 13:38 ` David Hildenbrand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).