Date: Thu, 27 Jun 2024 11:14:54 +0200
From: Eelco Chaudron <echaudro@...hat.com>
To: Ilya Maximets <i.maximets@....org>
Cc: Adrián Moreno <amorenoz@...hat.com>,
 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 27 Jun 2024, at 10:36, Ilya Maximets wrote:

> 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.

I'm not suggesting we change the uAPI implementation, but we could use the
emit_xxx() action with an eBPF probe on the action to perform other tasks.
This is just an example.

>> 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.

This is the part I don't like. emit_sample() should be treated as a
standalone action. While it may have potential dependencies on
OVS_ACTION_ATTR_SAMPLE, it should also be perfectly fine to use it
independently.

>>>> 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 guess I hinted at a local exit point in the specific netdev/netlink datapath, where exit is to the local host. So maybe we should call it emit_localhost?

>> 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