From patchwork Mon Nov 9 14:07:25 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 56221 Delivered-To: patch@linaro.org Received: by 10.112.155.196 with SMTP id vy4csp199669lbb; Mon, 9 Nov 2015 06:07:44 -0800 (PST) X-Received: by 10.66.153.166 with SMTP id vh6mr41000935pab.83.1447078064069; Mon, 09 Nov 2015 06:07:44 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id am2si22590105pad.210.2015.11.09.06.07.43 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 09 Nov 2015 06:07:44 -0800 (PST) Received-SPF: pass (google.com: domain of libc-alpha-return-64831-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; spf=pass (google.com: domain of libc-alpha-return-64831-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=libc-alpha-return-64831-patch=linaro.org@sourceware.org; dkim=pass header.i=@sourceware.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:references:cc:from:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; q=dns; s=default; b=RIe1IQ06wZ8gIRq1 L12gMQci79igcAcxlCvBT+tSenPqsuS3+XErjZDhHPzNqXS5Ld6+cP9NWO4MsFgb CaY8MA1YvqgcxHifbnnpNLOMtqH6TYfq3agzqmeWZ+6GsulWl6P+vYNHv2geX+Dw yH2ZRjS9eYnWV+eJVOubD3U6sGw= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:references:cc:from:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; s=default; bh=VfiwZsvtkolue1cQT4X+zk WZyQg=; b=oyRvo0OM/mwdJkLESBVy2VtezoHB1x2lGoKFqcY4aGMdzD7SibZf1p qr65SIKimzXRXUusbrxIXNNVsZdpKlXm2HhZu+xYqiuwzL7IcR6YrJIe27emX1Sz JXua3LO7dU6zumixt2PXi7MVz0g10GhRESOjAZJ2xM6/WiQOQiao4= Received: (qmail 55887 invoked by alias); 9 Nov 2015 14:07:35 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 55877 invoked by uid 89); 9 Nov 2015 14:07:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-yk0-f180.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-type :content-transfer-encoding; bh=O9yMap8ukRGQaZJhitxgqJgGRTAQenJAsFp6DBmU+LQ=; b=jE5BBAKe+vGrZzVKIb/ypU5lVLVMWeRIGXYsonZ8XDZiqr8fyX8U84et1kC9dL1/ut NmqqM+Es4zpNVsG/7gw4GoCkD5NmIIxMZwc2vIGeA57kMi8j5e2lpngcmJdNp4yTDO1z qcbDpQCxkOlgefTQysmCPKe2MF+xZXRGekY0zkduZzgfGa/X52lBVTEMZO1J+TFg3XuU Bdu3QUfeb658F7FKmPlll8/vKedGkWzBygEeI3NkqO5qwqCLZmJq8ooMEF1sc9XoVDnP exG6ziTzvdDwn5ha2W0357+79jiMPefyDJWlKfW87q6XH3KD6TvHUY5l7kFfD83zS/Qj SYtA== X-Gm-Message-State: ALoCoQl5y0Vv4f2Qtw0CHEX6S2Dw2ZqSF1hhffQA/W6S0pAMJfrofoS0jFWbpUfdLD49gunl94JL X-Received: by 10.129.83.4 with SMTP id h4mr22947290ywb.191.1447078048931; Mon, 09 Nov 2015 06:07:28 -0800 (PST) Subject: Re: [PATCH] Remove signal handling for nanosleep (bug 16364) To: Florian Weimer References: <1447023171-31542-1-git-send-email-adhemerval.zanella@linaro.com> <56405109.9070404@redhat.com> <56408F14.1040600@linaro.org> <56409561.7050707@redhat.com> Cc: libc-alpha@sourceware.org From: Adhemerval Zanella Message-ID: <5640A89D.80804@linaro.org> Date: Mon, 9 Nov 2015 12:07:25 -0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <56409561.7050707@redhat.com> On 09-11-2015 10:45, Florian Weimer wrote: > On 11/09/2015 01:18 PM, Adhemerval Zanella wrote: >> >> >> On 09-11-2015 05:53, Florian Weimer wrote: >>> On 11/08/2015 11:52 PM, Adhemerval Zanella wrote: >>>> Linux 2.6.32 and forward do not show the issue regarding SysV SIGCHLD >>>> vs. SIG_IGN for nanosleep which make it feasible to use it for sleep >>>> implementation without requiring any hacking to handle the spurious >>>> wake up. This patch simplifies the sleep code to call nanosleep >>>> directly. >>>> >>>> Checked on x86_64, ppc64le, and aarch64. >>> >>> Do you know the kernel commit which fixed this? >> >> I tried to track down the specific commit that fixed it, but I was unable >> to pin it down. uClibc developers also seemed to do same [1], but also >> they could not find out the exact commit (which I think might be the reason >> they are using glibc strategy still). musl uses plain nanosleep as the >> patch. >> >> [1] http://git.uclibc.org/uClibc/tree/libc/unistd/sleep.c > > Could you write a glibc test case for this? I don't feel comfortable > with removing the workaround without a test, and I can't find an > existing one. What about: [BZ #16364] * sysdeps/unix/sysv/linux/sleep.c (__sleep): Simplify code to use nanosleep without requiring to handle spurious wakeups. * posix/tst-nanosleep-signal.c: New file. * posix/Makefile (test): Add tst-nanosleep-signal. diff --git a/posix/Makefile b/posix/Makefile index aeb9890..cec06a6 100644 --- a/posix/Makefile +++ b/posix/Makefile @@ -74,8 +74,8 @@ tests := tstgetopt testfnm runtests runptests \ bug-regex21 bug-regex22 bug-regex23 bug-regex24 \ bug-regex25 bug-regex26 bug-regex27 bug-regex28 \ bug-regex29 bug-regex30 bug-regex31 bug-regex32 \ - bug-regex33 tst-nice tst-nanosleep tst-regex2 \ - transbug tst-rxspencer tst-pcre tst-boost \ + bug-regex33 tst-nice tst-nanosleep tst-nanosleep-signal \ + tst-regex2 transbug tst-rxspencer tst-pcre tst-boost \ bug-ga1 tst-vfork1 tst-vfork2 tst-vfork3 tst-waitid \ tst-getaddrinfo2 bug-glob1 bug-glob2 bug-glob3 tst-sysconf \ tst-execvp1 tst-execvp2 tst-execlp1 tst-execlp2 \ diff --git a/posix/tst-nanosleep-signal.c b/posix/tst-nanosleep-signal.c new file mode 100644 index 0000000..e6abab4 --- /dev/null +++ b/posix/tst-nanosleep-signal.c @@ -0,0 +1,105 @@ +/* Check if nanosleep handles correctly SIGCHLD. + Copyright (C) 2015 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + 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, see + . */ + +#include +#include +#include +#include +#include +#include + +static int errors = 0; + +static void +dummy (int sig) +{ +} + +/* Fork and terminate the child in 0.25s while parent waits on a nanosleep + for 0.5s. */ +static int +check_nanosleep_base (void) +{ + int ret; + pid_t f = fork (); + if (f == 0) + { + // child + struct timespec tv = { .tv_sec = 0, .tv_nsec = 250000000UL }; + nanosleep (&tv, &tv); + exit (0); + } + else + { + // parent + struct timespec tv = { .tv_sec = 0, .tv_nsec = 500000000UL }; + ret = nanosleep (&tv, &tv); + } + return ret; +} + +void +check_nanosleep_sigdfl(void) +{ + /* For SIG_DFL nanosleep should not be interrupted by SIGCHLD. */ + signal(SIGCHLD, SIG_DFL); + if (check_nanosleep_base ()) + { + puts ("error: nanosleep was interrupted with SIG_DFL"); + errors = 1; + } +} + +void +check_nanosleep_dummy(void) +{ + /* With a dummy handler nanosleep should be interrupted. */ + signal(SIGCHLD, dummy); + int ret = check_nanosleep_base (); + if (ret == 0) + { + puts ("error: nanosleep was not interrupted with dummy handler"); + errors = 1; + } +} + +void +check_nanosleep_sigign(void) +{ + /* For SIG_IGN nanosleep should not be interrupted by SIGCHLD. */ + signal(SIGCHLD, SIG_IGN); + if (check_nanosleep_base ()) + { + puts ("error: nanosleep was interrupted with SIG_IGN"); + errors = 1; + } +} + + +static int +do_test (void) +{ + check_nanosleep_sigdfl (); + check_nanosleep_dummy (); + check_nanosleep_sigign (); + + return errors; +} + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" diff --git a/sysdeps/unix/sysv/linux/sleep.c b/sysdeps/unix/sysv/linux/sleep.c index 2a06d5a..3885a34 100644 --- a/sysdeps/unix/sysv/linux/sleep.c +++ b/sysdeps/unix/sysv/linux/sleep.c @@ -17,133 +17,16 @@ License along with the GNU C Library; if not, see . */ -#include #include -#include -#include /* For the real memset prototype. */ #include -#include -#include +#include - -#if 0 -static void -cl (void *arg) -{ - (void) __sigprocmask (SIG_SETMASK, arg, (sigset_t *) NULL); -} -#endif - - -/* We are going to use the `nanosleep' syscall of the kernel. But the - kernel does not implement the stupid SysV SIGCHLD vs. SIG_IGN - behaviour for this syscall. Therefore we have to emulate it here. */ unsigned int __sleep (unsigned int seconds) { - const unsigned int max - = (unsigned int) (((unsigned long int) (~((time_t) 0))) >> 1); - struct timespec ts; - sigset_t set, oset; - unsigned int result; - - /* This is not necessary but some buggy programs depend on this. */ - if (__glibc_unlikely (seconds == 0)) - { -#ifdef CANCELLATION_P - CANCELLATION_P (THREAD_SELF); -#endif - return 0; - } - - ts.tv_sec = 0; - ts.tv_nsec = 0; - again: - if (sizeof (ts.tv_sec) <= sizeof (seconds)) - { - /* Since SECONDS is unsigned assigning the value to .tv_sec can - overflow it. In this case we have to wait in steps. */ - ts.tv_sec += MIN (seconds, max); - seconds -= (unsigned int) ts.tv_sec; - } - else - { - ts.tv_sec = (time_t) seconds; - seconds = 0; - } - - /* Linux will wake up the system call, nanosleep, when SIGCHLD - arrives even if SIGCHLD is ignored. We have to deal with it - in libc. We block SIGCHLD first. */ - __sigemptyset (&set); - __sigaddset (&set, SIGCHLD); - if (__sigprocmask (SIG_BLOCK, &set, &oset)) - return -1; - - /* If SIGCHLD is already blocked, we don't have to do anything. */ - if (!__sigismember (&oset, SIGCHLD)) - { - int saved_errno; - struct sigaction oact; - - __sigemptyset (&set); - __sigaddset (&set, SIGCHLD); - - /* We get the signal handler for SIGCHLD. */ - if (__sigaction (SIGCHLD, (struct sigaction *) NULL, &oact) < 0) - { - saved_errno = errno; - /* Restore the original signal mask. */ - (void) __sigprocmask (SIG_SETMASK, &oset, (sigset_t *) NULL); - __set_errno (saved_errno); - return -1; - } - - /* Note the sleep() is a cancellation point. But since we call - nanosleep() which itself is a cancellation point we do not - have to do anything here. */ - if (oact.sa_handler == SIG_IGN) - { - //__libc_cleanup_push (cl, &oset); - - /* We should leave SIGCHLD blocked. */ - while (1) - { - result = __nanosleep (&ts, &ts); - - if (result != 0 || seconds == 0) - break; - - if (sizeof (ts.tv_sec) <= sizeof (seconds)) - { - ts.tv_sec = MIN (seconds, max); - seconds -= (unsigned int) ts.tv_nsec; - } - } - - //__libc_cleanup_pop (0); - - saved_errno = errno; - /* Restore the original signal mask. */ - (void) __sigprocmask (SIG_SETMASK, &oset, (sigset_t *) NULL); - __set_errno (saved_errno); - - goto out; - } - - /* We should unblock SIGCHLD. Restore the original signal mask. */ - (void) __sigprocmask (SIG_SETMASK, &oset, (sigset_t *) NULL); - } - - result = __nanosleep (&ts, &ts); - if (result == 0 && seconds != 0) - goto again; - - out: - if (result != 0) - /* Round remaining time. */ - result = seconds + (unsigned int) ts.tv_sec + (ts.tv_nsec >= 500000000L); - - return result; + struct timespec ts = { .tv_sec = seconds, .tv_nsec = 0 }; + if (__nanosleep (&ts, &ts)) + return ts.tv_sec; + return 0; } weak_alias (__sleep, sleep)