Re: [ovs-dev] [RFC 02/11] ofproto_dpif: Check for psample support.
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
---
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
Re: [ovs-dev] [RFC 02/11] ofproto_dpif: Check for psample support.
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
>>> ---
>>> 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().
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC 02/11] ofproto_dpif: Check for psample support.
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
---
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(
Re: [ovs-dev] [RFC 02/11] ofproto_dpif: Check for psample support.
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
> ---
> 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.
> /* 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)?
> +
> +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.
>
> /* 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 a
[ovs-dev] [RFC 02/11] ofproto_dpif: Check for psample support.
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
---
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)
+{
+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);
+}
+
/* 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));
+
+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);
/* 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinf
