[ovs-dev] [PATCH v2 3/3] xlate: call tnl_neigh_snoop() from terminate_native_tunnel()

[Ben Pfaff]

On Thu, Dec 14, 2017 at 09:56:21AM +0100, Zoltan Balogh wrote:
> Move tunnel neigh snooping from do_xlate_actions() to
> terminate_native_tunnel() in order to keep ARP neighbor cache clean.
> Furthermore filter ARP reply and Neighbor Advertisement messages
> addressing tunnel endpoint.
> 
> Signed-off-by: Zoltan Balogh <zoltan.balogh at ericsson.com>

Thanks for working on the tunneling code.

I don't understand yet the motivation here.  What does it mean "to keep
ARP neighbor cache clean"?  Is this a bug fix, a performance fix, a
cleanup, or something else?

What is the goal of the filtering you mention?  Is that a second change,
that can and should be a separate patch, or is it closely coupled to the
first change you mention?

This gives me the following Clang errors:

    ../ofproto/ofproto-dpif-xlate.c:3749:13: error: cast from 'const uint8_t *' (aka 'const unsigned char *') to 'const struct in6_addr *' increases required alignment from 1 to 4 [-Werror,-Wcast-align]
    /usr/include/netinet/in.h:454:36: note: expanded from macro 'IN6_ARE_ADDR_EQUAL'
    ../ofproto/ofproto-dpif-xlate.c:3749:13: error: cast from 'const uint8_t *' (aka 'const unsigned char *') to 'const struct in6_addr *' increases required alignment from 1 to 4 [-Werror,-Wcast-align]
    /usr/include/netinet/in.h:455:36: note: expanded from macro 'IN6_ARE_ADDR_EQUAL'

The change to tnl-neigh-cache.c that just adds an #include directive
seems odd to me.

The treatment of the new xbridge_addr is different from the treatment of
everything already handled in xlate_ofproto_set().  Why does it need
special treatment?  Is it RCU safe?  Why does it need a refcount (none
of the other structs do)?

Thanks,

Ben.