Re: [PATCH] sunrpc/clnt_udp.c: always respect timeout(s)

[Florian Weimer <fweimer at redhat dot com>]

On 06/03/2016 11:26 AM, Pavel Raiskup wrote:

> Previously, when the port we were listening on was really busy,
> clntudp_call () ended up processing the UDP queue without properly
> defined time-limit (clntudp_call could keep processing forever
> without returning back to caller).
>
> The fix incorporates using of clock_gettime () to measure the real
> timeout, instead of simply counting the number of poll()'s.  We
> should not face some huge slowdown because we call clock_gettime
> only twice in most cases *and* never in busy loop.

Thanks.

I field a bug to track this:

  https://sourceware.org/bugzilla/show_bug.cgi?id=20257

The commit message and ChangeLog need to reference this (as “[BZ #2057]”).

> diff --git a/sunrpc/clnt_udp.c b/sunrpc/clnt_udp.c
> index 6ffa5f2..2cab4c2 100644
> --- a/sunrpc/clnt_udp.c
> +++ b/sunrpc/clnt_udp.c
> @@ -1,6 +1,8 @@
>  /*
>   * clnt_udp.c, Implements a UDP/IP based, client side RPC.
>   *
> + * Copyright (c) 2016, Free Software Foundation, Inc.
> + *
>   * Copyright (c) 2010, Oracle America, Inc.

Please do not change the copyright notice in this way.  Either add the 
full LGPL header, or leave it unchanged.

> +static bool_t
> +clntudp_call_timeouted (const struct timespec *start, int timeout, int *resend_timeout)
> +{
> +  struct timespec now;
> +  long remains, waited;
> +
> +  __clock_gettime (CLOCK_MONOTONIC, &now);

I was mistaken about the availability of CLOCK_MONOTONIC.  It seems to 
be missing on Hurd.  CLOCK_REALTIME is probably missing there as well, 
so we need fallback to gettimeofday.

We have similar code in resolv/res_send.c.  It needs to use 
CLOCK_MONOTONIC if available, but currently does not.  So this should 
really move to generic libc functionality (for separate testing, too).

I can take care of this if you want.  Unless you want to learn a bit 
about glibc symbol management.

> +  /* count everything in milliseconds */
> +  waited = (now.tv_sec  - start->tv_sec  - 1)          * 1000
> +         + (now.tv_nsec - start->tv_nsec + 1000000000) / 1000000;
> +  remains = timeout - waited;
> +  if (*resend_timeout > remains)
> +    *resend_timeout = remains;
> +  return (*resend_timeout <= 0);
> +}

This should perform a bit of overflow checking, or use unsigned long 
long at least.  It probably won't matter in practice because 2**31 
milliseconds is a long time, but let's cover this properly.

From an interface perspective, it seems preferable to compute an 
explicit deadline, and count the milliseconds towards that, rather than 
recomputing the absolute deadline from the millisecond value.

Thanks,
Florian