[...]
> >> 2. This patch doesn't cover cases where sampling is not actually
> >> the last action, but further actions do not yield any datapath
> >> actions. E.g. if you have a register resoration afterwards,
> >> which happens in automatically built pipelines like in OVN.
> >> Or if the last action after sampling is learn(). And things are
> >> getting more complicated if we take into account resubmits and
> >> processing in different bridges via patch ports.
> >
> > I agree this could be problematic. Maybe we should make sure the sample
> > is the last dp action and "fix it". A trick such as the one done for
> > sflow.
>
> These tricks are not accurate as well. I beleive they do not track
> the information well across tunnel encaps at least.
>
I am currently testing a patch implements this for both per-bridge and
per-flow sampling. I attach the essense (i.e: not the tests) of it at
the end [1]. Can you expand on your concerns about tunnel encaps?
> >
> >>
> >> 3. If someone is sampling the drops specifically, they know where
> >> to find the packets, because they configured the collector.
> >>
> >
> > I think drop stats and samples are two different things. There are
> > typically extacted by different tools and systems.
> >
> > Besides, what about per-bridge sampling (next patch)?
> > The user enables sampling on a bridge without explicitly doing it
> > for drops and suddenly the drop statistics dissapear.
>
> I think 'user enables sampling on a bridge' counts as an explicit
> altering of the datapath pipeline.
>
> >
> >> 4. Packets are not actually dropped, they are delivered to userspace
> >> or psample. It might make sense though to drop with a reson in
> >> case the upcall fails or psample fails to deliver to any entity
> >> and it is the last action in the datapath, but that's a kernel
> >> change to make.
> >>
> >
> > I don't want to get into another semantic discussion between consume,
> > free and drop or the dark corners of the OpenFlow protocol. For me it's
> > pretty clear that if the last action is to sample, the packet is
> > "dropped" in the sense that, from a switch' perspective, if it's not
> > forwarded/sent somewhere, it's dropped.
> >
> > I know you don't think the same :-).
> >
> > Would you then agree that the concept of dropping packets is very
> > unclear and OpenFlow does not make it easy (or even possible?) to
> > express a sampled drops and we should add an extension action to
> > explicitly drop packets?
>
> I agree that dropping packets is not clear in this case.
> I'm not a fan of OpenFlow drop action, it may cause extra logical issues.
> I'd go with a database knob instead.
>
> I'd say we can either drop these two patches for now and find a better
> solution in the next release cycle, or add an experimental global database
> knob that will control this behavior and will be disabled by default.
> Once we have better understanding how it behaves in a real world, we
> could switch the default or remove the feature if it causes issues or
> confusion.
>
> What do you think?
>
I guess this new know would be considered a new feature and therefore
not backportable to 3.4. So if this affects users (as is my suspicious),
we would have a non-fixable bug?
[1] Please consider this alternative to the two patches (it does not
include tests). FWIF, we could make this tweak configurable (default to
false) in the db.
---
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 0a0964348..da1ef0b49 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;
}
return cookie_offset;
@@ -8066,12 +8072,13 @@ 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. */
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 +8092,13 @@ xlate_optimize_odp_actions(struct xlate_in *xin)
last_action = a;
}
+ if (!last_action) {
+ 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 +8107,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. */
+ if (ctx->xout->last_observe_offset != UINT32_MAX &&
+ (char *) last_action == (char *) actions->data +
+ ctx->xout->last_observe_offset &&
+ ovs_explicit_drop_action_supported(ctx->xbridge->ofproto)) {
+ put_drop_action(actions, XLATE_OK);
}
}
@@ -8113,6 +8134,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 +8563,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)) {
- 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 {
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
