diff mbox series

[ovs-dev,RFC,v3,12/13] ofproto: xlate: Make bridge-sampled drops explicit.

Message ID 20240704075255.140530-13-amorenoz@redhat.com
State RFC
Delegated to: Simon Horman
Headers show
Series Introduce local sampling with NXAST_SAMPLE action. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Adrián Moreno July 4, 2024, 7:52 a.m. UTC
The decision to add or not the explicit drop action is currently based
on whether the resulting dp action list is empty or not.

This is OK for most of the cases but if per-flow IPFIX/sFlow sampling
is enabled on the bridge, it doesn't work as expected.

Fix this by taking into account the size of these sampling actions.

Fixes: a13a0209750c ("userspace: Improved packet drop statistics.")
Cc: anju.thomas@ericsson.com
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 ofproto/ofproto-dpif-xlate.c |  5 ++--
 tests/drop-stats.at          | 44 ++++++++++++++++++++++++++++++++++++
 tests/ofproto-dpif.at        |  4 ++--
 3 files changed, 49 insertions(+), 4 deletions(-)
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 094fe5d72..323a58cbf 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -8153,6 +8153,7 @@  xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
     uint64_t action_set_stub[1024 / 8];
     uint64_t frozen_actions_stub[1024 / 8];
     uint64_t actions_stub[256 / 8];
+    size_t sample_actions_len = 0;
     struct ofpbuf scratch_actions = OFPBUF_STUB_INITIALIZER(actions_stub);
     struct xlate_ctx ctx = {
         .xin = xin,
@@ -8412,7 +8413,7 @@  xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
             user_cookie_offset = compose_sflow_action(&ctx);
             compose_ipfix_action(&ctx, ODPP_NONE);
         }
-        size_t sample_actions_len = ctx.odp_actions->size;
+        sample_actions_len = ctx.odp_actions->size;
         bool ecn_drop = !tnl_process_ecn(flow);
 
         if (!ecn_drop
@@ -8575,7 +8576,7 @@  exit:
     }
 
     /* Install drop action if datapath supports explicit drop action. */
-    if (xin->odp_actions && !xin->odp_actions->size &&
+    if (xin->odp_actions && xin->odp_actions->size == sample_actions_len &&
         ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) {
         put_drop_action(xin->odp_actions, ctx.error);
     }
diff --git a/tests/drop-stats.at b/tests/drop-stats.at
index 44c5cc35b..31782ac52 100644
--- a/tests/drop-stats.at
+++ b/tests/drop-stats.at
@@ -192,6 +192,50 @@  ovs-appctl coverage/read-counter drop_action_too_many_mpls_labels
 OVS_VSWITCHD_STOP(["/|WARN|/d"])
 AT_CLEANUP
 
+AT_SETUP([drop-stats - sampling])
+
+OVS_VSWITCHD_START([dnl
+    set bridge br0 datapath_type=dummy \
+        protocols=OpenFlow10,OpenFlow13,OpenFlow14,OpenFlow15 -- \
+    add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1])
+
+AT_DATA([flows.txt], [dnl
+table=0,in_port=1,actions=drop
+])
+
+AT_CHECK([
+    ovs-ofctl del-flows br0
+    ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
+    ovs-ofctl -Oopenflow13 dump-flows br0 | ofctl_strip | sort | grep actions ], [0], [dnl
+ in_port=1 actions=drop
+])
+
+AT_CHECK([ovs-vsctl -- set bridge br0 ipfix=@fix -- \
+                    --id=@fix create ipfix targets=\"127.0.0.1:4739\" \
+                              sampling=1],
+         [0], [ignore])
+
+AT_CHECK([
+    ovs-appctl netdev-dummy/receive p1 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
+    ovs-appctl netdev-dummy/receive p1 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
+    ovs-appctl netdev-dummy/receive p1 'in_port(1),packet_type(ns=0,id=0),eth(src=3a:6d:d2:09:9c:ab,dst=1e:2c:e9:2a:66:9e),ipv4(src=192.168.10.10,dst=192.168.10.30,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
+], [0], [ignore])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/used:[[0-9]].[[0-9]]*s/used:0.0/' | sort], [0], [flow-dump from the main thread:
+recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), packets:2, bytes:212, used:0.0, actions:userspace(pid=0,ipfix(output_port=4294967295)),drop
+])
+
+ovs-appctl time/warp 5000
+
+AT_CHECK([
+ovs-appctl coverage/read-counter drop_action_of_pipeline
+], [0], [dnl
+3
+])
+
+OVS_VSWITCHD_STOP(["/sending to collector failed/d"])
+
+AT_CLEANUP
 AT_SETUP([drop-stats - sampling action])
 
 OVS_VSWITCHD_START([dnl
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 701b8a851..ba8f3b69c 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -8082,7 +8082,7 @@  for i in `seq 1 3`; do
 done
 AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*\(packets:\)/\1/' | sed 's/used:[[0-9]].[[0-9]]*s/used:0.001s/'], [0], [dnl
 flow-dump from the main thread:
-packets:2, bytes:68, used:0.001s, actions:userspace(pid=0,ipfix(output_port=4294967295))
+packets:2, bytes:68, used:0.001s, actions:userspace(pid=0,ipfix(output_port=4294967295)),drop
 ])
 
 AT_CHECK([ovs-appctl revalidator/purge])
@@ -8118,7 +8118,7 @@  for i in `seq 1 3`; do
 done
 AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*\(packets:\)/\1/' | sed 's/used:[[0-9]].[[0-9]]*s/used:0.001s/'], [0], [dnl
 flow-dump from the main thread:
-packets:2, bytes:68, used:0.001s, actions:sample(sample=100.0%,actions(meter(0),userspace(pid=0,ipfix(output_port=4294967295))))
+packets:2, bytes:68, used:0.001s, actions:sample(sample=100.0%,actions(meter(0),userspace(pid=0,ipfix(output_port=4294967295)))),drop
 ])
 
 dnl Remove the IPFIX configuration.