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 On Sat, Mar 12, 2016 at 1:25 AM, Ryota Ozaki <[email protected]> wrote: > On Fri, Mar 11, 2016 at 8:22 PM, Roy Marples <[email protected]> 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
