On Fri, Mar 15, 2024 at 12:37:54PM -0400, Eric Garver wrote:
> On Fri, Mar 15, 2024 at 04:55:50PM +0100, Eelco Chaudron wrote:
> >
> >
> > 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 <[email protected]>
> >
> > 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.
>
> ACK.
>
> > > +
> > > 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?
>
> The reason it was changed to user
> dpif_may_support_explicit_drop_action() was because it was suggested to
> avoid calling dpif_is_netdev() from ofproto. I assume it's similar for
> netdev_is_flow_api_enabled().
>
> "hw_offload" is probably a bad variable name. Maybe we completely avoid
> mentioning hw_offload here and in the log. Instead we do:
>
> + supported = dpif_may_support_explicit_drop_action(backer->dpif)
> && dpif_probe_feature(backer->dpif, "drop", &key, &actions,
> NULL);
>
>
> Is this agreeable?
>
> > > +
> > > + 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"
> > > : "");
>
> We would simply remove the log mentioning 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?
>
> I'll make the change.
>
> IIRC, unsetting "hw-offload" requires a restart. Setting it does not.
> Even though it's documented that a restart is required.
>
> other_config : hw-offload: optional string, either true or false
> Set this value to true to enable netdev flow offload.
>
> The default value is false. Changing this value requires
> restarting the daemon
> [..]
>
> https://www.openvswitch.org/support/dist-docs/ovs-vswitchd.conf.db.5.html
Testing and code inspection, i.e. netdev_set_flow_api_enabled(), confirm
that we don't support setting hw-offload=false without a restart. We
only support false --> true at runtime.
As such, I won't add support for this here.
> > > + }
> > > +
> > > + 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
> > > [email protected]
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> > _______________________________________________
> > 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
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
