On 5/13/24 09:17, Eelco Chaudron wrote: > > > On 10 May 2024, at 15:06, Ilya Maximets wrote: > >> On 5/10/24 14:01, Eelco Chaudron wrote: >>> >>> >>> On 10 May 2024, at 12:45, Adrian Moreno wrote: >>> >>>> On 5/10/24 12:06 PM, Eelco Chaudron wrote: >>>>> On 24 Apr 2024, at 21:53, Adrian Moreno wrote: >>>>> >>>>>> The new odp sample attributes allow userspace to specify a group_id and >>>>>> user-defined cookie to be passed down to psample. >>>>>> >>>>>> Add support for parsing and formatting such action. >>>>>> >>>>>> Signed-off-by: Adrian Moreno <[email protected]> >>>>> >>>>> Hi Adrian, >>>>> >>>>> See my comments below inline. >>>>> >>>>> //Eelco >>>>> >>>>>> --- >>>>>> include/linux/openvswitch.h | 49 +++++++++--- >>>>>> lib/odp-execute.c | 3 + >>>>>> lib/odp-util.c | 150 ++++++++++++++++++++++++++--------- >>>>>> ofproto/ofproto-dpif-ipfix.c | 2 + >>>>>> python/ovs/flow/odp.py | 2 + >>>>>> tests/odp.at | 5 ++ >>>>>> 6 files changed, 163 insertions(+), 48 deletions(-) >>>>>> >>>>>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h >>>>>> index d9fb991ef..3e748405c 100644 >>>>>> --- a/include/linux/openvswitch.h >>>>>> +++ b/include/linux/openvswitch.h >>>>>> @@ -696,6 +696,7 @@ enum ovs_flow_attr { >>>>>> #define OVS_UFID_F_OMIT_MASK (1 << 1) >>>>>> #define OVS_UFID_F_OMIT_ACTIONS (1 << 2) >>>>>> >>>>>> +#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16 >>>>>> /** >>>>>> * enum ovs_sample_attr - Attributes for %OVS_ACTION_ATTR_SAMPLE >>>>>> action. >>>>>> * @OVS_SAMPLE_ATTR_PROBABILITY: 32-bit fraction of packets to sample >>>>>> with >>>>>> @@ -703,15 +704,27 @@ enum ovs_flow_attr { >>>>>> * %UINT32_MAX samples all packets and intermediate values sample >>>>>> intermediate >>>>>> * fractions of packets. >>>>>> * @OVS_SAMPLE_ATTR_ACTIONS: Set of actions to execute in sampling >>>>>> event. >>>>>> - * Actions are passed as nested attributes. >>>>>> + * Actions are passed as nested attributes. Optional if >>>>>> + * OVS_SAMPLE_ATTR_PSAMPLE_GROUP is set. >>>>>> + * @OVS_SAMPLE_ATTR_PSAMPLE_GROUP: A 32-bit number to be used as >>>>>> psample group. >>>>>> + * Optional if OVS_SAMPLE_ATTR_ACTIONS is set. >>>>>> + * @OVS_SAMPLE_ATTR_PSAMPLE_COOKIE: A variable-length binary cookie >>>>>> that, if >>>>>> + * provided, will be copied to the psample cookie. >>>>> >>>>> Should we mention any limit on the actual length of the cookie? >>>>> >>>>> Any reason we explicitly call this psample? From an OVS perspective, this >>>>> could just be >>>>> SAMPLE_ATTR_FORWARD/OFFLOAD/DESTINATION/ENDPOINT/COLLECTOR/TARGET_COOKIE >>>>> and _GROUP. Other datapaths, do not have PSAMPLE. >>>>> >>>> >>>> See my response to your comment on the cover-letter for possible name >>>> alternatives. >>> >>> ACK, we can continue the discussion there. >>> >>>>> Thinking about it more, from an OVS perspective we should probably not >>>>> even send down a COOKIE, but we should send down an obs_domain_id and >>>>> obs_point_id, and then the kernel can move this into a cookie. >>>>> >>>> >>>> I did consider this. However, the opaque cookie fits well with the tc API >>>> which does not have two 32-bit values but an opaque 128-bit cookie. So if >>>> the action is offloaded OVS needs to "encode" the two 32-bit values into >>>> the cookie anyways. >>> >>> Which is fine to me, the OVS API should be clear that we have two u32 >>> entries. The cookie is implementation-specific, and we use the netlink >>> format as our API for all DPs. >> >> >> I'm not sure about this. It is a kernel interface and we can't deliver >> two separate values from the psample. So, passing two separate values >> to the kernel doesn't make a lot of sense. They are going to be mangled >> into a single cookie anyway and we'll have to document that somehow. >> We may as well always mangle the data from the beginning, document once >> at the top level and save ourselves from documenting a million differences >> between different implementations. While USDT could report them separately, >> I'm not sure there is a ton of value in that. > > Our OVS action API is not tight to psample at all, so passing in a psample > cookie > does not make sense. We should not pass in any psample references to the > sample > action API. If it looks like the below, the API is clean and we can mangle it > before > passing it to the psample function. I see no need to document this in the > kernel, as > all the kernel does is provide the TC action cookie (unrelated to the OVS > actions API).
We're passing this data directly to psample, not TC. And some documentation is needed. How users supposed to know where to find these observation domain and point? It should be specified how they are going to be seen on the output from psample. If kernel is doning the mangling, this should be documented in the kernel uAPI. If kernel receives opaque cookie and passes it directly to psample, then we can get away with only documenting the mangling process in OVS' docuemntation. > > + OVS_SAMPLE_ATTR_OBS_DOMAIN, /* Observation domain id, u32 > number. */ > + OVS_SAMPLE_ATTR_OBS_POINT, /* Observation point id, u32 number. > */ > + OVS_SAMPLE_ATTR_SYSTEM_TARGET, /* Flag to additionally sample to system > target. */ > + OVS_SAMPLE_ATTR_SYSTEM_GROUP, /* System target group id, u32 number. */ > <snip> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
