Re: [ovs-dev] [PATCH] Fix redundant datapath set ethernet action with NSH Decap
On 27 Apr 2021, at 14:42, Martin Varghese wrote:
> From: Martin Varghese
>
> 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
> Fixes: f839892a206a ("OF support and translation of generic encap and decap")
> Signed-off-by: Martin Varghese
> ---
> 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=0x,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@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=0x,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=0x1a041234567820001408fedcba9876543210),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=0x1a041234567820001408fedcba9876543210),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([
>
Re: [ovs-dev] [PATCH] Fix redundant datapath set ethernet action with NSH Decap
Hi Martin,
Somehow I didn’t receive this patch email via the ovs-dev mailing list, perhaps
one of the many spam filters on the way interfered. Don't know if this response
email will be recognized by ovs patchwork.
The nsh.at lines are broken wrongly in this email, but they look OK in
patchworks.
Otherwise, LGTM.
Acked-by: Jan Scheurich
/Jan
> -Original Message-
> From: Varghese, Martin (Nokia - IN/Bangalore)
>
> Sent: Monday, 3 May, 2021 15:25
> To: Jan Scheurich
> Subject: RE: [PATCH] Fix redundant datapath set ethernet action with NSH
> Decap
>
> Hi Jan,
>
> Could you please review this patch.
>
> Regards,
> Martin
>
> -Original Message-
> From: Martin Varghese
> Sent: Tuesday, April 27, 2021 6:13 PM
> To: d...@openvswitch.org; echau...@redhat.com;
> jan.scheur...@ericsson.com
> Cc: Varghese, Martin (Nokia - IN/Bangalore)
> Subject: [PATCH] Fix redundant datapath set ethernet action with NSH Decap
>
> From: Martin Varghese
>
> When a decap action is applied on NSH header encapsulatiing a ethernet
> packet a redundant set mac address action is programmed to the datapath.
>
> Fixes: f839892a206a ("OF support and translation of generic encap and
> decap")
> Signed-off-by: Martin Varghese
> ---
> 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))) {
> 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=0x,dl_src=00:00:00:00:00:00,dl_dst=11:22:33:44:55:6
> 6,dl_type=0x894f,nsh_flags=0,nsh_ttl=63,nsh_mdtype=1,nsh_np=3,nsh_spi=0
> x1234,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,c
> 2=0x0,c3=0x0,c4=0x0),push_eth(src=00:00:00:00:00:00,dst=11:22:33:44:55:6
> 6),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,
> c
> +2=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@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=0x112
> 23344,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:9
> +e),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=0x11
> +223344,c2=0x0,c3=0x0,c4=0x0),push_eth(src=00:00:00:00:00:00,dst=11:22:
> 3
> +3: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=0x,dl_src=00:00:00:00:00:00,dl_dst=11:22:33:44:55:6
> 6,dl_type=0x894f,nsh_flags=0,nsh_ttl=63,nsh_mdtype=2,nsh_np=3,nsh_spi=0
> x1234,nsh_si=255,nw_proto=0,nw_tos=0,nw_ecn=0,nw_ttl=0
> Megaflow:
>
[ovs-dev] [PATCH] Fix redundant datapath set ethernet action with NSH Decap
From: Martin Varghese
When a decap action is applied on NSH header encapsulatiing a
ethernet packet a redundant set mac address action is programmed
to the datapath.
Fixes: f839892a206a ("OF support and translation of generic encap and decap")
Signed-off-by: Martin Varghese
---
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))) {
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=0x,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@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=0x,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=0x1a041234567820001408fedcba9876543210),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=0x1a041234567820001408fedcba9876543210),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@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=0x1a041234567820001408fedcba9876543210),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)
