On 12 Jul 2024, at 1:32, 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 <[email protected]>

This patch looks good to me. One small comment/nit below. The rest looks good.

Acked-by: Eelco Chaudron <[email protected]>

> ---
>  lib/dpif.c             |  7 +++++++
>  lib/dpif.h             |  1 +
>  ofproto/ofproto-dpif.c | 46 ++++++++++++++++++++++++++++++++++++++++++
>  ofproto/ofproto-dpif.h |  6 +++++-
>  vswitchd/vswitch.xml   |  5 +++++
>  5 files changed, 64 insertions(+), 1 deletion(-)
>
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 94db4630e..ab633fd27 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 1405d614e..41a5e989d 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,44 @@ 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;
> +    bool supported;
> +
> +    /* Intentionally bogus dl_type. */
> +    struct flow flow = {
> +        .dl_type = CONSTANT_HTONS(0x1234),
> +    };
> +    struct odp_flow_key_parms odp_parms = {
> +        .flow = &flow,
> +        .probe = true,
> +    };
> +
> +    ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
> +    odp_flow_key_from_flow(&odp_parms, &key);
> +    ofpbuf_init(&actions, 32);
> +
> +    /* Generate a random max-size cookie. */
> +    random_bytes(cookie, sizeof cookie);
> +
> +    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);
> +
> +    ofpbuf_uninit(&actions);
> +    VLOG_INFO("%s: Datapath %s psample action", 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 +1742,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);
> @@ -5821,6 +5866,7 @@ get_datapath_cap(const char *datapath_type, struct smap 
> *cap)
>      smap_add(cap, "lb_output_action", s->lb_output_action ? "true" : 
> "false");
>      smap_add(cap, "ct_zero_snat", s->ct_zero_snat ? "true" : "false");
>      smap_add(cap, "add_mpls", s->add_mpls ? "true" : "false");
> +    smap_add(cap, "psample", s->psample ? "true" : "false");
>
>      /* The ct_tuple_flush is implemented on dpif level, so it is supported
>       * for all backers. */
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index d33f73df8..ea18ccedf 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. */
> @@ -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 */
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index e3afb78a4..c6eb598d3 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -6511,6 +6511,11 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 
> type=patch options:peer=p1 \
>          called <code>NXT_CT_FLUSH</code>. The <code>NXT_CT_FLUSH</code>
>          extensions allows to flush CT entries based on specified parameters.
>        </column>
> +      <column name="capabilities" key="psample"
> +              type='{"type": "boolean"}'>
> +        True if the datapath supports OVS_ACTION_ATTR_PSAMPLE. If false,
> +        local sampling will not be supported.

nit: Remember to change this if we add userspace support, or maybe make it more 
clear with something like:

  If false, local sampling will not be supported with the Linux kernel datapath.

> +      </column>
>      </group>
>
>      <column name="ct_zone_default_limit">
> -- 
> 2.45.2

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to