[PATCH] Fix linuxthreads with pthread_attr_setstack{,addr}

[PATCH] Fix linuxthreads with pthread_attr_setstack{,addr}

[Jakub Jelinek]

Hi!

Despite what the bugzilla says, I believe the problem is not related
to TLS.  After pthread_join successfully returns, the user thread stack
may be still in use (both by the thread manager (thread descriptor,
i.e. !TLS only) and by the actual thread being joined (both the thread
descriptor and stack being in use)).
The following patch for p_userstack threads waits in pthread_join
till thread manager tells it will not use the thread stack any longer.

2004-04-29  Jakub Jelinek  <jakub@redhat.com>

	[BZ #110]
	* descr.h (struct _pthread_descr_struct): Add p_userstack_joining
	field.
	* internals.h (struct pthread_request): Add REQ_FREE_AND_NOTIFY
	request type.
	* join.c (pthread_join): Use REQ_FREE_AND_NOTIFY if th->p_userstack
	instead of REQ_FREE and suspend till thread manager awakens us.
	* manager.c (pthread_handle_free): Add req_thread argument.  If
	non-NULL, restart that thread if freed thread is already dead or
	set p_userstack_joining.
	(__pthread_manager): Adjust caller.  Handle REQ_FREE_AND_NOTIFY.
	(pthread_exited): Restart p_userstack_joining thread.

	* Makefile (tests): Add tst-stack2.
	* tst-stack2.c: New test.

--- libc/linuxthreads/join.c.jj	2002-12-18 02:14:09.000000000 +0100
+++ libc/linuxthreads/join.c	2004-04-29 17:49:25.384460978 +0200
@@ -173,9 +173,13 @@ int pthread_join(pthread_t thread_id, vo
   if (__pthread_manager_request >= 0) {
     request.req_thread = self;
     request.req_kind = REQ_FREE;
+    if (th->p_userstack)
+      request.req_kind = REQ_FREE_AND_NOTIFY;
     request.req_args.free.thread_id = thread_id;
     TEMP_FAILURE_RETRY(__libc_write(__pthread_manager_request,
 				    (char *) &request, sizeof(request)));
+    if (request.req_kind == REQ_FREE_AND_NOTIFY)
+      suspend (self);
   }
   return 0;
 }
--- libc/linuxthreads/descr.h.jj	2003-09-17 13:42:30.000000000 +0200
+++ libc/linuxthreads/descr.h	2004-04-29 17:48:26.765966175 +0200
@@ -189,6 +189,8 @@ struct _pthread_descr_struct
 #endif
   size_t p_alloca_cutoff;	/* Maximum size which should be allocated
 				   using alloca() instead of malloc().  */
+  pthread_descr p_userstack_joining; /* Thread waiting until thread stack
+					can be freed.  */
   /* New elements must be added at the end.  */
 } __attribute__ ((aligned(32))); /* We need to align the structure so that
 				    doubles are aligned properly.  This is 8
--- libc/linuxthreads/internals.h.jj	2003-09-17 13:42:30.000000000 +0200
+++ libc/linuxthreads/internals.h	2004-04-29 11:13:00.000000000 +0200
@@ -81,7 +81,7 @@ struct pthread_request {
   pthread_descr req_thread;     /* Thread doing the request */
   enum {                        /* Request kind */
     REQ_CREATE, REQ_FREE, REQ_PROCESS_EXIT, REQ_MAIN_THREAD_EXIT,
-    REQ_POST, REQ_DEBUG, REQ_KICK, REQ_FOR_EACH_THREAD
+    REQ_POST, REQ_DEBUG, REQ_KICK, REQ_FOR_EACH_THREAD, REQ_FREE_AND_NOTIFY
   } req_kind;
   union {                       /* Arguments for request */
     struct {                    /* For REQ_CREATE: */
@@ -90,7 +90,7 @@ struct pthread_request {
       void * arg;               /*   argument to start function */
       sigset_t mask;            /*   signal mask */
     } create;
-    struct {                    /* For REQ_FREE: */
+    struct {                    /* For REQ_FREE and REQ_FREE_AND_NOTIFY: */
       pthread_t thread_id;      /*   identifier of thread to free */
     } free;
     struct {                    /* For REQ_PROCESS_EXIT: */
--- libc/linuxthreads/manager.c.jj	2003-12-19 17:57:04.000000000 +0100
+++ libc/linuxthreads/manager.c	2004-04-29 17:57:58.221553893 +0200
@@ -101,7 +101,7 @@ static int pthread_handle_create(pthread
                                  sigset_t *mask, int father_pid,
 				 int report_events,
 				 td_thr_events_t *event_maskp);
-static void pthread_handle_free(pthread_t th_id);
+static void pthread_handle_free(pthread_t th_id, pthread_descr req_thread);
 static void pthread_handle_exit(pthread_descr issuing_thread, int exitcode)
      __attribute__ ((noreturn));
 static void pthread_reap_children(void);
@@ -187,8 +187,11 @@ __pthread_manager(void *arg)
         restart(request.req_thread);
         break;
       case REQ_FREE:
-	pthread_handle_free(request.req_args.free.thread_id);
+	pthread_handle_free(request.req_args.free.thread_id, NULL);
         break;
+      case REQ_FREE_AND_NOTIFY:
+	pthread_handle_free(request.req_args.free.thread_id, request.req_thread);
+	break;
       case REQ_PROCESS_EXIT:
         pthread_handle_exit(request.req_thread,
                             request.req_args.exit.code);
@@ -917,7 +920,7 @@ static void pthread_free(pthread_descr t
 
 static void pthread_exited(pid_t pid)
 {
-  pthread_descr th;
+  pthread_descr th, userstack_joining;
   int detached;
   /* Find thread with that pid */
   for (th = __pthread_main_thread->p_nextlive;
@@ -950,9 +953,14 @@ static void pthread_exited(pid_t pid)
 	    }
 	}
       detached = th->p_detached;
+      userstack_joining = th->p_userstack_joining;
       __pthread_unlock(th->p_lock);
       if (detached)
-	pthread_free(th);
+	{
+	  pthread_free(th);
+	  if (userstack_joining)
+	    restart (userstack_joining);
+	}
       break;
     }
   }
@@ -984,7 +992,7 @@ static void pthread_reap_children(void)
 /* Try to free the resources of a thread when requested by pthread_join
    or pthread_detach on a terminated thread. */
 
-static void pthread_handle_free(pthread_t th_id)
+static void pthread_handle_free(pthread_t th_id, pthread_descr req_thread)
 {
   pthread_handle handle = thread_handle(th_id);
   pthread_descr th;
@@ -994,17 +1002,22 @@ static void pthread_handle_free(pthread_
     /* pthread_reap_children has deallocated the thread already,
        nothing needs to be done */
     __pthread_unlock(&handle->h_lock);
+    if (req_thread)
+      restart (req_thread);
     return;
   }
   th = handle->h_descr;
   if (th->p_exited) {
     __pthread_unlock(&handle->h_lock);
     pthread_free(th);
+    if (req_thread)
+      restart (req_thread);
   } else {
     /* The Unix process of the thread is still running.
        Mark the thread as detached so that the thread manager will
        deallocate its resources when the Unix process exits. */
     th->p_detached = 1;
+    th->p_userstack_joining = req_thread;
     __pthread_unlock(&handle->h_lock);
   }
 }
--- libc/linuxthreads/Makefile.jj	2004-02-23 09:46:19.000000000 +0100
+++ libc/linuxthreads/Makefile	2004-04-29 11:31:57.164316902 +0200
@@ -111,7 +111,7 @@ tests = ex1 ex2 ex3 ex4 ex5 ex6 ex7 ex8 
 	ex17 ex18 tst-cancel tst-context bug-sleep \
 	tst-cancel1 tst-cancel2 tst-cancel3 tst-cancel4 tst-cancel5 \
 	tst-cancel6 tst-cancel7 tst-cancel8 tst-popen tst-popen2 tst-attr1 \
-	tst-stack1
+	tst-stack1 tst-stack2
 test-srcs = tst-signal
 # These tests are linked with libc before libpthread
 tests-reverse += tst-cancel5
--- libc/linuxthreads/tst-stack2.c.jj	2004-04-29 15:34:20.398979901 +0200
+++ libc/linuxthreads/tst-stack2.c	2004-04-29 17:58:36.314727101 +0200
@@ -0,0 +1,96 @@
+/* Copyright (C) 2004 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Jakub Jelinek <jakub@redhat.com>, 2004.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, write to the Free
+   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+   02111-1307 USA.  */
+
+#include <limits.h>
+#include <pthread.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+
+static int seen;
+
+static void *
+tf (void *p)
+{
+  ++seen;
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  int result = 0;
+  for (int i = 0; i < 128; ++i)
+    {
+      void *stack;
+      int res = posix_memalign (&stack, getpagesize (), 16 * PTHREAD_STACK_MIN);
+      if (res)
+	{
+	  printf ("malloc failed %s\n", strerror (res));
+	  return 1;
+	}
+
+      pthread_attr_t attr;
+      pthread_attr_init (&attr);
+
+      res = pthread_attr_setstack (&attr, stack, 16 * PTHREAD_STACK_MIN);
+      if (res)
+	{
+	  printf ("pthread_attr_setstack failed %d\n", res);
+	  result = 1;
+	}
+
+      /* Create the thread.  */
+      pthread_t th;
+      res = pthread_create (&th, &attr, tf, NULL);
+      if (res)
+	{
+	  printf ("pthread_create failed %d\n", res);
+	  result = 1;
+	  break;
+	}
+      else
+	{
+	  res = pthread_join (th, NULL);
+	  if (res)
+	    {
+	      printf ("pthread_join failed %d\n", res);
+	      result = 1;
+	      break;
+	    }
+	}
+
+      free (stack);
+      pthread_attr_destroy (&attr);
+
+    }
+
+  if (seen != 128)
+    {
+      printf ("seen %d != 128\n", seen);
+      result = 1;
+    }
+
+  return result;
+}
+
+#define TIMEOUT 10
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"

	Jakub

Re: [PATCH] Fix linuxthreads with pthread_attr_setstack{,addr}

[Jakub Jelinek]

On Thu, Apr 29, 2004 at 04:19:02PM +0200, Jakub Jelinek wrote:
> Hi!
> 
> Despite what the bugzilla says, I believe the problem is not related
> to TLS.  After pthread_join successfully returns, the user thread stack
> may be still in use (both by the thread manager (thread descriptor,
> i.e. !TLS only) and by the actual thread being joined (both the thread
> descriptor and stack being in use)).
> The following patch for p_userstack threads waits in pthread_join
> till thread manager tells it will not use the thread stack any longer.

POSIX says on pthread_join:

"For instance, after pthread_join() returns, any application-provided stack storage
 could be reclaimed."

so I guess we need to support that.

	Jakub

Re: [PATCH] Fix linuxthreads with pthread_attr_setstack{,addr}

[Wolfram Gloger]

Hello,

> POSIX says on pthread_join:
> 
> "For instance, after pthread_join() returns, any application-provided stack storage
>  could be reclaimed."

Chapter and verse?  This sounds rather vague, too.

On comp.programming.threads, pthreads architect Dave Butenhof has
claimed repeatedly that POSIX makes _no_ such guarantees, i.e. that
strictly speaking you can _never_ free user-defined stack space..
Eg.:

http://groups.google.de/groups?hl=de&lr=&ie=UTF-8&oe=ISO-8859-1&selm=3A797AB3.A57D43E9%40compaq.com

Regards,
Wolfram.

Re: [PATCH] Fix linuxthreads with pthread_attr_setstack{,addr}

[Jakub Jelinek]

On Fri, Apr 30, 2004 at 05:14:50PM -0000, Wolfram Gloger wrote:
> Hello,
> 
> > POSIX says on pthread_join:
> > 
> > "For instance, after pthread_join() returns, any application-provided stack storage
> >  could be reclaimed."
> 
> Chapter and verse?  This sounds rather vague, too.
> 
> On comp.programming.threads, pthreads architect Dave Butenhof has
> claimed repeatedly that POSIX makes _no_ such guarantees, i.e. that
> strictly speaking you can _never_ free user-defined stack space..
> Eg.:
> 
> http://groups.google.de/groups?hl=de&lr=&ie=UTF-8&oe=ISO-8859-1&selm=3A797AB3.A57D43E9%40compaq.com

It is in pthread_join's RATIONALE:
http://www.opengroup.org/onlinepubs/009695399/functions/pthread_join.html

	Jakub

Re: [PATCH] Fix linuxthreads with pthread_attr_setstack{,addr}

[Wolfram Gloger]

Hi,

> It is in pthread_join's RATIONALE:
> http://www.opengroup.org/onlinepubs/009695399/functions/pthread_join.html

Interesting.  AFAIC remember, this has changed from when I looked at
it in 2001.  Although only in the "informative" section, I would
actually welcome such a guarantee, and think we should make it
possible to free application stacks after a return from
pthread_join().

Regards,
Wolfram.

Re: [PATCH] Fix linuxthreads with pthread_attr_setstack{,addr}

[Jakub Jelinek]

On Fri, Apr 30, 2004 at 06:04:17PM -0000, Wolfram Gloger wrote:
> Hi,
> 
> > It is in pthread_join's RATIONALE:
> > http://www.opengroup.org/onlinepubs/009695399/functions/pthread_join.html
> 
> Interesting.  AFAIC remember, this has changed from when I looked at
> it in 2001.  Although only in the "informative" section, I would
> actually welcome such a guarantee, and think we should make it
> possible to free application stacks after a return from
> pthread_join().

Actually, the yesterday released TC2 clarifies user defined stacks
and indeed a conforming application cannot free ever user defined stack
once it has been used successfully in some pthread_create or SIGEV_THREAD
notification. 2.9.8 is new in TC2:
http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_09.html#tag_02_09_08

	Jakub

Re: [PATCH] Fix linuxthreads with pthread_attr_setstack{,addr}

[Ulrich Drepper]

I won't add this patch since, as clarified in FC2, user-provided stacks
can never be reused or used otherwise.  They are fine for threads which
run forever but otherwise user-provided stacks shouldn't be used.

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