On Tue, Dec 12 2017, Jeremie Courreges-Anglas <[email protected]> wrote:
> On Sun, Dec 10 2017, Scott Cheloha <[email protected]> wrote:
>> Hi,
>>
>> These timeouts in sunrpc need to be based on the monotonic
>> clock to avoid a race with adjtime(2), settimeofday(2), etc.
>>
>> There are obvious possible improvements here and elsewhere
>> in sunrpc. Here especially the time-related variable names
>> could be made more descriptive, the various BSD time macros
>> could be more effectively employed, and ppoll(2) can (now) be
>> leveraged to ignore EINTR and simplify those loops. But that
>> all belongs in a separate diff.
>>
>> Likewise, there are dead timevals elsewhere in RPC
>> that I can remove in a separate diff.
>>
>> Then again, this is ancient upstream code. Does cleanup here
>> put too much burden on the project? Like, I imagine that the
>> sort of cleanup I'm seeing here could make merging changes
>> from elsewhere painful.
>
> Well, afaik upstream can be considered dead, and looking at cvs log
> people have implemented a bunch of changes and improvements to these
> files, so I think that further improvements are welcome.
>
>> I have also included here my sys/time.h diff from like two
>> seconds ago, which enables the use of TIMEVAL_TO_TIMESPEC
>> in the body of the if statement in clnt_udp.c. Its use makes
>> the diff simpler.
>
> (The sys/time.h change has since been committed.)
>
>> This is probably not adequately tested, though it compiles
>> and I'm not seeing any issues in my (small) NFS setup.
>>
>> Thoughts and feedback?
>
> I'll schedule some time for a review tomorrow, more eyes would be
> welcome.
Looks fine to me and works fine here. Here's a refreshed diff to be
applied from libc/.
Index: rpc/clnt_tcp.c
===================================================================
RCS file: /d/cvs/src/lib/libc/rpc/clnt_tcp.c,v
retrieving revision 1.29
diff -u -p -r1.29 clnt_tcp.c
--- rpc/clnt_tcp.c 1 Nov 2015 03:45:29 -0000 1.29
+++ rpc/clnt_tcp.c 13 Dec 2017 15:39:07 -0000
@@ -385,25 +385,26 @@ static int
readtcp(struct ct_data *ct, caddr_t buf, int len)
{
struct pollfd pfd[1];
- struct timeval start, after, duration, tmp;
- int delta, r, save_errno;
+ struct timespec start, after, duration, tmp, delta, wait;
+ int r, save_errno;
if (len == 0)
return (0);
pfd[0].fd = ct->ct_sock;
pfd[0].events = POLLIN;
- delta = ct->ct_wait.tv_sec * 1000 + ct->ct_wait.tv_usec / 1000;
- gettimeofday(&start, NULL);
+ TIMEVAL_TO_TIMESPEC(&ct->ct_wait, &wait);
+ delta = wait;
+ clock_gettime(CLOCK_MONOTONIC, &start);
for (;;) {
- r = poll(pfd, 1, delta);
+ r = ppoll(pfd, 1, &delta, NULL);
save_errno = errno;
- gettimeofday(&after, NULL);
- timersub(&start, &after, &duration);
- timersub(&ct->ct_wait, &duration, &tmp);
- delta = tmp.tv_sec * 1000 + tmp.tv_usec / 1000;
- if (delta <= 0)
+ clock_gettime(CLOCK_MONOTONIC, &after);
+ timespecsub(&start, &after, &duration);
+ timespecsub(&wait, &duration, &tmp);
+ delta = tmp;
+ if (delta.tv_sec < 0 || !timespecisset(&delta))
r = 0;
switch (r) {
Index: rpc/clnt_udp.c
===================================================================
RCS file: /d/cvs/src/lib/libc/rpc/clnt_udp.c,v
retrieving revision 1.32
diff -u -p -r1.32 clnt_udp.c
--- rpc/clnt_udp.c 1 Nov 2015 03:45:29 -0000 1.32
+++ rpc/clnt_udp.c 13 Dec 2017 15:39:07 -0000
@@ -216,19 +216,20 @@ clntudp_call(CLIENT *cl, /* client handl
struct sockaddr_in from;
struct rpc_msg reply_msg;
XDR reply_xdrs;
- struct timeval time_waited, start, after, tmp1, tmp2;
+ struct timespec time_waited, start, after, tmp1, tmp2, wait;
bool_t ok;
int nrefreshes = 2; /* number of times to refresh cred */
- struct timeval timeout;
+ struct timespec timeout;
if (cu->cu_total.tv_usec == -1)
- timeout = utimeout; /* use supplied timeout */
+ TIMEVAL_TO_TIMESPEC(&utimeout, &timeout); /* use supplied
timeout */
else
- timeout = cu->cu_total; /* use default timeout */
+ TIMEVAL_TO_TIMESPEC(&cu->cu_total, &timeout); /* use default
timeout */
pfd[0].fd = cu->cu_sock;
pfd[0].events = POLLIN;
- timerclear(&time_waited);
+ timespecclear(&time_waited);
+ TIMEVAL_TO_TIMESPEC(&cu->cu_wait, &wait);
call_again:
xdrs = &(cu->cu_outxdrs);
xdrs->x_op = XDR_ENCODE;
@@ -254,7 +255,7 @@ send_again:
/*
* Hack to provide rpc-based message passing
*/
- if (!timerisset(&timeout))
+ if (!timespecisset(&timeout))
return (cu->cu_error.re_status = RPC_TIMEDOUT);
/*
@@ -266,14 +267,13 @@ send_again:
reply_msg.acpted_rply.ar_results.where = resultsp;
reply_msg.acpted_rply.ar_results.proc = xresults;
- gettimeofday(&start, NULL);
+ clock_gettime(CLOCK_MONOTONIC, &start);
for (;;) {
- switch (poll(pfd, 1,
- cu->cu_wait.tv_sec * 1000 + cu->cu_wait.tv_usec / 1000)) {
+ switch (ppoll(pfd, 1, &wait, NULL)) {
case 0:
- timeradd(&time_waited, &cu->cu_wait, &tmp1);
+ timespecadd(&time_waited, &wait, &tmp1);
time_waited = tmp1;
- if (timercmp(&time_waited, &timeout, <))
+ if (timespeccmp(&time_waited, &timeout, <))
goto send_again;
return (cu->cu_error.re_status = RPC_TIMEDOUT);
case 1:
@@ -286,11 +286,11 @@ send_again:
/* FALLTHROUGH */
case -1:
if (errno == EINTR) {
- gettimeofday(&after, NULL);
- timersub(&after, &start, &tmp1);
- timeradd(&time_waited, &tmp1, &tmp2);
+ clock_gettime(CLOCK_MONOTONIC, &after);
+ timespecsub(&after, &start, &tmp1);
+ timespecadd(&time_waited, &tmp1, &tmp2);
time_waited = tmp2;
- if (timercmp(&time_waited, &timeout, <))
+ if (timespeccmp(&time_waited, &timeout, <))
continue;
return (cu->cu_error.re_status = RPC_TIMEDOUT);
}
Index: rpc/svc_tcp.c
===================================================================
RCS file: /d/cvs/src/lib/libc/rpc/svc_tcp.c,v
retrieving revision 1.37
diff -u -p -r1.37 svc_tcp.c
--- rpc/svc_tcp.c 1 Nov 2015 03:45:29 -0000 1.37
+++ rpc/svc_tcp.c 13 Dec 2017 15:39:07 -0000
@@ -321,7 +321,7 @@ svctcp_destroy(SVCXPRT *xprt)
* All read operations timeout after 35 seconds.
* A timeout is fatal for the connection.
*/
-static struct timeval wait_per_try = { 35, 0 };
+static struct timespec wait_per_try = { 35, 0 };
/*
* reads data from the tcp conection.
@@ -332,31 +332,30 @@ static int
readtcp(SVCXPRT *xprt, caddr_t buf, int len)
{
int sock = xprt->xp_sock;
- int delta, nready;
- struct timeval start;
- struct timeval tmp1, tmp2;
+ int nready;
+ struct timespec start, delta, tmp1, tmp2;
struct pollfd pfd[1];
/*
* All read operations timeout after 35 seconds.
* A timeout is fatal for the connection.
*/
- delta = wait_per_try.tv_sec * 1000;
- gettimeofday(&start, NULL);
+ delta = wait_per_try;
+ clock_gettime(CLOCK_MONOTONIC, &start);
pfd[0].fd = sock;
pfd[0].events = POLLIN;
do {
- nready = poll(pfd, 1, delta);
+ nready = ppoll(pfd, 1, &delta, NULL);
switch (nready) {
case -1:
if (errno != EINTR)
goto fatal_err;
- gettimeofday(&tmp1, NULL);
- timersub(&tmp1, &start, &tmp2);
- timersub(&wait_per_try, &tmp2, &tmp1);
- if (tmp1.tv_sec < 0 || !timerisset(&tmp1))
+ clock_gettime(CLOCK_MONOTONIC, &tmp1);
+ timespecsub(&tmp1, &start, &tmp2);
+ timespecsub(&wait_per_try, &tmp2, &tmp1);
+ if (tmp1.tv_sec < 0 || !timespecisset(&tmp1))
goto fatal_err;
- delta = tmp1.tv_sec * 1000 + tmp1.tv_usec / 1000;
+ delta = tmp1;
continue;
case 0:
goto fatal_err;
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
