On Mon, Feb 20, 2023 at 5:05 AM Han Ding <hand...@chinatelecom.cn> 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.
>
> Gratuitous ARP message is a generally link-level broadcast messages and
> carry the same IP in sender and target fields. This patch add the check
> in function is_garp() whether the arp is a broadcast message and whether
> the arp is an arp request or an arp reply.
>
> Signed-off-by: Han Ding <hand...@chinatelecom.cn>
> ---
>
> Notes:
> v2:
> - Add the check whether the ARP packet is a broadcast message in
> is_garp().
>
> v3:
> - Add the check whether the packet is an arp request or an arp reply in
> is_garp().
>
> v4:
> - Add description of differences between patch versions
> - Use WC_MASK_FIELD() macro instead of memset.
>
> lib/flow.h | 16 +++++++++++++---
> ofproto/ofproto-dpif-xlate.c | 32 ++------------------------------
> 2 files changed, 15 insertions(+), 33 deletions(-)
>
> diff --git a/lib/flow.h b/lib/flow.h
> index c647ad83c..037deb6cd 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -1133,10 +1133,20 @@ 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))) {
This line is too long.
Other than that, This patch looks good to me. The new behaviour seems
to be more correct than before as well.
Thanks,
M
> + return false;
> + }
>
> + if (wc) {
> + WC_MASK_FIELD(wc, 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
