On 7 Jul 2024, at 22:09, Adrian Moreno wrote:
> When an action set ends in a an OFP_SAMPLE action, it means the packet
> will be dropped and sampled. Make the drop explicit in this case so that
> drop statistics remain accurate.
>
> This could be done outside of the sample action, i.e: "sample(...),drop"
> but datapaths optimize sample actions that are found in the last
> position. So, taking into account that datapaths already report when the
> last sample probability fails, it is safe to put the drop inside the
> sample, i.e: "sample(...,drop)".
Some style comments below, as this was discussed with Ilya, I let him comment
on the feature side.
> Signed-off-by: Adrian Moreno <amore...@redhat.com>
> ---
> ofproto/ofproto-dpif-xlate.c | 16 +++++++--
> tests/drop-stats.at | 65 ++++++++++++++++++++++++++++++++++++
> tests/ofproto-dpif.at | 44 ++++++++++++++++++++++++
> 3 files changed, 123 insertions(+), 2 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index b9546dc5b..094fe5d72 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -715,6 +715,7 @@ static void xlate_xbundle_copy(struct xbridge *, struct
> xbundle *);
> static void xlate_xport_copy(struct xbridge *, struct xbundle *,
> struct xport *);
> static void xlate_xcfg_free(struct xlate_cfg *);
> +static void put_drop_action(struct ofpbuf *, enum xlate_error);
>
> /* Tracing helpers. */
>
> @@ -3392,6 +3393,8 @@ struct sample_userspace_args {
> struct compose_sample_args {
> uint32_t probability; /* Number of packets out of
> * UINT32_MAX to sample. */
> + bool last; /* If it's the last action and
> a
> + * drop action must be added.
> */
Should describing this means this is the last action not be enough?
Maybe also rename it to be last_action, so it's more clear what this means.
> struct sample_userspace_args *userspace; /* Optional,
> * arguments for userspace. */
> struct sample_psample_args *psample; /* Optional,
> @@ -3456,6 +3459,11 @@ compose_sample_action(struct xlate_ctx *ctx,
> ovs_assert(res == 0);
> }
>
> + if (args->last &&
> + ovs_explicit_drop_action_supported(ctx->xbridge->ofproto)) {
> + put_drop_action(ctx->odp_actions, ctx->error);
Any reason why we use ctx->error here? Should we just not use XLATE_OK?
> + }
> +
> if (is_sample) {
> nl_msg_end_nested(ctx->odp_actions, actions_offset);
> nl_msg_end_nested(ctx->odp_actions, sample_offset);
> @@ -3490,6 +3498,7 @@ compose_sflow_action(struct xlate_ctx *ctx)
>
> args.probability = dpif_sflow_get_probability(sflow);
> args.userspace = &userspace;
> + args.last = false;
>
> return compose_sample_action(ctx, &args);
> }
> @@ -3542,6 +3551,7 @@ compose_ipfix_action(struct xlate_ctx *ctx, odp_port_t
> output_odp_port)
>
> args.probability = dpif_ipfix_get_bridge_exporter_probability(ipfix);
> args.userspace = &userspace;
> + args.last = false;
>
> compose_sample_action(ctx, &args);
> }
> @@ -5974,7 +5984,8 @@ xlate_fill_ipfix_sample(struct xlate_ctx *ctx,
>
> static void
> xlate_sample_action(struct xlate_ctx *ctx,
> - const struct ofpact_sample *os)
> + const struct ofpact_sample *os,
> + bool last)
> {
> uint8_t cookie_buf[sizeof(os->obs_domain_id) + sizeof(os->obs_point_id)];
> struct dpif_lsample *lsample = ctx->xbridge->lsample;
> @@ -5991,6 +6002,7 @@ xlate_sample_action(struct xlate_ctx *ctx,
> * the same percentage. */
> compose_args.probability =
> ((uint32_t) os->probability << 16) | os->probability;
> + compose_args.last = last;
>
> if (ipfix) {
> xlate_fill_ipfix_sample(ctx, os, ipfix, &userspace);
> @@ -7726,7 +7738,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> break;
>
> case OFPACT_SAMPLE:
> - xlate_sample_action(ctx, ofpact_get_SAMPLE(a));
> + xlate_sample_action(ctx, ofpact_get_SAMPLE(a), last);
> break;
>
> case OFPACT_CLONE:
> diff --git a/tests/drop-stats.at b/tests/drop-stats.at
> index 1d3af98da..44c5cc35b 100644
> --- a/tests/drop-stats.at
> +++ b/tests/drop-stats.at
> @@ -191,3 +191,68 @@ ovs-appctl coverage/read-counter
> drop_action_too_many_mpls_labels
>
> OVS_VSWITCHD_STOP(["/|WARN|/d"])
> AT_CLEANUP
> +
> +AT_SETUP([drop-stats - sampling action])
> +
> +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 -- \
> + add-port br0 p2 -- set Interface p2 type=dummy ofport_request=2])
> +
> +AT_DATA([flows.txt], [dnl
> +table=0,in_port=1,actions=sample(probability=65535,collector_set_id=1)
> +table=0,in_port=2,actions=sample(probability=32767,collector_set_id=1)
> +])
> +
> +AT_CHECK([
> + ovs-ofctl del-flows br0
> + ovs-ofctl -Oopenflow13 add-flows br0 flows.txt
> +])
> +
> +AT_CHECK([ovs-vsctl --id=@br0 get Bridge br0 \
> + -- --id=@ipfix create IPFIX targets=\"127.0.0.1:4739\" \
> + -- create Flow_Sample_Collector_Set id=1 bridge=@br0
> ipfix=@ipfix],
> + [0], [ignore])
Maybe we can try this and other places to be more <80 friendly?
> +
> +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])
Maybe a 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,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=4294967295)),drop
> +])
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([
> +ovs-appctl coverage/read-counter drop_action_of_pipeline
> +], [0], [dnl
> +3
> +])
> +
> +AT_CHECK([ovs-appctl dpctl/del-flows])
> +
> +AT_CHECK([
> + ovs-appctl netdev-dummy/receive p2
> 'in_port(2),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 p2
> 'in_port(2),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 p2
> 'in_port(2),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(2),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
> packets:2, bytes:212, used:0.0,
> actions:sample(sample=50.0%,actions(userspace(pid=0,flow_sample(probability=32767,collector_set_id=1,obs_domain_id=0,obs_point_id=0,output_port=4294967295)),drop))
> +])
> +
> +ovs-appctl time/warp 5000
> +
> +AT_CHECK([ovs-appctl coverage/read-counter drop_action_of_pipeline >
> explicit_drops])
> +AT_CHECK([ovs-appctl coverage/read-counter datapath_drop_sample_error >
> sample_drops])
> +
> +AT_CHECK([expr $(cat sample_drops) + $(cat explicit_drops)], [0], [dnl
> +6
> +])
> +
> +OVS_VSWITCHD_STOP(["/sending to collector failed/d"])
> +AT_CLEANUP
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 2a04977a7..701b8a851 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -12282,3 +12282,47 @@ Datapath actions: EXPECTED_ACT
>
> OVS_VSWITCHD_STOP()
> AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - Local sampling - drop])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2
> +
> +AT_CHECK([ovs-appctl dpif/set-dp-features --force br0 psample true], [0],
> [ignore])
> +
> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> + -- create Flow_Sample_Collector_Set id=1 bridge=@br0
> local_group_id=42],
> + [0], [ignore])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=slowpath pktps stats
> bands=type=drop rate=2'])
> +
> +AT_DATA([flows.txt], [dnl
> +in_port=1,
> actions=sample(probability=32767,collector_set_id=1,obs_domain_id=100,obs_point_id=200)
> +in_port=2,
> actions=sample(probability=65535,collector_set_id=1,obs_domain_id=100,obs_point_id=200)
> +])
> +
> +AT_CHECK([ovs-ofctl --protocols=OpenFlow10 add-flows br0 flows.txt])
> +
> +m4_define([TRACE_PKT], [m4_join([,],
> + [eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)],
> + [ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no)],
> + [icmp(type=8,code=0)])])
> +
> +m4_define([EXPECTED_ACT1], [m4_join([],
> + [sample(sample=50.0%,actions(],
> + [psample(group=42,cookie=0x64000000c8),],
> + [drop],
> + [))],
> +)])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1) TRACE_PKT'], [0],
> [stdout])
> +AT_CHECK([tail -1 stdout], [0], [dnl
> +Datapath actions: EXPECTED_ACT1
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(2) TRACE_PKT'], [0],
> [stdout])
> +AT_CHECK([tail -1 stdout], [0], [dnl
> +Datapath actions: psample(group=42,cookie=0x64000000c8),drop
> +])
> +
> +OVS_VSWITCHD_STOP()
> +AT_CLEANUP
> --
> 2.45.2
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
