Re: RFC: nexthop cache separation

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

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.

> - 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). Also, I'm pretty sure that these flags would be relatively
unused by packages.

+#define RTF_CONNECTED	0x100		/* host address route */

/* hosts on this route are neighbours */

Is probably a better description for the purpose of the flag.

> - 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!)

>   - ARP entries that are created by arp ... temp
>     can be overwritten now

Unsure what you mean here, can you expand on your description please?

Roy