Re: Question about socket timeouts

On Fri, Aug 23, 2013 at 5:29 PM, John Baldwin <jhb_at_freebsd.org> wrote:
> On Friday, August 23, 2013 9:53:12 am Davide Italiano wrote:
>> On Fri, Aug 23, 2013 at 3:45 PM, John Baldwin <jhb_at_freebsd.org> wrote:
>> > 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
>>
>> Hi,
>> I think I can switch this code to new timeout KPI, but this will
>> require the timeout field of 'struct sockbuf' to be changed from 'int'
>> to 'sbintime_t' which breaks binary compatibility. Do you have any
>> strong objections about this? If any, I would like this to happen ABI
>> freeze, so it looks like this is the right moment.
>
> This should be fine to change now, it just can't be MFC'd (which we can't
> do anyway).
>
> --
> John Baldwin

Please consider the following patch:
http://people.freebsd.org/~davide/review/socket_timeout.diff
I've tested it and it works OK. I got a timeout which is ~= 25ms using
the testcase provided by the user.
The only doubt I have is about the range check, I've changed a bit
because the 'integer' part of sbintime_t fits in 32-bits, but I'm not
sure it's the best way of doing this.

Thanks,

-- 
Davide

"There are no solved problems; there are only problems that are more
or less solved" -- Henri Poincare