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

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>
---
 ofproto/ofproto-dpif-xlate.c | 32 ++------------------------------
 1 file changed, 2 insertions(+), 30 deletions(-)

--
2.27.0

On 12/1/22 07:39, 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.

Hi.  Thanks for the patch!

These function are indeed similar, but they are not identical.  It looks
like is_gratuitous_arp() is handling arp replies along with arp requests,
it also checks for the packet to be broadcast.  is_garp() doesn't do that.

In case we want to keep only one implementation, we should probably keep
the more robust version, which is is_gratuitous_arp().

What do you think?

Best regards, Ilya Maximets.

> 
> Signed-off-by: Han Ding <handing@chinatelecom.cn>
> ---
>  ofproto/ofproto-dpif-xlate.c | 32 ++------------------------------
>  1 file changed, 2 insertions(+), 30 deletions(-)
> 
> 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
>On 12/1/22 07:39, 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.
>
>Hi.  Thanks for the patch!
>
>These function are indeed similar, but they are not identical.  It looks
>like is_gratuitous_arp() is handling arp replies along with arp requests,
>it also checks for the packet to be broadcast.  is_garp() doesn't do that.
>
>In case we want to keep only one implementation, we should probably keep
>the more robust version, which is is_gratuitous_arp().
>
>What do you think?
>
>Best regards, Ilya Maximets.
>
Thanks for reviewing this patch.  Now I think there are some defects all in is_gratuitous_arp() 
and is_garp(). Gratuitous arp is a special arp packet where the source and destination IP 
are both set to the same IP and the destination MAC is the broadcast address ff:ff:ff:ff:ff:ff. 
Function is_gratuitous_arp() check the broadcast, and whether the src ip and dst ip are same. But 
when the packet is arp reply, the function don't check whether the src ip and dst ip are same. 
The function is_garp check the arp request and arp reply and whether the src ip and dst ip are same.
But it doesn't check for the packet to be broadcast. 

So, I think we should probably keep is_garp() and modify it. Because is_garp() is a inline function in flow.h.
But is_gratuitous_arp() is a static function defined in ofproto-dpif-xlate.c.

Did I miss something ?

Best regards, Han Ding. 


>> 
>> Signed-off-by: Han Ding <handing@chinatelecom.cn>
>> ---
>>  ofproto/ofproto-dpif-xlate.c | 32 ++------------------------------
>>  1 file changed, 2 insertions(+), 30 deletions(-)
>> 
>> 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
>