diff mbox series

[ovs-dev,v2] ofproto-dpif-xlate: avoid successive ct_clear datapath actions

Message ID 162133301920.596425.5363293849951682159.stgit@wsfd-netdev64.ntdv.lab.eng.bos.redhat.com
State New
Headers show
Series [ovs-dev,v2] ofproto-dpif-xlate: avoid successive ct_clear datapath actions | expand

Commit Message

Eelco Chaudron May 18, 2021, 10:17 a.m. UTC
Due to flow lookup optimizations, especially in the resubmit/clone cases,
we might end up with multiple ct_clear actions, which are not necessary.

This patch only adds the ct_clear action to the datapath if any ct state
is tracked.

Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
v2: Insert ct_clear only when ct information is tracked vs tracking successive
    ct_clear actions.

ofproto/ofproto-dpif-xlate.c |    4 +++-
 tests/ofproto-dpif.at        |   25 +++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

Comments

Timothy Redaelli May 24, 2021, 12:39 p.m. UTC | #1
On Tue, 18 May 2021 06:17:48 -0400
Eelco Chaudron <echaudro@redhat.com> wrote:

> Due to flow lookup optimizations, especially in the resubmit/clone cases,
> we might end up with multiple ct_clear actions, which are not necessary.
> 
> This patch only adds the ct_clear action to the datapath if any ct state
> is tracked.
> 
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> ---
> v2: Insert ct_clear only when ct information is tracked vs tracking successive
>     ct_clear actions.
> 
> ofproto/ofproto-dpif-xlate.c |    4 +++-
>  tests/ofproto-dpif.at        |   25 +++++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)


Acked-By: Timothy Redaelli <tredaelli@redhat.com>
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 7108c8a30..479e459fc 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -7127,7 +7127,9 @@  do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
 
         case OFPACT_CT_CLEAR:
-            compose_ct_clear_action(ctx);
+            if (ctx->conntracked) {
+                compose_ct_clear_action(ctx);
+            }
             break;
 
         case OFPACT_NAT:
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 24bbd884c..7d4f8fa1c 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -10842,6 +10842,31 @@  dnl
 NXT_PACKET_IN (xid=0x0): table_id=1 cookie=0x0 total_len=106 in_port=2 (via action) data_len=106 (unbuffered)
 udp,vlan_tci=0x0000,dl_src=50:54:00:00:00:0a,dl_dst=50:54:00:00:00:09,nw_src=10.1.1.2,nw_dst=10.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=2,tp_dst=1 udp_csum:553
 ])
+
+dnl The next test verifies that ct_clear at the datapath only gets executed
+dnl if conntrack information is present.
+AT_DATA([flows.txt], [dnl
+table=0 in_port=1 actions=ct_clear,ct_clear,ct_clear,p2
+])
+AT_CHECK([ovs-ofctl del-flows br0])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=p1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2'], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+  [Datapath actions: 2
+])
+AT_DATA([flows.txt], [dnl
+table=0 in_port=1 ip actions=ct_clear,ct(table=1)
+table=1 in_port=1 actions=ct_clear,ct_clear,goto_table:2
+table=2 in_port=1 actions=ct_clear,p2
+])
+AT_CHECK([ovs-ofctl del-flows br0])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=p1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2'], [0], [stdout])
+AT_CHECK([grep Datapath stdout | sed 's/recirc(.*)/recirc(X)/'], [0],
+  [Datapath actions: ct,recirc(X)
+Datapath actions: ct_clear,2
+])
+
 OVS_VSWITCHD_STOP
 AT_CLEANUP