From owner-freebsd-net@FreeBSD.ORG Mon Jun 18 21:20:10 2007 Return-Path: X-Original-To: net@FreeBSD.org Delivered-To: freebsd-net@FreeBSD.ORG Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 20FC316A400; Mon, 18 Jun 2007 21:20:10 +0000 (UTC) (envelope-from rizzo@icir.org) Received: from xorpc.icir.org (xorpc.icir.org [192.150.187.68]) by mx1.freebsd.org (Postfix) with ESMTP id 0AE3B13C45B; Mon, 18 Jun 2007 21:20:10 +0000 (UTC) (envelope-from rizzo@icir.org) Received: from xorpc.icir.org (localhost [127.0.0.1]) by xorpc.icir.org (8.12.11/8.13.6) with ESMTP id l5ILK9pa041009; Mon, 18 Jun 2007 14:20:09 -0700 (PDT) (envelope-from rizzo@xorpc.icir.org) Received: (from rizzo@localhost) by xorpc.icir.org (8.12.11/8.12.3/Submit) id l5ILK9r8041008; Mon, 18 Jun 2007 14:20:09 -0700 (PDT) (envelope-from rizzo) Date: Mon, 18 Jun 2007 14:20:09 -0700 From: Luigi Rizzo To: Qing Li Message-ID: <20070618142009.A40302@xorpc.icir.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5.1i In-Reply-To: ; from qingli@speakeasy.net on Mon, Jun 18, 2007 at 05:51:44PM +0000 Cc: qingli@FreeBSD.org, Gleb Smirnoff , net@FreeBSD.org Subject: Re: new ARP code review 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: Mon, 18 Jun 2007 21:20:10 -0000 On Mon, Jun 18, 2007 at 05:51:44PM +0000, Qing Li wrote: > > [luigi:] > > > > i agree that the timing is a bit tight for inclusion, especially > > because the work dates back to 2004 if not before, and i think Qing > > Li took over development at least two years ago - not a great track > > record in terms of dedication to the work. I'd rather not see it > > rushed in :) > > > > Not sure how to respond to your comment here ... it wasn't meant as criticism, but just a consideration that there is no point to rush this change in when it has been idle for so long. Stalls occur for many reasons, I (and maybe others) thought you were busy on other stuff, maybe you were waiting for more feedback. But the bottom line is that we are now in a code freeze and this doesn't seem a good time for pushing something in. Add to this that Andre is temporarily on holidays. I hope now people will give you the feedback that you hoped to get a couple of years ago. > I emailed to net@ and developers@ for review after I put in the support > for IPv6, and made the new functions generic more than two years ago. I > received one full review from Gleb and a partial review from Andre. And > that patch has been sitting there in my home directory on > people.freebsd.org/~qingli ever since. The very last patch I put there is > dated April 19, 2005 (for the then -current). This time around, I got two > other reviews, and that's it. > > I'm certainly open for any suggestion on how to get more reviews > from the community. And let me know if you have any other specific > work items that you want done so you don't feel being rushed. ... > > the splitting is exactly the goal of this work and is by design. > > The mapping between the L3 and L2 addresses has nothing to do with > > the IP route lookup, and it should be elsewhere (namely, in the hash > > table or whatever data structure is appropriate). > > > > Eventually, with this structure you can do the route lookup > > only when you need to find the next hop (e.g. when a route > > changes etc.) and just the much-cheaper L3-L2 map in other cases. > > > > Even if the current implementation keeps doing both, this change > > is a step towards a separation of the two functions and enabling > > more cleanup in the code. > > > > I hope you don't disagree on the design. As for actual performance, > > we may pay something, as we did if you compare 4.x and 6.x/7.x, > > but then the opportunities for parallelization, reduction of > > contention and further code simplifications are well worth it. > > > > The current code necessary for creating ARP entries through > arp_rtrequest(), and the subsequent call paths are convoluted and > difficult to understand. The same approach was imported in the ND6 code. > This work has eliminated these types of code and the logic flows much > better. > > A couple of people raised the two-lookup performance issue, but > "Do you agree in principle ..." is exactly the kind of reviews I was > hoping for, but received none so far. This was the gating issue > for me for proceeding further two years ago and remains so today. Obviously i totally agree with the principle, and even with the implementation, having discussed the original design with Andre (and implemented it). I think the motivations i gave above are hard to criticize. Certainly, it would be good to put somewhere in the code a few comments (even just the previous paragraphs in this email) describing the design goals (and possibly open issues and/or possible-but-yet-unimplemented optimizations). This should address the concerns on performance that people may have. I might have a few style comments (e.g. putting the small block first in the if/then/else blocks) and also, of course, complete the locking (you mentioned it is incomplete; i see #if 0'ed code, and i did not address locking issues back in 2004 because this code was still under Giant.) cheers luigi