Thanks for reviewing. I will change them in next version.
--------------
Han Ding
>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
