On 13 May 2024, at 14:44, Adrian Moreno wrote:
> On 5/13/24 2:38 PM, Ilya Maximets wrote:
>> 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 <amore...@redhat.com>
>>>>>>>
>>>>>>> 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.
>>
>
> Besides, passing an opaque cookie allows future versions of OVS to add more
> data (in addition to obs_{domain,point}) without changing uAPI.
I See pros/and cons here, but if we prefer the cookie, it needs to be specific
to the target type, i.e. if we go with system, OVS_SAMPLE_ATTR_SYSTEM_COOKIE.
>>>
>>> + 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
