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.
/* 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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
