pthread wastes memory with mlockall(MCL_FUTURE)

  On 2015-09-18 15:45 -0400, Rich Felker wrote:
> On Fri, Sep 18, 2015 at 08:29:52PM +0100, Balazs Kezes wrote:
> > So here's what I think pthreads should do: First mmap with PROT_NONE
> > and only then should mprotect read/write the stack pages.
> >
> > Does that sound reasonable?
>
> Yes.

So here's the simplest patch I could come up with:



I've verified in my pthreads example that pthreads doesn't waste memory
with this patch applied. That's not really a nice fix though but this
allocatestack() function looks too scary to me. :(
  

On Fri, Sep 18, 2015 at 09:11:01PM +0100, Balazs Kezes wrote:
> On 2015-09-18 15:45 -0400, Rich Felker wrote:
> > On Fri, Sep 18, 2015 at 08:29:52PM +0100, Balazs Kezes wrote:
> > > So here's what I think pthreads should do: First mmap with PROT_NONE
> > > and only then should mprotect read/write the stack pages.
> > >
> > > Does that sound reasonable?
> >
> > Yes.
> 
> So here's the simplest patch I could come up with:
> 
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index 753da61..c6065dc 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -501,12 +501,21 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
>  	    size += pagesize_m1 + 1;
>  #endif
>  
> -	  mem = mmap (NULL, size, prot,
> +	  /* Map with PROT_NONE first and only then mprotect the pages to avoid
> +	     the kernel unnecessary reserving the pages in the case of
> +	     mlockall(MCL_FUTURE).  */
> +	  mem = mmap (NULL, size, PROT_NONE,
>  		      MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
>  
>  	  if (__glibc_unlikely (mem == MAP_FAILED))
>  	    return errno;
>  
> +	  if (__glibc_unlikely (mprotect (mem, size, prot) != 0))
> +	    {
> +	      munmap(mem, size);
> +	      return errno;
> +	    }
> +
>  	  /* SIZE is guaranteed to be greater than zero.
>  	     So we can never get a null pointer back from mmap.  */
>  	  assert (mem != NULL);
> 
> 
> I've verified in my pthreads example that pthreads doesn't waste memory
> with this patch applied. That's not really a nice fix though but this
> allocatestack() function looks too scary to me. :(

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.

Rich