On 5/10/24 12:06 PM, Eelco Chaudron wrote:
On 24 Apr 2024, at 21:53, Adrian Moreno wrote:
Only kernel datapath supports psample so check that the datapath is not
userspace and that it accepts the new attributes.
Signed-off-by: Adrian Moreno <[email protected]>
---
ofproto/ofproto-dpif.c | 59 ++++++++++++++++++++++++++++++++++++++++++
ofproto/ofproto-dpif.h | 6 ++++-
2 files changed, 64 insertions(+), 1 deletion(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 32d037be6..3cee2795a 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -25,6 +25,7 @@
#include "coverage.h"
#include "cfm.h"
#include "ct-dpif.h"
+#include "dpif-netdev.h"
#include "fail-open.h"
#include "guarded-list.h"
#include "hmapx.h"
@@ -873,6 +874,12 @@ ovs_lb_output_action_supported(struct ofproto_dpif
*ofproto)
return ofproto->backer->rt_support.lb_output_action;
}
+bool
+ovs_psample_supported(struct ofproto_dpif *ofproto)
As mentioned in the previous patch, I do not like the psample terminology. It
refers to an implementation-specific name, we should come up with an agnostic
name, like ’system/local’, but there are probably better names.
+{
+ return ofproto->backer->rt_support.psample;
+}
+
/* Tests whether 'backer''s datapath supports recirculation. Only newer
* datapaths support OVS_KEY_ATTR_RECIRC_ID in keys. We need to disable some
* features on older datapaths that don't support this feature.
@@ -1440,6 +1447,14 @@ dpif_supports_ct_zero_snat(struct dpif_backer *backer)
return supported;
}
+static bool check_psample(struct dpif_backer *backer);
+
+static bool
+dpif_supports_psample(struct dpif_backer *backer)
+{
+ return !dpif_is_netdev(backer->dpif) && check_psample(backer);
+}
This looks odd, we should not do any datapath specific checks here. We should
just call check_psample() on the netdev datapath, and it should fail. If we
ever add support for a similar feature we need to remove this code anyway.
Guess it also omits the log message for userspace.
I agree. Unfortunately, I didn't see any action verification code in the netdev
datapath that (as the kernel does) would make the action installation fail. I'll
double check but I think it's just not capable of detecting unsupported
attributes on flow installation.
/* Tests whether 'backer''s datapath supports the
* OVS_ACTION_ATTR_CHECK_PKT_LEN action. */
static bool
@@ -1609,6 +1624,49 @@ check_add_mpls(struct dpif_backer *backer)
return supported;
}
+/* Tests whether 'backer''s datapath supports the OVS_SAMPLE_ATTR_PSAMPLE
+ * attribute. */
+static bool
+check_psample(struct dpif_backer *backer)
+{
+ uint8_t cookie[OVS_PSAMPLE_COOKIE_MAX_SIZE];
+ struct odputil_keybuf keybuf;
+ struct ofpbuf actions;
+ struct ofpbuf key;
+ struct flow flow;
+ bool supported;
+ size_t offset;
+
+ struct odp_flow_key_parms odp_parms = {
+ .flow = &flow,
+ .probe = true,
+ };
+
+ memset(&flow, 0, sizeof flow);
+ ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
+ odp_flow_key_from_flow(&odp_parms, &key);
+ ofpbuf_init(&actions, 64);
+
+ /* Generate a random max-size cookie. */
+ random_bytes(&cookie[0], sizeof(cookie));
Any specific reason for adding the [0] to cookie (same below)?
Not really.
+
+ offset = nl_msg_start_nested(&actions, OVS_ACTION_ATTR_SAMPLE);
+ nl_msg_put_u32(&actions, OVS_SAMPLE_ATTR_PROBABILITY, 1);
+ nl_msg_put_u32(&actions, OVS_SAMPLE_ATTR_PSAMPLE_GROUP, 10);
+ nl_msg_put_unspec(&actions, OVS_SAMPLE_ATTR_PSAMPLE_COOKIE, &cookie[0],
+ sizeof(cookie));
+ nl_msg_end_nested(&actions, offset);
+
+ supported = dpif_probe_feature(backer->dpif, "psample", &key,
+ &actions, NULL);
+ ofpbuf_uninit(&actions);
+ VLOG_INFO("%s: Datapath %s psample",
+ dpif_name(backer->dpif),
+ supported ? "supports" : "does not support");
+ return supported;
+}
+
+
#define CHECK_FEATURE__(NAME, SUPPORT, FIELD, VALUE, ETHTYPE) \
static bool \
check_##NAME(struct dpif_backer *backer) \
@@ -1698,6 +1756,7 @@ check_support(struct dpif_backer *backer)
dpif_supports_lb_output_action(backer->dpif);
backer->rt_support.ct_zero_snat = dpif_supports_ct_zero_snat(backer);
backer->rt_support.add_mpls = check_add_mpls(backer);
+ backer->rt_support.psample = dpif_supports_psample(backer);
Just call check_psample() here directly.
Se above. I think check_psample will return true for netdev datapath.
/* Flow fields. */
backer->rt_support.odp.ct_state = check_ct_state(backer);
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index d33f73df8..3db4263c7 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -213,7 +213,10 @@ struct group_dpif *group_dpif_lookup(struct ofproto_dpif *,
DPIF_SUPPORT_FIELD(bool, ct_zero_snat, "Conntrack all-zero IP SNAT") \
\
/* True if the datapath supports add_mpls action. */ \
- DPIF_SUPPORT_FIELD(bool, add_mpls, "MPLS Label add")
+ DPIF_SUPPORT_FIELD(bool, add_mpls, "MPLS Label add") \
+ \
+ /* True if the datapath supports psample. */ \
+ DPIF_SUPPORT_FIELD(bool, psample, "Psample")
/* Stores the various features which the corresponding backer supports. */
@@ -411,5 +414,6 @@ bool ofproto_dpif_ct_zone_timeout_policy_get_name(
uint8_t nw_proto, char **tp_name, bool *unwildcard);
bool ovs_explicit_drop_action_supported(struct ofproto_dpif *);
+bool ovs_psample_supported(struct ofproto_dpif *);
#endif /* ofproto-dpif.h */
--
2.44.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
