On Tue, Jul 09, 2024 at 04:22:04PM GMT, Eelco Chaudron wrote:
>
>
> On 9 Jul 2024, at 16:00, Adrián Moreno wrote:
>
> > 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?
>
> Well, in case of an error all actions are deleted, and only an explicit drop
> (or no) actions are added.
>
> For the sample() action, the drop reason should always be ok, or else the
> action should not be there. So I would prefer to make this clear by using
> XLATE_OK.
OK. I'll change that.
>
> >>> + }
> >>> +
> >>> 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
