From owner-freebsd-net@FreeBSD.ORG Thu Jul 7 10:47:14 2011 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 982751065678 for ; Thu, 7 Jul 2011 10:47:14 +0000 (UTC) (envelope-from andre@freebsd.org) Received: from c00l3r.networx.ch (c00l3r.networx.ch [62.48.2.2]) by mx1.freebsd.org (Postfix) with ESMTP id EE5218FC14 for ; Thu, 7 Jul 2011 10:47:13 +0000 (UTC) Received: (qmail 59084 invoked from network); 7 Jul 2011 09:46:43 -0000 Received: from localhost (HELO [127.0.0.1]) ([127.0.0.1]) (envelope-sender ) by c00l3r.networx.ch (qmail-ldap-1.03) with SMTP for ; 7 Jul 2011 09:46:43 -0000 Message-ID: <4E158EB3.80203@freebsd.org> Date: Thu, 07 Jul 2011 12:47:15 +0200 From: Andre Oppermann User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.18) Gecko/20110616 Thunderbird/3.1.11 MIME-Version: 1.0 To: Mikolaj Golub References: <867h7zxvd2.fsf@kopusha.home.net> In-Reply-To: <867h7zxvd2.fsf@kopusha.home.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Kostik Belousov , freebsd-net@freebsd.org, Pawel Jakub Dawidek Subject: Re: soreceive_stream: issues with O_NONBLOCK X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Jul 2011 10:47:14 -0000 On 03.07.2011 17:00, Mikolaj Golub wrote: > Hi, Hi Mikolaj, > Trying soreceive_stream I found that many applications (like firefox, pidgin, > gnus) might hang in soreceive_stream/sbwait. Many thanks for testing soreceive_stream. > It was shown up that the issue was with O_NONBLOCK connections -- they blocked > in recv() when should not have been. > > This can be checked with this simple test: > > http://people.freebsd.org/~trociny/test_nonblock.c > > In soreceive_stream we have the following code that looks wrong: > > 1968 /* Socket buffer is empty and we shall not block. */ > 1969 if (sb->sb_cc == 0&& > 1970 ((sb->sb_flags& SS_NBIO) || (flags& (MSG_DONTWAIT|MSG_NBIO)))) { > 1971 error = EAGAIN; > 1972 goto out; > 1973 } > > It should check so->so_state agains SS_NBIO, not sb->sb_flags. But just Indeed. > changing this is not enough. This check is called too early -- before checking > that socket state is not SBS_CANTRCVMORE. As a result, if the peer closes the > connection recv() returns EAGAIN instead of 0. See this example: > > http://people.freebsd.org/~trociny/test_close.c > > So I moved the "nonblock" check below SBS_CANTRCVMORE check and ended up with > this patch: > > http://people.freebsd.org/~trociny/uipc_socket.c.soreceive_stream.patch > > It works for me fine. OK. > Also, this part looks wrong: > > 1958 /* We will never ever get anything unless we are connected. */ > 1959 if (!(so->so_state& (SS_ISCONNECTED|SS_ISDISCONNECTED))) { > 1960 /* When disconnecting there may be still some data left. */ > 1961 if (sb->sb_cc> 0) > 1962 goto deliver; > 1963 if (!(so->so_state& SS_ISDISCONNECTED)) > 1964 error = ENOTCONN; > 1965 goto out; > 1966 } > > Why we check in 1959 that state is not SS_ISDISCONNECTED? If it is valid then > the check at 1963 is useless becase it will be always true. Shouldn't it be > something like below? > > if (!(so->so_state& (SS_ISCONNECTED|SS_ISCONNECTING))) { > /* When disconnecting there may be still some data left. */ > if (sb->sb_cc> 0) > goto deliver; > error = ENOTCONN; > goto out; > } > > (I don't see why we souldn't set ENOTCONN if the state is SS_ISDISCONNECTED). ENOTCONN is only valid in the beginning of the connection. When we reach SS_INDISCONNECTED the connection was connected before and we have to return a 0-read to signal EOF. It can be simplified by immediately setting the error and going out. The other cases can't ever be true. Please try this patch: http://people.freebsd.org/~andre/soreceive_stream.diff-20110707 I can't fully test it myself due to being 1000km from my development machine and having only limited hw access for rebooting. > And the last :-). Currently, to try soreceive_stream one need to rebuild kernel > with TCP_SORECEIVE_STREAM and then set tunable net.inet.tcp.soreceive_stream. > Why do we need TCP_SORECEIVE_STREAM option? Wouldn't tunable be enough? It > would simplify trying soreceive_stream by users and we might have more > testing/feedback. Done. See r223839. -- Andre