On 11 Mar 2024, at 18:51, 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>
Thanks for submitting the changes requested. Some comments below.
Cheers,
Eelco
> ---
> 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 a265e05ad253..fce6456f47c8 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 2ec1cd1854ab..7bacd2120c78 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;
> @@ -807,6 +812,7 @@ open_dpif_backer(const char *type, struct dpif_backer
> **backerp)
>
> shash_add(&all_dpif_backers, type, backer);
>
> + atomic_init(&backer->rt_support.explicit_drop_action, false);
> check_support(backer);
> atomic_count_init(&backer->tnl_count, 0);
>
> @@ -855,7 +861,10 @@ 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;
nit: I would add a new line here.
> + atomic_read_relaxed(&ofproto->backer->rt_support.explicit_drop_action,
> + &value);
> + return value;
> }
>
> bool
> @@ -1379,6 +1388,41 @@ 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;
> + struct flow flow;
> + bool supported;
> + bool hw_offload;
> +
> + 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_use_stack(&actions, &actbuf, sizeof actbuf);
> + nl_msg_put_u32(&actions, OVS_ACTION_ATTR_DROP, XLATE_OK);
> +
> + supported = dpif_probe_feature(backer->dpif, "drop", &key, &actions,
> NULL);
> + hw_offload = !dpif_may_support_explicit_drop_action(backer->dpif);
The use case of this function seems odd here. This since
dpif_may_support_explicit_drop_action() doesn't imply that we're specifically
checking for HW offload being the issue. If someone updates the function with
another reason for it not being supported we will break the implementation
here. What do you think?
> +
> + VLOG_INFO("%s: Datapath %s explicit drop action%s",
> + dpif_name(backer->dpif),
> + (supported) ? "supports" : "does not support",
> + (hw_offload) ? ", but not enabled due to hardware offload" :
> "");
> +
> + return supported && !hw_offload;
> +}
> +
> /* Tests whether 'backer''s datapath supports the all-zero SNAT case. */
> static bool
> dpif_supports_ct_zero_snat(struct dpif_backer *backer)
> @@ -1649,8 +1693,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 +1711,27 @@ check_support(struct dpif_backer *backer)
> backer->rt_support.odp.nd_ext = check_nd_extensions(backer);
> }
>
> +/* For TC, if vswitchd started with other_config:hw-offload set to "false",
> and
> + * the configuration is now "true", then we need to re-probe datapath support
> + * for the "drop" action. */
> +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)
> + && !check_drop_action(backer)) {
> + atomic_store_relaxed(&backer->rt_support.explicit_drop_action,
> false);
> + return true;
This should probably also work in the reverse direction. If it’s enabled on
startup, but now disabled?
> + }
> +
> + return false;
> +}
> +
> static int
> construct(struct ofproto *ofproto_)
> {
> @@ -5708,6 +5773,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);
> @@ -5743,8 +5809,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");
> @@ -6273,7 +6340,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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
