On 3 Apr 2024, at 16:35, Eric Garver wrote:
> Kernel support has been added for this action. As such, we need to probe
> the datapath for support.
>
> Signed-off-by: Eric Garver <e...@garver.life>
> ---
> include/linux/openvswitch.h | 2 +-
> lib/dpif.c | 6 ++-
> lib/dpif.h | 2 +-
> ofproto/ofproto-dpif.c | 77 ++++++++++++++++++++++++++++++++++---
> ofproto/ofproto-dpif.h | 4 +-
> 5 files changed, 81 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index d18754c84f62..d9fb991ef234 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -1086,11 +1086,11 @@ enum ovs_action_attr {
> OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
> + OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
>
> #ifndef __KERNEL__
> OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/
> OVS_ACTION_ATTR_TUNNEL_POP, /* u32 port number. */
> - OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
> OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */
> #endif
> __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 0f480bec48d0..23eb18495a66 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -28,6 +28,7 @@
> #include "dpctl.h"
> #include "dpif-netdev.h"
> #include "flow.h"
> +#include "netdev-offload.h"
> #include "netdev-provider.h"
> #include "netdev.h"
> #include "netlink.h"
> @@ -1935,9 +1936,10 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
> }
>
> bool
> -dpif_supports_explicit_drop_action(const struct dpif *dpif)
> +dpif_may_support_explicit_drop_action(const struct dpif *dpif)
> {
> - return dpif_is_netdev(dpif);
> + /* TC does not support offloading this action. */
> + return dpif_is_netdev(dpif) || !netdev_is_flow_api_enabled();
> }
>
> bool
> diff --git a/lib/dpif.h b/lib/dpif.h
> index 0f2dc2ef3c55..a764e8a592bd 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -940,7 +940,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_supports_explicit_drop_action(const struct dpif *);
> +bool dpif_may_support_explicit_drop_action(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 c4e2e867ecdc..32d037be607c 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -221,6 +221,7 @@ static void ct_zone_config_init(struct dpif_backer
> *backer);
> static void ct_zone_config_uninit(struct dpif_backer *backer);
> static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
> static void ct_zone_limits_commit(struct dpif_backer *backer);
> +static bool recheck_support_explicit_drop_action(struct dpif_backer *backer);
>
> static inline struct ofproto_dpif *
> ofproto_dpif_cast(const struct ofproto *ofproto)
> @@ -391,6 +392,10 @@ type_run(const char *type)
> udpif_set_threads(backer->udpif, n_handlers, n_revalidators);
> }
>
> + if (recheck_support_explicit_drop_action(backer)) {
> + backer->need_revalidate = REV_RECONFIGURE;
> + }
> +
> if (backer->need_revalidate) {
> struct ofproto_dpif *ofproto;
> struct simap_node *node;
> @@ -855,7 +860,11 @@ ovs_native_tunneling_is_on(struct ofproto_dpif *ofproto)
> bool
> ovs_explicit_drop_action_supported(struct ofproto_dpif *ofproto)
> {
> - return ofproto->backer->rt_support.explicit_drop_action;
> + bool value;
> +
> + atomic_read_relaxed(&ofproto->backer->rt_support.explicit_drop_action,
> + &value);
> + return value;
> }
>
> bool
> @@ -1379,6 +1388,40 @@ check_ct_timeout_policy(struct dpif_backer *backer)
> return !error;
> }
>
> +/* Tests whether backer's datapath supports the OVS_ACTION_ATTR_DROP action.
> */
> +static bool
> +check_drop_action(struct dpif_backer *backer)
> +{
> + struct odputil_keybuf keybuf;
> + uint8_t actbuf[NL_A_U32_SIZE];
> + struct ofpbuf actions;
> + struct ofpbuf key;
> + bool supported;
> +
> + struct flow flow = {
> + .dl_type = CONSTANT_HTONS(0x1234), /* bogus */
> + };
> + 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_use_stack(&actions, &actbuf, sizeof actbuf);
> + nl_msg_put_u32(&actions, OVS_ACTION_ATTR_DROP, XLATE_OK);
> +
> + supported = dpif_may_support_explicit_drop_action(backer->dpif) &&
> + dpif_probe_feature(backer->dpif, "drop", &key, &actions,
> NULL);
> +
> + VLOG_INFO("%s: Datapath %s explicit drop action",
> + dpif_name(backer->dpif),
> + (supported) ? "supports" : "does not support");
> +
> + return supported;
> +}
> +
> /* Tests whether 'backer''s datapath supports the all-zero SNAT case. */
> static bool
> dpif_supports_ct_zero_snat(struct dpif_backer *backer)
> @@ -1649,8 +1692,8 @@ check_support(struct dpif_backer *backer)
> backer->rt_support.max_hash_alg = check_max_dp_hash_alg(backer);
> backer->rt_support.check_pkt_len = check_check_pkt_len(backer);
> backer->rt_support.ct_timeout = check_ct_timeout_policy(backer);
> - backer->rt_support.explicit_drop_action =
> - dpif_supports_explicit_drop_action(backer->dpif);
> + atomic_store_relaxed(&backer->rt_support.explicit_drop_action,
> + check_drop_action(backer));
> backer->rt_support.lb_output_action =
> dpif_supports_lb_output_action(backer->dpif);
> backer->rt_support.ct_zero_snat = dpif_supports_ct_zero_snat(backer);
> @@ -1667,6 +1710,28 @@ check_support(struct dpif_backer *backer)
> backer->rt_support.odp.nd_ext = check_nd_extensions(backer);
> }
>
> +/* TC does not support offloading the explicit drop action. As such we need
> to
> + * re-probe the datapath if hw-offload has been modified.
> + * Note: We don't support true --> false transition as that requires a
> restart.
> + * See netdev_set_flow_api_enabled(). */
> +static bool
> +recheck_support_explicit_drop_action(struct dpif_backer *backer)
> +{
> + bool explicit_drop_action;
> +
> + atomic_read_relaxed(&backer->rt_support.explicit_drop_action,
> + &explicit_drop_action);
> +
> + if (explicit_drop_action
> + && !dpif_may_support_explicit_drop_action(backer->dpif)) {
> + ovs_assert(!check_drop_action(backer));
If we only call this for the log message, would just adding the log message
here not be better?
It avoids the installation of a flow, and I have not thought this through, but
it might confuse the revalidator.
> + atomic_store_relaxed(&backer->rt_support.explicit_drop_action,
> false);
> + return true;
> + }
> +
> + return false;
> +}
> +
> static int
> construct(struct ofproto *ofproto_)
> {
> @@ -5714,6 +5779,7 @@ ct_zone_limit_protection_update(const char
> *datapath_type, bool protected)
> static void
> get_datapath_cap(const char *datapath_type, struct smap *cap)
> {
> + bool explicit_drop_action;
> struct dpif_backer_support *s;
> struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
> datapath_type);
> @@ -5749,8 +5815,9 @@ get_datapath_cap(const char *datapath_type, struct smap
> *cap)
> smap_add_format(cap, "max_hash_alg", "%"PRIuSIZE, s->max_hash_alg);
> smap_add(cap, "check_pkt_len", s->check_pkt_len ? "true" : "false");
> smap_add(cap, "ct_timeout", s->ct_timeout ? "true" : "false");
> + atomic_read_relaxed(&s->explicit_drop_action, &explicit_drop_action);
> smap_add(cap, "explicit_drop_action",
> - s->explicit_drop_action ? "true" :"false");
> + explicit_drop_action ? "true" :"false");
> 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");
> @@ -6279,7 +6346,7 @@ show_dp_feature_bool(struct ds *ds, const char
> *feature, const bool *b)
> ds_put_format(ds, "%s: %s\n", feature, *b ? "Yes" : "No");
> }
>
> -static void OVS_UNUSED
> +static void
> show_dp_feature_atomic_bool(struct ds *ds, const char *feature,
> const atomic_bool *b)
> {
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index 92d33aa64709..d33f73df8aed 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -51,6 +51,7 @@
> #include "hmapx.h"
> #include "odp-util.h"
> #include "id-pool.h"
> +#include "ovs-atomic.h"
> #include "ovs-thread.h"
> #include "ofproto-provider.h"
> #include "util.h"
> @@ -202,7 +203,8 @@ struct group_dpif *group_dpif_lookup(struct ofproto_dpif
> *,
> DPIF_SUPPORT_FIELD(bool, ct_timeout, "Conntrack timeout policy") \
> \
> /* True if the datapath supports explicit drop action. */ \
> - DPIF_SUPPORT_FIELD(bool, explicit_drop_action, "Explicit Drop action") \
> + DPIF_SUPPORT_FIELD(atomic_bool, explicit_drop_action, \
> + "Explicit Drop action") \
> \
> /* True if the datapath supports balance_tcp optimization */ \
> DPIF_SUPPORT_FIELD(bool, lb_output_action, "Optimized Balance TCP mode")\
> --
> 2.43.0
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
