Re: [PATCH v2 5/5] fork: zero vmap stack using clear_page() instead of memset()

[Mateusz Guzik <mjguzik@gmail.com>]

On Wed, May 07, 2025 at 02:46:31PM +0200, Linus Walleij wrote:
> From: Pasha Tatashin <pasha.tatashin@soleen.com>
> 
> Do not zero the whole span of the stack, but instead only the
> pages that are part of the vm_area.
> 
> As several architectures have optimized implementations of
> clear_page(), this will give the architecture a clear idea
> of what is going on and will speed up the clearing operation.
> 
> Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
> Link: https://lore.kernel.org/20240311164638.2015063-7-pasha.tatashin@soleen.com
> [linus.walleij@linaro.org: Rebased]
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  kernel/fork.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 6ac4674fdf04081fbcbf1eb99167a4c990a58506..d3f000b846c634221c7dafe6e64185a276c5a08b 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -271,13 +271,15 @@ static int memcg_charge_kernel_stack(struct vm_struct *vm_area)
>  static int alloc_thread_stack_node(struct task_struct *tsk, int node)
>  {
>  	struct vm_struct *vm_area;
> +	int nr_pages;
>  	void *stack;
> -	int i;
> +	int i, j;
>  
>  	for (i = 0; i < NR_CACHED_STACKS; i++) {
>  		vm_area = this_cpu_xchg(cached_stacks[i], NULL);
>  		if (!vm_area)
>  			continue;
> +		nr_pages = vm_area->nr_pages;
>  
>  		if (memcg_charge_kernel_stack(vm_area)) {
>  			vfree(vm_area->addr);
> @@ -290,7 +292,8 @@ static int alloc_thread_stack_node(struct task_struct *tsk, int node)
>  		stack = kasan_reset_tag(vm_area->addr);
>  
>  		/* Clear stale pointers from reused stack. */
> -		memset(stack, 0, THREAD_SIZE);
> +		for (j = 0; j < nr_pages; j++)
> +			clear_page(page_address(vm_area->pages[j]));
>  
>  		tsk->stack_vm_area = vm_area;
>  		tsk->stack = stack;
> 
> -- 
> 2.49.0
> 
> 

I don't know if the logic works here as far as the speed up goes -- you
are in fact *taking away* information what the caller is doing.

In order to actually allow archs to optimize this you would need a
clearing func which grabs the page count.

For example, suppose 'rep stosb' is the optimal way to handle the 16K
stacks. With a clear_pages(addr, n) routine it gets issued once with the
appropriate size the CPU knows about. In code as provided here you get 4
invocations instead.

There was a patchset to support multi-page clearing, but it only covers
x86-64: https://lore.kernel.org/all/20250414034607.762653-1-ankur.a.arora@oracle.com/

Side note: funnily enough so happens memset on x86-64 is pretty bad the
moment, never using rep stos regardless of size. But that's an argument
for adding rep, after which perf will be about the same.

tl;dr I think this patch should be dropped. if multi-page clearing shows
up for more archs then this is a thing to consider.