On 5/10/24 1:49 PM, Ilya Maximets wrote:
On 5/10/24 13:03, Adrian Moreno wrote:
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 <amore...@redhat.com>
---
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.
It's expected that userspace datapath is a reference implementation that
supports any actions, because it executes most of the actions via odp-execute
module that supposed to know all the actions.
This one however doesn't make sense for userspace datapath, so we have to
check beforehand. Adding extra parsing of actions may have significant
performance impact for upcalls.
However, ofproto-dpif module should not know any datapath details and must
not include dpif-netdev.h, the logic should be moved to dpif.c. For example,
see what we did for the explicit drop action in
dpif_may_support_explicit_drop_action().
Thanks for this pointer. My mind kept telling me I'd seen this before related to
the drop action, but couldn't find it.
--
Adrián
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
