Re: Question about socket timeouts

On Friday, August 23, 2013 2:27:58 am Vitja Makarov wrote:
> 2013/8/22 John Baldwin <jhb_at_freebsd.org>:
> > On Thursday, August 22, 2013 12:18:48 am Vitja Makarov wrote:
> >> 2013/8/21 John Baldwin <jhb_at_freebsd.org>:
> >> > On Monday, August 19, 2013 11:13:02 pm Daniel Eischen wrote:
> >> >> On Mon, 19 Aug 2013, Adrian Chadd wrote:
> >> >>
> >> >> > Yes! Please file a PR!
> >> >>
> >> >> This sorta implies that both are acceptable (although,
> >> >> the Linux behavior seems more desirable).
> >> >>
> >> >>    http://austingroupbugs.net/view.php?id=369
> >> >
> >> > No, that says "round up", so it does mean that the requested timeout
> >> > should be the minimum amount slept.  tvtohz() does this.  Really odd
> >> > that the socket code is using its own version of this rather than
> >> > tvtohz().
> >> >
> >> > Oh, I bet this just predates tvtohz().  Interesting that it keeps getting
> >> > bug fixes in its history that simply using tvtohz() would have solved.
> >> >
> >> > Try this:
> >> >
> >> > Index: uipc_socket.c
> >> > ===================================================================
> >> > --- uipc_socket.c       (revision 254570)
> >> > +++ uipc_socket.c       (working copy)
> >> > _at__at_ -2699,21 +2699,16 _at__at_ sosetopt(struct socket *so, struct sockopt *sopt)
> >> >                         if (error)
> >> >                                 goto bad;
> >> >
> >> > -                       /* assert(hz > 0); */
> >> >                         if (tv.tv_sec < 0 || tv.tv_sec > INT_MAX / hz ||
> >> >                             tv.tv_usec < 0 || tv.tv_usec >= 1000000) {
> >> >                                 error = EDOM;
> >> >                                 goto bad;
> >> >                         }
> >> > -                       /* assert(tick > 0); */
> >> > -                       /* assert(ULONG_MAX - INT_MAX >= 1000000); */
> >> > -                       val = (u_long)(tv.tv_sec * hz) + tv.tv_usec / tick;
> >> > -                       if (val > INT_MAX) {
> >> > +                       val = tvtohz(&tv);
> >> > +                       if (val == INT_MAX) {
> >> >                                 error = EDOM;
> >> >                                 goto bad;
> >> >                         }
> >> > -                       if (val == 0 && tv.tv_usec != 0)
> >> > -                               val = 1;
> >> >
> >> >                         switch (sopt->sopt_name) {
> >> >                         case SO_SNDTIMEO:
> >> >
> >>
> >> That must help. But I want to see the issue solved in the next
> >> release. I can't apply patch to the production system. Btw in
> >> production environment we have kern.hz set to 1000 so it's not a
> >> problem there.
> >
> > Can you test this in some way in a test environment?
> >
> 
> Ok, sorry for posting out of the list.
> 
> Simple test program is attached. Without your patch timeout expires in
> about 20ms. With it it's ~40ms.
> 
> 40 instead of 30 is beacuse of odd tick added by tvtohz().

Ok, thanks.  tvtohz() will be good to MFC (and I will do that), but for
HEAD I think we can fix this to use a precise timeout.  I've cc'd davide_at_
so he can take a look at that.

-- 
John Baldwin