Re: [PATCH 2/3] set pthread stack size to at least PTHREAD_STACK_MIN

[Hannes Reinecke <hare@suse.de>]

Benjamin Marzinski wrote:
> Attempting to set the stacksize of a pthread to below
> PTHREAD_STACK_MIN causes pthread_attr_setstacksize() to fail, which
> means that the thread will use the default stack size.  This fix
> makes sure that multipathd never requests a stack size less than
> PTHREAD_STACK_MIN.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/log_pthread.c |    6 +++++-
>  libmultipath/waiter.c      |    5 ++++-
>  multipathd/main.c          |    5 ++++-
>  3 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
> index a1d4a10..5d2fe76 100644
> --- a/libmultipath/log_pthread.c
> +++ b/libmultipath/log_pthread.c
> @@ -6,6 +6,7 @@
>  #include <stdarg.h>
>  #include <pthread.h>
>  #include <sys/mman.h>
> +#include <limits.h>
>  
>  #include <memory.h>
>  
> @@ -52,6 +53,7 @@ static void * log_thread (void * et)
>  
>  void log_thread_start (void)
>  {
> +	size_t stacksize = 64 * 1024;
>  	pthread_attr_t attr;
>  
>  	logdbg(stderr,"enter log_thread_start\n");
> @@ -65,7 +67,9 @@ void log_thread_start (void)
>  	pthread_cond_init(logev_cond, NULL);
>  
>  	pthread_attr_init(&attr);
> -	pthread_attr_setstacksize(&attr, 64 * 1024);
> +	if (stacksize < PTHREAD_STACK_MIN)
> +		stacksize = PTHREAD_STACK_MIN;
> +	pthread_attr_setstacksize(&attr, stacksize);
>  
>  	if (log_init("multipathd", 0)) {
>  		fprintf(stderr,"can't initialize log buffer\n");
[ .. ]
Hmm. I don't quite agree. I run into the same problem, but having
discovered that we're not checking any return values at all here
we should rather do the prudent thing and check them once and for all.

I've chosen this approach:

diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
index 9e9aebe..c33480e 100644
--- a/libmultipath/log_pthread.c
+++ b/libmultipath/log_pthread.c
@@ -53,9 +53,30 @@ static void * log_thread (void * et)
 void log_thread_start (void)
 {
        pthread_attr_t attr;
+       size_t stacksize;
 
        logdbg(stderr,"enter log_thread_start\n");
 
+       if (pthread_attr_init(&attr)) {
+               fprintf(stderr,"can't initialize log thread\n");
+               exit(1);
+       }
+
+       if (pthread_attr_getstacksize(&attr, &stacksize) != 0)
+               stacksize = PTHREAD_STACK_MIN:
+
+       /* Check if the stacksize is large enough */
+       if (stacksize < (64 * 1024))
+               stacksize = 64 * 1024;
+
+       /* Set stacksize and try to reinitialize attr if failed */
+       if (stacksize > PTHREAD_STACK_MIN &&
+           pthread_attr_setstacksize(&attr, stacksize) != 0 &&
+           pthread_attr_init(&attr)) {
+               fprintf(stderr,"can't set log thread stack size\n");
+               exit(1);
+       }
+
        logq_lock = (pthread_mutex_t *) malloc(sizeof(pthread_mutex_t));
        logev_lock = (pthread_mutex_t *) malloc(sizeof(pthread_mutex_t));
        logev_cond = (pthread_cond_t *) malloc(sizeof(pthread_cond_t));
@@ -64,9 +85,6 @@ void log_thread_start (void)
        pthread_mutex_init(logev_lock, NULL);
        pthread_cond_init(logev_cond, NULL);
 
-       pthread_attr_init(&attr);
-       pthread_attr_setstacksize(&attr, 64 * 1024);
-
        if (log_init("multipathd", 0)) {
                fprintf(stderr,"can't initialize log buffer\n");
                exit(1);


This way we'll at least be notified if something goes wrong in
the future. We shouldn't make the same mistake again and
ignore error codes which don't happen to trigger now.

If agreed I'll post the full patch here.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)