Re: RFC: nexthop cache separation

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

On Fri, Mar 11, 2016 at 8:22 PM, Roy Marples <roy%marples.name@localhost> wrote:
> Hi
>
> On 11/03/2016 08:40, Ryota Ozaki wrote:
>> This proposal separates Layer 2 nexthop caches (ARP
>> and NDP entries) from the routing table and instead
>> stores them in each interface. This change obsoletes
>> the concept of cloning and cloned routes; nexthop
>> caches won't be bound to any routes.
>
> Nice work!!!
>
>>
>> Here is a patch (tl;tr):
>>   http://www.netbsd.org/~ozaki-r/separate-nexthop-caches.diff
>
> I'll try and review this in detail, but I'm really pushed for time right
> now so might be a while :(
>
> Here's a few thoughts though:
>
> nd6_is_addr_neighbor() needs to test for RTF_CONNECTED.
> The comment (which you retained) in that function even says this, but no
> check is actually made anymore. This should be fixed.

Okay, I'll check it.

>
>> - RTF_CLONING and RTF_CLONED are obsolete
>>   - Keep the definitions to not break package
>>     builds
>
> I think we should just drop (by this i mean comment not, not physically
> removed from the header) the RTF_CLONING and RTF_CLONED defines.
> FreeBSD has already done this, and I hate having useless defines for
> removed functonality as the header is then effectively lying about the
> system we are compiling for.
>
> Packages should just #ifdef around this (dhcpcd already does this for
> example).

Okay I'll do so. If we do so, we have to announce somewhere
packages people around?

> Also, I'm pretty sure that these flags would be relatively
> unused by packages.

Yeah, I think so. I checked pkgsrc/net.

>
> +#define RTF_CONNECTED  0x100           /* host address route */
>
> /* hosts on this route are neighbours */
>
> Is probably a better description for the purpose of the flag.

I agree. I'll use it.

>
>> - RTM_RESOLVE and RTF_XSORELVE are obsolete
>>   - The definitions remain to not break
>>     package builds (may not be needed)
>> - RTF_LLINFO is obsolete
>>   - The definition remains
>
> Disagree again for the same reasons above.
>
>>   - arp/ndp -d don't remove interface addresses
>>     - They were removed (unexpectedly?)
>
> I hope you don't mean they remove the address from the interface :)
>
> Do you mean they no longer remove the address reported by arp/ndp for
> the hardware address? If so, I think we should try and retain that
> functionality if possible (I actually use it!)

I'm sorry for my bad wording. I meant arp/ndp -d doesn't remove
an ARP entry for an IP address assigned to an interface and its
MAC address. I'll try to fix if you need the original behavior.

>
>>   - ARP entries that are created by arp ... temp
>>     can be overwritten now
>
> Unsure what you mean here, can you expand on your description please?

arp command can add an ARP entry with an expire time, not permanent one,
by appending "temp" option. The original behavior of arp cannot overwrite
the ARP entry (by arp -s <same_ip> <mac>). OTOH, the new behavior can
overwrite it.

Thanks,
  ozaki-r