diff mbox series

[ovs-dev,v3] ofproto-dpif-xlate: Remove repeated function for judge garp

Message ID 202302071304413050971@chinatelecom.cn
State Changes Requested
Headers show
Series [ovs-dev,v3] ofproto-dpif-xlate: Remove repeated function for judge garp | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Han Ding Feb. 7, 2023, 5:04 a.m. UTC 
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 <handing@chinatelecom.cn>
---
 lib/flow.h                   | 15 +++++++++++++--
 ofproto/ofproto-dpif-xlate.c | 32 ++------------------------------
 2 files changed, 15 insertions(+), 32 deletions(-)

--
2.27.0

Comments

Simon Horman Feb. 7, 2023, 10:41 a.m. UTC | #1 
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 <handing@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.

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.

[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
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ilya Maximets Feb. 7, 2023, 12:23 p.m. UTC | #2 
On 2/7/23 06:04, 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 <handing@chinatelecom.cn>
> ---
>  lib/flow.h                   | 15 +++++++++++++--
>  ofproto/ofproto-dpif-xlate.c | 32 ++------------------------------
>  2 files changed, 15 insertions(+), 32 deletions(-)
> 
> 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);

WC_MASK_FIELD() macro should be used instead of memset.

> +        }
> +
> +        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
Han Ding Feb. 8, 2023, 1:26 a.m. UTC | #3 
>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 <handing@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
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
Han Ding Feb. 8, 2023, 1:30 a.m. UTC | #4 
>On 2/7/23 06:04, 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 <handing@chinatelecom.cn>
>> ---
>>  lib/flow.h                   | 15 +++++++++++++--
>>  ofproto/ofproto-dpif-xlate.c | 32 ++------------------------------
>>  2 files changed, 15 insertions(+), 32 deletions(-)
>> 
>> 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);
>
>WC_MASK_FIELD() macro should be used instead of memset.

Thanks for review. I will fix it in next version.
>
>> +        }
>> +
>> +        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
>
diff mbox series

Patch

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