On 4/5/24 15:01, Eric Garver wrote: > On Fri, Apr 05, 2024 at 11:39:58AM +0200, Eelco Chaudron wrote: >> >> >> 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,
We're not going to install the flow, because there is a check for dpif_may_support_explicit_drop_action() in the check_drop_action(). >> and I have not thought this through, but it might confuse the revalidator. So, the revalidator will not be confused. > > So you and Ilya would rather I insert a VLOG_INFO() here? That's fine > with me. I'd keep as is, unless Eelco has a strong opinion towards the log. > > Can we agree on the message before I post another revision? > > VLOG_INFO("%s: Datapath supports explicit drop action, " > "but disabled due to hw-offload=true.", > dpif_name(backer->dpif)) > > Is that agreeable? > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev