Re: [ovs-dev] [PATCH v1 11/13] ofproto: xlate: Make flow-sampled drops explicit.
[...]
> >> 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
Re: [ovs-dev] [PATCH v1 11/13] ofproto: xlate: Make flow-sampled drops explicit.
On 7/11/24 00:19, Adrián Moreno wrote: > On Wed, Jul 10, 2024 at 11:31:42PM GMT, Ilya Maximets wrote: >> On 7/7/24 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)". >>> >>> Signed-off-by: Adrian Moreno >>> --- >>> ofproto/ofproto-dpif-xlate.c | 16 +++-- >>> tests/drop-stats.at | 65 >>> tests/ofproto-dpif.at| 44 >>> 3 files changed, 123 insertions(+), 2 deletions(-) >> >> Hi, Adrian. As we discussed before on the kernel list, I'm not sure >> this is a right change to make for a fwe reasons: >> >> 1. We do not know the user's intentions. We do not know if they >>wanted to drop these packets or their goal was to sample them >>and it just happened to be the last action in the list, because >>they put it after the output action and not before. >> > > If there are datapath actions after output action you will probably get > drops reported in both datapaths. Not if it was a sampling action. > >> 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. > >> >> 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? > >> 5. Drop reporting in either datapath implementation is not 100% >>accurate anyway and requires users to know the kernel internals. >> > > Shouldn't we try to make them more accurate, not less? We're not making them less accurate. > >> Some of that also applies to the next patch in the set. >> > > I can take some of the critics for this patch but for me, the next one > is bluntly fixing a bug: Per-bridge sflow/IPFIX should not break drop > statistics. It doesn't break the drop statistics if we see sampling as terminal action. It's not a drop from the datapath perspective. > >> For the patch itself, you should also check that the action set is >> actually empty (not the action list) after the sa
Re: [ovs-dev] [PATCH v1 11/13] ofproto: xlate: Make flow-sampled drops explicit.
On Wed, Jul 10, 2024 at 11:31:42PM GMT, Ilya Maximets wrote: > On 7/7/24 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)". > > > > Signed-off-by: Adrian Moreno > > --- > > ofproto/ofproto-dpif-xlate.c | 16 +++-- > > tests/drop-stats.at | 65 > > tests/ofproto-dpif.at| 44 > > 3 files changed, 123 insertions(+), 2 deletions(-) > > Hi, Adrian. As we discussed before on the kernel list, I'm not sure > this is a right change to make for a fwe reasons: > > 1. We do not know the user's intentions. We do not know if they >wanted to drop these packets or their goal was to sample them >and it just happened to be the last action in the list, because >they put it after the output action and not before. > If there are datapath actions after output action you will probably get drops reported in both datapaths. > 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. > > 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. > 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? > 5. Drop reporting in either datapath implementation is not 100% >accurate anyway and requires users to know the kernel internals. > Shouldn't we try to make them more accurate, not less? > Some of that also applies to the next patch in the set. > I can take some of the critics for this patch but for me, the next one is bluntly fixing a bug: Per-bridge sflow/IPFIX should not break drop statistics. > For the patch itself, you should also check that the action set is > actually empty (not the action list) after the sample action translation. Aye, what about doing the sflow trick and look at the datapath flow alone? > There is a chance that the clone action is disabled and sample is > used in place of a clone, I'm not sure this case is covered. That case does not go through compose_sample_action but encodes the datapath action directly. > And the formatting in tests seems very inconsistent. > I agree, I tried to follow the drop-stats.at "style" and the result is pretty bad. > Best regards, Ilya Maximets. > Thanks. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 11/13] ofproto: xlate: Make flow-sampled drops explicit.
On 7/7/24 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)". > > Signed-off-by: Adrian Moreno > --- > ofproto/ofproto-dpif-xlate.c | 16 +++-- > tests/drop-stats.at | 65 > tests/ofproto-dpif.at| 44 > 3 files changed, 123 insertions(+), 2 deletions(-) Hi, Adrian. As we discussed before on the kernel list, I'm not sure this is a right change to make for a fwe reasons: 1. We do not know the user's intentions. We do not know if they wanted to drop these packets or their goal was to sample them and it just happened to be the last action in the list, because they put it after the output action and not before. 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. 3. If someone is sampling the drops specifically, they know where to find the packets, because they configured the collector. 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. 5. Drop reporting in either datapath implementation is not 100% accurate anyway and requires users to know the kernel internals. Some of that also applies to the next patch in the set. For the patch itself, you should also check that the action set is actually empty (not the action list) after the sample action translation. There is a chance that the clone action is disabled and sample is used in place of a clone, I'm not sure this case is covered. And the formatting in tests seems very inconsistent. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 11/13] ofproto: xlate: Make flow-sampled drops explicit.
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
> >>> ---
> >>> 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.prob
Re: [ovs-dev] [PATCH v1 11/13] ofproto: xlate: Make flow-sampled drops explicit.
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
>>> ---
>>> 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.
>>> +}
>>> +
>>> 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,
>>> b
Re: [ovs-dev] [PATCH v1 11/13] ofproto: xlate: Make flow-sampled drops explicit.
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
> > ---
> > 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
> > --
Re: [ovs-dev] [PATCH v1 11/13] ofproto: xlate: Make flow-sampled drops explicit.
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
> ---
> 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])
Re: [ovs-dev] [PATCH v1 11/13] ofproto: xlate: Make flow-sampled drops explicit.
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)".
>
> Signed-off-by: Adrian Moreno
> ---
> 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.
> */
> 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);
> +}
> +
> 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([ov
[ovs-dev] [PATCH v1 11/13] ofproto: xlate: Make flow-sampled drops explicit.
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)".
Signed-off-by: Adrian Moreno
---
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. */
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);
+}
+
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])
+
+AT_CHECK([
+ovs-appctl netdev-dummy/receive p1
