From owner-freebsd-net@FreeBSD.ORG Tue Sep 2 00:05:39 2014 Return-Path: Delivered-To: net@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 ESMTPS id 6536699E for ; Tue, 2 Sep 2014 00:05:39 +0000 (UTC) Received: from lauren.room52.net (lauren.room52.net [210.50.193.198]) by mx1.freebsd.org (Postfix) with ESMTP id 29ECD1452 for ; Tue, 2 Sep 2014 00:05:38 +0000 (UTC) Received: from lgwl-lstewart2.corp.netflix.com (c110-22-7-57.eburwd3.vic.optusnet.com.au [110.22.7.57]) by lauren.room52.net (Postfix) with ESMTPSA id BCC587E820; Tue, 2 Sep 2014 10:05:35 +1000 (EST) Message-ID: <540509C1.6090006@freebsd.org> Date: Tue, 02 Sep 2014 10:05:21 +1000 From: Lawrence Stewart User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Midori Kato Subject: Re: DCTCP implementation References: <5338FF05.1020302@sfc.wide.ad.jp> In-Reply-To: <5338FF05.1020302@sfc.wide.ad.jp> X-Enigmail-Version: 1.6 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=0.0 required=5.0 tests=UNPARSEABLE_RELAY autolearn=unavailable version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on lauren.room52.net Cc: "Eggert, Lars" , hiren panchasara , net@freebsd.org X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 02 Sep 2014 00:05:39 -0000 Hi Midori (& Lars), On 03/31/14 16:37, Midori Kato wrote: > Hi FreeBSD developpers, > > I'm Midori Kato. I'm working on the DCTCP implementation in the FreeBSD > with Lars Eggert. I mail you because I would like to ask you a code > review and testing. The attached patch is not good enough to test our > code. Please give me your message. I will send an ECN marking > implmenetation in dummynet and test scripts personally to you. [snip] Firstly, please let me apologise for not providing follow up feedback and review after our initial discussion of your work last year. It was always my intention to come back to this but life has been particularly crazy the past 12 months and I have been unsuccessful in my attempts to keep a number of balls in the air. Hiren has been diligently working on shepherding your code into the FreeBSD source tree and has been prodding me for a review which I finally got around to yesterday. Unfortunately, my understanding is that non developers are unable to interact with the code review system being used, so we're moving the discussion back to the mailing list so that you and others can participate. You can read the review discussion history to date at [1]. Leaving editorial work on the documentation aside for the time being, I'd like to discuss the KPI changes and stack integration aspects of the patch to start with. Specifically: - The ect_handler KPI addition seems redundant to me and I believe the patch can be simplified by removing it entirely. - The ecnpkt_handler KPI addition takes arguments which are too specific to DCTCP, and is too indiscriminate in what it matches i.e. all packets for ECN enabled connections. I would argue we either add a fully generalist hook which would catch all packets of an ESTABLISHED connection, and have the dctcp module check if ECN is enabled or not before continuing processing (not my preferred option), or we extract the essence of what dctcp_ecnpkt_handler() needs to do into a less generic hook or bit of meta data passed in to an existing hook via struct cc_var (I need to study the code a bit more, but will likely strongly push for this option). - I don't believe the code correctly handles the case of dctcp being used on connections which did not successfully negotiate ECN. There is more to discuss but I think getting these high level technical things resolved first will help guide the remainder of the review discussion. Cheers, Lawrence [1] https://reviews.freebsd.org/D604