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, and I have not thought this through, > but it might confuse the revalidator.
So you and Ilya would rather I insert a VLOG_INFO() here? That's fine with me. 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