new ARP code review

Tue Jun 19 07:29:54 UTC 2007

Luigi Rizzo wrote:
> 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.)
gosh it's been a few years since I was in that code, but here goes...

I have some thoughts on this.
firstly, while it is interesting to have an arp table (ok LLA table) 
on each interface, I'm not sure that it gains you very much.

As mentioned elsewhere, the connection of the arp information with the 
routing table menas that the arp lookup is virtually free.
Or, at least it used to be in the Uniprocessor world. It's hard to beat free.

I can imagine however that the situation has changed since locking became
a factor. I suppose it depends upon what locking is required in arplookup() to 
make sure that the route (rt) is not modified while the  ll info is being 
extracted.
 What are the locking ramifications? 

The comment 
 "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."
makes me wonder..If we are not caching the arp code in the route any more,
then how do we avoid doing a route lookup on each packet?
I've looked at the patch for a few minutes and I haven't spotted the lladdr
being cached anywhere though the comment above suggests it is..

BTW having a per interface arp table does make sense if there a s a particular
thread that is responsible for that interface as only it would need
access to teh table and it could be done lock-free if one was careful enough.

> 
> 	cheers
> 	luigi
> _______________________________________________
> freebsd-net at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-net
> To unsubscribe, send any mail to "freebsd-net-unsubscribe at freebsd.org"