From patchwork Fri Nov 14 02:17:52 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alexandre Oliva X-Patchwork-Id: 3729 Received: (qmail 31835 invoked by alias); 14 Nov 2014 02:18:25 -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 31820 invoked by uid 89); 14 Nov 2014 02:18:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.5 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com From: Alexandre Oliva To: Roland McGrath Cc: libc-alpha@sourceware.org Subject: Re: BZ#15819: introduce internal function to ease poll retry with timeout References: <20141113221831.D15A62C3B18@topped-with-meat.com> Date: Fri, 14 Nov 2014 00:17:52 -0200 In-Reply-To: <20141113221831.D15A62C3B18@topped-with-meat.com> (Roland McGrath's message of "Thu, 13 Nov 2014 14:18:31 -0800 (PST)") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 On Nov 13, 2014, Roland McGrath wrote: > prefer new local headers *check* > The __ treatment is never necessary for local-scope names in internal > source files (including headers). It only matters for global names, or > uses in installed headers. I figured names outside the reserved namespace could be used in our tests, and then they'd interfere. I guess the reasoning is that, since we control the tests too, we can just fix any such conflicts in the tests, rigth? > Drop the unnecessary prefixes to make the code easier to read. *check* This revised patch fixes both of these issues, and it poisons __poll and poll at the end of include/poll-noeintr.h. Retested on x86_64-linux-gnu. Ok to install? for ChangeLog [BZ #15819] * NEWS: Update. * nis/nis_callback.c (internal_nis_do_callback): Call __poll_noeintr, drop TEMP_FAILURE_RETRY. * nscd/nscd_helper.c (wait_on_socket): Call __poll_noeintr, drop loop and gettimeofday timeout recomputation. * sunrpc/clnt_udp.c (clntudp_call): Call __poll_noeintr. * sunrpc/clnt_unix.c (readunix): Likewise. * sunrpc/rtime.c (rtime): Likewise. * sunrpc/svc_run.c (svc_run): Likewise. * sunrpc/svc_tcp.c (readtcp): Likewise. * sunrpc/svc_unix.c (readunix): Likewise. * include/poll-noeintr.h: New header, included instead of sys/poll.h in all of the above. --- NEWS | 8 ++-- include/poll-noeintr.h | 86 ++++++++++++++++++++++++++++++++++++++++++++++++ nis/nis_callback.c | 5 +-- nscd/nscd_helper.c | 26 +-------------- sunrpc/clnt_udp.c | 7 +--- sunrpc/clnt_unix.c | 24 +++++-------- sunrpc/rtime.c | 6 +-- sunrpc/svc_run.c | 6 +-- sunrpc/svc_tcp.c | 31 +++++++---------- sunrpc/svc_unix.c | 31 +++++++---------- 10 files changed, 133 insertions(+), 97 deletions(-) create mode 100644 include/poll-noeintr.h diff --git a/NEWS b/NEWS index 9ed697c..af86a93 100644 --- a/NEWS +++ b/NEWS @@ -9,10 +9,10 @@ Version 2.21 * The following bugs are resolved with this release: - 6652, 12926, 14132, 14138, 14171, 14498, 15215, 15884, 17266, 17344, - 17363, 17370, 17371, 17411, 17460, 17475, 17485, 17501, 17506, 17508, - 17522, 17555, 17570, 17571, 17572, 17573, 17574, 17582, 17583, 17584, - 17585, 17589. + 6652, 12926, 14132, 14138, 14171, 14498, 15215, 15819, 15884, 17266, + 17344, 17363, 17370, 17371, 17411, 17460, 17475, 17485, 17501, 17506, + 17508, 17522, 17555, 17570, 17571, 17572, 17573, 17574, 17582, 17583, + 17584, 17585, 17589. * New locales: tu_IN, bh_IN. diff --git a/include/poll-noeintr.h b/include/poll-noeintr.h new file mode 100644 index 0000000..d94deb4 --- /dev/null +++ b/include/poll-noeintr.h @@ -0,0 +1,86 @@ +/* Wrapper for __poll that restarts on EINTR + Copyright (C) 2014 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 + . */ + +#ifndef _POLL_EINTR_H +#define _POLL_EINTR_H + +#include +#include +#include + +static inline int +__poll_noeintr (struct pollfd *fds, unsigned long int nfds, int timeout) +{ + int ret; + + retry_poll: + ret = __poll (fds, nfds, timeout); + + if (ret == -1 && __glibc_unlikely (errno == EINTR)) + { + /* Handle the case where the poll() call is interrupted by a + signal. We cannot just use TEMP_FAILURE_RETRY since it might + lead to infinite loops. We can't tell how long poll has + already waited, and we can't assume the existence of a + higher-precision clock, but that's ok-ish: the timeout is a + lower bound, we just have to make sure we don't wait + indefinitely. */ + struct timespec tscur, tsend; + /* Remember which clock to use. */ + static clockid_t xclk = CLOCK_MONOTONIC; + clockid_t clk = xclk; + + try_another_clock: + if (__glibc_unlikely (__clock_gettime (clk, &tscur) == -1)) + { + if (errno == EINVAL && clk == CLOCK_MONOTONIC) + { + xclk = clk = CLOCK_REALTIME; + goto try_another_clock; + } + + /* At least CLOCK_REALTIME should always be supported, but + if clock_gettime fails for any other reason, the best we + can do is to retry with a slightly lower timeout, until + we complete without interruption. */ + timeout--; + goto retry_poll; + } + + tsend.tv_sec = tscur.tv_sec + timeout / 1000; + tsend.tv_nsec = tscur.tv_nsec + timeout % 1000 * 1000000L + 500000; + + while (1) + { + ret = __poll (fds, nfds, + (tsend.tv_sec - tscur.tv_sec) * 1000 + + (tsend.tv_nsec - tscur.tv_nsec) / 1000000); + if (ret != -1 || errno != EINTR) + break; + + (void) __clock_gettime (clk, &tscur); + } + } + + return ret; +} + +#pragma poison __poll +#pragma poison poll + +#endif /* _POLL_EINTR_H */ diff --git a/nis/nis_callback.c b/nis/nis_callback.c index e352733..b7492fb 100644 --- a/nis/nis_callback.c +++ b/nis/nis_callback.c @@ -26,7 +26,7 @@ #include #include #include -#include +#include #include #include #include @@ -215,8 +215,7 @@ internal_nis_do_callback (struct dir_binding *bptr, netobj *cookie, my_pollfd[i].revents = 0; } - switch (i = TEMP_FAILURE_RETRY (__poll (my_pollfd, svc_max_pollfd, - 25*1000))) + switch (i = __poll_noeintr (my_pollfd, svc_max_pollfd, 25*1000)) { case -1: return NIS_CBERROR; diff --git a/nscd/nscd_helper.c b/nscd/nscd_helper.c index ee3b67f..301be42 100644 --- a/nscd/nscd_helper.c +++ b/nscd/nscd_helper.c @@ -27,7 +27,7 @@ #include #include #include -#include +#include #include #include #include @@ -53,29 +53,7 @@ wait_on_socket (int sock, long int usectmo) struct pollfd fds[1]; fds[0].fd = sock; fds[0].events = POLLIN | POLLERR | POLLHUP; - int n = __poll (fds, 1, usectmo); - if (n == -1 && __builtin_expect (errno == EINTR, 0)) - { - /* Handle the case where the poll() call is interrupted by a - signal. We cannot just use TEMP_FAILURE_RETRY since it might - lead to infinite loops. */ - struct timeval now; - (void) __gettimeofday (&now, NULL); - long int end = now.tv_sec * 1000 + usectmo + (now.tv_usec + 500) / 1000; - long int timeout = usectmo; - while (1) - { - n = __poll (fds, 1, timeout); - if (n != -1 || errno != EINTR) - break; - - /* Recompute the timeout time. */ - (void) __gettimeofday (&now, NULL); - timeout = end - (now.tv_sec * 1000 + (now.tv_usec + 500) / 1000); - } - } - - return n; + return __poll_noeintr (fds, 1, usectmo); } diff --git a/sunrpc/clnt_udp.c b/sunrpc/clnt_udp.c index 6ffa5f2..38b11e1 100644 --- a/sunrpc/clnt_udp.c +++ b/sunrpc/clnt_udp.c @@ -37,7 +37,7 @@ #include #include #include -#include +#include #include #include #include @@ -378,9 +378,8 @@ send_again: anyup = 0; for (;;) { - switch (__poll (&fd, 1, milliseconds)) + switch (__poll_noeintr (&fd, 1, milliseconds)) { - case 0: if (anyup == 0) { @@ -407,8 +406,6 @@ send_again: * updated. */ case -1: - if (errno == EINTR) - continue; cu->cu_error.re_errno = errno; return (cu->cu_error.re_status = RPC_CANTRECV); } diff --git a/sunrpc/clnt_unix.c b/sunrpc/clnt_unix.c index 32d88b9..40eb269 100644 --- a/sunrpc/clnt_unix.c +++ b/sunrpc/clnt_unix.c @@ -51,7 +51,7 @@ #include #include #include -#include +#include #include #include #include @@ -551,22 +551,16 @@ readunix (char *ctptr, char *buf, int len) fd.fd = ct->ct_sock; fd.events = POLLIN; - while (TRUE) + switch (__poll_noeintr (&fd, 1, milliseconds)) { - switch (__poll (&fd, 1, milliseconds)) - { - case 0: - ct->ct_error.re_status = RPC_TIMEDOUT; - return -1; + case 0: + ct->ct_error.re_status = RPC_TIMEDOUT; + return -1; - case -1: - if (errno == EINTR) - continue; - ct->ct_error.re_status = RPC_CANTRECV; - ct->ct_error.re_errno = errno; - return -1; - } - break; + case -1: + ct->ct_error.re_status = RPC_CANTRECV; + ct->ct_error.re_errno = errno; + return -1; } switch (len = __msgread (ct->ct_sock, buf, len)) { diff --git a/sunrpc/rtime.c b/sunrpc/rtime.c index d224624..862493b 100644 --- a/sunrpc/rtime.c +++ b/sunrpc/rtime.c @@ -43,7 +43,7 @@ #include #include #include -#include +#include #include #include #include @@ -102,9 +102,7 @@ rtime (struct sockaddr_in *addrp, struct rpc_timeval *timep, milliseconds = (timeout->tv_sec * 1000) + (timeout->tv_usec / 1000); fd.fd = s; fd.events = POLLIN; - do - res = __poll (&fd, 1, milliseconds); - while (res < 0 && errno == EINTR); + res = __poll_noeintr (&fd, 1, milliseconds); if (res <= 0) { if (res == 0) diff --git a/sunrpc/svc_run.c b/sunrpc/svc_run.c index 90dfc94..1f54d84 100644 --- a/sunrpc/svc_run.c +++ b/sunrpc/svc_run.c @@ -34,7 +34,7 @@ #include #include #include -#include +#include #include /* This function can be used as a signal handler to terminate the @@ -83,11 +83,9 @@ svc_run (void) my_pollfd[i].revents = 0; } - switch (i = __poll (my_pollfd, max_pollfd, -1)) + switch (i = __poll_noeintr (my_pollfd, max_pollfd, -1)) { case -1: - if (errno == EINTR) - continue; perror (_("svc_run: - poll failed")); break; case 0: diff --git a/sunrpc/svc_tcp.c b/sunrpc/svc_tcp.c index 913f05f..2ab98dc 100644 --- a/sunrpc/svc_tcp.c +++ b/sunrpc/svc_tcp.c @@ -58,7 +58,7 @@ #include #include #include -#include +#include #include #include @@ -317,26 +317,19 @@ readtcp (char *xprtptr, char *buf, int len) int milliseconds = 35 * 1000; struct pollfd pollfd; - do + pollfd.fd = sock; + pollfd.events = POLLIN; + switch (__poll_noeintr (&pollfd, 1, milliseconds)) { - pollfd.fd = sock; - pollfd.events = POLLIN; - switch (__poll (&pollfd, 1, milliseconds)) - { - case -1: - if (errno == EINTR) - continue; - /*FALLTHROUGH*/ - case 0: - goto fatal_err; - default: - if ((pollfd.revents & POLLERR) || (pollfd.revents & POLLHUP) - || (pollfd.revents & POLLNVAL)) - goto fatal_err; - break; - } + case -1: + case 0: + goto fatal_err; + default: + if ((pollfd.revents & POLLERR) || (pollfd.revents & POLLHUP) + || (pollfd.revents & POLLNVAL)) + goto fatal_err; + break; } - while ((pollfd.revents & POLLIN) == 0); if ((len = __read (sock, buf, len)) > 0) return len; diff --git a/sunrpc/svc_unix.c b/sunrpc/svc_unix.c index 963276b2..cbc9891 100644 --- a/sunrpc/svc_unix.c +++ b/sunrpc/svc_unix.c @@ -59,7 +59,7 @@ #include #include #include -#include +#include #include #include #include @@ -419,26 +419,19 @@ readunix (char *xprtptr, char *buf, int len) int milliseconds = 35 * 1000; struct pollfd pollfd; - do + pollfd.fd = sock; + pollfd.events = POLLIN; + switch (__poll_noeintr (&pollfd, 1, milliseconds)) { - pollfd.fd = sock; - pollfd.events = POLLIN; - switch (__poll (&pollfd, 1, milliseconds)) - { - case -1: - if (errno == EINTR) - continue; - /*FALLTHROUGH*/ - case 0: - goto fatal_err; - default: - if ((pollfd.revents & POLLERR) || (pollfd.revents & POLLHUP) - || (pollfd.revents & POLLNVAL)) - goto fatal_err; - break; - } + case -1: + case 0: + goto fatal_err; + default: + if ((pollfd.revents & POLLERR) || (pollfd.revents & POLLHUP) + || (pollfd.revents & POLLNVAL)) + goto fatal_err; + break; } - while ((pollfd.revents & POLLIN) == 0); if ((len = __msgread (sock, buf, len)) > 0) return len;