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 <[email protected]> >>>>>>> --- >>>>>>> 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. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
