Date: Tue, 02 Sep 2014 10:05:21 +1000 From: Lawrence Stewart <lstewart@freebsd.org> To: Midori Kato <katoon@sfc.wide.ad.jp> Cc: "Eggert, Lars" <lars@netapp.com>, hiren panchasara <hiren.panchasara@gmail.com>, net@freebsd.org Subject: Re: DCTCP implementation Message-ID: <540509C1.6090006@freebsd.org> In-Reply-To: <5338FF05.1020302@sfc.wide.ad.jp> References: <5338FF05.1020302@sfc.wide.ad.jp>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?540509C1.6090006>