new ARP code review

[Gleb Smirnoff]

[moving to net@] 

On Mon, Jun 18, 2007 at 07:31:18AM -0700, Luigi Rizzo wrote:
L> On Mon, Jun 18, 2007 at 05:29:55PM +0400, Gleb Smirnoff wrote:
L> > On Sat, Jun 09, 2007 at 01:10:44AM +0000, Qing Li wrote:
L> > Q>     Please review my new ARP patch and send me your feedbacks.
L> > Q>     The patch is for both ARP and ND6, and is accessible from 
L> > Q>     my home directory at
L> > Q>        http://people.freebsd.org/~qingli/newarp-06-08-2007
L> > Q> 
L> > Q>     The files if_llatbl.c and if_llatbl.h live under
L> > Q>     /usr/src/sys/net/.
L> > Q> 
L> > Q>     I still have some locking issues and I hope to resolve 
L> > Q>     these over the weekend. 
L> > Q> 
L> > Q>     Glebius had given me a bunch of comments long ago and
L> > Q>     I'm just digging these up now and trying to incorporate
L> > Q>     his suggestions.
L> > 
L> > Can you please prod me when you have incorporated my suggestions,
L> > and I will try to find a free time to review your patch again.
L> > I am still concerned about performance impact of this change.
L> > Have you measured it since?
L> 
L> is this a generic objection or you have something specific in mind ?

Now we have a single lookup. On every output packet the radix head
lock is acquired and a radix trie is searched for the given IP
address. This lookup returns us the hardware address that this IP
uses (if it is on local net). The packet is passed into output queue.

In the suggested patch we are doing two lookups: routing table
(radix trie) and the hash table. I suppose, this is going to be
slower than one lookup. We should also take into account that
hash tables has its separate locking.

L> I am asking because this code is derived (and probably without
L> too many changes) from an older version designed together with Andre
L> and implemented by myself and a student of mine, and as far as i
L> can remember it only decouples the arp table from the routing table
L> with no change in algorithms, so if anything it will be faster because
L> of smaller tables and less contention in accessing information.

The code is derived, but it has changed somewhat since. And, at the
time when I reviewed that code, I was the only reviewer of that
particular Qings patch. My review isn't enough.

L> > Q>     I would like to make this code a part of 7.0 release.
L> > 
L> > I would like to see opinion on this from as much people as
L> > possible.
L> 
L> i agree that the timing is a bit tight for inclusion, especially
L> because the work dates back to 2004 if not before, and i think Qing
L> Li took over development at least two years ago - not a great track
L> record in terms of dedication to the work. I'd rather not see it
L> rushed in :)
L> 
L> However, maybe the ABI changes (e.g. the additional argument to
L> arpresolve and nd6_storelladdr(), and the extra/renamed fields in struct
L> ifnet, keeping them unused for the time being) could be done now to
L> avoid more ABI changes later ?

-- 
Totus tuus, Glebius.
GLEBIUS-RIPN GLEB-RIPE