From owner-freebsd-net@freebsd.org Fri Apr 29 01:40:45 2016 Return-Path: Delivered-To: freebsd-net@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 28739B20389 for ; Fri, 29 Apr 2016 01:40:45 +0000 (UTC) (envelope-from daemon-user@freebsd.org) Received: from reviews.nyi.freebsd.org (reviews.nyi.freebsd.org [IPv6:2610:1c1:1:607c::16:b]) by mx1.freebsd.org (Postfix) with ESMTP id E6B371360 for ; Fri, 29 Apr 2016 01:40:44 +0000 (UTC) (envelope-from daemon-user@freebsd.org) Received: by reviews.nyi.freebsd.org (Postfix, from userid 1346) id 4261CDB62; Fri, 29 Apr 2016 01:40:44 +0000 (UTC) Date: Fri, 29 Apr 2016 01:40:44 +0000 To: freebsd-net@freebsd.org From: "sepherosa_gmail.com (Sepherosa Ziehau)" Reply-to: D5872+325+9dea0574509cdbb3@reviews.freebsd.org Subject: [Differential] D5872: tcp: Don't prematurely drop receiving-only connections Message-ID: <61203690be93262c73401e765d755a38@localhost.localdomain> X-Priority: 3 X-Phabricator-Sent-This-Message: Yes X-Mail-Transport-Agent: MetaMTA X-Auto-Response-Suppress: All X-Phabricator-Mail-Tags: Thread-Topic: D5872: tcp: Don't prematurely drop receiving-only connections X-Herald-Rules: <64> X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-Cc: X-Phabricator-Cc: X-Phabricator-Cc: Precedence: bulk In-Reply-To: References: Thread-Index: MmVmNzYzNzljOGQxMmM4MWI4MmNjYzcxMzczIFciu5w= MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="utf-8" X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.21 List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Apr 2016 01:40:45 -0000 sepherosa_gmail.com added a comment. In https://reviews.freebsd.org/D5872#130805, @lstewart wrote: > In https://reviews.freebsd.org/D5872#130179, @sepherosa_gmail.com wrote: > > > We probably can leave the cwnd resetting to later rexmt timeout or possible later fast retransmit (I think fast retransmit could kick in under some cases, if ENOBUFS happened); instead of resetting the cwnd immediately upon ENOBUFS. > > > Please leave the manipulation of cwnd as is so as to avoid conflating two different changes. The manipulation of cwnd on local drop has nothing to do with the subject of this particular change. Yep, I am not going to delete the cwnd reset in this patch. > Also, the patch we're reviewing here should be the commit candidate i.e. what will actually land in the tree. I understand that you need to test with something different than the commit candidate, but that should just be noted in the review description. We all need to see the final code as there are many subtleties here that require close attention from as many sets of eyes as possible. This is what I am testing now. Since this path is not on a hot code path and we obviously don't want to have timers unset for data/FIN/SYN, can we just use the "if() panic" here? And another thing is abstract it as a macro. Maybe we just leave the macro abstraction to the next step? So, if both of following is true: - We agree to use "if() panic" here, instead of KASSERT - Do the macro abstraction in the next step Then the current patch is exactly what I want to put into the tree. REVISION DETAIL https://reviews.freebsd.org/D5872 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: sepherosa_gmail.com, network, glebius, adrian, delphij, decui_microsoft.com, honzhan_microsoft.com, howard0su_gmail.com, freebsd-net-list, lstewart, hiren, jtl, transport Cc: gnn, mike-karels.net, jtl