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
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[EMAIL PROTECTED]"
