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 <amore...@redhat.com>
>> ---
>> 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
