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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

+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").

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>

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>
>
>