Re: pthread wastes memory with mlockall(MCL_FUTURE)

[Rich Felker <dalias at libc dot org>]

On Sun, Sep 20, 2015 at 02:27:12PM +0100, Balazs Kezes wrote:
> On 2015-09-18 19:22 -0400, Rich Felker wrote:
> > If this works, I think it's only due to a kernel bug of failing to
> > apply the lock after mprotect. It's also going to be considerably
> > slower, I think. What I had in mind was switching around the existing
> > mmap/mprotect order, not adding an extra mprotect.
> 
> Here's a better patch. It will readd the permissions in two steps. First
> it will only add for this pthread structure at the end of the
> allocation. Then at a later step it will readd it for the rest of the
> stack (this part I didn't touch so that's why this patch is short).
> 
> I don't really like this two-step mprotect call but unless the stack
> grows down, you can't avoid it (and I didn't special case that option).
> 
> But review carefully because I don't know how to test it better than
> with my little test application and make xcheck.
> 
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index 753da61..2959816 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -499,11 +499,11 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
>  #if MULTI_PAGE_ALIASING != 0
>  	  if ((size % MULTI_PAGE_ALIASING) == 0)
>  	    size += pagesize_m1 + 1;
>  #endif
>  
> -	  mem = mmap (NULL, size, prot,
> +	  mem = mmap (NULL, size, PROT_NONE,
>  		      MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
>  
>  	  if (__glibc_unlikely (mem == MAP_FAILED))
>  	    return errno;
>  
> @@ -542,13 +542,25 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
>  				    - __static_tls_size)
>  				    & ~__static_tls_align_m1)
>  				   - TLS_PRE_TCB_SIZE);
>  #endif
>  
> +	  /* We allocated the memory without permissions in order for the kernel
> +	     to not allocate the guard pages in case of mlockall(MCL_FUTURE).
> +	     Now readd the permissions for pd's page.  */

This comment misses the real point: avoiding committing memory.
MCL_FUTURE is just a particular side effect of this.

> +	  void *pdpage = (void*) ((uintptr_t) pd & ~pagesize_m1);
> +	  size_t pdsize = mem + size - pdpage;
> +	  if (__glibc_unlikely (mprotect (pdpage, pdsize, prot) != 0))
> +	    {
> +	      (void) munmap (mem, size);
> +	      return errno;
> +	    }
> +

You're still adding a new mprotect rather than just inverting the one
that's already there (somewhere else) adding the PROT_NONE for the
guard page. Is there a reason this is hard to do right on glibc?

Rich