From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3524 invoked by alias); 17 Oct 2018 19:46:17 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 3511 invoked by uid 89); 17 Oct 2018 19:46:17 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=suspend X-HELO: mail-qt1-f194.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:from:to:references:openpgp:autocrypt:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=RTzXNpChXr7ucTAh6KcZeSehYnySuQsnPks7h+nmVpE=; b=hB/0bbmZfbT7S2KhpqzIGlbKHVI0rQ+urvi/wG8u7KdmKpdQJluunRZFq8iS/cs/Zv 440wtYag0M9OynKk0a3Qp2PUxUm9Q5nKOYgy1AuU+LdW9Xun6iYoo7f9JuOFyIBbdQq1 uJeZ46gpeKkFvavtq/yw6uaDfZ+sJP2uxwDY4= Return-Path: Subject: Re: [PATCH 3/3] posix: Use posix_spawn on system From: Adhemerval Zanella To: libc-alpha@sourceware.org References: <20180915151622.17789-1-adhemerval.zanella@linaro.org> <20180915151622.17789-3-adhemerval.zanella@linaro.org> Openpgp: preference=signencrypt Message-ID: Date: Wed, 17 Oct 2018 20:13:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20180915151622.17789-3-adhemerval.zanella@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-10/txt/msg00315.txt.bz2 On 15/09/2018 12:16, Adhemerval Zanella wrote: > This patch uses posix_spawn on system implementation. On Linux this has > the advantage of much lower memory consumption (usually 32 Kb minimum for > the mmap stack area). > > Although POSIX does not require, glibc system implementation aims to be > thread and cancellation safe. While reentracy handling does not require > any direct change of current strategy, cancellation requires a posix_spawn > to be cancellable. This is done by adding an internal > __posix_spawn_cancellable which does not disable cancellation neither > change process signal mask. > > The cancellation code is also moved to generic implementation and enabled > only if SIGCANCEL is defined (similar on how the cancellation handler is > enabled on nptl-init.c). > > Checked on x86_64-linux-gnu and i686-linux-gnu. A small fix below (I can resend the patch if required). > diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c > index 85239cedbf..ed5c613e42 100644 > --- a/sysdeps/unix/sysv/linux/spawni.c > +++ b/sysdeps/unix/sysv/linux/spawni.c > @@ -138,11 +138,11 @@ __spawni_child (void *arguments) > for (int sig = 1; sig < _NSIG; ++sig) > { > if ((attr->__flags & POSIX_SPAWN_SETSIGDEF) > - && sigismember (&attr->__sd, sig)) > + && __sigismember (&attr->__sd, sig)) > { > sa.sa_handler = SIG_DFL; > } > - else if (sigismember (&hset, sig)) > + else if (__sigismember (&hset, sig)) > { > if (__is_internal_signal (sig)) > sa.sa_handler = SIG_IGN; > @@ -330,10 +330,14 @@ __spawnix (pid_t * pid, const char *file, > if (__glibc_unlikely (stack == MAP_FAILED)) > return errno; > > - /* Disable asynchronous cancellation. */ > int state; > - __libc_ptf_call (__pthread_setcancelstate, > - (PTHREAD_CANCEL_DISABLE, &state), 0); > + if ((xflags & SPAWN_XFLAGS_ENABLE_CANCEL) == 0) > + { > + /* Disable asynchronous cancellation. */ > + __libc_ptf_call (__pthread_setcancelstate, > + (PTHREAD_CANCEL_DISABLE, &state), 0); > + __libc_signal_block_all (&args.oldmask); > + } In fact I think it would be safer to just enable glibc internal signals instead of current process mask. I changed it locally to: if (xflags & SPAWN_XFLAGS_ENABLE_CANCEL) __libc_signal_block_app (&args.oldmask); else { /* Disable asynchronous cancellation. */ __libc_ptf_call (__pthread_setcancelstate, (PTHREAD_CANCEL_DISABLE, &state), 0); __libc_signal_block_all (&args.oldmask); } > > /* Child must set args.err to something non-negative - we rely on > the parent and child sharing VM. */ > @@ -347,8 +351,6 @@ __spawnix (pid_t * pid, const char *file, > args.envp = envp; > args.xflags = xflags; > > - __libc_signal_block_all (&args.oldmask); > - > /* The clone flags used will create a new child that will run in the same > memory space (CLONE_VM) and the execution of calling thread will be > suspend until the child calls execve or _exit. > @@ -389,9 +391,11 @@ __spawnix (pid_t * pid, const char *file, > if ((ec == 0) && (pid != NULL)) > *pid = new_pid; > > - __libc_signal_restore_set (&args.oldmask); > - > - __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0); > + if ((xflags & SPAWN_XFLAGS_ENABLE_CANCEL) == 0) > + { > + __libc_signal_restore_set (&args.oldmask); > + __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0); > + } As as before I changed locally to: __libc_signal_restore_set (&args.oldmask); if ((xflags & SPAWN_XFLAGS_ENABLE_CANCEL) == 0) __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);