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