Question about socket timeouts
Vitja Makarov
vitja.makarov at gmail.com
Fri Aug 23 14:04:06 UTC 2013
2013/8/23 Davide Italiano <davide at freebsd.org>:
> 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)
>>> >> > @@ -2699,21 +2699,16 @@ 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@
>> 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.
>
I think that for socket's timeouts it's ok to have a HZ-precision. It
would be much more important to implement high-precision timeouts for
select() and friends, if it's not done yet (sorry I'm running 9.1).
--
vitja.
More information about the freebsd-current
mailing list