Message-ID: <b42e503b-663d-4d44-86d1-ab93feec4593@ovn.org>
Date: Thu, 27 Jun 2024 11:23:09 +0200
From: Ilya Maximets <i.maximets@....org>
To: Eelco Chaudron <echaudro@...hat.com>
Cc: i.maximets@....org, 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 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, 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.

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

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