Re: [ovs-dev] [PATCH v2 1/3] ofproto-dpif-xlate: No clone when tunnel push is last action

2022-04-14 Thread Rosemarie O'Riorden 
Hi Tao,

Thank you for looking at my patch.
Could you let me know where you saw this error so I can look into it?
I didn't see any failed tests.

Thanks,
Rosemarie

On Wed, Apr 13, 2022 at 10:44 PM Tao Liu  wrote:
>
> On Wed, Apr 13, 2022 at 05:30:19PM -0400, Rosemarie O'Riorden wrote:
> > When OVS sees a tunnel push with a nested list next, it will not
> > clone the packet, as a clone is not needed. However, a clone action will
> > still be created with the tunnel push encapulated inside. There is no
> > need to create the clone action in this case, as extra parsing will need
> > to be performed, which is less efficient.
> >
> > Signed-off-by: Rosemarie O'Riorden 
> > ---
> > v2:
> >  - Fixes tests that are affected by this change
> >
> >  ofproto/ofproto-dpif-xlate.c  | 15 --
> >  tests/nsh.at  |  6 +--
> >  tests/packet-type-aware.at| 24 -
> >  tests/tunnel-push-pop-ipv6.at | 20 
> >  tests/tunnel-push-pop.at  | 95 +--
> >  5 files changed, 103 insertions(+), 57 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 9bd0b0a38..319bb6db5 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -3720,7 +3720,10 @@ native_tunnel_output(struct xlate_ctx *ctx, const 
> > struct xport *xport,
> >  size_t clone_ofs = 0;
> >  size_t push_action_size;
> >
> > -clone_ofs = nl_msg_start_nested(ctx->odp_actions, 
> > OVS_ACTION_ATTR_CLONE);
> > +if (!is_last_action) {
> > +clone_ofs = nl_msg_start_nested(ctx->odp_actions,
> > +OVS_ACTION_ATTR_CLONE);
> > +}
>
> Hi, this breaks tunnel encap offload, we should also consider it in :
>
> parse_flow_actions
> parse_clone_actions
>
> >  odp_put_tnl_push_action(ctx->odp_actions, &tnl_push_data);
> >  push_action_size = ctx->odp_actions->size;
> >
> > @@ -3763,10 +3766,12 @@ native_tunnel_output(struct xlate_ctx *ctx, const 
> > struct xport *xport,
> >  }
> >  xlate_cache_steal_entries(backup_xcache, ctx->xin->xcache);
> >
> > -if (ctx->odp_actions->size > push_action_size) {
> > -nl_msg_end_non_empty_nested(ctx->odp_actions, clone_ofs);
> > -} else {
> > -nl_msg_cancel_nested(ctx->odp_actions, clone_ofs);
> > +if (!is_last_action) {
> > +if (ctx->odp_actions->size > push_action_size) {
> > +nl_msg_end_non_empty_nested(ctx->odp_actions, clone_ofs);
> > +} else {
> > +nl_msg_cancel_nested(ctx->odp_actions, clone_ofs);
> > +}
> >  }
> >
> >  /* Restore context status. */
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 1/3] ofproto-dpif-xlate: No clone when tunnel push is last action

2022-04-13 Thread Tao Liu 
On Wed, Apr 13, 2022 at 05:30:19PM -0400, Rosemarie O'Riorden wrote:
> When OVS sees a tunnel push with a nested list next, it will not
> clone the packet, as a clone is not needed. However, a clone action will
> still be created with the tunnel push encapulated inside. There is no
> need to create the clone action in this case, as extra parsing will need
> to be performed, which is less efficient.
> 
> Signed-off-by: Rosemarie O'Riorden 
> ---
> v2:
>  - Fixes tests that are affected by this change
> 
>  ofproto/ofproto-dpif-xlate.c  | 15 --
>  tests/nsh.at  |  6 +--
>  tests/packet-type-aware.at| 24 -
>  tests/tunnel-push-pop-ipv6.at | 20 
>  tests/tunnel-push-pop.at  | 95 +--
>  5 files changed, 103 insertions(+), 57 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 9bd0b0a38..319bb6db5 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3720,7 +3720,10 @@ native_tunnel_output(struct xlate_ctx *ctx, const 
> struct xport *xport,
>  size_t clone_ofs = 0;
>  size_t push_action_size;
>  
> -clone_ofs = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE);
> +if (!is_last_action) {
> +clone_ofs = nl_msg_start_nested(ctx->odp_actions,
> +OVS_ACTION_ATTR_CLONE);
> +}

Hi, this breaks tunnel encap offload, we should also consider it in :

parse_flow_actions
parse_clone_actions

>  odp_put_tnl_push_action(ctx->odp_actions, &tnl_push_data);
>  push_action_size = ctx->odp_actions->size;
>  
> @@ -3763,10 +3766,12 @@ native_tunnel_output(struct xlate_ctx *ctx, const 
> struct xport *xport,
>  }
>  xlate_cache_steal_entries(backup_xcache, ctx->xin->xcache);
>  
> -if (ctx->odp_actions->size > push_action_size) {
> -nl_msg_end_non_empty_nested(ctx->odp_actions, clone_ofs);
> -} else {
> -nl_msg_cancel_nested(ctx->odp_actions, clone_ofs);
> +if (!is_last_action) {
> +if (ctx->odp_actions->size > push_action_size) {
> +nl_msg_end_non_empty_nested(ctx->odp_actions, clone_ofs);
> +} else {
> +nl_msg_cancel_nested(ctx->odp_actions, clone_ofs);
> +}
>  }
>  
>  /* Restore context status. */
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 1/3] ofproto-dpif-xlate: No clone when tunnel push is last action

2022-04-13 Thread Rosemarie O'Riorden 
Please disregard the "1/3" in the title; it is an error. This is just a
single patch.


Rosemarie O'Riorden

On 4/13/22 17:30, Rosemarie O'Riorden wrote:
> When OVS sees a tunnel push with a nested list next, it will not
> clone the packet, as a clone is not needed. However, a clone action will
> still be created with the tunnel push encapulated inside. There is no
> need to create the clone action in this case, as extra parsing will need
> to be performed, which is less efficient.
>
> Signed-off-by: Rosemarie O'Riorden 
> ---
> v2:
>  - Fixes tests that are affected by this change
>
>  ofproto/ofproto-dpif-xlate.c  | 15 --
>  tests/nsh.at  |  6 +--
>  tests/packet-type-aware.at| 24 -
>  tests/tunnel-push-pop-ipv6.at | 20 
>  tests/tunnel-push-pop.at  | 95 +--
>  5 files changed, 103 insertions(+), 57 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 9bd0b0a38..319bb6db5 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3720,7 +3720,10 @@ native_tunnel_output(struct xlate_ctx *ctx, const 
> struct xport *xport,
>  size_t clone_ofs = 0;
>  size_t push_action_size;
>  
> -clone_ofs = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE);
> +if (!is_last_action) {
> +clone_ofs = nl_msg_start_nested(ctx->odp_actions,
> +OVS_ACTION_ATTR_CLONE);
> +}
>  odp_put_tnl_push_action(ctx->odp_actions, &tnl_push_data);
>  push_action_size = ctx->odp_actions->size;
>  
> @@ -3763,10 +3766,12 @@ native_tunnel_output(struct xlate_ctx *ctx, const 
> struct xport *xport,
>  }
>  xlate_cache_steal_entries(backup_xcache, ctx->xin->xcache);
>  
> -if (ctx->odp_actions->size > push_action_size) {
> -nl_msg_end_non_empty_nested(ctx->odp_actions, clone_ofs);
> -} else {
> -nl_msg_cancel_nested(ctx->odp_actions, clone_ofs);
> +if (!is_last_action) {
> +if (ctx->odp_actions->size > push_action_size) {
> +nl_msg_end_non_empty_nested(ctx->odp_actions, clone_ofs);
> +} else {
> +nl_msg_cancel_nested(ctx->odp_actions, clone_ofs);
> +}
>  }
>  
>  /* Restore context status. */
> diff --git a/tests/nsh.at b/tests/nsh.at
> index 4d49f1201..acda379fd 100644
> --- a/tests/nsh.at
> +++ b/tests/nsh.at
> @@ -724,7 +724,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(4),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=192.168.10.30,frag=no),
>  packets:1, bytes:98, used:0.0s, 
> actions:pop_eth,push_nsh(flags=0,ttl=63,mdtype=1,np=1,spi=0x3000,si=255,c1=0x0,c2=0x0,c3=0x0,c4=0x0),clone(tnl_push(tnl_port(4789),header(size=50,type=4,eth(dst=aa:55:00:00:00:03,src=aa:55:00:00:00:01,dl_type=0x0800),ipv4(src=10.0.0.1,dst=10.0.0.3,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan(flags=0xc04,vni=0x0)),out_port(1)),set(ipv4(src=30.0.0.1,dst=30.0.0.3)),tnl_pop(4789))
> +recirc_id(0),in_port(4),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=192.168.10.30,frag=no),
>  packets:1, bytes:98, used:0.0s, 
> actions:pop_eth,push_nsh(flags=0,ttl=63,mdtype=1,np=1,spi=0x3000,si=255,c1=0x0,c2=0x0,c3=0x0,c4=0x0),tnl_push(tnl_port(4789),header(size=50,type=4,eth(dst=aa:55:00:00:00:03,src=aa:55:00:00:00:01,dl_type=0x0800),ipv4(src=10.0.0.1,dst=10.0.0.3,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan(flags=0xc04,vni=0x0)),out_port(1)),set(ipv4(src=30.0.0.1,dst=30.0.0.3)),tnl_pop(4789)
>  
> tunnel(tun_id=0x0,src=30.0.0.1,dst=30.0.0.3,flags(-df-csum+key)),recirc_id(0),in_port(4789),packet_type(ns=1,id=0x894f),eth_type(0x894f),nsh(np=1,spi=0x3000,si=255),
>  packets:1, bytes:108, used:0.0s, actions:pop_nsh(),recirc(0x1)
>  
> tunnel(tun_id=0x0,src=30.0.0.1,dst=30.0.0.3,flags(-df-csum+key)),recirc_id(0x1),in_port(4789),packet_type(ns=1,id=0x800),eth_type(0x0800),ipv4(frag=no),
>  packets:1, bytes:84, used:0.0s, 
> actions:push_eth(src=00:00:00:00:00:00,dst=aa:55:aa:55:00:03),6
>  ])
> @@ -778,8 +778,8 @@ 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(4),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=192.168.10.20/255.255.255.248,frag=no),
>  packets:1, bytes:98, used:0.0s, 
> actions:pop_eth,push_nsh(flags=0,ttl=63,mdtype=1,np=1,spi=0x3020,si=255,c1=0x0,c2=0x0,c3=0x0,c4=0x0),clone(tnl_push(tnl_port(4789),header(size=50,type=4,eth(dst=aa:55:00:00:00:02,src=aa:55:00:00:00:01,dl_type=0x0800),ipv4(src=10.0.0.1,dst=10.0.0.2,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan(flags=0xc04,vni=0x0)),out_port(1)),set(ipv4(src=20.0.0.1

[ovs-dev] [PATCH v2 1/3] ofproto-dpif-xlate: No clone when tunnel push is last action

2022-04-13 Thread Rosemarie O'Riorden 
When OVS sees a tunnel push with a nested list next, it will not
clone the packet, as a clone is not needed. However, a clone action will
still be created with the tunnel push encapulated inside. There is no
need to create the clone action in this case, as extra parsing will need
to be performed, which is less efficient.

Signed-off-by: Rosemarie O'Riorden 
---
v2:
 - Fixes tests that are affected by this change

 ofproto/ofproto-dpif-xlate.c  | 15 --
 tests/nsh.at  |  6 +--
 tests/packet-type-aware.at| 24 -
 tests/tunnel-push-pop-ipv6.at | 20 
 tests/tunnel-push-pop.at  | 95 +--
 5 files changed, 103 insertions(+), 57 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 9bd0b0a38..319bb6db5 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3720,7 +3720,10 @@ native_tunnel_output(struct xlate_ctx *ctx, const struct 
xport *xport,
 size_t clone_ofs = 0;
 size_t push_action_size;
 
-clone_ofs = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE);
+if (!is_last_action) {
+clone_ofs = nl_msg_start_nested(ctx->odp_actions,
+OVS_ACTION_ATTR_CLONE);
+}
 odp_put_tnl_push_action(ctx->odp_actions, &tnl_push_data);
 push_action_size = ctx->odp_actions->size;
 
@@ -3763,10 +3766,12 @@ native_tunnel_output(struct xlate_ctx *ctx, const 
struct xport *xport,
 }
 xlate_cache_steal_entries(backup_xcache, ctx->xin->xcache);
 
-if (ctx->odp_actions->size > push_action_size) {
-nl_msg_end_non_empty_nested(ctx->odp_actions, clone_ofs);
-} else {
-nl_msg_cancel_nested(ctx->odp_actions, clone_ofs);
+if (!is_last_action) {
+if (ctx->odp_actions->size > push_action_size) {
+nl_msg_end_non_empty_nested(ctx->odp_actions, clone_ofs);
+} else {
+nl_msg_cancel_nested(ctx->odp_actions, clone_ofs);
+}
 }
 
 /* Restore context status. */
diff --git a/tests/nsh.at b/tests/nsh.at
index 4d49f1201..acda379fd 100644
--- a/tests/nsh.at
+++ b/tests/nsh.at
@@ -724,7 +724,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(4),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=192.168.10.30,frag=no),
 packets:1, bytes:98, used:0.0s, 
actions:pop_eth,push_nsh(flags=0,ttl=63,mdtype=1,np=1,spi=0x3000,si=255,c1=0x0,c2=0x0,c3=0x0,c4=0x0),clone(tnl_push(tnl_port(4789),header(size=50,type=4,eth(dst=aa:55:00:00:00:03,src=aa:55:00:00:00:01,dl_type=0x0800),ipv4(src=10.0.0.1,dst=10.0.0.3,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan(flags=0xc04,vni=0x0)),out_port(1)),set(ipv4(src=30.0.0.1,dst=30.0.0.3)),tnl_pop(4789))
+recirc_id(0),in_port(4),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=192.168.10.30,frag=no),
 packets:1, bytes:98, used:0.0s, 
actions:pop_eth,push_nsh(flags=0,ttl=63,mdtype=1,np=1,spi=0x3000,si=255,c1=0x0,c2=0x0,c3=0x0,c4=0x0),tnl_push(tnl_port(4789),header(size=50,type=4,eth(dst=aa:55:00:00:00:03,src=aa:55:00:00:00:01,dl_type=0x0800),ipv4(src=10.0.0.1,dst=10.0.0.3,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan(flags=0xc04,vni=0x0)),out_port(1)),set(ipv4(src=30.0.0.1,dst=30.0.0.3)),tnl_pop(4789)
 
tunnel(tun_id=0x0,src=30.0.0.1,dst=30.0.0.3,flags(-df-csum+key)),recirc_id(0),in_port(4789),packet_type(ns=1,id=0x894f),eth_type(0x894f),nsh(np=1,spi=0x3000,si=255),
 packets:1, bytes:108, used:0.0s, actions:pop_nsh(),recirc(0x1)
 
tunnel(tun_id=0x0,src=30.0.0.1,dst=30.0.0.3,flags(-df-csum+key)),recirc_id(0x1),in_port(4789),packet_type(ns=1,id=0x800),eth_type(0x0800),ipv4(frag=no),
 packets:1, bytes:84, used:0.0s, 
actions:push_eth(src=00:00:00:00:00:00,dst=aa:55:aa:55:00:03),6
 ])
@@ -778,8 +778,8 @@ 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(4),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=192.168.10.20/255.255.255.248,frag=no),
 packets:1, bytes:98, used:0.0s, 
actions:pop_eth,push_nsh(flags=0,ttl=63,mdtype=1,np=1,spi=0x3020,si=255,c1=0x0,c2=0x0,c3=0x0,c4=0x0),clone(tnl_push(tnl_port(4789),header(size=50,type=4,eth(dst=aa:55:00:00:00:02,src=aa:55:00:00:00:01,dl_type=0x0800),ipv4(src=10.0.0.1,dst=10.0.0.2,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan(flags=0xc04,vni=0x0)),out_port(1)),set(ipv4(src=20.0.0.1,dst=20.0.0.2)),tnl_pop(4789))
-tunnel(tun_id=0x0,src=20.0.0.1,dst=20.0.0.2,flags(-df-csum+key)),recirc_id(0),in_port(4789),packet_type(ns=1,id=0x894f),eth_type(0x894f),nsh(spi=0x3020,si=255),
 packets:1, bytes:108, used:0.0s, 
actions:push_eth(src=00:00:00:00:00:00,dst=11:22:33:44:55:66),set(nsh(spi=0x3020,si=254)),p