| Message ID | 20240307170810.63088-5-eric@garver.life |
|---|---|
| State | Changes Requested |
| 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 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 <eric@garver.life> > --- > 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? > - > 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. > > 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. > + (supported) ? "supports" : "does not support", > + (hw_offload) ? ", but not enabled due to hardware offload being " > + "enabled" : ""); May skip the 'being enabled' part. > + > + 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. > + 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. > + return true; > + } > + } > + > + return false; > +} > + > static int > construct(struct ofproto *ofproto_) > { > @@ -5706,6 +5785,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); > @@ -5741,8 +5821,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"); > 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")\
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 <eric@garver.life> > > --- > > 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
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); -} - 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" 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), + (supported) ? "supports" : "does not support", + (hw_offload) ? ", but not enabled due to hardware offload being " + "enabled" : ""); + + 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) { + 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. + */ + return true; + } + } + + return false; +} + static int construct(struct ofproto *ofproto_) { @@ -5706,6 +5785,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); @@ -5741,8 +5821,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"); 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 | 1 - ofproto/ofproto-dpif.c | 89 +++++++++++++++++++++++++++++++++++-- ofproto/ofproto-dpif.h | 4 +- 5 files changed, 89 insertions(+), 13 deletions(-)