Re: [PATCH] mm: mmap: map MAP_STACK to VM_NOHUGEPAGE only if THP is enabled

[Yang Shi <yang@os.amperecomputing.com>]

On 5/2/25 5:46 AM, Lorenzo Stoakes wrote:
> +cc Andrew.
>
> Ignacio, you should always include Andrew in patch submissions to mm :)
>
> +cc Yang Shi who added this in the first place in commit c4608d1bf7c6 ("mm:
> mmap: map MAP_STACK to VM_NOHUGEPAGE").

Thanks for cc'ing me.

>
> On Fri, May 02, 2025 at 11:31:41AM +0200, Ignacio Moreno Gonzalez via B4 Relay wrote:
>> From: Ignacio Moreno Gonzalez <Ignacio.MorenoGonzalez@kuka.com>
>>
>> commit c4608d1bf7c6 ("mm: mmap: map MAP_STACK to VM_NOHUGEPAGE") maps
>> the mmap option MAP_STACK to VM_NOHUGEPAGE. This is also done if
>> CONFIG_TRANSPARENT_HUGETABLES is not defined. But in that case, the
>> VM_NOHUGEPAGE does not make sense. For instance, when calling madvise()
>> with MADV_NOHUGEPAGE, an error is always returned.
>>
>> Signed-off-by: Ignacio Moreno Gonzalez <Ignacio.MorenoGonzalez@kuka.com>
> I don't see how can this cause a problem, and it fixes one in practice, so
> LGTM. Though see note below about CRIU :)
>
> I also added a nit below, if you address this you can re-use my tag.
>
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

Yeah, I agree. Looks good to me too. Reviewed-by: Yang Shi 
<yang@os.amperecomputing.com>

>
> Thanks!
>
> Do we want to back port this to stable kernels? If so we should have a:
>
> Fixes: c4608d1bf7c6 ("mm: mmap: map MAP_STACK to VM_NOHUGEPAGE")
> cc: stable@vger.kernel.org
>
> Appended here, and Greg's scripts should automagically backport, assuming
> no conflicts or such (I don't _think_ there would be...)
>
>> ---
>> I discovered this issue when trying to use the tool CRIU to checkpoint
>> and restore a container. Our running kernel is compiled without
>> CONFIG_TRANSPARENT_HUGETABLES. CRIU parses the output of
>> /proc/<pid>/smaps and saves the "nh" flag. When trying to restore the
>> container, CRIU fails to restore the "nh" mappings, since madvise()
>> MADV_NOHUGEPAGE always returns an error because
>> CONFIG_TRANSPARENT_HUGETABLES is not defined.
> Yeah this is really not a stable or valid use of the /proc/$pid/[s]maps
> interface :P CRIU is sort of a blurry line of relying on internal
> implementation details so we're kinda not obligated to prevent breakages.
>
> CRIU is kinda relying on internal implementation details so debatable as to
> whether we should be bending over backwards to support.
>
> BUT, we also don't want to cause unwanted issues if there's a simple fix
> and this seems reasonable to me.
>
>> ---
>>   include/linux/mman.h | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/include/linux/mman.h b/include/linux/mman.h
>> index bce214fece16b9af3791a2baaecd6063d0481938..1e83bc0e3db670b04743f5208826e87455a05325 100644
>> --- a/include/linux/mman.h
>> +++ b/include/linux/mman.h
>> @@ -155,7 +155,9 @@ calc_vm_flag_bits(struct file *file, unsigned long flags)
>>   	return _calc_vm_trans(flags, MAP_GROWSDOWN,  VM_GROWSDOWN ) |
>>   	       _calc_vm_trans(flags, MAP_LOCKED,     VM_LOCKED    ) |
>>   	       _calc_vm_trans(flags, MAP_SYNC,	     VM_SYNC      ) |
>> +#if defined(CONFIG_TRANSPARENT_HUGEPAGE)
> NIT, but can we use ifdef here for consistency? Thanks.
>
>>   	       _calc_vm_trans(flags, MAP_STACK,	     VM_NOHUGEPAGE) |
>> +#endif
>>   	       arch_calc_vm_flag_bits(file, flags);
>>   }
>>
>>
>> ---
>> base-commit: fc96b232f8e7c0a6c282f47726b2ff6a5fb341d2
>> change-id: 20250428-map-map_stack-to-vm_nohugepage-only-if-thp-is-enabled-ce40a1de095d
>>
>> Best regards,
>> --
>> Ignacio Moreno Gonzalez <Ignacio.MorenoGonzalez@kuka.com>
>>
>>