[PATCH] NPTL proposed large page fix for pthread_create.

[PATCH] NPTL proposed large page fix for pthread_create.

[Steven Munroe]

[-- Attachment #1: Type: text/plain, Size: 409 bytes --]

This patch fixes the problem on systems that support multiple page
sizes, but we don't want to change the PTHREAD_STACK_MIN const that is
still valid for the smaller page size and the ATTR_FLAG_STACKADDR case.
For the case where the runtime allocates the stack we need to make sure
the 2 page minimum is meet.  Where the PTHREAD_STACK_MIN is less than
the 2 page minimum, bump the allocation up to 2 pages.



[-- Attachment #2: ppc-64kpage-20051128.txt --]
[-- Type: text/plain, Size: 4703 bytes --]

2005-12-05  Steven Munroe  <sjmunroe@us.ibm.com>

	* allocatestack.c (allocate_stack): Handle large page case.
	* tst-stack2.c: Include unistd.h.
	(do_test): Add tests for PTHREAD_STACK_MIN < pagesize and
	user allocated stack of PTHREAD_STACK_MIN bytes.

diff -urN libc24-cvstip-20051128/nptl/allocatestack.c libc24/nptl/allocatestack.c
--- libc24-cvstip-20051128/nptl/allocatestack.c	2005-10-03 19:40:01.000000000 -0500
+++ libc24/nptl/allocatestack.c	2005-12-05 15:56:03.076424936 -0600
@@ -407,11 +407,27 @@
       /* Make sure the size of the stack is enough for the guard and
 	 eventually the thread descriptor.  */
       guardsize = (attr->guardsize + pagesize_m1) & ~pagesize_m1;
-      if (__builtin_expect (size < (guardsize + __static_tls_size
-				    + MINIMAL_REST_STACK + pagesize_m1 + 1),
+      if (__builtin_expect (size < ((guardsize + __static_tls_size
+				    + MINIMAL_REST_STACK
+				    + pagesize_m1) & ~pagesize_m1),
 			    0))
-	/* The stack is too small (or the guard too large).  */
-	return EINVAL;
+	{
+	/* The stack is too small (or the guard too large), but this might not
+	   be an error.  If we are on a system that supports multiple page
+	   sizes,  the current page size may be larger then PTHREAD_STACK_MIN,
+	   but we need to allocate a minimum of two pages for guard and stack.
+	   In this case we can allocate the two page minimum.  */
+	  if ((size >= PTHREAD_STACK_MIN)
+	    && (PTHREAD_STACK_MIN< ((pagesize_m1 + 1)  * 2)))
+	    /* The requested size is >= PTHREAD_STACK_MIN,  so the application
+	       expect this to work. But PTHREAD_STACK_MIN is < 2 of the
+	       current pages,  so bump the stack size up to two pages at the
+	       current page size.  */
+	    size = (pagesize_m1 + 1) * 2;
+	  else
+	    /* Not the large page case so return EINVAL.  */
+	    return EINVAL;
+	}
 
       /* Try to get a stack from the cache.  */
       reqsize = size;
diff -urN libc24-cvstip-20051128/nptl/tst-stack2.c libc24/nptl/tst-stack2.c
--- libc24-cvstip-20051128/nptl/tst-stack2.c	2003-09-03 00:06:52.000000000 -0500
+++ libc24/nptl/tst-stack2.c	2005-12-05 15:45:30.599457656 -0600
@@ -23,6 +23,7 @@
 #include <pthread.h>
 #include <stdlib.h>
 #include <stdio.h>
+#include <unistd.h>
 
 static int seen;
 
@@ -38,21 +39,94 @@
 {
   pthread_attr_t attr;
   pthread_attr_init (&attr);
+  long pagesize;
 
   int result = 0;
-  int res = pthread_attr_setstacksize (&attr, PTHREAD_STACK_MIN);
+  int res;
+
+  pagesize = sysconf(_SC_PAGESIZE);
+  printf ("sysconf(_SC_PAGESIZE) = %ld\n", pagesize);
+  
+  res = pthread_attr_setstacksize (&attr, PTHREAD_STACK_MIN);
   if (res)
     {
       printf ("pthread_attr_setstacksize failed %d\n", res);
       result = 1;
     }
 
-  /* Create the thread.  */
+  /* Create the thread with PTHREAD_STACK_MIN stack size.  */
   pthread_t th;
   res = pthread_create (&th, &attr, tf, NULL);
   if (res)
     {
       printf ("pthread_create failed %d\n", res);
+      
+      if ((PTHREAD_STACK_MIN) < (2 * pagesize))
+        {
+	/* This should have worked because the system has multiple pages sizes
+	   and it is currently running with pagesize >= PTHREAD_STACK_MIN.  */
+          puts("PTHREAD_STACK_MIN < (2 * pagesize)");
+	}
+      else
+        result = 1;
+    }
+  else
+    {
+      res = pthread_join (th, NULL);
+      if (res)
+	{
+	  printf ("pthread_join failed %d\n", res);
+	  result = 1;
+	}
+    }
+    
+  /* The PTHREAD_STACK_MIN should always be valid when stackaddr is set.  */
+  
+  void *stack;
+  size_t size = PTHREAD_STACK_MIN;
+
+  
+  if (posix_memalign (&stack, getpagesize (), size) != 0)
+    {
+      puts ("out of memory while allocating the stack memory");
+      return (1);
+    }
+
+  if (pthread_attr_init (&attr) != 0)
+    {
+      puts ("attr_init failed");
+      return (1);
+    }
+
+  if (pthread_attr_setstack (&attr, stack, size) != 0)
+    {
+      puts ("attr_setstack failed");
+      return (1);
+    }
+    
+  res = pthread_create (&th, &attr, tf, NULL);
+  if (res)
+    {
+      printf ("pthread_create failed %d\n", res);
+      result = 1;
+    }
+  else
+    {
+      res = pthread_join (th, NULL);
+      if (res)
+	{
+	  printf ("pthread_join failed %d\n", res);
+	  result = 1;
+	}
+    }
+
+  /* The default pthread_attr_t should always work, even if the page
+     size is larger then expected.  */
+    
+  res = pthread_create (&th, NULL, tf, NULL);
+  if (res)
+    {
+      printf ("pthread_create failed %d\n", res);
       result = 1;
     }
   else
@@ -65,9 +139,9 @@
 	}
     }
 
-  if (seen != 1)
+  if (seen != 3)
     {
-      printf ("seen %d != 1\n", seen);
+      printf ("seen %d != 3\n", seen);
       result = 1;
     }
 

Re: [PATCH] NPTL proposed large page fix for pthread_create.

[Ulrich Drepper]

Steven Munroe wrote:
> +	{
> +	/* The stack is too small (or the guard too large), but this might not
> +	   be an error.  If we are on a system that supports multiple page
> +	   sizes,  the current page size may be larger then PTHREAD_STACK_MIN,
> +	   but we need to allocate a minimum of two pages for guard and stack.
> +	   In this case we can allocate the two page minimum.  */
> +	  if ((size >= PTHREAD_STACK_MIN)
> +	    && (PTHREAD_STACK_MIN< ((pagesize_m1 + 1)  * 2)))
> +	    /* The requested size is >= PTHREAD_STACK_MIN,  so the application
> +	       expect this to work. But PTHREAD_STACK_MIN is < 2 of the
> +	       current pages,  so bump the stack size up to two pages at the
> +	       current page size.  */
> +	    size = (pagesize_m1 + 1) * 2;
> +	  else
> +	    /* Not the large page case so return EINVAL.  */
> +	    return EINVAL;
> +	}

This is completely bogus.  The if expression change is OK, but if the 
stack is too small, it is too small.  Why on earth would you think that 
if the test for the minimally required stack size fails you can just add 
a test like this to work around it?  This second test is completely 
independent of the actual size needed.

If the modified test I checked in fails there is *nothing* which can be 
done.  There is no negotiating about the required stack size.  You 
cannot just ignore this fact.  If all this means programs which work 
with normal page sizes now fail, then that's a sad outcome.  Just 
another piece of evidence of the poor ABI design.


And as usual, you completely ignore the coding conventions.  Well, I can 
ignore, too.  I'll ignore every patch you sent which does not use the 
correct coding conventions.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖