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 <amore...@redhat.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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev