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.
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.
The collector itself could be called system/local collector, or something like
that. This way it can use for example psample for kernel and UDST probes for
usespace. Both can pass the group and cookie (obs_domain_id and obs_point_id).
Also, the presence of any of this should not dictate the psample action, we
probably need a specific OVS_SAMPLE_ATTR_SYSTEM_TARGET type nl flag.
It clearer and it might simplify option verification a bit, but isn't a bit
redundant? There is no flag for action execution for instance, the presence of
the attribute is enough.
An alternative would be to have nested attributes, e.g:
enum ovs_sample_attr {
OVS_SAMPLE_ATTR_UNSPEC,
OVS_SAMPLE_ATTR_PROBABILITY, /* u32 number */
OVS_SAMPLE_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_* attributes. */
OVS_SAMPLE_ATTR_SYSTEM, /* Nested OVS_SAMPLE_SYSTEM_ATTR_*
attributes. */
__OVS_SAMPLE_ATTR_MAX,
};
enum ovs_sample_system_attr {
OVS_SAMPLE_SYSTEM_ATTR_OBS_DOMAIN, /* u32 number */
OVS_SAMPLE_SYSTEM_ATTR_OBS_POINT, /* u32 number */
OVS_SAMPLE_SYSTEM_ATTR_COOKIE, /* Nested OVS_ACTION_ATTR_*
__OVS_SSAMPLE_SYSTEM_ATTR_MAX,
};
WDYT?
So I would envision your delta to look something like this:
enum ovs_sample_attr {
OVS_SAMPLE_ATTR_UNSPEC,
- OVS_SAMPLE_ATTR_PROBABILITY, /* u32 number */
- OVS_SAMPLE_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_* attributes. */
+ OVS_SAMPLE_ATTR_PROBABILITY, /* u32 number */
+ OVS_SAMPLE_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_
+ * attributes.
+ */
+ 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. */
__OVS_SAMPLE_ATTR_MAX,
*
- * Executes the specified actions with the given probability on a per-packet
- * basis.
+ * Either OVS_SAMPLE_ATTR_PSAMPLE_GROUP or OVS_SAMPLE_ATTR_ACTIONS must be
+ * specified.
+ *
+ * Executes the specified actions and/or sends the packet to psample
+ * with the given probability on a per-packet basis.
*/
enum ovs_sample_attr {
OVS_SAMPLE_ATTR_UNSPEC,
- OVS_SAMPLE_ATTR_PROBABILITY, /* u32 number */
- OVS_SAMPLE_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_* attributes. */
+ OVS_SAMPLE_ATTR_PROBABILITY, /* u32 number */
+ OVS_SAMPLE_ATTR_ACTIONS, /* Nested OVS_ACTION_ATTR_
Missing *
Yeah, it's a pitty the "*" makes checkpatch raise a warning.
+ * attributes.
+ */
+ OVS_SAMPLE_ATTR_PSAMPLE_GROUP, /* u32 number */
+ OVS_SAMPLE_ATTR_PSAMPLE_COOKIE, /* binary */
__OVS_SAMPLE_ATTR_MAX,
#ifdef __KERNEL__
@@ -722,13 +735,27 @@ enum ovs_sample_attr {
#define OVS_SAMPLE_ATTR_MAX (__OVS_SAMPLE_ATTR_MAX - 1)
#ifdef __KERNEL__
+
+/* Definition for flags in struct sample_arg. */
+enum {
+ /* When actions in sample will not change the flows. */
+ OVS_SAMPLE_ARG_FLAG_EXEC = 1 << 0,
+ /* When set, the packet will be sent to psample. */
+ OVS_SAMPLE_ARG_FLAG_PSAMPLE = 1 << 1,
Same on psample name here, should it be FLAG_SYSTEM_TARGET instead? See the
above comment about an explicit option.
+};
+
struct sample_arg {
- bool exec; /* When true, actions in sample will not
- * change flow keys. False otherwise.
- */
- u32 probability; /* Same value as
- * 'OVS_SAMPLE_ATTR_PROBABILITY'.
- */
+ u16 flags; /* Flags that modify the behavior of the
+ * action. See SAMPLE_ARG_FLAG_*
+ */
+ u32 probability; /* Same value as
+ * 'OVS_SAMPLE_ATTR_PROBABILITY'.
+ */
+ u32 group_id; /* Same value as
+ * 'OVS_SAMPLE_ATTR_PSAMPLE_GROUP'.
+ */
+ u8 cookie_len; /* Length of psample cookie */
+ char cookie[OVS_PSAMPLE_COOKIE_MAX_SIZE]; /* psample cookie data. */
Would it make sense for the cookie also to be u8?
Yes.
};
#endif
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 081e4d432..d8ee93429 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -716,6 +716,9 @@ odp_execute_sample(void *dp, struct dp_packet *packet, bool
steal,
subactions = a;
break;
+ /* Ignored in userspace datapath. */
+ case OVS_SAMPLE_ATTR_PSAMPLE_GROUP:
+ case OVS_SAMPLE_ATTR_PSAMPLE_COOKIE:
case OVS_SAMPLE_ATTR_UNSPEC:
case __OVS_SAMPLE_ATTR_MAX:
default:
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 21f34d955..611b5229e 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -227,12 +227,16 @@ format_odp_sample_action(struct ds *ds, const struct
nlattr *attr,
{
static const struct nl_policy ovs_sample_policy[] = {
[OVS_SAMPLE_ATTR_PROBABILITY] = { .type = NL_A_U32 },
- [OVS_SAMPLE_ATTR_ACTIONS] = { .type = NL_A_NESTED }
+ [OVS_SAMPLE_ATTR_ACTIONS] = { .type = NL_A_NESTED,
+ .optional = true },
+ [OVS_SAMPLE_ATTR_PSAMPLE_GROUP] = { .type = NL_A_U32,
+ .optional = true },
+ [OVS_SAMPLE_ATTR_PSAMPLE_COOKIE] = { .type = NL_A_UNSPEC,
+ .optional = true }
};
struct nlattr *a[ARRAY_SIZE(ovs_sample_policy)];
+ const struct nlattr *nla;
double percentage;
- const struct nlattr *nla_acts;
- int len;
ds_put_cstr(ds, "sample");
@@ -244,13 +248,33 @@ format_odp_sample_action(struct ds *ds, const struct
nlattr *attr,
percentage = (100.0 * nl_attr_get_u32(a[OVS_SAMPLE_ATTR_PROBABILITY])) /
UINT32_MAX;
- ds_put_format(ds, "(sample=%.1f%%,", percentage);
+ ds_put_format(ds, "(sample=%.1f%%", percentage);
- ds_put_cstr(ds, "actions(");
- nla_acts = nl_attr_get(a[OVS_SAMPLE_ATTR_ACTIONS]);
- len = nl_attr_get_size(a[OVS_SAMPLE_ATTR_ACTIONS]);
- format_odp_actions(ds, nla_acts, len, portno_names);
- ds_put_format(ds, "))");
+ nla = a[OVS_SAMPLE_ATTR_PSAMPLE_GROUP];
+ if (nla) {
+ ds_put_format(ds, ",group_id=%d", nl_attr_get_u32(nla));
+
+ nla = a[OVS_SAMPLE_ATTR_PSAMPLE_COOKIE];
+
+ if (nla) {
+ size_t i;
+ const uint8_t *c = nl_attr_get(nla);
+ ds_put_cstr(ds, ",cookie=");
+ for (i = 0; i < nl_attr_get_size(nla); i++) {
+ ds_put_format(ds, "%02x", c[i]);
+ }
+ }
+ }
+
+ nla = a[OVS_SAMPLE_ATTR_ACTIONS];
+ if (nla) {
We used to display an empty action list if no actions were present, now we do
not show actions at all. Any reason why, as this changes the existing output?
Before odp sample() _always_ carried nested actions. Yeh, someone could add a
dpctl action directly with an empty action set (which is pretty abusurd as the
action will do nothing), but OVS would never create an ODP sample with no nested
actions. Now it does, and in those cases having "actions()" is redundant.
+ ds_put_cstr(ds, ",actions(");
+ format_odp_actions(ds, nl_attr_get(nla), nl_attr_get_size(nla),
+ portno_names);
+ ds_put_format(ds, "))");
+ } else {
+ ds_put_format(ds, ")");
+ }
}
static void
@@ -1348,6 +1372,84 @@ format_odp_actions(struct ds *ds, const struct nlattr
*actions,
}
}
+static int
+parse_action_list(struct parse_odp_context *context, const char *s,
+ struct ofpbuf *actions);
Move this forward declaration to the top of this file with the others.
Ack.
+static int
+parse_odp_sample_action(const char *s, struct parse_odp_context *context,
+ struct ofpbuf *actions)
+{
+ double percentage;
+ uint32_t group_id;
+ size_t sample_ofs, actions_ofs;
+ double probability;
+ int parsed = 0;
+ int n;
+
In this function, you assume a fixed order of options, but this should not be
required, see other actions, i.e. any order should be accepted.
Well, this action before also assumed "sample" would go before "actions". And so
does check_pkt_len, add_mpls, push_vlan and userspace. Only ct and nat seem to
accept unordered arguments AFAICS.
I thought, flow text interface only has to consistent with itself, meanding it
should be able to parse whatever it can generate.
+ if (!ovs_scan(s, "sample(sample=%lf%%,%n", &percentage, &parsed)) {
+ return -EINVAL;
+ }
+
+ probability = floor(UINT32_MAX * (percentage / 100.0) + .5);
+ sample_ofs = nl_msg_start_nested(actions, OVS_ACTION_ATTR_SAMPLE);
+ nl_msg_put_u32(actions, OVS_SAMPLE_ATTR_PROBABILITY,
+ (probability <= 0 ? 0
+ : probability >= UINT32_MAX ? UINT32_MAX
+ : probability));
+
+ if (ovs_scan(s + parsed, "group_id=%"PRIu32"%n", &group_id, &n)) {
+ parsed += n;
+
+ nl_msg_put_u32(actions, OVS_SAMPLE_ATTR_PSAMPLE_GROUP, group_id);
+
+ if (ovs_scan(s + parsed, ",cookie=%n", &n)) {
You could use ovs_scan_len(), search for ovs_scan_len(s, &n,
"md2=0x%511[0-9a-fA-F]", buf) in this file.
Thanks for the pointer.
+ struct ofpbuf buf;
+ size_t size;
+ char *end;
+
+ parsed += n;
+ ofpbuf_init(&buf, OVS_PSAMPLE_COOKIE_MAX_SIZE);
+
+ end = ofpbuf_put_hex(&buf, s + parsed, &size);
+ if (!end ||
+ size > OVS_PSAMPLE_COOKIE_MAX_SIZE ||
+ (end[0] != ')' && end[0] != ',')) {
+ ofpbuf_uninit(&buf);
+ return -EINVAL;
+ }
+
+ nl_msg_put_unspec(actions, OVS_SAMPLE_ATTR_PSAMPLE_COOKIE,
+ buf.data, buf.size);
+
+ ofpbuf_uninit(&buf);
+ parsed = end - s;
+ }
+ if (s[parsed] == ')') {
+ nl_msg_end_nested(actions, sample_ofs);
+ return parsed + 1;
+ } else if (s[parsed] == ',') {
+ parsed += 1;
+ } else {
+ return -EINVAL;
+ }
+ }
+
+ if (ovs_scan(s + parsed, "actions(%n", &n)) {
+ parsed += n;
+ actions_ofs = nl_msg_start_nested(actions, OVS_SAMPLE_ATTR_ACTIONS);
+ int retval = parse_action_list(context, s + parsed, actions);
+ if (retval < 0) {
+ return retval;
+ }
+ parsed += retval;
+ nl_msg_end_nested(actions, actions_ofs);
+ nl_msg_end_nested(actions, sample_ofs);
+ return s[parsed + 1] == ')' ? parsed + 2 : -EINVAL;
+ }
+
+ return -EINVAL;
+}
+
/* Separate out parse_odp_userspace_action() function. */
static int
parse_odp_userspace_action(const char *s, struct ofpbuf *actions)
@@ -2561,34 +2663,8 @@ parse_odp_action__(struct parse_odp_context *context,
const char *s,
}
{
- double percentage;
- int n = -1;
-
- if (ovs_scan(s, "sample(sample=%lf%%,actions(%n", &percentage, &n)
- && percentage >= 0. && percentage <= 100.0) {
- size_t sample_ofs, actions_ofs;
- double probability;
-
- probability = floor(UINT32_MAX * (percentage / 100.0) + .5);
- sample_ofs = nl_msg_start_nested(actions, OVS_ACTION_ATTR_SAMPLE);
- nl_msg_put_u32(actions, OVS_SAMPLE_ATTR_PROBABILITY,
- (probability <= 0 ? 0
- : probability >= UINT32_MAX ? UINT32_MAX
- : probability));
-
- actions_ofs = nl_msg_start_nested(actions,
- OVS_SAMPLE_ATTR_ACTIONS);
- int retval = parse_action_list(context, s + n, actions);
- if (retval < 0) {
- return retval;
- }
-
-
- n += retval;
- nl_msg_end_nested(actions, actions_ofs);
- nl_msg_end_nested(actions, sample_ofs);
-
- return s[n + 1] == ')' ? n + 2 : -EINVAL;
+ if (ovs_scan(s, "sample(")) {
This should become a strncmp() like the others.
Yes, thanks.
+ return parse_odp_sample_action(s, context, actions);
}
}
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index cd65dae7e..407b2b38a 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -3064,6 +3064,8 @@ dpif_ipfix_read_sample_actions(const struct flow *flow,
&sample_actions);
break;
+ case OVS_SAMPLE_ATTR_PSAMPLE_GROUP:
+ case OVS_SAMPLE_ATTR_PSAMPLE_COOKIE:
case OVS_SAMPLE_ATTR_UNSPEC:
case __OVS_SAMPLE_ATTR_MAX:
default:
diff --git a/python/ovs/flow/odp.py b/python/ovs/flow/odp.py
index 7d9b165d4..bc0495eb4 100644
--- a/python/ovs/flow/odp.py
+++ b/python/ovs/flow/odp.py
@@ -349,6 +349,8 @@ class ODPFlow(Flow):
KVDecoders(
{
"sample": (lambda x: float(x.strip("%"))),
+ "group_id": decode_int,
+ "cookie": decode_default,
"actions": nested_kv_decoder(
KVDecoders(
decoders=_decoders,
diff --git a/tests/odp.at b/tests/odp.at
index ba20604e4..4a2435c97 100644
--- a/tests/odp.at
+++ b/tests/odp.at
@@ -327,6 +327,9 @@ push_vlan(tpid=0x9100,vid=13,pcp=5)
push_vlan(tpid=0x9100,vid=13,pcp=5,cfi=0)
pop_vlan
sample(sample=9.7%,actions(1,2,3,push_vlan(vid=1,pcp=2)))
+sample(sample=9.7%,group_id=25)
+sample(sample=9.7%,group_id=12,cookie=0102)
+sample(sample=9.7%,group_id=12,cookie=0102030405060708090a0b0c0d0e0f,actions(1,2,3,push_vlan(vid=1,pcp=2)))
Add some more with the mixed options order, to verify they work also.
Sure.
set(tunnel(tun_id=0xabcdef1234567890,src=1.1.1.1,dst=2.2.2.2,ttl=64,flags(df|csum|key)))
set(tunnel(tun_id=0xabcdef1234567890,src=1.1.1.1,dst=2.2.2.2,ttl=64,flags(key)))
tnl_pop(4)
@@ -406,11 +409,13 @@ AT_DATA([actions.txt], [dnl
encap_nsh@:{@
tnl_push(tnl_port(6),header(size=94,type=112,eth(dst=f8:bc:12:44:34:b6,src=f8:bc:12:46:58:e0,dl_type=0x86dd),ipv6(src=2001:cafe::88,dst=2001:cafe::92,label=0,proto=43,tclass=0x0,hlimit=64),srv6(segments_left=2,segs(2001:cafe::90,2001:cafe::91))),out_port(1))
tnl_push(tnl_port(6),header(size=126,type=112,eth(dst=f8:bc:12:44:34:b6,src=f8:bc:12:46:58:e0,dl_type=0x86dd),ipv6(src=2001:cafe::88,dst=2001:cafe::92,label=0,proto=43,tclass=0x0,hlimit=64),srv6(segments_left=2,segs(2001:cafe::90,2001:cafe::91,2001:cafe::92,2001:cafe::93))),out_port(1))
+sample(sample=9.7%,group_id=12,cookie=0102030405060708090a0b0c0d0e0f0f0f)
Maybe add some more? Like invalid group_id, invalid cookie length, or string.
OK
])
AT_CHECK_UNQUOTED([ovstest test-odp parse-actions < actions.txt], [0], [dnl
odp_actions_from_string: error
odp_actions_from_string: error
odp_actions_from_string: error
+odp_actions_from_string: error
])
AT_CLEANUP
--
2.44.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
