Date: Mon, 18 Jun 2007 14:20:09 -0700 From: Luigi Rizzo <rizzo@icir.org> To: Qing Li <qingli@speakeasy.net> Cc: qingli@FreeBSD.org, Gleb Smirnoff <glebius@FreeBSD.org>, net@FreeBSD.org Subject: Re: new ARP code review Message-ID: <20070618142009.A40302@xorpc.icir.org> In-Reply-To: <W873011106246441182189104@webmail1>; from qingli@speakeasy.net on Mon, Jun 18, 2007 at 05:51:44PM %2B0000 References: <W873011106246441182189104@webmail1>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070618142009.A40302>