On 12 Jul 2024, at 1:32, Adrian Moreno wrote: > When the flow translation results in a datapath action list whose last > action is an "observational" action, i.e: one generated for IPFIX, > sFlow or local sampling applications, the packet is actually going to be > dropped (and observed). > > In that case, add an explicit drop action so that drop statistics remain > accurate. > > Combine the "optimizations" and other odp_actions "tweaks" into a single > function.
I'm running out of time to properly review (and look for alternatives), so I'll add some general comments, and let you and Ilya work out the details when I'm on PTO ;) > Signed-off-by: Adrian Moreno <[email protected]> > --- > ofproto/ofproto-dpif-xlate.c | 52 +++++++++++----- > ofproto/ofproto-dpif-xlate.h | 4 ++ > tests/drop-stats.at | 113 +++++++++++++++++++++++++++++++++++ > tests/ofproto-dpif.at | 47 ++++++++++++++- > 4 files changed, 200 insertions(+), 16 deletions(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 0a0964348..15e89e6a8 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3415,6 +3415,7 @@ compose_sample_action(struct xlate_ctx *ctx, > struct ofproto *ofproto = &ctx->xin->ofproto->up; > uint32_t meter_id = ofproto->slowpath_meter_id; > size_t cookie_offset = 0; > + size_t observe_offset; > > /* The meter action is only used to throttle userspace actions. > * If they are not needed and the sampling rate is 100%, avoid generating > @@ -3432,6 +3433,7 @@ compose_sample_action(struct xlate_ctx *ctx, > } > > if (args->psample) { > + observe_offset = ctx->odp_actions->size; > odp_put_psample_action(ctx->odp_actions, > args->psample->group_id, > (void *) &args->psample->cookie, > @@ -3443,6 +3445,7 @@ compose_sample_action(struct xlate_ctx *ctx, > nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, > meter_id); > } > > + observe_offset = ctx->odp_actions->size; > odp_port_t odp_port = ofp_port_to_odp_port( > ctx->xbridge, ctx->xin->flow.in_port.ofp_port); > uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port); > @@ -3457,6 +3460,9 @@ compose_sample_action(struct xlate_ctx *ctx, > if (is_sample) { > nl_msg_end_nested(ctx->odp_actions, actions_offset); > nl_msg_end_nested(ctx->odp_actions, sample_offset); > + ctx->xout->last_observe_offset = sample_offset; > + } else { > + ctx->xout->last_observe_offset = observe_offset; Not sure why, but I do not like the idea of storing offsets in the ctx. Thinking out loud without any checking; We are looping through all actions in xlate_optimize_odp_actions() is there no easy way to figure it out there? > } > > return cookie_offset; > @@ -8066,12 +8072,14 @@ xlate_wc_finish(struct xlate_ctx *ctx) > } > } > > -/* This will optimize the odp actions generated. For now, it will remove > - * trailing clone actions that are unnecessary. */ > +/* This will tweak the odp actions generated. For now, it will: > + * - Remove trailing clone actions that are unnecessary. > + * - Add an explicit drop action if the last action is observational or the > + * action list is empty. */ > static void > -xlate_optimize_odp_actions(struct xlate_in *xin) > +xlate_tweak_odp_actions(struct xlate_ctx *ctx) > { > - struct ofpbuf *actions = xin->odp_actions; > + struct ofpbuf *actions = ctx->xin->odp_actions; > struct nlattr *last_action = NULL; > struct nlattr *a; > int left; > @@ -8085,9 +8093,16 @@ xlate_optimize_odp_actions(struct xlate_in *xin) > last_action = a; > } > > + if (!last_action) { > + if (ovs_explicit_drop_action_supported(ctx->xbridge->ofproto)) { > + put_drop_action(actions, XLATE_OK); > + } > + return; > + } > + > /* Remove the trailing clone() action, by directly embedding the nested > * actions. */ > - if (last_action && nl_attr_type(last_action) == OVS_ACTION_ATTR_CLONE) { > + if (nl_attr_type(last_action) == OVS_ACTION_ATTR_CLONE) { > void *dest; > > nl_msg_reset_size(actions, > @@ -8096,6 +8111,16 @@ xlate_optimize_odp_actions(struct xlate_in *xin) > > dest = nl_msg_put_uninit(actions, nl_attr_get_size(last_action)); > memmove(dest, nl_attr_get(last_action), > nl_attr_get_size(last_action)); > + return; > + } > + > + /* If the last action of the list is an observability action, add an > + * explicit drop action so that drop statistics remain reliable. */ What if the sample was part of the last clone we are removing? > + if (ctx->xout->last_observe_offset != UINT32_MAX && > + (char *) last_action == (char *) actions->data + Change to unsigned char same as existing code? > + ctx->xout->last_observe_offset && > + ovs_explicit_drop_action_supported(ctx->xbridge->ofproto)) { > + put_drop_action(actions, XLATE_OK); > } > } > > @@ -8113,6 +8138,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out > *xout) > *xout = (struct xlate_out) { > .slow = 0, > .recircs = RECIRC_REFS_EMPTY_INITIALIZER, > + .last_observe_offset = UINT32_MAX, > }; > > struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp); > @@ -8541,17 +8567,15 @@ exit: > xout->slow = 0; > if (xin->odp_actions) { > ofpbuf_clear(xin->odp_actions); > + /* Make the drop explicit if the datapath supports it. */ > + if (ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) { > + put_drop_action(xin->odp_actions, ctx.error); > + } > } > } else { > - /* In the non-error case, see if we can further optimize the datapath > - * rules by removing redundant (clone) actions. */ > - xlate_optimize_odp_actions(xin); > - } > - > - /* Install drop action if datapath supports explicit drop action. */ > - if (xin->odp_actions && !xin->odp_actions->size && > - ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) { If you make this configurable, please ensure the current behavior is maintained. > - put_drop_action(xin->odp_actions, ctx.error); > + /* In the non-error case, see if we can further optimize or tweak > + * datapath actions. */ > + xlate_tweak_odp_actions(&ctx); > } > > /* Since congestion drop and forwarding drop are not exactly > diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h > index 08f9397d8..d973a634a 100644 > --- a/ofproto/ofproto-dpif-xlate.h > +++ b/ofproto/ofproto-dpif-xlate.h > @@ -61,6 +61,10 @@ struct xlate_out { > > /* Recirc action IDs on which references are held. */ > struct recirc_refs recircs; > + > + /* Keep track of the last action whose purpose is purely observational. > + * e.g: IPFIX, sFlow, local sampling. */ > + uint32_t last_observe_offset; > }; > > struct xlate_in { > diff --git a/tests/drop-stats.at b/tests/drop-stats.at > index 1d3af98da..820ca9c06 100644 > --- a/tests/drop-stats.at > +++ b/tests/drop-stats.at > @@ -191,3 +191,116 @@ ovs-appctl coverage/read-counter > drop_action_too_many_mpls_labels > > OVS_VSWITCHD_STOP(["/|WARN|/d"]) > AT_CLEANUP > + > +AT_SETUP([drop-stats - 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]) > + > +for i in $(seq 1 3); do > +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)' > +], [0], [ignore]) > +done > + > +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 There is a mix of bare and AT_CHECK() time/warps. Ilya what is the preferred way? My preference will be: AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore]) > + > +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_STOP(["/sending to collector failed/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),load:0->reg0 > +]) > + > +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]) > + > +for i in $(seq 1 3); do > +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)' > +], [0], [ignore]) > +done > + > +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, dnl > +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 > +]) Maybe (multiple times) AT_CHECK([ovs-appctl coverage/read-counter drop_action_of_pipeline], [0], [dnl 3 ]) > + > +AT_CHECK([ovs-appctl dpctl/del-flows]) > + > +for i in $(seq 1 3); do > +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)' > +], [0], [ignore]) > +done > + > +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, dnl > +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 > +], [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 5edb22fb5..c94dca9b1 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. > @@ -12295,3 +12295,46 @@ Datapath actions: EXPECTED_ACT > > OVS_VSWITCHD_STOP("/Enabling an unsupported feature is very dangerous/d") > 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 > +]) Maybe a test case for a last action inside a clone, i.e. clone(psample(group=42,cookie=0x64000000c8)) --> psample(group=42,cookie=0x64000000c8),drop > + > +OVS_VSWITCHD_STOP("/Enabling an unsupported feature is very dangerous/d") > +AT_CLEANUP > -- > 2.45.2 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
