Re: RFC: nexthop cache separation

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

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 <ozaki-r%netbsd.org@localhost> wrote:
> 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