Re: [ovs-dev] [PATCH net-next v2 7/9] net: openvswitch: do not notify drops inside sample
On 6/19/24 22:40, Adrián Moreno wrote:
> On Wed, Jun 19, 2024 at 08:21:02PM GMT, Ilya Maximets wrote:
>> On 6/19/24 08:35, Adrián Moreno wrote:
>>> On Tue, Jun 18, 2024 at 05:44:05PM GMT, Ilya Maximets wrote:
On 6/18/24 12:50, Adrián Moreno wrote:
> On Tue, Jun 18, 2024 at 12:22:23PM GMT, Ilya Maximets wrote:
>> On 6/18/24 09:00, Adrián Moreno wrote:
>>> On Mon, Jun 17, 2024 at 02:10:37PM GMT, Ilya Maximets wrote:
On 6/17/24 13:55, Ilya Maximets wrote:
> On 6/3/24 20:56, Adrian Moreno wrote:
>> The OVS_ACTION_ATTR_SAMPLE action is, in essence,
>> observability-oriented.
>>
>> Apart from some corner case in which it's used a replacement of
>> clone()
>> for old kernels, it's really only used for sFlow, IPFIX and now,
>> local emit_sample.
>>
>> With this in mind, it doesn't make much sense to report
>> OVS_DROP_LAST_ACTION inside sample actions.
>>
>> For instance, if the flow:
>>
>> actions:sample(..,emit_sample(..)),2
>>
>> triggers a OVS_DROP_LAST_ACTION skb drop event, it would be extremely
>> confusing for users since the packet did reach its destination.
>>
>> This patch makes internal action execution silently consume the skb
>> instead of notifying a drop for this case.
>>
>> Unfortunately, this patch does not remove all potential sources of
>> confusion since, if the sample action itself is the last action, e.g:
>>
>> actions:sample(..,emit_sample(..))
>>
>> we actually _should_ generate a OVS_DROP_LAST_ACTION event, but we
>> aren't.
>>
>> Sadly, this case is difficult to solve without breaking the
>> optimization by which the skb is not cloned on last sample actions.
>> But, given explicit drop actions are now supported, OVS can just add
>> one
>> after the last sample() and rewrite the flow as:
>>
>> actions:sample(..,emit_sample(..)),drop
>>
>> Signed-off-by: Adrian Moreno
>> ---
>> net/openvswitch/actions.c | 13 +++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index 33f6d93ba5e4..54fc1abcff95 100644
>> --- a/net/openvswitch/actions.c
>> +++ b/net/openvswitch/actions.c
>> @@ -82,6 +82,15 @@ static struct action_fifo __percpu *action_fifos;
>> static struct action_flow_keys __percpu *flow_keys;
>> static DEFINE_PER_CPU(int, exec_actions_level);
>>
>> +static inline void ovs_drop_skb_last_action(struct sk_buff *skb)
>> +{
>> +/* Do not emit packet drops inside sample(). */
>> +if (OVS_CB(skb)->probability)
>> +consume_skb(skb);
>> +else
>> +ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
>> +}
>> +
>> /* Make a clone of the 'key', using the pre-allocated percpu
>> 'flow_keys'
>> * space. Return NULL if out of key spaces.
>> */
>> @@ -1061,7 +1070,7 @@ static int sample(struct datapath *dp, struct
>> sk_buff *skb,
>> if ((arg->probability != U32_MAX) &&
>> (!arg->probability || get_random_u32() > arg->probability))
>> {
>> if (last)
>> -ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
>> +ovs_drop_skb_last_action(skb);
Always consuming the skb at this point makes sense, since having
smaple()
as a last action is a reasonable thing to have. But this looks more
like
a fix for the original drop reason patch set.
>>>
>>> I don't think consuming the skb at this point makes sense. It was very
>>> intentionally changed to a drop since a very common use-case for
>>> sampling is drop-sampling, i.e: replacing an empty action list (that
>>> triggers OVS_DROP_LAST_ACTION) with a sample(emit_sample()). Ideally,
>>> that replacement should not have any effect on the number of
>>> OVS_DROP_LAST_ACTION being reported as the packets are being treated in
>>> the same way (only observed in one case).
>>>
>>>
>> return 0;
>> }
>>
>> @@ -1579,7 +1588,7 @@ static int do_execute_actions(struct datapath
>> *dp, struct sk_buff *skb,
>> }
>> }
>>
>> -ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
>> +ovs_drop_skb_last_action(skb);
>
> I don't think I agree with this one. If we have a sample() action
> with
> a lot of different actions inside and we
Re: [ovs-dev] [PATCH net-next v2 7/9] net: openvswitch: do not notify drops inside sample
On Wed, Jun 19, 2024 at 08:21:02PM GMT, Ilya Maximets wrote:
> On 6/19/24 08:35, Adrián Moreno wrote:
> > On Tue, Jun 18, 2024 at 05:44:05PM GMT, Ilya Maximets wrote:
> >> On 6/18/24 12:50, Adrián Moreno wrote:
> >>> On Tue, Jun 18, 2024 at 12:22:23PM GMT, Ilya Maximets wrote:
> On 6/18/24 09:00, Adrián Moreno wrote:
> > On Mon, Jun 17, 2024 at 02:10:37PM GMT, Ilya Maximets wrote:
> >> On 6/17/24 13:55, Ilya Maximets wrote:
> >>> On 6/3/24 20:56, Adrian Moreno wrote:
> The OVS_ACTION_ATTR_SAMPLE action is, in essence,
> observability-oriented.
>
> Apart from some corner case in which it's used a replacement of
> clone()
> for old kernels, it's really only used for sFlow, IPFIX and now,
> local emit_sample.
>
> With this in mind, it doesn't make much sense to report
> OVS_DROP_LAST_ACTION inside sample actions.
>
> For instance, if the flow:
>
> actions:sample(..,emit_sample(..)),2
>
> triggers a OVS_DROP_LAST_ACTION skb drop event, it would be extremely
> confusing for users since the packet did reach its destination.
>
> This patch makes internal action execution silently consume the skb
> instead of notifying a drop for this case.
>
> Unfortunately, this patch does not remove all potential sources of
> confusion since, if the sample action itself is the last action, e.g:
>
> actions:sample(..,emit_sample(..))
>
> we actually _should_ generate a OVS_DROP_LAST_ACTION event, but we
> aren't.
>
> Sadly, this case is difficult to solve without breaking the
> optimization by which the skb is not cloned on last sample actions.
> But, given explicit drop actions are now supported, OVS can just add
> one
> after the last sample() and rewrite the flow as:
>
> actions:sample(..,emit_sample(..)),drop
>
> Signed-off-by: Adrian Moreno
> ---
> net/openvswitch/actions.c | 13 +++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 33f6d93ba5e4..54fc1abcff95 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -82,6 +82,15 @@ static struct action_fifo __percpu *action_fifos;
> static struct action_flow_keys __percpu *flow_keys;
> static DEFINE_PER_CPU(int, exec_actions_level);
>
> +static inline void ovs_drop_skb_last_action(struct sk_buff *skb)
> +{
> +/* Do not emit packet drops inside sample(). */
> +if (OVS_CB(skb)->probability)
> +consume_skb(skb);
> +else
> +ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
> +}
> +
> /* Make a clone of the 'key', using the pre-allocated percpu
> 'flow_keys'
> * space. Return NULL if out of key spaces.
> */
> @@ -1061,7 +1070,7 @@ static int sample(struct datapath *dp, struct
> sk_buff *skb,
> if ((arg->probability != U32_MAX) &&
> (!arg->probability || get_random_u32() > arg->probability))
> {
> if (last)
> -ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
> +ovs_drop_skb_last_action(skb);
> >>
> >> Always consuming the skb at this point makes sense, since having
> >> smaple()
> >> as a last action is a reasonable thing to have. But this looks more
> >> like
> >> a fix for the original drop reason patch set.
> >>
> >
> > I don't think consuming the skb at this point makes sense. It was very
> > intentionally changed to a drop since a very common use-case for
> > sampling is drop-sampling, i.e: replacing an empty action list (that
> > triggers OVS_DROP_LAST_ACTION) with a sample(emit_sample()). Ideally,
> > that replacement should not have any effect on the number of
> > OVS_DROP_LAST_ACTION being reported as the packets are being treated in
> > the same way (only observed in one case).
> >
> >
> return 0;
> }
>
> @@ -1579,7 +1588,7 @@ static int do_execute_actions(struct datapath
> *dp, struct sk_buff *skb,
> }
> }
>
> -ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
> +ovs_drop_skb_last_action(skb);
> >>>
> >>> I don't think I agree with this one. If we have a sample() action
> >>> with
> >>> a lot of different actions inside and we reached the end while the
> >>> last
>
Re: [ovs-dev] [PATCH net-next v2 7/9] net: openvswitch: do not notify drops inside sample
On 6/19/24 08:35, Adrián Moreno wrote:
> On Tue, Jun 18, 2024 at 05:44:05PM GMT, Ilya Maximets wrote:
>> On 6/18/24 12:50, Adrián Moreno wrote:
>>> On Tue, Jun 18, 2024 at 12:22:23PM GMT, Ilya Maximets wrote:
On 6/18/24 09:00, Adrián Moreno wrote:
> On Mon, Jun 17, 2024 at 02:10:37PM GMT, Ilya Maximets wrote:
>> On 6/17/24 13:55, Ilya Maximets wrote:
>>> On 6/3/24 20:56, Adrian Moreno wrote:
The OVS_ACTION_ATTR_SAMPLE action is, in essence,
observability-oriented.
Apart from some corner case in which it's used a replacement of clone()
for old kernels, it's really only used for sFlow, IPFIX and now,
local emit_sample.
With this in mind, it doesn't make much sense to report
OVS_DROP_LAST_ACTION inside sample actions.
For instance, if the flow:
actions:sample(..,emit_sample(..)),2
triggers a OVS_DROP_LAST_ACTION skb drop event, it would be extremely
confusing for users since the packet did reach its destination.
This patch makes internal action execution silently consume the skb
instead of notifying a drop for this case.
Unfortunately, this patch does not remove all potential sources of
confusion since, if the sample action itself is the last action, e.g:
actions:sample(..,emit_sample(..))
we actually _should_ generate a OVS_DROP_LAST_ACTION event, but we
aren't.
Sadly, this case is difficult to solve without breaking the
optimization by which the skb is not cloned on last sample actions.
But, given explicit drop actions are now supported, OVS can just add
one
after the last sample() and rewrite the flow as:
actions:sample(..,emit_sample(..)),drop
Signed-off-by: Adrian Moreno
---
net/openvswitch/actions.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 33f6d93ba5e4..54fc1abcff95 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -82,6 +82,15 @@ static struct action_fifo __percpu *action_fifos;
static struct action_flow_keys __percpu *flow_keys;
static DEFINE_PER_CPU(int, exec_actions_level);
+static inline void ovs_drop_skb_last_action(struct sk_buff *skb)
+{
+ /* Do not emit packet drops inside sample(). */
+ if (OVS_CB(skb)->probability)
+ consume_skb(skb);
+ else
+ ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
+}
+
/* Make a clone of the 'key', using the pre-allocated percpu
'flow_keys'
* space. Return NULL if out of key spaces.
*/
@@ -1061,7 +1070,7 @@ static int sample(struct datapath *dp, struct
sk_buff *skb,
if ((arg->probability != U32_MAX) &&
(!arg->probability || get_random_u32() > arg->probability))
{
if (last)
- ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
+ ovs_drop_skb_last_action(skb);
>>
>> Always consuming the skb at this point makes sense, since having smaple()
>> as a last action is a reasonable thing to have. But this looks more like
>> a fix for the original drop reason patch set.
>>
>
> I don't think consuming the skb at this point makes sense. It was very
> intentionally changed to a drop since a very common use-case for
> sampling is drop-sampling, i.e: replacing an empty action list (that
> triggers OVS_DROP_LAST_ACTION) with a sample(emit_sample()). Ideally,
> that replacement should not have any effect on the number of
> OVS_DROP_LAST_ACTION being reported as the packets are being treated in
> the same way (only observed in one case).
>
>
return 0;
}
@@ -1579,7 +1588,7 @@ static int do_execute_actions(struct datapath
*dp, struct sk_buff *skb,
}
}
- ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
+ ovs_drop_skb_last_action(skb);
>>>
>>> I don't think I agree with this one. If we have a sample() action with
>>> a lot of different actions inside and we reached the end while the last
>>> action didn't consume the skb, then we should report that. E.g.
>>> "sample(emit_sample(),push_vlan(),set(eth())),2" should report that the
>>> cloned skb was dropped. "sample(push_vlan(),emit_sample())" should not.
>>>
>
> What is the use case for such action list?
Re: [ovs-dev] [PATCH net-next v2 7/9] net: openvswitch: do not notify drops inside sample
On Tue, Jun 18, 2024 at 05:44:05PM GMT, Ilya Maximets wrote:
> On 6/18/24 12:50, Adrián Moreno wrote:
> > On Tue, Jun 18, 2024 at 12:22:23PM GMT, Ilya Maximets wrote:
> >> On 6/18/24 09:00, Adrián Moreno wrote:
> >>> On Mon, Jun 17, 2024 at 02:10:37PM GMT, Ilya Maximets wrote:
> On 6/17/24 13:55, Ilya Maximets wrote:
> > On 6/3/24 20:56, Adrian Moreno wrote:
> >> The OVS_ACTION_ATTR_SAMPLE action is, in essence,
> >> observability-oriented.
> >>
> >> Apart from some corner case in which it's used a replacement of clone()
> >> for old kernels, it's really only used for sFlow, IPFIX and now,
> >> local emit_sample.
> >>
> >> With this in mind, it doesn't make much sense to report
> >> OVS_DROP_LAST_ACTION inside sample actions.
> >>
> >> For instance, if the flow:
> >>
> >> actions:sample(..,emit_sample(..)),2
> >>
> >> triggers a OVS_DROP_LAST_ACTION skb drop event, it would be extremely
> >> confusing for users since the packet did reach its destination.
> >>
> >> This patch makes internal action execution silently consume the skb
> >> instead of notifying a drop for this case.
> >>
> >> Unfortunately, this patch does not remove all potential sources of
> >> confusion since, if the sample action itself is the last action, e.g:
> >>
> >> actions:sample(..,emit_sample(..))
> >>
> >> we actually _should_ generate a OVS_DROP_LAST_ACTION event, but we
> >> aren't.
> >>
> >> Sadly, this case is difficult to solve without breaking the
> >> optimization by which the skb is not cloned on last sample actions.
> >> But, given explicit drop actions are now supported, OVS can just add
> >> one
> >> after the last sample() and rewrite the flow as:
> >>
> >> actions:sample(..,emit_sample(..)),drop
> >>
> >> Signed-off-by: Adrian Moreno
> >> ---
> >> net/openvswitch/actions.c | 13 +++--
> >> 1 file changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> >> index 33f6d93ba5e4..54fc1abcff95 100644
> >> --- a/net/openvswitch/actions.c
> >> +++ b/net/openvswitch/actions.c
> >> @@ -82,6 +82,15 @@ static struct action_fifo __percpu *action_fifos;
> >> static struct action_flow_keys __percpu *flow_keys;
> >> static DEFINE_PER_CPU(int, exec_actions_level);
> >>
> >> +static inline void ovs_drop_skb_last_action(struct sk_buff *skb)
> >> +{
> >> + /* Do not emit packet drops inside sample(). */
> >> + if (OVS_CB(skb)->probability)
> >> + consume_skb(skb);
> >> + else
> >> + ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
> >> +}
> >> +
> >> /* Make a clone of the 'key', using the pre-allocated percpu
> >> 'flow_keys'
> >> * space. Return NULL if out of key spaces.
> >> */
> >> @@ -1061,7 +1070,7 @@ static int sample(struct datapath *dp, struct
> >> sk_buff *skb,
> >>if ((arg->probability != U32_MAX) &&
> >>(!arg->probability || get_random_u32() > arg->probability))
> >> {
> >>if (last)
> >> - ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
> >> + ovs_drop_skb_last_action(skb);
>
> Always consuming the skb at this point makes sense, since having smaple()
> as a last action is a reasonable thing to have. But this looks more like
> a fix for the original drop reason patch set.
>
> >>>
> >>> I don't think consuming the skb at this point makes sense. It was very
> >>> intentionally changed to a drop since a very common use-case for
> >>> sampling is drop-sampling, i.e: replacing an empty action list (that
> >>> triggers OVS_DROP_LAST_ACTION) with a sample(emit_sample()). Ideally,
> >>> that replacement should not have any effect on the number of
> >>> OVS_DROP_LAST_ACTION being reported as the packets are being treated in
> >>> the same way (only observed in one case).
> >>>
> >>>
> >>return 0;
> >>}
> >>
> >> @@ -1579,7 +1588,7 @@ static int do_execute_actions(struct datapath
> >> *dp, struct sk_buff *skb,
> >>}
> >>}
> >>
> >> - ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
> >> + ovs_drop_skb_last_action(skb);
> >
> > I don't think I agree with this one. If we have a sample() action with
> > a lot of different actions inside and we reached the end while the last
> > action didn't consume the skb, then we should report that. E.g.
> > "sample(emit_sample(),push_vlan(),set(eth())),2" should report that the
> > cloned skb was dropped. "sample(push_vlan(),emit_sample())" should not.
> >
> >>>
> >>> What is the use case for such action list? Having an action branch
> >>> executed
Re: [ovs-dev] [PATCH net-next v2 7/9] net: openvswitch: do not notify drops inside sample
On 6/18/24 12:50, Adrián Moreno wrote:
> On Tue, Jun 18, 2024 at 12:22:23PM GMT, Ilya Maximets wrote:
>> On 6/18/24 09:00, Adrián Moreno wrote:
>>> On Mon, Jun 17, 2024 at 02:10:37PM GMT, Ilya Maximets wrote:
On 6/17/24 13:55, Ilya Maximets wrote:
> On 6/3/24 20:56, Adrian Moreno wrote:
>> The OVS_ACTION_ATTR_SAMPLE action is, in essence,
>> observability-oriented.
>>
>> Apart from some corner case in which it's used a replacement of clone()
>> for old kernels, it's really only used for sFlow, IPFIX and now,
>> local emit_sample.
>>
>> With this in mind, it doesn't make much sense to report
>> OVS_DROP_LAST_ACTION inside sample actions.
>>
>> For instance, if the flow:
>>
>> actions:sample(..,emit_sample(..)),2
>>
>> triggers a OVS_DROP_LAST_ACTION skb drop event, it would be extremely
>> confusing for users since the packet did reach its destination.
>>
>> This patch makes internal action execution silently consume the skb
>> instead of notifying a drop for this case.
>>
>> Unfortunately, this patch does not remove all potential sources of
>> confusion since, if the sample action itself is the last action, e.g:
>>
>> actions:sample(..,emit_sample(..))
>>
>> we actually _should_ generate a OVS_DROP_LAST_ACTION event, but we
>> aren't.
>>
>> Sadly, this case is difficult to solve without breaking the
>> optimization by which the skb is not cloned on last sample actions.
>> But, given explicit drop actions are now supported, OVS can just add one
>> after the last sample() and rewrite the flow as:
>>
>> actions:sample(..,emit_sample(..)),drop
>>
>> Signed-off-by: Adrian Moreno
>> ---
>> net/openvswitch/actions.c | 13 +++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index 33f6d93ba5e4..54fc1abcff95 100644
>> --- a/net/openvswitch/actions.c
>> +++ b/net/openvswitch/actions.c
>> @@ -82,6 +82,15 @@ static struct action_fifo __percpu *action_fifos;
>> static struct action_flow_keys __percpu *flow_keys;
>> static DEFINE_PER_CPU(int, exec_actions_level);
>>
>> +static inline void ovs_drop_skb_last_action(struct sk_buff *skb)
>> +{
>> +/* Do not emit packet drops inside sample(). */
>> +if (OVS_CB(skb)->probability)
>> +consume_skb(skb);
>> +else
>> +ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
>> +}
>> +
>> /* Make a clone of the 'key', using the pre-allocated percpu 'flow_keys'
>> * space. Return NULL if out of key spaces.
>> */
>> @@ -1061,7 +1070,7 @@ static int sample(struct datapath *dp, struct
>> sk_buff *skb,
>> if ((arg->probability != U32_MAX) &&
>> (!arg->probability || get_random_u32() > arg->probability))
>> {
>> if (last)
>> -ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
>> +ovs_drop_skb_last_action(skb);
Always consuming the skb at this point makes sense, since having smaple()
as a last action is a reasonable thing to have. But this looks more like
a fix for the original drop reason patch set.
>>>
>>> I don't think consuming the skb at this point makes sense. It was very
>>> intentionally changed to a drop since a very common use-case for
>>> sampling is drop-sampling, i.e: replacing an empty action list (that
>>> triggers OVS_DROP_LAST_ACTION) with a sample(emit_sample()). Ideally,
>>> that replacement should not have any effect on the number of
>>> OVS_DROP_LAST_ACTION being reported as the packets are being treated in
>>> the same way (only observed in one case).
>>>
>>>
>> return 0;
>> }
>>
>> @@ -1579,7 +1588,7 @@ static int do_execute_actions(struct datapath *dp,
>> struct sk_buff *skb,
>> }
>> }
>>
>> -ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
>> +ovs_drop_skb_last_action(skb);
>
> I don't think I agree with this one. If we have a sample() action with
> a lot of different actions inside and we reached the end while the last
> action didn't consume the skb, then we should report that. E.g.
> "sample(emit_sample(),push_vlan(),set(eth())),2" should report that the
> cloned skb was dropped. "sample(push_vlan(),emit_sample())" should not.
>
>>>
>>> What is the use case for such action list? Having an action branch
>>> executed randomly doesn't make sense to me if it's not some
>>> observability thing (which IMHO should not trigger drops).
>>
>> It is exactly my point. A list of actions that doesn't end is some sort
>> of a terminal action (output, drop, etc) does not make a lot of sense and
Re: [ovs-dev] [PATCH net-next v2 7/9] net: openvswitch: do not notify drops inside sample
On Tue, Jun 18, 2024 at 12:22:23PM GMT, Ilya Maximets wrote:
> On 6/18/24 09:00, Adrián Moreno wrote:
> > On Mon, Jun 17, 2024 at 02:10:37PM GMT, Ilya Maximets wrote:
> >> On 6/17/24 13:55, Ilya Maximets wrote:
> >>> On 6/3/24 20:56, Adrian Moreno wrote:
> The OVS_ACTION_ATTR_SAMPLE action is, in essence,
> observability-oriented.
>
> Apart from some corner case in which it's used a replacement of clone()
> for old kernels, it's really only used for sFlow, IPFIX and now,
> local emit_sample.
>
> With this in mind, it doesn't make much sense to report
> OVS_DROP_LAST_ACTION inside sample actions.
>
> For instance, if the flow:
>
> actions:sample(..,emit_sample(..)),2
>
> triggers a OVS_DROP_LAST_ACTION skb drop event, it would be extremely
> confusing for users since the packet did reach its destination.
>
> This patch makes internal action execution silently consume the skb
> instead of notifying a drop for this case.
>
> Unfortunately, this patch does not remove all potential sources of
> confusion since, if the sample action itself is the last action, e.g:
>
> actions:sample(..,emit_sample(..))
>
> we actually _should_ generate a OVS_DROP_LAST_ACTION event, but we
> aren't.
>
> Sadly, this case is difficult to solve without breaking the
> optimization by which the skb is not cloned on last sample actions.
> But, given explicit drop actions are now supported, OVS can just add one
> after the last sample() and rewrite the flow as:
>
> actions:sample(..,emit_sample(..)),drop
>
> Signed-off-by: Adrian Moreno
> ---
> net/openvswitch/actions.c | 13 +++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 33f6d93ba5e4..54fc1abcff95 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -82,6 +82,15 @@ static struct action_fifo __percpu *action_fifos;
> static struct action_flow_keys __percpu *flow_keys;
> static DEFINE_PER_CPU(int, exec_actions_level);
>
> +static inline void ovs_drop_skb_last_action(struct sk_buff *skb)
> +{
> +/* Do not emit packet drops inside sample(). */
> +if (OVS_CB(skb)->probability)
> +consume_skb(skb);
> +else
> +ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
> +}
> +
> /* Make a clone of the 'key', using the pre-allocated percpu 'flow_keys'
> * space. Return NULL if out of key spaces.
> */
> @@ -1061,7 +1070,7 @@ static int sample(struct datapath *dp, struct
> sk_buff *skb,
> if ((arg->probability != U32_MAX) &&
> (!arg->probability || get_random_u32() > arg->probability))
> {
> if (last)
> -ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
> +ovs_drop_skb_last_action(skb);
> >>
> >> Always consuming the skb at this point makes sense, since having smaple()
> >> as a last action is a reasonable thing to have. But this looks more like
> >> a fix for the original drop reason patch set.
> >>
> >
> > I don't think consuming the skb at this point makes sense. It was very
> > intentionally changed to a drop since a very common use-case for
> > sampling is drop-sampling, i.e: replacing an empty action list (that
> > triggers OVS_DROP_LAST_ACTION) with a sample(emit_sample()). Ideally,
> > that replacement should not have any effect on the number of
> > OVS_DROP_LAST_ACTION being reported as the packets are being treated in
> > the same way (only observed in one case).
> >
> >
> return 0;
> }
>
> @@ -1579,7 +1588,7 @@ static int do_execute_actions(struct datapath *dp,
> struct sk_buff *skb,
> }
> }
>
> -ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
> +ovs_drop_skb_last_action(skb);
> >>>
> >>> I don't think I agree with this one. If we have a sample() action with
> >>> a lot of different actions inside and we reached the end while the last
> >>> action didn't consume the skb, then we should report that. E.g.
> >>> "sample(emit_sample(),push_vlan(),set(eth())),2" should report that the
> >>> cloned skb was dropped. "sample(push_vlan(),emit_sample())" should not.
> >>>
> >
> > What is the use case for such action list? Having an action branch
> > executed randomly doesn't make sense to me if it's not some
> > observability thing (which IMHO should not trigger drops).
>
> It is exactly my point. A list of actions that doesn't end is some sort
> of a terminal action (output, drop, etc) does not make a lot of sense and
> hence should be signaled as an unexpected
Re: [ovs-dev] [PATCH net-next v2 7/9] net: openvswitch: do not notify drops inside sample
On 6/18/24 09:00, Adrián Moreno wrote:
> On Mon, Jun 17, 2024 at 02:10:37PM GMT, Ilya Maximets wrote:
>> On 6/17/24 13:55, Ilya Maximets wrote:
>>> On 6/3/24 20:56, Adrian Moreno wrote:
The OVS_ACTION_ATTR_SAMPLE action is, in essence,
observability-oriented.
Apart from some corner case in which it's used a replacement of clone()
for old kernels, it's really only used for sFlow, IPFIX and now,
local emit_sample.
With this in mind, it doesn't make much sense to report
OVS_DROP_LAST_ACTION inside sample actions.
For instance, if the flow:
actions:sample(..,emit_sample(..)),2
triggers a OVS_DROP_LAST_ACTION skb drop event, it would be extremely
confusing for users since the packet did reach its destination.
This patch makes internal action execution silently consume the skb
instead of notifying a drop for this case.
Unfortunately, this patch does not remove all potential sources of
confusion since, if the sample action itself is the last action, e.g:
actions:sample(..,emit_sample(..))
we actually _should_ generate a OVS_DROP_LAST_ACTION event, but we aren't.
Sadly, this case is difficult to solve without breaking the
optimization by which the skb is not cloned on last sample actions.
But, given explicit drop actions are now supported, OVS can just add one
after the last sample() and rewrite the flow as:
actions:sample(..,emit_sample(..)),drop
Signed-off-by: Adrian Moreno
---
net/openvswitch/actions.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 33f6d93ba5e4..54fc1abcff95 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -82,6 +82,15 @@ static struct action_fifo __percpu *action_fifos;
static struct action_flow_keys __percpu *flow_keys;
static DEFINE_PER_CPU(int, exec_actions_level);
+static inline void ovs_drop_skb_last_action(struct sk_buff *skb)
+{
+ /* Do not emit packet drops inside sample(). */
+ if (OVS_CB(skb)->probability)
+ consume_skb(skb);
+ else
+ ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
+}
+
/* Make a clone of the 'key', using the pre-allocated percpu 'flow_keys'
* space. Return NULL if out of key spaces.
*/
@@ -1061,7 +1070,7 @@ static int sample(struct datapath *dp, struct
sk_buff *skb,
if ((arg->probability != U32_MAX) &&
(!arg->probability || get_random_u32() > arg->probability)) {
if (last)
- ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
+ ovs_drop_skb_last_action(skb);
>>
>> Always consuming the skb at this point makes sense, since having smaple()
>> as a last action is a reasonable thing to have. But this looks more like
>> a fix for the original drop reason patch set.
>>
>
> I don't think consuming the skb at this point makes sense. It was very
> intentionally changed to a drop since a very common use-case for
> sampling is drop-sampling, i.e: replacing an empty action list (that
> triggers OVS_DROP_LAST_ACTION) with a sample(emit_sample()). Ideally,
> that replacement should not have any effect on the number of
> OVS_DROP_LAST_ACTION being reported as the packets are being treated in
> the same way (only observed in one case).
>
>
return 0;
}
@@ -1579,7 +1588,7 @@ static int do_execute_actions(struct datapath *dp,
struct sk_buff *skb,
}
}
- ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
+ ovs_drop_skb_last_action(skb);
>>>
>>> I don't think I agree with this one. If we have a sample() action with
>>> a lot of different actions inside and we reached the end while the last
>>> action didn't consume the skb, then we should report that. E.g.
>>> "sample(emit_sample(),push_vlan(),set(eth())),2" should report that the
>>> cloned skb was dropped. "sample(push_vlan(),emit_sample())" should not.
>>>
>
> What is the use case for such action list? Having an action branch
> executed randomly doesn't make sense to me if it's not some
> observability thing (which IMHO should not trigger drops).
It is exactly my point. A list of actions that doesn't end is some sort
of a terminal action (output, drop, etc) does not make a lot of sense and
hence should be signaled as an unexpected drop, so users can re-check the
pipeline in case they missed the terminal action somehow.
>
>>> The only actions that are actually consuming the skb are "output",
>>> "userspace", "recirc" and now "emit_sample". "output" and "recirc" are
>>> consuming the skb "naturally" by stealing it when it is the last action.
>>> "userspace" has an explicit check to consume the skb if it is the
Re: [ovs-dev] [PATCH net-next v2 7/9] net: openvswitch: do not notify drops inside sample
On Mon, Jun 17, 2024 at 02:10:37PM GMT, Ilya Maximets wrote:
> On 6/17/24 13:55, Ilya Maximets wrote:
> > On 6/3/24 20:56, Adrian Moreno wrote:
> >> The OVS_ACTION_ATTR_SAMPLE action is, in essence,
> >> observability-oriented.
> >>
> >> Apart from some corner case in which it's used a replacement of clone()
> >> for old kernels, it's really only used for sFlow, IPFIX and now,
> >> local emit_sample.
> >>
> >> With this in mind, it doesn't make much sense to report
> >> OVS_DROP_LAST_ACTION inside sample actions.
> >>
> >> For instance, if the flow:
> >>
> >> actions:sample(..,emit_sample(..)),2
> >>
> >> triggers a OVS_DROP_LAST_ACTION skb drop event, it would be extremely
> >> confusing for users since the packet did reach its destination.
> >>
> >> This patch makes internal action execution silently consume the skb
> >> instead of notifying a drop for this case.
> >>
> >> Unfortunately, this patch does not remove all potential sources of
> >> confusion since, if the sample action itself is the last action, e.g:
> >>
> >> actions:sample(..,emit_sample(..))
> >>
> >> we actually _should_ generate a OVS_DROP_LAST_ACTION event, but we aren't.
> >>
> >> Sadly, this case is difficult to solve without breaking the
> >> optimization by which the skb is not cloned on last sample actions.
> >> But, given explicit drop actions are now supported, OVS can just add one
> >> after the last sample() and rewrite the flow as:
> >>
> >> actions:sample(..,emit_sample(..)),drop
> >>
> >> Signed-off-by: Adrian Moreno
> >> ---
> >> net/openvswitch/actions.c | 13 +++--
> >> 1 file changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> >> index 33f6d93ba5e4..54fc1abcff95 100644
> >> --- a/net/openvswitch/actions.c
> >> +++ b/net/openvswitch/actions.c
> >> @@ -82,6 +82,15 @@ static struct action_fifo __percpu *action_fifos;
> >> static struct action_flow_keys __percpu *flow_keys;
> >> static DEFINE_PER_CPU(int, exec_actions_level);
> >>
> >> +static inline void ovs_drop_skb_last_action(struct sk_buff *skb)
> >> +{
> >> + /* Do not emit packet drops inside sample(). */
> >> + if (OVS_CB(skb)->probability)
> >> + consume_skb(skb);
> >> + else
> >> + ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
> >> +}
> >> +
> >> /* Make a clone of the 'key', using the pre-allocated percpu 'flow_keys'
> >> * space. Return NULL if out of key spaces.
> >> */
> >> @@ -1061,7 +1070,7 @@ static int sample(struct datapath *dp, struct
> >> sk_buff *skb,
> >>if ((arg->probability != U32_MAX) &&
> >>(!arg->probability || get_random_u32() > arg->probability)) {
> >>if (last)
> >> - ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
> >> + ovs_drop_skb_last_action(skb);
>
> Always consuming the skb at this point makes sense, since having smaple()
> as a last action is a reasonable thing to have. But this looks more like
> a fix for the original drop reason patch set.
>
I don't think consuming the skb at this point makes sense. It was very
intentionally changed to a drop since a very common use-case for
sampling is drop-sampling, i.e: replacing an empty action list (that
triggers OVS_DROP_LAST_ACTION) with a sample(emit_sample()). Ideally,
that replacement should not have any effect on the number of
OVS_DROP_LAST_ACTION being reported as the packets are being treated in
the same way (only observed in one case).
> >>return 0;
> >>}
> >>
> >> @@ -1579,7 +1588,7 @@ static int do_execute_actions(struct datapath *dp,
> >> struct sk_buff *skb,
> >>}
> >>}
> >>
> >> - ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
> >> + ovs_drop_skb_last_action(skb);
> >
> > I don't think I agree with this one. If we have a sample() action with
> > a lot of different actions inside and we reached the end while the last
> > action didn't consume the skb, then we should report that. E.g.
> > "sample(emit_sample(),push_vlan(),set(eth())),2" should report that the
> > cloned skb was dropped. "sample(push_vlan(),emit_sample())" should not.
> >
What is the use case for such action list? Having an action branch
executed randomly doesn't make sense to me if it's not some
observability thing (which IMHO should not trigger drops).
> > The only actions that are actually consuming the skb are "output",
> > "userspace", "recirc" and now "emit_sample". "output" and "recirc" are
> > consuming the skb "naturally" by stealing it when it is the last action.
> > "userspace" has an explicit check to consume the skb if it is the last
> > action. "emit_sample" should have the similar check. It should likely
> > be added at the point of action introduction instead of having a separate
> > patch.
> >
Unlinke "output", "recirc", "userspace", etc. with emit_sample the
packet does not continue it's way through the datapath.
It would be very confusing if OVS starts monitoring drops
Re: [ovs-dev] [PATCH net-next v2 7/9] net: openvswitch: do not notify drops inside sample
On 6/17/24 13:55, Ilya Maximets wrote:
> On 6/3/24 20:56, Adrian Moreno wrote:
>> The OVS_ACTION_ATTR_SAMPLE action is, in essence,
>> observability-oriented.
>>
>> Apart from some corner case in which it's used a replacement of clone()
>> for old kernels, it's really only used for sFlow, IPFIX and now,
>> local emit_sample.
>>
>> With this in mind, it doesn't make much sense to report
>> OVS_DROP_LAST_ACTION inside sample actions.
>>
>> For instance, if the flow:
>>
>> actions:sample(..,emit_sample(..)),2
>>
>> triggers a OVS_DROP_LAST_ACTION skb drop event, it would be extremely
>> confusing for users since the packet did reach its destination.
>>
>> This patch makes internal action execution silently consume the skb
>> instead of notifying a drop for this case.
>>
>> Unfortunately, this patch does not remove all potential sources of
>> confusion since, if the sample action itself is the last action, e.g:
>>
>> actions:sample(..,emit_sample(..))
>>
>> we actually _should_ generate a OVS_DROP_LAST_ACTION event, but we aren't.
>>
>> Sadly, this case is difficult to solve without breaking the
>> optimization by which the skb is not cloned on last sample actions.
>> But, given explicit drop actions are now supported, OVS can just add one
>> after the last sample() and rewrite the flow as:
>>
>> actions:sample(..,emit_sample(..)),drop
>>
>> Signed-off-by: Adrian Moreno
>> ---
>> net/openvswitch/actions.c | 13 +++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index 33f6d93ba5e4..54fc1abcff95 100644
>> --- a/net/openvswitch/actions.c
>> +++ b/net/openvswitch/actions.c
>> @@ -82,6 +82,15 @@ static struct action_fifo __percpu *action_fifos;
>> static struct action_flow_keys __percpu *flow_keys;
>> static DEFINE_PER_CPU(int, exec_actions_level);
>>
>> +static inline void ovs_drop_skb_last_action(struct sk_buff *skb)
>> +{
>> +/* Do not emit packet drops inside sample(). */
>> +if (OVS_CB(skb)->probability)
>> +consume_skb(skb);
>> +else
>> +ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
>> +}
>> +
>> /* Make a clone of the 'key', using the pre-allocated percpu 'flow_keys'
>> * space. Return NULL if out of key spaces.
>> */
>> @@ -1061,7 +1070,7 @@ static int sample(struct datapath *dp, struct sk_buff
>> *skb,
>> if ((arg->probability != U32_MAX) &&
>> (!arg->probability || get_random_u32() > arg->probability)) {
>> if (last)
>> -ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
>> +ovs_drop_skb_last_action(skb);
Always consuming the skb at this point makes sense, since having smaple()
as a last action is a reasonable thing to have. But this looks more like
a fix for the original drop reason patch set.
>> return 0;
>> }
>>
>> @@ -1579,7 +1588,7 @@ static int do_execute_actions(struct datapath *dp,
>> struct sk_buff *skb,
>> }
>> }
>>
>> -ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
>> +ovs_drop_skb_last_action(skb);
>
> I don't think I agree with this one. If we have a sample() action with
> a lot of different actions inside and we reached the end while the last
> action didn't consume the skb, then we should report that. E.g.
> "sample(emit_sample(),push_vlan(),set(eth())),2" should report that the
> cloned skb was dropped. "sample(push_vlan(),emit_sample())" should not.
>
> The only actions that are actually consuming the skb are "output",
> "userspace", "recirc" and now "emit_sample". "output" and "recirc" are
> consuming the skb "naturally" by stealing it when it is the last action.
> "userspace" has an explicit check to consume the skb if it is the last
> action. "emit_sample" should have the similar check. It should likely
> be added at the point of action introduction instead of having a separate
> patch.
>
> Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v2 7/9] net: openvswitch: do not notify drops inside sample
On 6/3/24 20:56, Adrian Moreno wrote:
> The OVS_ACTION_ATTR_SAMPLE action is, in essence,
> observability-oriented.
>
> Apart from some corner case in which it's used a replacement of clone()
> for old kernels, it's really only used for sFlow, IPFIX and now,
> local emit_sample.
>
> With this in mind, it doesn't make much sense to report
> OVS_DROP_LAST_ACTION inside sample actions.
>
> For instance, if the flow:
>
> actions:sample(..,emit_sample(..)),2
>
> triggers a OVS_DROP_LAST_ACTION skb drop event, it would be extremely
> confusing for users since the packet did reach its destination.
>
> This patch makes internal action execution silently consume the skb
> instead of notifying a drop for this case.
>
> Unfortunately, this patch does not remove all potential sources of
> confusion since, if the sample action itself is the last action, e.g:
>
> actions:sample(..,emit_sample(..))
>
> we actually _should_ generate a OVS_DROP_LAST_ACTION event, but we aren't.
>
> Sadly, this case is difficult to solve without breaking the
> optimization by which the skb is not cloned on last sample actions.
> But, given explicit drop actions are now supported, OVS can just add one
> after the last sample() and rewrite the flow as:
>
> actions:sample(..,emit_sample(..)),drop
>
> Signed-off-by: Adrian Moreno
> ---
> net/openvswitch/actions.c | 13 +++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 33f6d93ba5e4..54fc1abcff95 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -82,6 +82,15 @@ static struct action_fifo __percpu *action_fifos;
> static struct action_flow_keys __percpu *flow_keys;
> static DEFINE_PER_CPU(int, exec_actions_level);
>
> +static inline void ovs_drop_skb_last_action(struct sk_buff *skb)
> +{
> + /* Do not emit packet drops inside sample(). */
> + if (OVS_CB(skb)->probability)
> + consume_skb(skb);
> + else
> + ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
> +}
> +
> /* Make a clone of the 'key', using the pre-allocated percpu 'flow_keys'
> * space. Return NULL if out of key spaces.
> */
> @@ -1061,7 +1070,7 @@ static int sample(struct datapath *dp, struct sk_buff
> *skb,
> if ((arg->probability != U32_MAX) &&
> (!arg->probability || get_random_u32() > arg->probability)) {
> if (last)
> - ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
> + ovs_drop_skb_last_action(skb);
> return 0;
> }
>
> @@ -1579,7 +1588,7 @@ static int do_execute_actions(struct datapath *dp,
> struct sk_buff *skb,
> }
> }
>
> - ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
> + ovs_drop_skb_last_action(skb);
I don't think I agree with this one. If we have a sample() action with
a lot of different actions inside and we reached the end while the last
action didn't consume the skb, then we should report that. E.g.
"sample(emit_sample(),push_vlan(),set(eth())),2" should report that the
cloned skb was dropped. "sample(push_vlan(),emit_sample())" should not.
The only actions that are actually consuming the skb are "output",
"userspace", "recirc" and now "emit_sample". "output" and "recirc" are
consuming the skb "naturally" by stealing it when it is the last action.
"userspace" has an explicit check to consume the skb if it is the last
action. "emit_sample" should have the similar check. It should likely
be added at the point of action introduction instead of having a separate
patch.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next v2 7/9] net: openvswitch: do not notify drops inside sample
On Mon, Jun 03, 2024 at 08:56:41PM +0200, Adrian Moreno wrote: > The OVS_ACTION_ATTR_SAMPLE action is, in essence, > observability-oriented. > > Apart from some corner case in which it's used a replacement of clone() > for old kernels, it's really only used for sFlow, IPFIX and now, > local emit_sample. > > With this in mind, it doesn't make much sense to report > OVS_DROP_LAST_ACTION inside sample actions. > > For instance, if the flow: > > actions:sample(..,emit_sample(..)),2 > > triggers a OVS_DROP_LAST_ACTION skb drop event, it would be extremely > confusing for users since the packet did reach its destination. > > This patch makes internal action execution silently consume the skb > instead of notifying a drop for this case. > > Unfortunately, this patch does not remove all potential sources of > confusion since, if the sample action itself is the last action, e.g: > > actions:sample(..,emit_sample(..)) > > we actually _should_ generate a OVS_DROP_LAST_ACTION event, but we aren't. > > Sadly, this case is difficult to solve without breaking the > optimization by which the skb is not cloned on last sample actions. > But, given explicit drop actions are now supported, OVS can just add one > after the last sample() and rewrite the flow as: > > actions:sample(..,emit_sample(..)),drop > > Signed-off-by: Adrian Moreno Reviewed-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net-next v2 7/9] net: openvswitch: do not notify drops inside sample
The OVS_ACTION_ATTR_SAMPLE action is, in essence,
observability-oriented.
Apart from some corner case in which it's used a replacement of clone()
for old kernels, it's really only used for sFlow, IPFIX and now,
local emit_sample.
With this in mind, it doesn't make much sense to report
OVS_DROP_LAST_ACTION inside sample actions.
For instance, if the flow:
actions:sample(..,emit_sample(..)),2
triggers a OVS_DROP_LAST_ACTION skb drop event, it would be extremely
confusing for users since the packet did reach its destination.
This patch makes internal action execution silently consume the skb
instead of notifying a drop for this case.
Unfortunately, this patch does not remove all potential sources of
confusion since, if the sample action itself is the last action, e.g:
actions:sample(..,emit_sample(..))
we actually _should_ generate a OVS_DROP_LAST_ACTION event, but we aren't.
Sadly, this case is difficult to solve without breaking the
optimization by which the skb is not cloned on last sample actions.
But, given explicit drop actions are now supported, OVS can just add one
after the last sample() and rewrite the flow as:
actions:sample(..,emit_sample(..)),drop
Signed-off-by: Adrian Moreno
---
net/openvswitch/actions.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 33f6d93ba5e4..54fc1abcff95 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -82,6 +82,15 @@ static struct action_fifo __percpu *action_fifos;
static struct action_flow_keys __percpu *flow_keys;
static DEFINE_PER_CPU(int, exec_actions_level);
+static inline void ovs_drop_skb_last_action(struct sk_buff *skb)
+{
+ /* Do not emit packet drops inside sample(). */
+ if (OVS_CB(skb)->probability)
+ consume_skb(skb);
+ else
+ ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
+}
+
/* Make a clone of the 'key', using the pre-allocated percpu 'flow_keys'
* space. Return NULL if out of key spaces.
*/
@@ -1061,7 +1070,7 @@ static int sample(struct datapath *dp, struct sk_buff
*skb,
if ((arg->probability != U32_MAX) &&
(!arg->probability || get_random_u32() > arg->probability)) {
if (last)
- ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
+ ovs_drop_skb_last_action(skb);
return 0;
}
@@ -1579,7 +1588,7 @@ static int do_execute_actions(struct datapath *dp, struct
sk_buff *skb,
}
}
- ovs_kfree_skb_reason(skb, OVS_DROP_LAST_ACTION);
+ ovs_drop_skb_last_action(skb);
return 0;
}
--
2.45.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
