From owner-freebsd-current@FreeBSD.ORG Fri Aug 23 13:53:14 2013 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 301DA517; Fri, 23 Aug 2013 13:53:14 +0000 (UTC) (envelope-from davide.italiano@gmail.com) Received: from mail-vc0-x231.google.com (mail-vc0-x231.google.com [IPv6:2607:f8b0:400c:c03::231]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id D0FF128D3; Fri, 23 Aug 2013 13:53:13 +0000 (UTC) Received: by mail-vc0-f177.google.com with SMTP id gf12so428703vcb.22 for ; Fri, 23 Aug 2013 06:53:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=bqNXBvazA6++umhdVOXxJKsVk7g02vSUH9cCaO7tJeE=; b=vTSvtAm/6X+qvpeQGdoBDoAJi5GLEc3EzmuWonm8IT81hBO3yhN9wv8YKG8GYEuyGi 16/Nn5NldbStbTY0ivEQeVdD7YGGkPyjdqvux9FPZfzU5FGoSoBjCvB+W87+9ziZfi9p OfXTXe+4aTLdRKjEgFYKea8AmaDQQ3xZqKemmkmOpuL/U90qtN5Yecqh6vUj0Uc2eCke /luzgDHETl70x6e2qWTSg+nfp8YXzJYuzcBDMLaWCHPKVQebDo5JTwjvysEW2BMIEdXY G05H2Ve4f8uuDvCnY1yN1p3COaacLOXuS1JX8SjDh3Dk77ctMejaxiOjDoWjDdPKUTRs Fhvw== MIME-Version: 1.0 X-Received: by 10.52.119.139 with SMTP id ku11mr81043vdb.42.1377265993052; Fri, 23 Aug 2013 06:53:13 -0700 (PDT) Sender: davide.italiano@gmail.com Received: by 10.220.65.132 with HTTP; Fri, 23 Aug 2013 06:53:12 -0700 (PDT) In-Reply-To: <201308230945.28701.jhb@freebsd.org> References: <201308221408.08203.jhb@freebsd.org> <201308230945.28701.jhb@freebsd.org> Date: Fri, 23 Aug 2013 15:53:12 +0200 X-Google-Sender-Auth: wfF3tZBsgRxuvrrteQ8ePGlnB8I Message-ID: Subject: Re: Question about socket timeouts From: Davide Italiano To: John Baldwin Content-Type: text/plain; charset=ISO-8859-1 Cc: Vitja Makarov , freebsd-current X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Aug 2013 13:53:14 -0000 On Fri, Aug 23, 2013 at 3:45 PM, John Baldwin wrote: > On Friday, August 23, 2013 2:27:58 am Vitja Makarov wrote: >> 2013/8/22 John Baldwin : >> > On Thursday, August 22, 2013 12:18:48 am Vitja Makarov wrote: >> >> 2013/8/21 John Baldwin : >> >> > 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. Thanks, -- Davide "There are no solved problems; there are only problems that are more or less solved" -- Henri Poincare