new ARP code review

[Luigi Rizzo]

On Mon, Jun 18, 2007 at 07:15:51PM +0400, Gleb Smirnoff wrote:
> [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> > 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.

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.

	cheers
	luigi

> 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