On Thu, Mar 07, 2024 at 08:35:30PM +0100, Ilya Maximets wrote:
> On 3/7/24 18:08, 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]>
> > ---
> > include/linux/openvswitch.h | 2 +-
> > lib/dpif.c | 6 ---
> > lib/dpif.h | 1 -
> > ofproto/ofproto-dpif.c | 89 +++++++++++++++++++++++++++++++++++--
> > ofproto/ofproto-dpif.h | 4 +-
> > 5 files changed, 89 insertions(+), 13 deletions(-)
>
> Not a full review, but a few comments inline.
>
> This patch will need to remove the OVS_UNUSED mark.
>
> >
> > 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..28fa40879655 100644
> > --- a/lib/dpif.c
> > +++ b/lib/dpif.c
> > @@ -1934,12 +1934,6 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
> > return dpif_is_netdev(dpif);
> > }
> >
> > -bool
> > -dpif_supports_explicit_drop_action(const struct dpif *dpif)
> > -{
> > - return dpif_is_netdev(dpif);
> > -}
>
> I'm not sure if calling dpif_is_netdev() from ofproto-dpif layer
> is a good idea. Maybe add the hwol check into this function and
> rename it into dpif_may_support_explicit_drop_action()?
>
> And call this from two places in ofproto-dpif.c below.
>
> What do you think?
Makes sense. I'll give it a try.
> > -
> > bool
> > dpif_supports_lb_output_action(const struct dpif *dpif)
> > {
> > diff --git a/lib/dpif.h b/lib/dpif.h
> > index 0f2dc2ef3c55..8dc5da45b3ef 100644
> > --- a/lib/dpif.h
> > +++ b/lib/dpif.h
> > @@ -940,7 +940,6 @@ 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_synced_dp_layers(struct dpif *);
> >
> > /* Log functions. */
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 4e22ee0e8045..b9d35d6efa62 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -72,6 +72,8 @@
> > #include "util.h"
> > #include "uuid.h"
> > #include "vlan-bitmap.h"
> > +#include "dpif-netdev.h"
> > +#include "netdev-offload.h"
>
> Moving the checks back to dpif.c will get rid of these strange
> includes.
ACK
> >
> > VLOG_DEFINE_THIS_MODULE(ofproto_dpif);
> >
> > @@ -221,6 +223,8 @@ 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 check_drop_action(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 +395,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 +815,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 +864,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;
> > + atomic_read_relaxed(&ofproto->backer->rt_support.explicit_drop_action,
> > + &value);
> > + return value;
> > }
> >
> > bool
> > @@ -1379,6 +1391,43 @@ 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);
> > +
> > + /* TC does not support offloading this action. */
> > + hw_offload = netdev_is_flow_api_enabled() &&
> > !dpif_is_netdev(backer->dpif);
> > +
> > + VLOG_INFO("%s: Datapath %s drop action%s", dpif_name(backer->dpif),
>
> s/drop action/explicit drop action/
>
> Otherwise it may look misleading in the log as flow dumps do show
> 'drop' actions.
ACK
> > + (supported) ? "supports" : "does not support",
> > + (hw_offload) ? ", but not enabled due to hardware offload
> > being "
> > + "enabled" : "");
>
> May skip the 'being enabled' part.
ACK
> > +
> > + 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)
> > @@ -1647,8 +1696,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);
> > @@ -1665,6 +1714,36 @@ 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) {
>
> '{' should be on a new line.
ACK
> > + bool explicit_drop_action;
> > +
> > + atomic_read_relaxed(&backer->rt_support.explicit_drop_action,
> > + &explicit_drop_action);
> > +
> > + if (explicit_drop_action
> > + && netdev_is_flow_api_enabled() && !dpif_is_netdev(backer->dpif)) {
> > + bool new_explicit_drop_action = check_drop_action(backer);
> > +
> > + if (!new_explicit_drop_action) {
> > + atomic_compare_exchange_strong_relaxed(
> > +
> > &backer->rt_support.explicit_drop_action,
> > + &explicit_drop_action,
> > + new_explicit_drop_action);
> > + /* If the cmpx fails, it means the value changed to false in a
> > + * different thread. As such, we still want to trigger a
> > + * reconfigure.
> > + */
>
> This function should not be called in parallel, so atomic exchange
> may be unnecessary. It should be fine to just write it unconditionally
> and return the new value.
ACK
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
