On 7 Jul 2024, at 22:09, Adrian Moreno wrote:
> 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.
Same comments as on the previous patch.
> Fixes: a13a0209750c ("userspace: Improved packet drop statistics.")
> Cc: anju.tho...@ericsson.com
> Signed-off-by: Adrian Moreno <amore...@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 --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);
See other patch, do we need ctx.error here?
> }
> 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])
Should this maybe be 'bridge 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])
>
For loop?
> +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.
> --
> 2.45.2
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
