| Message ID | 20240311175134.67252-5-eric@garver.life |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dpif: probe support for OVS_ACTION_ATTR_DROP | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
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 <eric@garver.life> 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. > + 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? > + > + 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" : ""); > + > + 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? > + } > + > + 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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 <eric@garver.life> > > 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 > > + } > > + > > + 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 15 Mar 2024, at 17:37, 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 <eric@garver.life> >> >> 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. Yes, we might just remove hw_offload, and do something like: (may_support) ? ", but not enabled due to a conflicting configuration" : ""); But this might just raise the question, of what the conflicting configuration might be ;) >>> + 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" : ""); >>> + >>> + 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. In theory, unsetting it will keep active flows in TC, only new/updated flows will be put in the kernel. So triggering the reconfigure in both will at least solve some of the potential problems ;) > 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 > >>> + } >>> + >>> + 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 >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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 <eric@garver.life> > > > > 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 > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 15 Mar 2024, at 18:59, Eric Garver wrote: > 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 <eric@garver.life> >>> >>> 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. ACK, was not aware of this... Thanks, no I know ;) >>>> + } >>>> + >>>> + 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 >>>> dev@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
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; + 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); + + 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" : ""); + + 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; + } + + 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")\
Kernel support has been added for this action. As such, we need to probe the datapath for support. Signed-off-by: Eric Garver <eric@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(-)