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

[Pavel Raiskup <praiskup at redhat dot com>]

On Wednesday, June 15, 2016 10:00:34 AM CEST Florian Weimer wrote:
> 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.

Those two I'm able to fix easily, however ..

> > +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.

... I was afraid about portability here.  Are we talking about some new
glibc API, or just internal symbol?  Something like doing best-effort
CLOCK_MONOTONIC and falling back to gettimeofday when not possible?
Agreed here that it is better approach, and I can definitely learn about
glibc symbol management from your patches .. thanks for looking at it,
btw.

> > +  /* 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.

Ack.  Especially if it is going to be used in different scope, too.

>  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.

I'm still not sure what you mean by deadline.  It is not time-period, but
rather point in time..  feel free to change that approach as I don't see
an issue there, too.

What I just tried to avoid is to measure the delta between subsequent
__clock_gettime() calls and then subtracting this from "remaining
milliseconds" variable.  That sounds like the delta could be measured
inaccurately, and the whole timeout could be inaccurate.  Then, we should
probably count still in "absolute" milliseconds against the deadline.

Thanks, Pavel