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

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

On 5/3/25 2:50 AM, Lorenzo Stoakes wrote:
> +cc Matthew
>
> On Fri, May 02, 2025 at 01:53:15PM -0700, Yang Shi wrote:
>>
>> 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.
> No problem!
>
>>> 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!
>
> I also believe Matthew's suggestion (see [0]) is very much worth doing
> here, in my view doing both is a sensible approach, would your R-b tag
> extend to that?

Yes. VM_NOHUGEPAGE is a no-op on !THP kernel anyway. It is pointless to 
fail it. It should be unlikely any real life application rely on this 
behavior (returning -EINVAL).

>
> In which case, Ignacio - could you apply both Matthew's change and address
> the nit below and send a v2?
>
> Thanks!
>
> [0]: https://lore.kernel.org/linux-mm/aBTCtOXBhUK_FLU6@casper.infradead.org/
>
>>> 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>
>>>>
>>>>