[ovs-dev] [PATCH] Fix redundant datapath set ethernet action with NSH Decap

[Eelco Chaudron]

On 27 Apr 2021, at 14:42, Martin Varghese wrote:

> From: Martin Varghese <martin.varghese at nokia.com>
>
> When a decap action is applied on NSH header encapsulatiing a
> ethernet packet a redundant set mac address action is programmed
> to the datapath.

Patch looks good to me, small style nit below.

Acked-by: Eelco Chaudron <echaudro at redhat.com>

> Fixes: f839892a206a ("OF support and translation of generic encap and decap")
> Signed-off-by: Martin Varghese <martin.varghese at nokia.com>
> ---
>  lib/odp-util.c               | 3 ++-
>  ofproto/ofproto-dpif-xlate.c | 2 ++
>  tests/nsh.at                 | 8 ++++----
>  3 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index e1199d1da..9d558082f 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -7830,7 +7830,8 @@ commit_set_ether_action(const struct flow *flow, struct flow *base_flow,
>      struct offsetof_sizeof ovs_key_ethernet_offsetof_sizeof_arr[] =
>          OVS_KEY_ETHERNET_OFFSETOF_SIZEOF_ARR;
>
> -    if (flow->packet_type != htonl(PT_ETH)) {
> +    if ((flow->packet_type != htonl(PT_ETH)) ||
> +        (base_flow->packet_type != htonl(PT_ETH))) {

Guess this can be simplified to the following (see also style guide):

-    if ((flow->packet_type != htonl(PT_ETH)) ||
-        (base_flow->packet_type != htonl(PT_ETH))) {
+    if (flow->packet_type != htonl(PT_ETH) ||
+        base_flow->packet_type != htonl(PT_ETH)) {
         return;

>          return;
>      }
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 7108c8a30..a6f4ea334 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -6549,6 +6549,8 @@ xlate_generic_decap_action(struct xlate_ctx *ctx,
>                   * Delay generating pop_eth to the next commit. */
>                  flow->packet_type = htonl(PACKET_TYPE(OFPHTN_ETHERTYPE,
>                                                        ntohs(flow->dl_type)));
> +                flow->dl_src = eth_addr_zero;
> +                flow->dl_dst = eth_addr_zero;
>                  ctx->wc->masks.dl_type = OVS_BE16_MAX;
>              }
>              return false;
> diff --git a/tests/nsh.at b/tests/nsh.at
> index d5c772ff0..e84134e42 100644
> --- a/tests/nsh.at
> +++ b/tests/nsh.at
> @@ -105,7 +105,7 @@ bridge("br0")
>
>  Final flow: in_port=1,vlan_tci=0x0000,dl_src=00:00:00:00:00:00,dl_dst=11:22:33:44:55:66,dl_type=0x894f,nsh_flags=0,nsh_ttl=63,nsh_mdtype=1,nsh_np=3,nsh_spi=0x1234,nsh_si=255,nsh_c1=0x11223344,nsh_c2=0x0,nsh_c3=0x0,nsh_c4=0x0,nw_proto=0,nw_tos=0,nw_ecn=0,nw_ttl=0
>  Megaflow: recirc_id=0,eth,ip,in_port=1,dl_dst=66:77:88:99:aa:bb,nw_frag=no
> -Datapath actions: push_nsh(flags=0,ttl=63,mdtype=1,np=3,spi=0x1234,si=255,c1=0x11223344,c2=0x0,c3=0x0,c4=0x0),push_eth(src=00:00:00:00:00:00,dst=11:22:33:44:55:66),pop_eth,pop_nsh(),set(eth(dst=11:22:33:44:55:66)),recirc(0x1)
> +Datapath actions: push_nsh(flags=0,ttl=63,mdtype=1,np=3,spi=0x1234,si=255,c1=0x11223344,c2=0x0,c3=0x0,c4=0x0),push_eth(src=00:00:00:00:00:00,dst=11:22:33:44:55:66),pop_eth,pop_nsh(),recirc(0x1)
>  ])
>
>  AT_CHECK([
> @@ -139,7 +139,7 @@ ovs-appctl time/warp 1000
>  AT_CHECK([
>      ovs-appctl dpctl/dump-flows dummy at ovs-dummy | strip_used | grep -v ipv6 | sort
>  ], [0], [flow-dump from the main thread:
> -recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth(dst=1e:2c:e9:2a:66:9e),eth_type(0x0800),ipv4(frag=no), packets:1, bytes:98, used:0.0s, actions:push_nsh(flags=0,ttl=63,mdtype=1,np=3,spi=0x1234,si=255,c1=0x11223344,c2=0x0,c3=0x0,c4=0x0),push_eth(src=00:00:00:00:00:00,dst=11:22:33:44:55:66),pop_eth,pop_nsh(),set(eth(dst=11:22:33:44:55:66)),recirc(0x3)
> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth(dst=1e:2c:e9:2a:66:9e),eth_type(0x0800),ipv4(frag=no), packets:1, bytes:98, used:0.0s, actions:push_nsh(flags=0,ttl=63,mdtype=1,np=3,spi=0x1234,si=255,c1=0x11223344,c2=0x0,c3=0x0,c4=0x0),push_eth(src=00:00:00:00:00:00,dst=11:22:33:44:55:66),pop_eth,pop_nsh(),recirc(0x3)
>  recirc_id(0x3),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), packets:1, bytes:98, used:0.0s, actions:2
>  ])
>
> @@ -232,7 +232,7 @@ bridge("br0")
>
>  Final flow: in_port=1,vlan_tci=0x0000,dl_src=00:00:00:00:00:00,dl_dst=11:22:33:44:55:66,dl_type=0x894f,nsh_flags=0,nsh_ttl=63,nsh_mdtype=2,nsh_np=3,nsh_spi=0x1234,nsh_si=255,nw_proto=0,nw_tos=0,nw_ecn=0,nw_ttl=0
>  Megaflow: recirc_id=0,eth,ip,in_port=1,dl_dst=66:77:88:99:aa:bb,nw_frag=no
> -Datapath actions: push_nsh(flags=0,ttl=63,mdtype=2,np=3,spi=0x1234,si=255,md2=0x10000a041234567820001408fedcba9876543210),push_eth(src=00:00:00:00:00:00,dst=11:22:33:44:55:66),pop_eth,pop_nsh(),set(eth(dst=11:22:33:44:55:66)),recirc(0x1)
> +Datapath actions: push_nsh(flags=0,ttl=63,mdtype=2,np=3,spi=0x1234,si=255,md2=0x10000a041234567820001408fedcba9876543210),push_eth(src=00:00:00:00:00:00,dst=11:22:33:44:55:66),pop_eth,pop_nsh(),recirc(0x1)
>  ])
>
>  AT_CHECK([
> @@ -266,7 +266,7 @@ ovs-appctl time/warp 1000
>  AT_CHECK([
>      ovs-appctl dpctl/dump-flows dummy at ovs-dummy | strip_used | grep -v ipv6 | sort
>  ], [0], [flow-dump from the main thread:
> -recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth(dst=1e:2c:e9:2a:66:9e),eth_type(0x0800),ipv4(frag=no), packets:1, bytes:98, used:0.0s, actions:push_nsh(flags=0,ttl=63,mdtype=2,np=3,spi=0x1234,si=255,md2=0x10000a041234567820001408fedcba9876543210),push_eth(src=00:00:00:00:00:00,dst=11:22:33:44:55:66),pop_eth,pop_nsh(),set(eth(dst=11:22:33:44:55:66)),recirc(0x3)
> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth(dst=1e:2c:e9:2a:66:9e),eth_type(0x0800),ipv4(frag=no), packets:1, bytes:98, used:0.0s, actions:push_nsh(flags=0,ttl=63,mdtype=2,np=3,spi=0x1234,si=255,md2=0x10000a041234567820001408fedcba9876543210),push_eth(src=00:00:00:00:00:00,dst=11:22:33:44:55:66),pop_eth,pop_nsh(),recirc(0x3)
>  recirc_id(0x3),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), packets:1, bytes:98, used:0.0s, actions:2
>  ])
>
> -- 
> 2.18.4