Re: RFC: nexthop cache separation

[Roy Marples <roy%marples.name@localhost>]

On Tuesday 22 March 2016 13:14:39 Ryota Ozaki wrote:
> Hi,
> 
> Here are new patches:
>   http://www.netbsd.org/~ozaki-r/separate-nexthop-caches-v2.diff
>   http://www.netbsd.org/~ozaki-r/separate-nexthop-caches-v2-diff.diff
> 
> Changes since v1:
> - Comment out osbolete RTF_* and RTM_* definitions
>   - Tweak some userland codes for the change
> - Restore checks of connected (cloning) routes in nd6_is_addr_neighbor
> - Restore the original behavior on removing ARP/NDP entries for
>   IP addresses of interface itself
> - Remove remaining use of RTF_LLINFO in the kernel
>   - I think we can remove it safely
> 
> Thanks,
>   ozaki-r

 	/*
 	 * Even if the address matches none of our addresses, it might match
 	 * a cloning route or be in the neighbor cache.
 	 */


So we need to change this comment now in nd6_is_addr_neighbor()
s/cloning/connected

I think the last question is which is the more expensive lookup? A connected 
route or on the neighbour cache? I suspect the former because it's at least a 
malloc/free whereas the latter is a lookup - so would it make more sense to 
swap the tests around so we assume a positive match?

For usr.bin/netstat/route.c, you drop the L flag usage, but not dropped it from 
the man page or options. Is this on purpose?

I've not had time to check it other than a cursory glance, but more - than + 
is always good!

One parting comment, one of style (my preference)
-#define RTF_CLONED	0x2000		/* this is a cloned route */
+/* #define RTF_CLONED	0x2000		   this is a cloned route */

Might be better as this
-#define RTF_CLONED	0x2000		/* this is a cloned route */
+//#define RTF_CLONED	0x2000		 /*  this is a cloned route */

So that the original comment markings are preserved. You have other mods like 
this in your patch, I just highlighted this one as it's an easy diff.
I dunno how others feel about that, maybe just me and you can ignore it.

Keep up the good work!

Roy