On Wed, Jul 10, 2024 at 01:28:29PM GMT, Ilya Maximets wrote: > On 7/7/24 22:08, Adrian Moreno wrote: > > Only kernel datapath supports this action so add a function in dpif.c > > that checks for that. > > > > Signed-off-by: Adrian Moreno <amore...@redhat.com> > > --- > > lib/dpif.c | 7 +++++++ > > lib/dpif.h | 1 + > > ofproto/ofproto-dpif.c | 43 ++++++++++++++++++++++++++++++++++++++++++ > > ofproto/ofproto-dpif.h | 7 ++++++- > > 4 files changed, 57 insertions(+), 1 deletion(-) > > > > diff --git a/lib/dpif.c b/lib/dpif.c > > index 71728badc..0a964ba89 100644 > > --- a/lib/dpif.c > > +++ b/lib/dpif.c > > @@ -1953,6 +1953,13 @@ dpif_supports_lb_output_action(const struct dpif > > *dpif) > > return dpif_is_netdev(dpif); > > } > > > > +bool > > +dpif_may_support_psample(const struct dpif *dpif) > > +{ > > + /* Userspace datapath does not support this action. */ > > + return !dpif_is_netdev(dpif); > > +} > > + > > /* Meters */ > > void > > dpif_meter_get_features(const struct dpif *dpif, > > diff --git a/lib/dpif.h b/lib/dpif.h > > index a764e8a59..6bef7d5b3 100644 > > --- a/lib/dpif.h > > +++ b/lib/dpif.h > > @@ -941,6 +941,7 @@ int dpif_get_pmds_for_port(const struct dpif * dpif, > > odp_port_t port_no, > > char *dpif_get_dp_version(const struct dpif *); > > bool dpif_supports_tnl_push_pop(const struct dpif *); > > bool dpif_may_support_explicit_drop_action(const struct dpif *); > > +bool dpif_may_support_psample(const struct dpif *); > > bool dpif_synced_dp_layers(struct dpif *); > > > > /* Log functions. */ > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index 4712d10a8..94c84d697 100644 > > --- a/ofproto/ofproto-dpif.c > > +++ b/ofproto/ofproto-dpif.c > > @@ -873,6 +873,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. > > @@ -1609,6 +1615,42 @@ check_add_mpls(struct dpif_backer *backer) > > return supported; > > } > > > > +/* Tests whether 'backer''s datapath supports the OVS_ACTION_ATTR_PSAMPLE > > + * action. */ > > +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; > > + > > + 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, sizeof(cookie)); > > Don't parenthesize the argument of sizeof. >
Ack. > > + > > + odp_put_psample_action(&actions, 10, cookie, sizeof cookie); > > + > > + supported = dpif_may_support_psample(backer->dpif) && > > + dpif_probe_feature(backer->dpif, "psample", &key, &actions, NULL); > > This function actually installs the flow into the kernel. > You need to make sure that no packet can match it, e.g. > by setting a bogus eth type as a match. Some existing > probe functions require specific matches, some are just > missing this part as well, but we should not blindly copy > bad patterns. > I guess I was always thinking on a freshly started system but you're right! If OVS is restarted, the installed flow can affect actual traffic. I'll add a bogus key to make sure it's not problematic. > > + > > + ofpbuf_uninit(&actions); > > + VLOG_INFO("%s: Datapath %s psample", dpif_name(backer->dpif), > > '%s: Datapath %s psample action' will be more clear. > Ack. > > + 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 +1740,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 = check_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..bc7a19dab 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 action. */ > > \ > > + DPIF_SUPPORT_FIELD(bool, psample, "psample") > > > > > > /* Stores the various features which the corresponding backer supports. */ > > @@ -412,4 +415,6 @@ bool ofproto_dpif_ct_zone_timeout_policy_get_name( > > > > bool ovs_explicit_drop_action_supported(struct ofproto_dpif *); > > > > +bool ovs_psample_supported(struct ofproto_dpif *); > > + > > #endif /* ofproto-dpif.h */ > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev