Message-ID: <617f9ff3-822e-4467-894c-f247fd9029ec@ovn.org>
Date: Thu, 27 Jun 2024 10:36:38 +0200
From: Ilya Maximets <i.maximets@....org>
To: Adrián Moreno <amorenoz@...hat.com>,
 Eelco Chaudron <echaudro@...hat.com>
Cc: i.maximets@....org, netdev@...r.kernel.org, aconole@...hat.com,
 horms@...nel.org, dev@...nvswitch.org, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Donald Hunter <donald.hunter@...il.com>, Pravin B Shelar <pshelar@....org>
Subject: Re: [PATCH net-next v5 05/10] net: openvswitch: add emit_sample
 action

On 6/27/24 09:52, Adrián Moreno wrote:
> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote:
>>
>>
>> On 26 Jun 2024, at 22:34, Adrián Moreno wrote:
>>
>>> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote:
>>>>
>>>>
>>>> On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
>>>>
>>>>> Add support for a new action: emit_sample.
>>>>>
>>>>> This action accepts a u32 group id and a variable-length cookie and uses
>>>>> the psample multicast group to make the packet available for
>>>>> observability.
>>>>>
>>>>> The maximum length of the user-defined cookie is set to 16, same as
>>>>> tc_cookie, to discourage using cookies that will not be offloadable.
>>>>
>>>> I’ll add the same comment as I had in the user space part, and that
>>>> is that I feel from an OVS perspective this action should be called
>>>> emit_local() instead of emit_sample() to make it Datapath independent.
>>>> Or quoting the earlier comment:
>>>>
>>>>
>>>> “I’ll start the discussion again on the naming. The name "emit_sample()"
>>>> does not seem appropriate. This function's primary role is to copy the
>>>> packet and send it to a local collector, which varies depending on the
>>>> datapath. For the kernel datapath, this collector is psample, while for
>>>> userspace, it will likely be some kind of probe. This action is distinct
>>>> from the sample() action by design; it is a standalone action that can
>>>> be combined with others.
>>>>
>>>> Furthermore, the action itself does not involve taking a sample; it
>>>> consistently pushes the packet to the local collector. Therefore, I
>>>> suggest renaming "emit_sample()" to "emit_local()". This same goes for
>>>> all the derivative ATTR naming.”
>>>>
>>>
>>> This is a blurry semantic area.
>>> IMO, "sample" is the act of extracting (potentially a piece of)
>>> someting, in this case, a packet. It is common to only take some packets
>>> as samples, so this action usually comes with some kind of "rate", but
>>> even if the rate is 1, it's still sampling in this context.
>>>
>>> OTOH, OVS kernel design tries to be super-modular and define small
>>> combinable actions, so the rate or probability generation is done with
>>> another action which is (IMHO unfortunately) named "sample".
>>>
>>> With that interpretation of the term it would actually make more sense
>>> to rename "sample" to something like "random" (of course I'm not
>>> suggestion we do it). "sample" without any nested action that actually
>>> sends the packet somewhere is not sampling, it's just doing something or
>>> not based on a probability. Where as "emit_sample" is sampling even if
>>> it's not nested inside a "sample".
>>
>> You're assuming we are extracting a packet for sampling, but this function
>> can be used for various other purposes. For instance, it could handle the
>> packet outside of the OVS pipeline through an eBPF program (so we are not
>> taking a sample, but continue packet processing outside of the OVS
>> pipeline). Calling it emit_sampling() in such cases could be very
>> confusing.

We can't change the implementation of the action once it is part of uAPI.
We have to document where users can find these packets and we can't just
change the destination later.

>>
> 
> Well, I guess that would be clearly abusing the action. You could say
> that of anything really. You could hook into skb_consume and continue
> processing the skb but that doesn't change the intended behavior of the
> drop action.
> 
> The intended behavior of the action is sampling, as is the intended
> behavior of "psample".

The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions",
that is it takes some packets from the whole packet stream and executes
actions of them.  Without tying this to observability purposes the name
makes sense as the first definition of the word is "to take a representative
part or a single item from a larger whole or group".

Now, our new action doesn't have this particular semantic in a way that
it doesn't take a part of a whole packet stream but rather using the
part already taken.  However, it is directly tied to the parent
OVS_ACTION_ATTR_SAMPLE action, since it reports probability of that parent
action.  If there is no parent, then probability is assumed to be 100%,
but that's just a corner case.  The name of a psample module has the
same semantics in its name, it doesn't sample on it's own, but it is
assuming that sampling was performed as it relays the rate of it.

And since we're directly tied here with both OVS_ACTION_ATTR_SAMPLE and
the psample module, the emit_sample() name makes sense to me.

> 
>>> Having said that, I don't have a super strong favor for "emit_sample". I'm
>>> OK with "emit_local" or "emit_packet" or even just "emit".

The 'local' or 'packet' variants are not descriptive enough on what we're
trying to achieve and do not explain why the probability is attached to
the action, i.e. do not explain the link between this action and the
OVS_ACTION_ATTR_SAMPLE.

emit_Psample() would be overly specific, I agree, but making the name too
generic will also make it hard to add new actions.  If we use some overly
broad term for this one, we may have to deal with overlapping semantics in
the future.

>>> I don't think any term will fully satisfy everyone so I hope we can find
>>> a reasonable compromise.
>>
>> My preference would be emit_local() as we hand it off to some local
>> datapath entity.

What is "local datapath entity" ?  psample module is not part of OVS datapath.
And what is "local" ?  OpenFlow has the OFPP_LOCAL port that is represented
by a bridge port on a datapath level, that will be another source of confusion
as it can be interpreted as sending a packet via a local bridge port.

>>
> 
> I'm OK removing the controversial term. Let's see what others think.
> 
>>>>> Signed-off-by: Adrian Moreno <amorenoz@...hat.com>
>>>>> ---
>>>>>  Documentation/netlink/specs/ovs_flow.yaml | 17 +++++++++
>>>>>  include/uapi/linux/openvswitch.h          | 28 ++++++++++++++
>>>>>  net/openvswitch/Kconfig                   |  1 +
>>>>>  net/openvswitch/actions.c                 | 45 +++++++++++++++++++++++
>>>>>  net/openvswitch/flow_netlink.c            | 33 ++++++++++++++++-
>>>>>  5 files changed, 123 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
>>>>> index 4fdfc6b5cae9..a7ab5593a24f 100644
>>>>> --- a/Documentation/netlink/specs/ovs_flow.yaml
>>>>> +++ b/Documentation/netlink/specs/ovs_flow.yaml
>>>>> @@ -727,6 +727,12 @@ attribute-sets:
>>>>>          name: dec-ttl
>>>>>          type: nest
>>>>>          nested-attributes: dec-ttl-attrs
>>>>> +      -
>>>>> +        name: emit-sample
>>>>> +        type: nest
>>>>> +        nested-attributes: emit-sample-attrs
>>>>> +        doc: |
>>>>> +          Sends a packet sample to psample for external observation.
>>>>>    -
>>>>>      name: tunnel-key-attrs
>>>>>      enum-name: ovs-tunnel-key-attr
>>>>> @@ -938,6 +944,17 @@ attribute-sets:
>>>>>        -
>>>>>          name: gbp
>>>>>          type: u32
>>>>> +  -
>>>>> +    name: emit-sample-attrs
>>>>> +    enum-name: ovs-emit-sample-attr
>>>>> +    name-prefix: ovs-emit-sample-attr-
>>>>> +    attributes:
>>>>> +      -
>>>>> +        name: group
>>>>> +        type: u32
>>>>> +      -
>>>>> +        name: cookie
>>>>> +        type: binary
>>>>>
>>>>>  operations:
>>>>>    name-prefix: ovs-flow-cmd-
>>>>> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
>>>>> index efc82c318fa2..8cfa1b3f6b06 100644
>>>>> --- a/include/uapi/linux/openvswitch.h
>>>>> +++ b/include/uapi/linux/openvswitch.h
>>>>> @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
>>>>>  };
>>>>>  #endif
>>>>>
>>>>> +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
>>>>> +/**
>>>>> + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
>>>>> + * action.
>>>>> + *
>>>>> + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
>>>>> + * sample.
>>>>> + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that contains
>>>>> + * user-defined metadata. The maximum length is OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE
>>>>> + * bytes.
>>>>> + *
>>>>> + * Sends the packet to the psample multicast group with the specified group and
>>>>> + * cookie. It is possible to combine this action with the
>>>>> + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being emitted.
>>>>
>>>> Although this include file is kernel-related, it will probably be re-used for
>>>> other datapaths, so should we be more general here?
>>>>
>>>
>>> The uAPI header documentation will be used for other datapaths? How so?
>>> At some point we should document what the action does from the kernel
>>> pov, right? Where should we do that if not here?
>>
>> Well you know how OVS works, all the data paths use the same netlink messages. Not sure how to solve this, but we could change the text a bit to be more general?
>>
>>  * For the Linux kernel it sends the packet to the psample multicast group
>>  * with the specified group and cookie. It is possible to combine this
>>  * action with the %OVS_ACTION_ATTR_TRUNC action to limit the size of the
>>  * packet being emitted.
>>
> 
> I know we reuse the kernel attributes I don't think the uAPI
> documentation should be less expressive just because some userspace
> application decides to reuse parts of it.
> 
> There are many kernel-specific terms all over the uAPI ("netdev",
> "netlink pid", "skb", even the action "userspace") that do not make
> sense in a non-kernel datapath.

+1

This is a kernel uAPI header it describes the behavior of the kernel.
Having parts like "For the Linux kernel" in here is awkward.

> 
> Maybe we can add such a comment in the copy of the header we store in
> the ovs tree?

Makes sense to me.

If we'll want to implement a similar action in userspace datapath,
we'll have to have a separate documentation for it anyway, since
the packets will end up in a different place for users to collect.

> 
> 
>>>>> + */
>>>>> +enum ovs_emit_sample_attr {
>>>>> +	OVS_EMIT_SAMPLE_ATTR_GROUP = 1,	/* u32 number. */
>>>>> +	OVS_EMIT_SAMPLE_ATTR_COOKIE,	/* Optional, user specified cookie. */
>>>>
>>>> As we start a new set of attributes maybe it would be good starting it off in
>>>> alphabetical order?
>>>>
>>>
>>> Having an optional attribute before a mandatory one seems strange to me,
>>> wouldn't you agree?
>>
>> I don't mind, but I don't have a strong opinion on it. If others don't mind,
>> I would leave it as is.
>>
> 
> I think I prefer to put mandatory attributes first.

That's my thought as well.  Though that might be broken if we ever need
more attributes.  But we do not extend individual actions that often.

Best regards, Ilya Maximets.

Powered by blists - more mailing lists