On 3/22/24 14:54, Eric Garver wrote: > Kernel support has been added for this action. As such, we need to probe > the datapath for support. > > Acked-by: Eelco Chaudron <echau...@redhat.com> > 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 | 78 ++++++++++++++++++++++++++++++++++--- > ofproto/ofproto-dpif.h | 4 +- > 5 files changed, 82 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 c4e2e867ecdc..b854ba3636ed 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);
This initialization seems unnecessary since the field will be initialized in the check_support() right after. And it also a bit out of place, since we are not initializing anything else. > check_support(backer); > atomic_count_init(&backer->tnl_count, 0); > > @@ -855,7 +861,12 @@ 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); > + May remove te empty lne here. > + return value; > } > > bool > @@ -1379,6 +1390,39 @@ 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; > + > + 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_may_support_explicit_drop_action(backer->dpif) && > + dpif_probe_feature(backer->dpif, "drop", &key, &actions, > NULL); Would be better if we probed actions with dpif_execute() instead of dpif_probe_feature(), since dpif_execute() doesn't interfere with the datapath, i.e. doesn't install actual flows. But I will not insist on that, since we do already have some actions probed this way. Maybe for a future larger cleanup. > + > + 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 +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,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) > + && !check_drop_action(backer)) { Shouldn't last two conditions be || instad of && ? > + atomic_store_relaxed(&backer->rt_support.explicit_drop_action, > false); > + return true; > + } > + > + return false; > +} > + > static int > construct(struct ofproto *ofproto_) > { > @@ -5714,6 +5780,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 +5816,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 +6347,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")\ _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev