Hello Han, "Han Ding" <hand...@chinatelecom.cn> writes:
> Commit ba07cf222a add the feature "Handle gratuitous ARP requests and > replies in tnl_arp_snoop()". But commit 83c2757bd1 just allow the ARP whitch > the destination address of the ARP is matched against the known xbridge > addresses. > So the modification of commit ba07cf222a is not effective. When ovs receive > the > gratuitous ARP from underlay gateway which the source address and destination > address are all gateway IP, tunnel neighbor will not be updated. > I think it would be clearer formatting the commits like below: $ git -P show -s --format="%h (\"%s\")" --abbrev=12 ba07cf222a ba07cf222a0c ("Handle gratuitous ARP requests and replies in tnl_arp_snoop()") $ git -P show -s --format="%h (\"%s\")" --abbrev=12 83c2757bd1 83c2757bd16e ("xlate: Move tnl_neigh_snoop() to terminate_native_tunnel()") I guess that the last commit deserves a Fixes tag as well. > Signed-off-by: Han Ding <hand...@chinatelecom.cn> > --- > > Notes: > v3 > Correct the spell mistake. > > v2 > Change author name. > > ofproto/ofproto-dpif-xlate.c | 10 +++++++--- > tests/tunnel-push-pop.at | 20 ++++++++++++++++++++ > 2 files changed, 27 insertions(+), 3 deletions(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 8e5d030ac..6c69f981b 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -4126,6 +4126,11 @@ xport_has_ip(const struct xport *xport) > return n_in6 ? true : false; > } > > +#define IS_VALID_NEIGHBOR_REPLY(flow, ctx) \ > +((flow->dl_type == htons(ETH_TYPE_ARP) || \ > + flow->nw_proto == IPPROTO_ICMPV6) && \ > + is_neighbor_reply_correct(ctx, flow)) > + Although terminate_native_tunnel() would be the only user, I guess a static function could be ok here, instead. > static bool > terminate_native_tunnel(struct xlate_ctx *ctx, const struct xport *xport, > struct flow *flow, struct flow_wildcards *wc, > @@ -4146,9 +4151,8 @@ terminate_native_tunnel(struct xlate_ctx *ctx, const > struct xport *xport, > /* If no tunnel port was found and it's about an ARP or ICMPv6 > packet, > * do tunnel neighbor snooping. */ > if (*tnl_port == ODPP_NONE && > - (flow->dl_type == htons(ETH_TYPE_ARP) || > - flow->nw_proto == IPPROTO_ICMPV6) && > - is_neighbor_reply_correct(ctx, flow)) { > + (IS_VALID_NEIGHBOR_REPLY(flow, ctx) || > + is_garp(flow, wc))) { AFAICT, this seems ok to me and the tests related to tunnel_push_pop succeed. There's probably some room for improvement in the code down to tnl_arp_snoop(), but I guess it's a bit out of scope of this patch. > tnl_neigh_snoop(flow, wc, ctx->xbridge->name, > ctx->xin->allow_side_effects); > } else if (*tnl_port != ODPP_NONE && > diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at > index c63344196..0bac362f4 100644 > --- a/tests/tunnel-push-pop.at > +++ b/tests/tunnel-push-pop.at > @@ -369,6 +369,26 @@ AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], > [0], [dnl > 1.1.2.92 f8:bc:12:44:34:b6 br0 > ]) > > +dnl Receiving Gratuitous ARP request with correct VLAN id should alter > tunnel neighbor cache > +AT_CHECK([ovs-appctl netdev-dummy/receive p0 > 'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:c8,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=1,sha=f8:bc:12:44:34:c8,tha=00:00:00:00:00:00))']) > + > +ovs-appctl time/warp 1000 > +ovs-appctl time/warp 1000 > + > +AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl > +1.1.2.92 f8:bc:12:44:34:c8 br0 > +]) > + > +dnl Receiving Gratuitous ARP reply with correct VLAN id should alter tunnel > neighbor cache > +AT_CHECK([ovs-appctl netdev-dummy/receive p0 > 'recirc_id(0),in_port(1),eth(src=f8:bc:12:44:34:b2,dst=ff:ff:ff:ff:ff:ff),eth_type(0x8100),vlan(vid=10,pcp=7),encap(eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.92,op=2,sha=f8:bc:12:44:34:b2,tha=f8:bc:12:44:34:b2))']) > + > +ovs-appctl time/warp 1000 > +ovs-appctl time/warp 1000 > + > +AT_CHECK([ovs-appctl tnl/neigh/show | grep br | sort], [0], [dnl > +1.1.2.92 f8:bc:12:44:34:b2 br0 > +]) > + > dnl Receive ARP reply without VLAN header > AT_CHECK([ovs-vsctl set port br0 tag=0]) > AT_CHECK([ovs-appctl tnl/neigh/flush], [0], [OK > -- > 2.27.0 > > > > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev