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

Zoltán Balogh zoltan.balogh at ericsson.com
Mon Jan 8 11:43:58 UTC 2018


Hi Ben,

> 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?

Jan gave a very good summary in his mail:
https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/342662.html

> 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'

Thank you for the review, I'm going to fix this.

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

Sorry, this is a leftover from a previous version. There is no need for the 
include.

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

The xbridge_addr holds the current IP addresses assigned to the bridge. These
can change by time. Xbridge_addr is passed to xlate_xbridge_set(), where it is
set as xbridge->addr. Most of the xbridge members are handled the same way
(counting references etc.) in xlate_xbridge_set().
Maybe it's disturbing to retrieve xbridge_addr in xlate_ofproto_set() and then
pass it to xlate_bridge_set().
Do you think, would it be better to store xbridge_addr in ofproto and update it
when an IP address is added or removed, then pass an ofproto member to
xlate_ofproto_set()?

Best regards,
Zoltan

> Thanks,
> 
> Ben.


More information about the dev mailing list