Date: Thu, 27 Jun 2024 10:15:55 +0000
From: Adrián Moreno <amorenoz@...hat.com>
To: Eelco Chaudron <echaudro@...hat.com>
Cc: Ilya Maximets <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 Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote:
>
>
> 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()?

"External" was the word I used for the original userspace RFC. The
rationale being: We're sending the packet to something external from OVS
(datapath or userspace). Compared with IPFIX-based observability which
where the sample is first processed ("internally") by ovs-vswitchd.

In userspace it kept the sampling/observability meaning because it was
part of the Flow_Sample_Collector_Set which is intrinsically an
observability thing.

However, in the datapath we loose that meaning and could be confused
with some external packet-processing entity. How about "external_observe"
or something that somehow keeps that meaning?


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