On Tue, Jul 09, 2024 at 11:46:06AM GMT, Eelco Chaudron wrote:
> 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.
>
Sure "last_action" is pretty much self-explanatory.
> > 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?
>
I was trying to thing in a case where ctx->error would not be XLATE_OK
but I couldn't. Nevertheless I thought if there is such case, we should
definitely report the right error code, right?
> > + }
> > +
> > 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?
>
Ack.
> > +
> > +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?
>
Ack.
> > +
> > +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
