new ARP code review

[Luigi Rizzo]

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