Message-ID: <97CA519E-AC24-4D61-819F-B3B5A88F89E4@redhat.com>
Date: Thu, 27 Jun 2024 11:31:41 +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 11:23, Ilya Maximets wrote:

> On 6/27/24 11:14, Eelco Chaudron wrote:
>>
>>
>> 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.
>
> Yeah, but as Adrian said below, you could do that with any action and
> this doesn't change the semantics of the action itself.

Well this was just an example, what if we have some other need for getting
a packet to userspace through emit_local() other than sampling? The
emit_sample() action naming in this case makes no sense.

>>>> 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.
>
> It is fine to use it, we just assume implicit 100% sampling.

Agreed, but the name does not make sense ;) I do not think we
currently have any actions that explicitly depend on each other
(there might be attributes carried over) and I want to keep it
as such.

>>>>>> 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?
>
> For me sending to localhost means sending to a loopback interface or otherwise
> sending the packet to the host networking stack.  And we're not doing that.

That might be confusing too... Maybe emit_external()?

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