On Thu, Jun 04, 2020 at 08:31:08PM +0800, [email protected] wrote:
> From: Tonghao Zhang <[email protected]>
> 
> This patch allows to install arp rules to tc dp.
> In the future, arp will be offloaded to hardware to
> be processed. So OvS enable this now.
> 
> $ ovs-appctl dpctl/add-flow 'recirc_id(0),in_port(3),eth(),\
>   eth_type(0x0806),arp(op=2,tha=00:50:56:e1:4b:ab,tip=10.255.1.116)' 2
> 
> $ ovs-appctl dpctl/dump-flows
>   ... arp(tip=10.255.1.116,op=2,tha=00:50:56:e1:4b:ab) ...
> 
> $ tc filter show dev <ethx> ingress
>   ...
>   eth_type arp
>   arp_tip 10.255.1.116
>   arp_op reply
>   arp_tha 00:50:56:e1:4b:ab
>   not_in_hw
>     action order 1: mirred (Egress Redirect to device <ethy>) stolen
>     ...
> 
> Cc: Simon Horman <[email protected]>
> Cc: Paul Blakey <[email protected]>
> Cc: Roi Dayan <[email protected]>
> Cc: Ben Pfaff <[email protected]>
> Cc: William Tu <[email protected]>
> Cc: Ilya Maximets <[email protected]>
> Signed-off-by: Tonghao Zhang <[email protected]>

Thanks for your patch, mostly it looks good to me.

Travis-CI flagged a minor problem, as described below.
Could you take a look into it and consider posting v2?

...

> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 72601dc6ba2b..c630b830ecd0 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -7993,7 +7993,8 @@ get_arp_key(const struct flow *flow, struct ovs_key_arp 
> *arp)
>  
>      arp->arp_sip = flow->nw_src;
>      arp->arp_tip = flow->nw_dst;
> -    arp->arp_op = htons(flow->nw_proto);
> +    arp->arp_op = flow->nw_proto == UINT8_MAX ?
> +                  UINT16_MAX: htons(flow->nw_proto);

This appears to cause a build error.

https://travis-ci.org/github/horms2/ovs/jobs/694632844#L2809

lib/odp-util.c:7997:36: error: restricted ovs_be16 degrades to integer
lib/odp-util.c:7996:17: error: incorrect type in assignment (different base 
types)
lib/odp-util.c:7996:17:    expected restricted ovs_be16 [usertype] arp_op
lib/odp-util.c:7996:17:    got int

Perhaps using OVS_BE16_MAX instead of UINT16_MAX is a good approach here.

Also, there should be a space both before and after ':'.
E.g.

        arp->arp_op = flow->nw_proto == UINT8_MAX ?
                OVS_BE16_MAX : htons(flow->nw_proto);

>      arp->arp_sha = flow->arp_sha;
>      arp->arp_tha = flow->arp_tha;
>  }

...
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to