Re: RFC: nexthop cache separation

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Thu, Mar 24, 2016 at 9:27 AM, Roy Marples <roy%marples.name@localhost> wrote:
> 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

Sure. I'll fix it.

>
> 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?

You're right. I'll do so.

>
> 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?

No, I'll drop it. I supposed to modify man pages after all changes
get agreement. I'm revising man pages now.

>
> 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.

Okay, if nobody complains, I'll change so.

>
> Keep up the good work!

Thanks :)


So patches are updated:
  http://www.netbsd.org/~ozaki-r/separate-nexthop-caches-v3.diff
  http://www.netbsd.org/~ozaki-r/separate-nexthop-caches-v3-diff.diff

Thanks,
  ozaki-r