On Fri, Mar 29, 2024 at 12:26:11AM +0100, Ilya Maximets wrote:
> 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 <[email protected]>
> > Signed-off-by: Eric Garver <[email protected]>
> > ---
> > 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.
Hrm.. It's _set_ in check_support(), my initial understanding of
ovs-atomics.h was that they also needed to be _initialized_. However,
all but clang has the same assignment macro for atomic_init(). This
macro is used throughout the code.
What do you think?
> > 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.
ACK.
> > + 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
