>On Tue, Feb 07, 2023 at 01:04:41PM +0800, Han Ding wrote: >> >> Function is_gratuitous_arp() and function is_garp() are all used to judge >> whether the flow is gratuitous arp. It is not necessary to use two functions >> to do the same thing and just keep one. >> >> Signed-off-by: Han Ding <hand...@chinatelecom.cn> >> --- >> lib/flow.h | 15 +++++++++++++-- >> ofproto/ofproto-dpif-xlate.c | 32 ++------------------------------ >> 2 files changed, 15 insertions(+), 32 deletions(-) > >Hi, > >it would be helpful if you included some text here describing the >differences between v3 and v2, and in turn, v2 and v1.
Thanks, I will add the description of differences between different versions in next version. > >I also think that this change implies many considerations, as per the >discussion of v2 [1]. And I think it would be worth summarising those in >the patch description - hunting down historical information when reviewing >v2 was an involved task, for me at least. And inevitably this topic will be >revisited in future. So it would be nice to be kind to our future selves. > >I would be happy to help draft some text if that helps. It's very helpful to me. Thank you very much! > >[1] https://mail.openvswitch.org/pipermail/ovs-dev/2023-January/400803.html > >As for the code changes, they look good to me :) > >> >> diff --git a/lib/flow.h b/lib/flow.h >> index c647ad83c..5b37543e0 100644 >> --- a/lib/flow.h >> +++ b/lib/flow.h >> @@ -1133,8 +1133,19 @@ static inline bool is_garp(const struct flow *flow, >> struct flow_wildcards *wc) >> { >> if (is_arp(flow)) { >> - return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) == >> - FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst)); >> + if (!eth_addr_is_broadcast(FLOW_WC_GET_AND_MASK_WC(flow, wc, >> dl_dst))) { >> + return false; >> + } >> + >> + if (wc) { >> + memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto); >> + } >> + >> + if (flow->nw_proto == ARP_OP_REQUEST || >> + flow->nw_proto == ARP_OP_REPLY) { >> + return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) == >> + FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst)); >> + } >> } >> >> return false; >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index a9cf3cbee..b3c13f6bf 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -2543,34 +2543,6 @@ output_normal(struct xlate_ctx *ctx, const struct >> xbundle *out_xbundle, >> memcpy(&ctx->xin->flow.vlans, &old_vlans, sizeof(old_vlans)); >> } >> >> -/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after >> - * migration. Older Citrix-patched Linux DomU used gratuitous ARP replies >> to >> - * indicate this; newer upstream kernels use gratuitous ARP requests. */ >> -static bool >> -is_gratuitous_arp(const struct flow *flow, struct flow_wildcards *wc) >> -{ >> - if (flow->dl_type != htons(ETH_TYPE_ARP)) { >> - return false; >> - } >> - >> - memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst); >> - if (!eth_addr_is_broadcast(flow->dl_dst)) { >> - return false; >> - } >> - >> - memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto); >> - if (flow->nw_proto == ARP_OP_REPLY) { >> - return true; >> - } else if (flow->nw_proto == ARP_OP_REQUEST) { >> - memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src); >> - memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst); >> - >> - return flow->nw_src == flow->nw_dst; >> - } else { >> - return false; >> - } >> -} >> - >> /* Determines whether packets in 'flow' within 'xbridge' should be >>forwarded or >> * dropped. Returns true if they may be forwarded, false if they should be >> * dropped. >> @@ -2619,7 +2591,7 @@ is_admissible(struct xlate_ctx *ctx, struct xport >> *in_port, >> mac = mac_learning_lookup(xbridge->ml, flow->dl_src, vlan); >> if (mac >> && mac_entry_get_port(xbridge->ml, mac) != >>in_xbundle->ofbundle >> - && (!is_gratuitous_arp(flow, ctx->wc) >> + && (!is_garp(flow, ctx->wc) >> || mac_entry_is_grat_arp_locked(mac))) { >> ovs_rwlock_unlock(&xbridge->ml->rwlock); >> xlate_report(ctx, OFT_DETAIL, >> @@ -3062,7 +3034,7 @@ xlate_normal(struct xlate_ctx *ctx) >> } >> >> /* Learn source MAC. */ >> - bool is_grat_arp = is_gratuitous_arp(flow, wc); >> + bool is_grat_arp = is_garp(flow, wc); >> if (ctx->xin->allow_side_effects >> && flow->packet_type == htonl(PT_ETH) >> && in_port && in_port->pt_mode != NETDEV_PT_LEGACY_L3 >> -- >> 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
