| Message ID | 20240322135439.11415-5-eric@garver.life |
|---|---|
| State | Changes Requested |
| Delegated to: | Eelco Chaudron |
| 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 | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 3/22/24 14:54, Eric Garver wrote: > Kernel support has been added for this action. As such, we need to probe > the datapath for support. > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > 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 | 78 ++++++++++++++++++++++++++++++++++--- > ofproto/ofproto-dpif.h | 4 +- > 5 files changed, 82 insertions(+), 10 deletions(-) > > diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h > index a265e05ad253..fce6456f47c8 100644 > --- a/include/linux/openvswitch.h > +++ b/include/linux/openvswitch.h > @@ -1086,11 +1086,11 @@ enum ovs_action_attr { > OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */ > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ > OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ > + OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */ > > #ifndef __KERNEL__ > OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/ > OVS_ACTION_ATTR_TUNNEL_POP, /* u32 port number. */ > - OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */ > OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */ > #endif > __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted > diff --git a/lib/dpif.c b/lib/dpif.c > index 0f480bec48d0..23eb18495a66 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > @@ -28,6 +28,7 @@ > #include "dpctl.h" > #include "dpif-netdev.h" > #include "flow.h" > +#include "netdev-offload.h" > #include "netdev-provider.h" > #include "netdev.h" > #include "netlink.h" > @@ -1935,9 +1936,10 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif) > } > > bool > -dpif_supports_explicit_drop_action(const struct dpif *dpif) > +dpif_may_support_explicit_drop_action(const struct dpif *dpif) > { > - return dpif_is_netdev(dpif); > + /* TC does not support offloading this action. */ > + return dpif_is_netdev(dpif) || !netdev_is_flow_api_enabled(); > } > > bool > diff --git a/lib/dpif.h b/lib/dpif.h > index 0f2dc2ef3c55..a764e8a592bd 100644 > --- a/lib/dpif.h > +++ b/lib/dpif.h > @@ -940,7 +940,7 @@ int dpif_get_pmds_for_port(const struct dpif * dpif, odp_port_t port_no, > > char *dpif_get_dp_version(const struct dpif *); > bool dpif_supports_tnl_push_pop(const struct dpif *); > -bool dpif_supports_explicit_drop_action(const struct dpif *); > +bool dpif_may_support_explicit_drop_action(const struct dpif *); > bool dpif_synced_dp_layers(struct dpif *); > > /* Log functions. */ > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index c4e2e867ecdc..b854ba3636ed 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -221,6 +221,7 @@ static void ct_zone_config_init(struct dpif_backer *backer); > static void ct_zone_config_uninit(struct dpif_backer *backer); > static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer); > static void ct_zone_limits_commit(struct dpif_backer *backer); > +static bool recheck_support_explicit_drop_action(struct dpif_backer *backer); > > static inline struct ofproto_dpif * > ofproto_dpif_cast(const struct ofproto *ofproto) > @@ -391,6 +392,10 @@ type_run(const char *type) > udpif_set_threads(backer->udpif, n_handlers, n_revalidators); > } > > + if (recheck_support_explicit_drop_action(backer)) { > + backer->need_revalidate = REV_RECONFIGURE; > + } > + > if (backer->need_revalidate) { > struct ofproto_dpif *ofproto; > struct simap_node *node; > @@ -807,6 +812,7 @@ open_dpif_backer(const char *type, struct dpif_backer **backerp) > > shash_add(&all_dpif_backers, type, backer); > > + atomic_init(&backer->rt_support.explicit_drop_action, false); This initialization seems unnecessary since the field will be initialized in the check_support() right after. And it also a bit out of place, since we are not initializing anything else. > check_support(backer); > atomic_count_init(&backer->tnl_count, 0); > > @@ -855,7 +861,12 @@ ovs_native_tunneling_is_on(struct ofproto_dpif *ofproto) > bool > ovs_explicit_drop_action_supported(struct ofproto_dpif *ofproto) > { > - return ofproto->backer->rt_support.explicit_drop_action; > + bool value; > + > + atomic_read_relaxed(&ofproto->backer->rt_support.explicit_drop_action, > + &value); > + May remove te empty lne here. > + return value; > } > > bool > @@ -1379,6 +1390,39 @@ check_ct_timeout_policy(struct dpif_backer *backer) > return !error; > } > > +/* Tests whether backer's datapath supports the OVS_ACTION_ATTR_DROP action. */ > +static bool > +check_drop_action(struct dpif_backer *backer) > +{ > + struct odputil_keybuf keybuf; > + uint8_t actbuf[NL_A_U32_SIZE]; > + struct ofpbuf actions; > + struct ofpbuf key; > + struct flow flow; > + bool supported; > + > + struct odp_flow_key_parms odp_parms = { > + .flow = &flow, > + .probe = true, > + }; > + > + memset(&flow, 0, sizeof flow); > + ofpbuf_use_stack(&key, &keybuf, sizeof keybuf); > + odp_flow_key_from_flow(&odp_parms, &key); > + > + ofpbuf_use_stack(&actions, &actbuf, sizeof actbuf); > + nl_msg_put_u32(&actions, OVS_ACTION_ATTR_DROP, XLATE_OK); > + > + supported = dpif_may_support_explicit_drop_action(backer->dpif) && > + dpif_probe_feature(backer->dpif, "drop", &key, &actions, NULL); Would be better if we probed actions with dpif_execute() instead of dpif_probe_feature(), since dpif_execute() doesn't interfere with the datapath, i.e. doesn't install actual flows. But I will not insist on that, since we do already have some actions probed this way. Maybe for a future larger cleanup. > + > + VLOG_INFO("%s: Datapath %s explicit drop action", > + dpif_name(backer->dpif), > + (supported) ? "supports" : "does not support"); > + > + return supported; > +} > + > /* Tests whether 'backer''s datapath supports the all-zero SNAT case. */ > static bool > dpif_supports_ct_zero_snat(struct dpif_backer *backer) > @@ -1649,8 +1693,8 @@ check_support(struct dpif_backer *backer) > backer->rt_support.max_hash_alg = check_max_dp_hash_alg(backer); > backer->rt_support.check_pkt_len = check_check_pkt_len(backer); > backer->rt_support.ct_timeout = check_ct_timeout_policy(backer); > - backer->rt_support.explicit_drop_action = > - dpif_supports_explicit_drop_action(backer->dpif); > + atomic_store_relaxed(&backer->rt_support.explicit_drop_action, > + check_drop_action(backer)); > backer->rt_support.lb_output_action = > dpif_supports_lb_output_action(backer->dpif); > backer->rt_support.ct_zero_snat = dpif_supports_ct_zero_snat(backer); > @@ -1667,6 +1711,28 @@ check_support(struct dpif_backer *backer) > backer->rt_support.odp.nd_ext = check_nd_extensions(backer); > } > > +/* TC does not support offloading the explicit drop action. As such we need to > + * re-probe the datapath if hw-offload has been modified. > + * Note: We don't support true --> false transition as that requires a restart. > + * See netdev_set_flow_api_enabled(). */ > +static bool > +recheck_support_explicit_drop_action(struct dpif_backer *backer) > +{ > + bool explicit_drop_action; > + > + atomic_read_relaxed(&backer->rt_support.explicit_drop_action, > + &explicit_drop_action); > + > + if (explicit_drop_action > + && !dpif_may_support_explicit_drop_action(backer->dpif) > + && !check_drop_action(backer)) { Shouldn't last two conditions be || instad of && ? > + atomic_store_relaxed(&backer->rt_support.explicit_drop_action, false); > + return true; > + } > + > + return false; > +} > + > static int > construct(struct ofproto *ofproto_) > { > @@ -5714,6 +5780,7 @@ ct_zone_limit_protection_update(const char *datapath_type, bool protected) > static void > get_datapath_cap(const char *datapath_type, struct smap *cap) > { > + bool explicit_drop_action; > struct dpif_backer_support *s; > struct dpif_backer *backer = shash_find_data(&all_dpif_backers, > datapath_type); > @@ -5749,8 +5816,9 @@ get_datapath_cap(const char *datapath_type, struct smap *cap) > smap_add_format(cap, "max_hash_alg", "%"PRIuSIZE, s->max_hash_alg); > smap_add(cap, "check_pkt_len", s->check_pkt_len ? "true" : "false"); > smap_add(cap, "ct_timeout", s->ct_timeout ? "true" : "false"); > + atomic_read_relaxed(&s->explicit_drop_action, &explicit_drop_action); > smap_add(cap, "explicit_drop_action", > - s->explicit_drop_action ? "true" :"false"); > + explicit_drop_action ? "true" :"false"); > smap_add(cap, "lb_output_action", s->lb_output_action ? "true" : "false"); > smap_add(cap, "ct_zero_snat", s->ct_zero_snat ? "true" : "false"); > smap_add(cap, "add_mpls", s->add_mpls ? "true" : "false"); > @@ -6279,7 +6347,7 @@ show_dp_feature_bool(struct ds *ds, const char *feature, const bool *b) > ds_put_format(ds, "%s: %s\n", feature, *b ? "Yes" : "No"); > } > > -static void OVS_UNUSED > +static void > show_dp_feature_atomic_bool(struct ds *ds, const char *feature, > const atomic_bool *b) > { > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h > index 92d33aa64709..d33f73df8aed 100644 > --- a/ofproto/ofproto-dpif.h > +++ b/ofproto/ofproto-dpif.h > @@ -51,6 +51,7 @@ > #include "hmapx.h" > #include "odp-util.h" > #include "id-pool.h" > +#include "ovs-atomic.h" > #include "ovs-thread.h" > #include "ofproto-provider.h" > #include "util.h" > @@ -202,7 +203,8 @@ struct group_dpif *group_dpif_lookup(struct ofproto_dpif *, > DPIF_SUPPORT_FIELD(bool, ct_timeout, "Conntrack timeout policy") \ > \ > /* True if the datapath supports explicit drop action. */ \ > - DPIF_SUPPORT_FIELD(bool, explicit_drop_action, "Explicit Drop action") \ > + DPIF_SUPPORT_FIELD(atomic_bool, explicit_drop_action, \ > + "Explicit Drop action") \ > \ > /* True if the datapath supports balance_tcp optimization */ \ > DPIF_SUPPORT_FIELD(bool, lb_output_action, "Optimized Balance TCP mode")\
On 3/29/24 00:26, Ilya Maximets wrote: > On 3/22/24 14:54, Eric Garver wrote: >> Kernel support has been added for this action. As such, we need to probe >> the datapath for support. >> >> Acked-by: Eelco Chaudron <echaudro@redhat.com> >> 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 | 78 ++++++++++++++++++++++++++++++++++--- >> ofproto/ofproto-dpif.h | 4 +- >> 5 files changed, 82 insertions(+), 10 deletions(-) >> >> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h >> index a265e05ad253..fce6456f47c8 100644 >> --- a/include/linux/openvswitch.h >> +++ b/include/linux/openvswitch.h >> @@ -1086,11 +1086,11 @@ enum ovs_action_attr { >> OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */ >> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ >> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ >> + OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */ >> >> #ifndef __KERNEL__ >> OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/ >> OVS_ACTION_ATTR_TUNNEL_POP, /* u32 port number. */ >> - OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */ >> OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */ >> #endif >> __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted >> diff --git a/lib/dpif.c b/lib/dpif.c >> index 0f480bec48d0..23eb18495a66 100644 >> --- a/lib/dpif.c >> +++ b/lib/dpif.c >> @@ -28,6 +28,7 @@ >> #include "dpctl.h" >> #include "dpif-netdev.h" >> #include "flow.h" >> +#include "netdev-offload.h" >> #include "netdev-provider.h" >> #include "netdev.h" >> #include "netlink.h" >> @@ -1935,9 +1936,10 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif) >> } >> >> bool >> -dpif_supports_explicit_drop_action(const struct dpif *dpif) >> +dpif_may_support_explicit_drop_action(const struct dpif *dpif) >> { >> - return dpif_is_netdev(dpif); >> + /* TC does not support offloading this action. */ >> + return dpif_is_netdev(dpif) || !netdev_is_flow_api_enabled(); >> } >> >> bool >> diff --git a/lib/dpif.h b/lib/dpif.h >> index 0f2dc2ef3c55..a764e8a592bd 100644 >> --- a/lib/dpif.h >> +++ b/lib/dpif.h >> @@ -940,7 +940,7 @@ int dpif_get_pmds_for_port(const struct dpif * dpif, odp_port_t port_no, >> >> char *dpif_get_dp_version(const struct dpif *); >> bool dpif_supports_tnl_push_pop(const struct dpif *); >> -bool dpif_supports_explicit_drop_action(const struct dpif *); >> +bool dpif_may_support_explicit_drop_action(const struct dpif *); >> bool dpif_synced_dp_layers(struct dpif *); >> >> /* Log functions. */ >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index c4e2e867ecdc..b854ba3636ed 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -221,6 +221,7 @@ static void ct_zone_config_init(struct dpif_backer *backer); >> static void ct_zone_config_uninit(struct dpif_backer *backer); >> static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer); >> static void ct_zone_limits_commit(struct dpif_backer *backer); >> +static bool recheck_support_explicit_drop_action(struct dpif_backer *backer); >> >> static inline struct ofproto_dpif * >> ofproto_dpif_cast(const struct ofproto *ofproto) >> @@ -391,6 +392,10 @@ type_run(const char *type) >> udpif_set_threads(backer->udpif, n_handlers, n_revalidators); >> } >> >> + if (recheck_support_explicit_drop_action(backer)) { >> + backer->need_revalidate = REV_RECONFIGURE; >> + } >> + >> if (backer->need_revalidate) { >> struct ofproto_dpif *ofproto; >> struct simap_node *node; >> @@ -807,6 +812,7 @@ open_dpif_backer(const char *type, struct dpif_backer **backerp) >> >> shash_add(&all_dpif_backers, type, backer); >> >> + atomic_init(&backer->rt_support.explicit_drop_action, false); > > This initialization seems unnecessary since the field will be initialized > in the check_support() right after. And it also a bit out of place, since > we are not initializing anything else. > >> check_support(backer); >> atomic_count_init(&backer->tnl_count, 0); >> >> @@ -855,7 +861,12 @@ ovs_native_tunneling_is_on(struct ofproto_dpif *ofproto) >> bool >> ovs_explicit_drop_action_supported(struct ofproto_dpif *ofproto) >> { >> - return ofproto->backer->rt_support.explicit_drop_action; >> + bool value; >> + >> + atomic_read_relaxed(&ofproto->backer->rt_support.explicit_drop_action, >> + &value); >> + > > May remove te empty lne here. > >> + return value; >> } >> >> bool >> @@ -1379,6 +1390,39 @@ check_ct_timeout_policy(struct dpif_backer *backer) >> return !error; >> } >> >> +/* Tests whether backer's datapath supports the OVS_ACTION_ATTR_DROP action. */ >> +static bool >> +check_drop_action(struct dpif_backer *backer) >> +{ >> + struct odputil_keybuf keybuf; >> + uint8_t actbuf[NL_A_U32_SIZE]; >> + struct ofpbuf actions; >> + struct ofpbuf key; >> + struct flow flow; >> + bool supported; >> + >> + struct odp_flow_key_parms odp_parms = { >> + .flow = &flow, >> + .probe = true, >> + }; >> + >> + memset(&flow, 0, sizeof flow); >> + ofpbuf_use_stack(&key, &keybuf, sizeof keybuf); >> + odp_flow_key_from_flow(&odp_parms, &key); >> + >> + ofpbuf_use_stack(&actions, &actbuf, sizeof actbuf); >> + nl_msg_put_u32(&actions, OVS_ACTION_ATTR_DROP, XLATE_OK); >> + >> + supported = dpif_may_support_explicit_drop_action(backer->dpif) && >> + dpif_probe_feature(backer->dpif, "drop", &key, &actions, NULL); > > Would be better if we probed actions with dpif_execute() instead > of dpif_probe_feature(), since dpif_execute() doesn't interfere > with the datapath, i.e. doesn't install actual flows. > > But I will not insist on that, since we do already have some > actions probed this way. Maybe for a future larger cleanup. Maybe add a match on some bogus dl_type like 0x1234 to the flow though, so we're sure that it doesn't just drop all packets unconditionally when installed. "flow.dl_type = htons(0x1234);" should be enough. > >> + >> + VLOG_INFO("%s: Datapath %s explicit drop action", >> + dpif_name(backer->dpif), >> + (supported) ? "supports" : "does not support"); >> + >> + return supported; >> +} >> + >> /* Tests whether 'backer''s datapath supports the all-zero SNAT case. */ >> static bool >> dpif_supports_ct_zero_snat(struct dpif_backer *backer) >> @@ -1649,8 +1693,8 @@ check_support(struct dpif_backer *backer) >> backer->rt_support.max_hash_alg = check_max_dp_hash_alg(backer); >> backer->rt_support.check_pkt_len = check_check_pkt_len(backer); >> backer->rt_support.ct_timeout = check_ct_timeout_policy(backer); >> - backer->rt_support.explicit_drop_action = >> - dpif_supports_explicit_drop_action(backer->dpif); >> + atomic_store_relaxed(&backer->rt_support.explicit_drop_action, >> + check_drop_action(backer)); >> backer->rt_support.lb_output_action = >> dpif_supports_lb_output_action(backer->dpif); >> backer->rt_support.ct_zero_snat = dpif_supports_ct_zero_snat(backer); >> @@ -1667,6 +1711,28 @@ check_support(struct dpif_backer *backer) >> backer->rt_support.odp.nd_ext = check_nd_extensions(backer); >> } >> >> +/* TC does not support offloading the explicit drop action. As such we need to >> + * re-probe the datapath if hw-offload has been modified. >> + * Note: We don't support true --> false transition as that requires a restart. >> + * See netdev_set_flow_api_enabled(). */ >> +static bool >> +recheck_support_explicit_drop_action(struct dpif_backer *backer) >> +{ >> + bool explicit_drop_action; >> + >> + atomic_read_relaxed(&backer->rt_support.explicit_drop_action, >> + &explicit_drop_action); >> + >> + if (explicit_drop_action >> + && !dpif_may_support_explicit_drop_action(backer->dpif) >> + && !check_drop_action(backer)) { > > Shouldn't last two conditions be || instad of && ? > >> + atomic_store_relaxed(&backer->rt_support.explicit_drop_action, false); >> + return true; >> + } >> + >> + return false; >> +} >> + >> static int >> construct(struct ofproto *ofproto_) >> { >> @@ -5714,6 +5780,7 @@ ct_zone_limit_protection_update(const char *datapath_type, bool protected) >> static void >> get_datapath_cap(const char *datapath_type, struct smap *cap) >> { >> + bool explicit_drop_action; >> struct dpif_backer_support *s; >> struct dpif_backer *backer = shash_find_data(&all_dpif_backers, >> datapath_type); >> @@ -5749,8 +5816,9 @@ get_datapath_cap(const char *datapath_type, struct smap *cap) >> smap_add_format(cap, "max_hash_alg", "%"PRIuSIZE, s->max_hash_alg); >> smap_add(cap, "check_pkt_len", s->check_pkt_len ? "true" : "false"); >> smap_add(cap, "ct_timeout", s->ct_timeout ? "true" : "false"); >> + atomic_read_relaxed(&s->explicit_drop_action, &explicit_drop_action); >> smap_add(cap, "explicit_drop_action", >> - s->explicit_drop_action ? "true" :"false"); >> + explicit_drop_action ? "true" :"false"); >> smap_add(cap, "lb_output_action", s->lb_output_action ? "true" : "false"); >> smap_add(cap, "ct_zero_snat", s->ct_zero_snat ? "true" : "false"); >> smap_add(cap, "add_mpls", s->add_mpls ? "true" : "false"); >> @@ -6279,7 +6347,7 @@ show_dp_feature_bool(struct ds *ds, const char *feature, const bool *b) >> ds_put_format(ds, "%s: %s\n", feature, *b ? "Yes" : "No"); >> } >> >> -static void OVS_UNUSED >> +static void >> show_dp_feature_atomic_bool(struct ds *ds, const char *feature, >> const atomic_bool *b) >> { >> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h >> index 92d33aa64709..d33f73df8aed 100644 >> --- a/ofproto/ofproto-dpif.h >> +++ b/ofproto/ofproto-dpif.h >> @@ -51,6 +51,7 @@ >> #include "hmapx.h" >> #include "odp-util.h" >> #include "id-pool.h" >> +#include "ovs-atomic.h" >> #include "ovs-thread.h" >> #include "ofproto-provider.h" >> #include "util.h" >> @@ -202,7 +203,8 @@ struct group_dpif *group_dpif_lookup(struct ofproto_dpif *, >> DPIF_SUPPORT_FIELD(bool, ct_timeout, "Conntrack timeout policy") \ >> \ >> /* True if the datapath supports explicit drop action. */ \ >> - DPIF_SUPPORT_FIELD(bool, explicit_drop_action, "Explicit Drop action") \ >> + DPIF_SUPPORT_FIELD(atomic_bool, explicit_drop_action, \ >> + "Explicit Drop action") \ >> \ >> /* True if the datapath supports balance_tcp optimization */ \ >> DPIF_SUPPORT_FIELD(bool, lb_output_action, "Optimized Balance TCP mode")\ >
On Fri, Mar 29, 2024 at 12:26:11AM +0100, Ilya Maximets wrote: > On 3/22/24 14:54, Eric Garver wrote: > > Kernel support has been added for this action. As such, we need to probe > > the datapath for support. > > > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > > 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 | 78 ++++++++++++++++++++++++++++++++++--- > > ofproto/ofproto-dpif.h | 4 +- > > 5 files changed, 82 insertions(+), 10 deletions(-) > > > > diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h > > index a265e05ad253..fce6456f47c8 100644 > > --- a/include/linux/openvswitch.h > > +++ b/include/linux/openvswitch.h > > @@ -1086,11 +1086,11 @@ enum ovs_action_attr { > > OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */ > > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ > > OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ > > + OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */ > > > > #ifndef __KERNEL__ > > OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/ > > OVS_ACTION_ATTR_TUNNEL_POP, /* u32 port number. */ > > - OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */ > > OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */ > > #endif > > __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted > > diff --git a/lib/dpif.c b/lib/dpif.c > > index 0f480bec48d0..23eb18495a66 100644 > > --- a/lib/dpif.c > > +++ b/lib/dpif.c > > @@ -28,6 +28,7 @@ > > #include "dpctl.h" > > #include "dpif-netdev.h" > > #include "flow.h" > > +#include "netdev-offload.h" > > #include "netdev-provider.h" > > #include "netdev.h" > > #include "netlink.h" > > @@ -1935,9 +1936,10 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif) > > } > > > > bool > > -dpif_supports_explicit_drop_action(const struct dpif *dpif) > > +dpif_may_support_explicit_drop_action(const struct dpif *dpif) > > { > > - return dpif_is_netdev(dpif); > > + /* TC does not support offloading this action. */ > > + return dpif_is_netdev(dpif) || !netdev_is_flow_api_enabled(); > > } > > > > bool > > diff --git a/lib/dpif.h b/lib/dpif.h > > index 0f2dc2ef3c55..a764e8a592bd 100644 > > --- a/lib/dpif.h > > +++ b/lib/dpif.h > > @@ -940,7 +940,7 @@ int dpif_get_pmds_for_port(const struct dpif * dpif, odp_port_t port_no, > > > > char *dpif_get_dp_version(const struct dpif *); > > bool dpif_supports_tnl_push_pop(const struct dpif *); > > -bool dpif_supports_explicit_drop_action(const struct dpif *); > > +bool dpif_may_support_explicit_drop_action(const struct dpif *); > > bool dpif_synced_dp_layers(struct dpif *); > > > > /* Log functions. */ > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index c4e2e867ecdc..b854ba3636ed 100644 > > --- a/ofproto/ofproto-dpif.c > > +++ b/ofproto/ofproto-dpif.c > > @@ -221,6 +221,7 @@ static void ct_zone_config_init(struct dpif_backer *backer); > > static void ct_zone_config_uninit(struct dpif_backer *backer); > > static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer); > > static void ct_zone_limits_commit(struct dpif_backer *backer); > > +static bool recheck_support_explicit_drop_action(struct dpif_backer *backer); > > > > static inline struct ofproto_dpif * > > ofproto_dpif_cast(const struct ofproto *ofproto) > > @@ -391,6 +392,10 @@ type_run(const char *type) > > udpif_set_threads(backer->udpif, n_handlers, n_revalidators); > > } > > > > + if (recheck_support_explicit_drop_action(backer)) { > > + backer->need_revalidate = REV_RECONFIGURE; > > + } > > + > > if (backer->need_revalidate) { > > struct ofproto_dpif *ofproto; > > struct simap_node *node; > > @@ -807,6 +812,7 @@ open_dpif_backer(const char *type, struct dpif_backer **backerp) > > > > shash_add(&all_dpif_backers, type, backer); > > > > + atomic_init(&backer->rt_support.explicit_drop_action, false); > > This initialization seems unnecessary since the field will be initialized > in the check_support() right after. And it also a bit out of place, since > we are not initializing anything else. Hrm.. It's _set_ in check_support(), my initial understanding of ovs-atomics.h was that they also needed to be _initialized_. However, all but clang has the same assignment macro for atomic_init(). This macro is used throughout the code. What do you think? > > check_support(backer); > > atomic_count_init(&backer->tnl_count, 0); > > > > @@ -855,7 +861,12 @@ ovs_native_tunneling_is_on(struct ofproto_dpif *ofproto) > > bool > > ovs_explicit_drop_action_supported(struct ofproto_dpif *ofproto) > > { > > - return ofproto->backer->rt_support.explicit_drop_action; > > + bool value; > > + > > + atomic_read_relaxed(&ofproto->backer->rt_support.explicit_drop_action, > > + &value); > > + > > May remove te empty lne here. ACK. > > + return value; > > } > > > > bool > > @@ -1379,6 +1390,39 @@ check_ct_timeout_policy(struct dpif_backer *backer) > > return !error; > > } > > > > +/* Tests whether backer's datapath supports the OVS_ACTION_ATTR_DROP action. */ > > +static bool > > +check_drop_action(struct dpif_backer *backer) > > +{ > > + struct odputil_keybuf keybuf; > > + uint8_t actbuf[NL_A_U32_SIZE]; > > + struct ofpbuf actions; > > + struct ofpbuf key; > > + struct flow flow; > > + bool supported; > > + > > + struct odp_flow_key_parms odp_parms = { > > + .flow = &flow, > > + .probe = true, > > + }; > > + > > + memset(&flow, 0, sizeof flow); > > + ofpbuf_use_stack(&key, &keybuf, sizeof keybuf); > > + odp_flow_key_from_flow(&odp_parms, &key); > > + > > + ofpbuf_use_stack(&actions, &actbuf, sizeof actbuf); > > + nl_msg_put_u32(&actions, OVS_ACTION_ATTR_DROP, XLATE_OK); > > + > > + supported = dpif_may_support_explicit_drop_action(backer->dpif) && > > + dpif_probe_feature(backer->dpif, "drop", &key, &actions, NULL); > > Would be better if we probed actions with dpif_execute() instead > of dpif_probe_feature(), since dpif_execute() doesn't interfere > with the datapath, i.e. doesn't install actual flows. > > But I will not insist on that, since we do already have some > actions probed this way. Maybe for a future larger cleanup. > > > + > > + VLOG_INFO("%s: Datapath %s explicit drop action", > > + dpif_name(backer->dpif), > > + (supported) ? "supports" : "does not support"); > > + > > + return supported; > > +} > > + > > /* Tests whether 'backer''s datapath supports the all-zero SNAT case. */ > > static bool > > dpif_supports_ct_zero_snat(struct dpif_backer *backer) > > @@ -1649,8 +1693,8 @@ check_support(struct dpif_backer *backer) > > backer->rt_support.max_hash_alg = check_max_dp_hash_alg(backer); > > backer->rt_support.check_pkt_len = check_check_pkt_len(backer); > > backer->rt_support.ct_timeout = check_ct_timeout_policy(backer); > > - backer->rt_support.explicit_drop_action = > > - dpif_supports_explicit_drop_action(backer->dpif); > > + atomic_store_relaxed(&backer->rt_support.explicit_drop_action, > > + check_drop_action(backer)); > > backer->rt_support.lb_output_action = > > dpif_supports_lb_output_action(backer->dpif); > > backer->rt_support.ct_zero_snat = dpif_supports_ct_zero_snat(backer); > > @@ -1667,6 +1711,28 @@ check_support(struct dpif_backer *backer) > > backer->rt_support.odp.nd_ext = check_nd_extensions(backer); > > } > > > > +/* TC does not support offloading the explicit drop action. As such we need to > > + * re-probe the datapath if hw-offload has been modified. > > + * Note: We don't support true --> false transition as that requires a restart. > > + * See netdev_set_flow_api_enabled(). */ > > +static bool > > +recheck_support_explicit_drop_action(struct dpif_backer *backer) > > +{ > > + bool explicit_drop_action; > > + > > + atomic_read_relaxed(&backer->rt_support.explicit_drop_action, > > + &explicit_drop_action); > > + > > + if (explicit_drop_action > > + && !dpif_may_support_explicit_drop_action(backer->dpif) > > + && !check_drop_action(backer)) { > > Shouldn't last two conditions be || instad of && ? > > > + atomic_store_relaxed(&backer->rt_support.explicit_drop_action, false); > > + return true; > > + } > > + > > + return false; > > +} > > + > > static int > > construct(struct ofproto *ofproto_) > > { > > @@ -5714,6 +5780,7 @@ ct_zone_limit_protection_update(const char *datapath_type, bool protected) > > static void > > get_datapath_cap(const char *datapath_type, struct smap *cap) > > { > > + bool explicit_drop_action; > > struct dpif_backer_support *s; > > struct dpif_backer *backer = shash_find_data(&all_dpif_backers, > > datapath_type); > > @@ -5749,8 +5816,9 @@ get_datapath_cap(const char *datapath_type, struct smap *cap) > > smap_add_format(cap, "max_hash_alg", "%"PRIuSIZE, s->max_hash_alg); > > smap_add(cap, "check_pkt_len", s->check_pkt_len ? "true" : "false"); > > smap_add(cap, "ct_timeout", s->ct_timeout ? "true" : "false"); > > + atomic_read_relaxed(&s->explicit_drop_action, &explicit_drop_action); > > smap_add(cap, "explicit_drop_action", > > - s->explicit_drop_action ? "true" :"false"); > > + explicit_drop_action ? "true" :"false"); > > smap_add(cap, "lb_output_action", s->lb_output_action ? "true" : "false"); > > smap_add(cap, "ct_zero_snat", s->ct_zero_snat ? "true" : "false"); > > smap_add(cap, "add_mpls", s->add_mpls ? "true" : "false"); > > @@ -6279,7 +6347,7 @@ show_dp_feature_bool(struct ds *ds, const char *feature, const bool *b) > > ds_put_format(ds, "%s: %s\n", feature, *b ? "Yes" : "No"); > > } > > > > -static void OVS_UNUSED > > +static void > > show_dp_feature_atomic_bool(struct ds *ds, const char *feature, > > const atomic_bool *b) > > { > > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h > > index 92d33aa64709..d33f73df8aed 100644 > > --- a/ofproto/ofproto-dpif.h > > +++ b/ofproto/ofproto-dpif.h > > @@ -51,6 +51,7 @@ > > #include "hmapx.h" > > #include "odp-util.h" > > #include "id-pool.h" > > +#include "ovs-atomic.h" > > #include "ovs-thread.h" > > #include "ofproto-provider.h" > > #include "util.h" > > @@ -202,7 +203,8 @@ struct group_dpif *group_dpif_lookup(struct ofproto_dpif *, > > DPIF_SUPPORT_FIELD(bool, ct_timeout, "Conntrack timeout policy") \ > > \ > > /* True if the datapath supports explicit drop action. */ \ > > - DPIF_SUPPORT_FIELD(bool, explicit_drop_action, "Explicit Drop action") \ > > + DPIF_SUPPORT_FIELD(atomic_bool, explicit_drop_action, \ > > + "Explicit Drop action") \ > > \ > > /* True if the datapath supports balance_tcp optimization */ \ > > DPIF_SUPPORT_FIELD(bool, lb_output_action, "Optimized Balance TCP mode")\ > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 4/2/24 16:22, Eric Garver wrote: > On Fri, Mar 29, 2024 at 12:26:11AM +0100, Ilya Maximets wrote: >> On 3/22/24 14:54, Eric Garver wrote: >>> Kernel support has been added for this action. As such, we need to probe >>> the datapath for support. >>> >>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >>> 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 | 78 ++++++++++++++++++++++++++++++++++--- >>> ofproto/ofproto-dpif.h | 4 +- >>> 5 files changed, 82 insertions(+), 10 deletions(-) >>> >>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h >>> index a265e05ad253..fce6456f47c8 100644 >>> --- a/include/linux/openvswitch.h >>> +++ b/include/linux/openvswitch.h >>> @@ -1086,11 +1086,11 @@ enum ovs_action_attr { >>> OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */ >>> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ >>> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ >>> + OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */ >>> >>> #ifndef __KERNEL__ >>> OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/ >>> OVS_ACTION_ATTR_TUNNEL_POP, /* u32 port number. */ >>> - OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */ >>> OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */ >>> #endif >>> __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted >>> diff --git a/lib/dpif.c b/lib/dpif.c >>> index 0f480bec48d0..23eb18495a66 100644 >>> --- a/lib/dpif.c >>> +++ b/lib/dpif.c >>> @@ -28,6 +28,7 @@ >>> #include "dpctl.h" >>> #include "dpif-netdev.h" >>> #include "flow.h" >>> +#include "netdev-offload.h" >>> #include "netdev-provider.h" >>> #include "netdev.h" >>> #include "netlink.h" >>> @@ -1935,9 +1936,10 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif) >>> } >>> >>> bool >>> -dpif_supports_explicit_drop_action(const struct dpif *dpif) >>> +dpif_may_support_explicit_drop_action(const struct dpif *dpif) >>> { >>> - return dpif_is_netdev(dpif); >>> + /* TC does not support offloading this action. */ >>> + return dpif_is_netdev(dpif) || !netdev_is_flow_api_enabled(); >>> } >>> >>> bool >>> diff --git a/lib/dpif.h b/lib/dpif.h >>> index 0f2dc2ef3c55..a764e8a592bd 100644 >>> --- a/lib/dpif.h >>> +++ b/lib/dpif.h >>> @@ -940,7 +940,7 @@ int dpif_get_pmds_for_port(const struct dpif * dpif, odp_port_t port_no, >>> >>> char *dpif_get_dp_version(const struct dpif *); >>> bool dpif_supports_tnl_push_pop(const struct dpif *); >>> -bool dpif_supports_explicit_drop_action(const struct dpif *); >>> +bool dpif_may_support_explicit_drop_action(const struct dpif *); >>> bool dpif_synced_dp_layers(struct dpif *); >>> >>> /* Log functions. */ >>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >>> index c4e2e867ecdc..b854ba3636ed 100644 >>> --- a/ofproto/ofproto-dpif.c >>> +++ b/ofproto/ofproto-dpif.c >>> @@ -221,6 +221,7 @@ static void ct_zone_config_init(struct dpif_backer *backer); >>> static void ct_zone_config_uninit(struct dpif_backer *backer); >>> static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer); >>> static void ct_zone_limits_commit(struct dpif_backer *backer); >>> +static bool recheck_support_explicit_drop_action(struct dpif_backer *backer); >>> >>> static inline struct ofproto_dpif * >>> ofproto_dpif_cast(const struct ofproto *ofproto) >>> @@ -391,6 +392,10 @@ type_run(const char *type) >>> udpif_set_threads(backer->udpif, n_handlers, n_revalidators); >>> } >>> >>> + if (recheck_support_explicit_drop_action(backer)) { >>> + backer->need_revalidate = REV_RECONFIGURE; >>> + } >>> + >>> if (backer->need_revalidate) { >>> struct ofproto_dpif *ofproto; >>> struct simap_node *node; >>> @@ -807,6 +812,7 @@ open_dpif_backer(const char *type, struct dpif_backer **backerp) >>> >>> shash_add(&all_dpif_backers, type, backer); >>> >>> + atomic_init(&backer->rt_support.explicit_drop_action, false); >> >> This initialization seems unnecessary since the field will be initialized >> in the check_support() right after. And it also a bit out of place, since >> we are not initializing anything else. > > Hrm.. It's _set_ in check_support(), my initial understanding of > ovs-atomics.h was that they also needed to be _initialized_. However, > all but clang has the same assignment macro for atomic_init(). This > macro is used throughout the code. > > What do you think? As long as the value is not used before setting and compiler doesn't complain it should be OK to not initialize it. If the compiler complains or will start complaining at some point (there should be no reason for that), I'd prefer we replace xmalloc with xzalloc instead of explicitly initializing this particular field. Best regards, Ilya Maximets.
On Fri, Mar 29, 2024 at 12:26:11AM +0100, Ilya Maximets wrote: > On 3/22/24 14:54, Eric Garver wrote: [..] > > @@ -1649,8 +1693,8 @@ check_support(struct dpif_backer *backer) > > backer->rt_support.max_hash_alg = check_max_dp_hash_alg(backer); > > backer->rt_support.check_pkt_len = check_check_pkt_len(backer); > > backer->rt_support.ct_timeout = check_ct_timeout_policy(backer); > > - backer->rt_support.explicit_drop_action = > > - dpif_supports_explicit_drop_action(backer->dpif); > > + atomic_store_relaxed(&backer->rt_support.explicit_drop_action, > > + check_drop_action(backer)); > > backer->rt_support.lb_output_action = > > dpif_supports_lb_output_action(backer->dpif); > > backer->rt_support.ct_zero_snat = dpif_supports_ct_zero_snat(backer); > > @@ -1667,6 +1711,28 @@ check_support(struct dpif_backer *backer) > > backer->rt_support.odp.nd_ext = check_nd_extensions(backer); > > } > > > > +/* TC does not support offloading the explicit drop action. As such we need to > > + * re-probe the datapath if hw-offload has been modified. > > + * Note: We don't support true --> false transition as that requires a restart. > > + * See netdev_set_flow_api_enabled(). */ > > +static bool > > +recheck_support_explicit_drop_action(struct dpif_backer *backer) > > +{ > > + bool explicit_drop_action; > > + > > + atomic_read_relaxed(&backer->rt_support.explicit_drop_action, > > + &explicit_drop_action); > > + > > + if (explicit_drop_action > > + && !dpif_may_support_explicit_drop_action(backer->dpif) > > + && !check_drop_action(backer)) { > > Shouldn't last two conditions be || instad of && ? Assuming you mean: if (explicit_drop_action && (!dpif_may_support_explicit_drop_action(backer->dpif) || !check_drop_action(backer))) { Then I don't think so. This function is periodically called from type_run(). If we use || here, then the usual case is that dpif_may_support_explicit_drop_action() will return true, i.e. hw-offload=false. That would force check_drop_action() to be called. Basically we want to avoid calling check_drop_action() by guarding it with the much cheaper dpif_may_support_explicit_drop_action(). > > + atomic_store_relaxed(&backer->rt_support.explicit_drop_action, false); > > + return true; > > + } > > + > > + return false; > > +} > > + [..]
On 4/3/24 00:28, Eric Garver wrote: > On Fri, Mar 29, 2024 at 12:26:11AM +0100, Ilya Maximets wrote: >> On 3/22/24 14:54, Eric Garver wrote: > [..] >>> @@ -1649,8 +1693,8 @@ check_support(struct dpif_backer *backer) >>> backer->rt_support.max_hash_alg = check_max_dp_hash_alg(backer); >>> backer->rt_support.check_pkt_len = check_check_pkt_len(backer); >>> backer->rt_support.ct_timeout = check_ct_timeout_policy(backer); >>> - backer->rt_support.explicit_drop_action = >>> - dpif_supports_explicit_drop_action(backer->dpif); >>> + atomic_store_relaxed(&backer->rt_support.explicit_drop_action, >>> + check_drop_action(backer)); >>> backer->rt_support.lb_output_action = >>> dpif_supports_lb_output_action(backer->dpif); >>> backer->rt_support.ct_zero_snat = dpif_supports_ct_zero_snat(backer); >>> @@ -1667,6 +1711,28 @@ check_support(struct dpif_backer *backer) >>> backer->rt_support.odp.nd_ext = check_nd_extensions(backer); >>> } >>> >>> +/* TC does not support offloading the explicit drop action. As such we need to >>> + * re-probe the datapath if hw-offload has been modified. >>> + * Note: We don't support true --> false transition as that requires a restart. >>> + * See netdev_set_flow_api_enabled(). */ >>> +static bool >>> +recheck_support_explicit_drop_action(struct dpif_backer *backer) >>> +{ >>> + bool explicit_drop_action; >>> + >>> + atomic_read_relaxed(&backer->rt_support.explicit_drop_action, >>> + &explicit_drop_action); >>> + >>> + if (explicit_drop_action >>> + && !dpif_may_support_explicit_drop_action(backer->dpif) >>> + && !check_drop_action(backer)) { >> >> Shouldn't last two conditions be || instad of && ? > > Assuming you mean: > > if (explicit_drop_action && > (!dpif_may_support_explicit_drop_action(backer->dpif) || > !check_drop_action(backer))) { > > Then I don't think so. This function is periodically called from > type_run(). If we use || here, then the usual case is that > dpif_may_support_explicit_drop_action() will return true, i.e. > hw-offload=false. That would force check_drop_action() to be called. > > Basically we want to avoid calling check_drop_action() by guarding it > with the much cheaper dpif_may_support_explicit_drop_action(). Hmm, OK. But why do we call check_drop_action() here at all then? Just for the log message? Maybe it's better then to do something like: if (explicit_drop_action && && !dpif_may_support_explicit_drop_action(backer->dpif)) { ovs_assert(!check_drop_action(backer)); atomic_store_relaxed( ..., false); return true; } What do you think? Best regards, Ilya Maximets.
On Wed, Apr 03, 2024 at 12:42:25AM +0200, Ilya Maximets wrote: > On 4/3/24 00:28, Eric Garver wrote: > > On Fri, Mar 29, 2024 at 12:26:11AM +0100, Ilya Maximets wrote: > >> On 3/22/24 14:54, Eric Garver wrote: > > [..] > >>> @@ -1649,8 +1693,8 @@ check_support(struct dpif_backer *backer) > >>> backer->rt_support.max_hash_alg = check_max_dp_hash_alg(backer); > >>> backer->rt_support.check_pkt_len = check_check_pkt_len(backer); > >>> backer->rt_support.ct_timeout = check_ct_timeout_policy(backer); > >>> - backer->rt_support.explicit_drop_action = > >>> - dpif_supports_explicit_drop_action(backer->dpif); > >>> + atomic_store_relaxed(&backer->rt_support.explicit_drop_action, > >>> + check_drop_action(backer)); > >>> backer->rt_support.lb_output_action = > >>> dpif_supports_lb_output_action(backer->dpif); > >>> backer->rt_support.ct_zero_snat = dpif_supports_ct_zero_snat(backer); > >>> @@ -1667,6 +1711,28 @@ check_support(struct dpif_backer *backer) > >>> backer->rt_support.odp.nd_ext = check_nd_extensions(backer); > >>> } > >>> > >>> +/* TC does not support offloading the explicit drop action. As such we need to > >>> + * re-probe the datapath if hw-offload has been modified. > >>> + * Note: We don't support true --> false transition as that requires a restart. > >>> + * See netdev_set_flow_api_enabled(). */ > >>> +static bool > >>> +recheck_support_explicit_drop_action(struct dpif_backer *backer) > >>> +{ > >>> + bool explicit_drop_action; > >>> + > >>> + atomic_read_relaxed(&backer->rt_support.explicit_drop_action, > >>> + &explicit_drop_action); > >>> + > >>> + if (explicit_drop_action > >>> + && !dpif_may_support_explicit_drop_action(backer->dpif) > >>> + && !check_drop_action(backer)) { > >> > >> Shouldn't last two conditions be || instad of && ? > > > > Assuming you mean: > > > > if (explicit_drop_action && > > (!dpif_may_support_explicit_drop_action(backer->dpif) || > > !check_drop_action(backer))) { > > > > Then I don't think so. This function is periodically called from > > type_run(). If we use || here, then the usual case is that > > dpif_may_support_explicit_drop_action() will return true, i.e. > > hw-offload=false. That would force check_drop_action() to be called. > > > > Basically we want to avoid calling check_drop_action() by guarding it > > with the much cheaper dpif_may_support_explicit_drop_action(). > > Hmm, OK. But why do we call check_drop_action() here at all then? > Just for the log message? Yeah, I guess just for the log. > Maybe it's better then to do something like: > > if (explicit_drop_action && > && !dpif_may_support_explicit_drop_action(backer->dpif)) { > ovs_assert(!check_drop_action(backer)); > atomic_store_relaxed( ..., false); > return true; > } > > What do you think? That will work. Alternatively we could add a VLOG_INFO() here. I like the assert though as it keeps everything neatly in check_drop_action(). > Best regards, Ilya Maximets. > _______________________________________________ > 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 c4e2e867ecdc..b854ba3636ed 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -221,6 +221,7 @@ static void ct_zone_config_init(struct dpif_backer *backer); static void ct_zone_config_uninit(struct dpif_backer *backer); static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer); static void ct_zone_limits_commit(struct dpif_backer *backer); +static bool recheck_support_explicit_drop_action(struct dpif_backer *backer); static inline struct ofproto_dpif * ofproto_dpif_cast(const struct ofproto *ofproto) @@ -391,6 +392,10 @@ type_run(const char *type) udpif_set_threads(backer->udpif, n_handlers, n_revalidators); } + if (recheck_support_explicit_drop_action(backer)) { + backer->need_revalidate = REV_RECONFIGURE; + } + if (backer->need_revalidate) { struct ofproto_dpif *ofproto; struct simap_node *node; @@ -807,6 +812,7 @@ open_dpif_backer(const char *type, struct dpif_backer **backerp) shash_add(&all_dpif_backers, type, backer); + atomic_init(&backer->rt_support.explicit_drop_action, false); check_support(backer); atomic_count_init(&backer->tnl_count, 0); @@ -855,7 +861,12 @@ ovs_native_tunneling_is_on(struct ofproto_dpif *ofproto) bool ovs_explicit_drop_action_supported(struct ofproto_dpif *ofproto) { - return ofproto->backer->rt_support.explicit_drop_action; + bool value; + + atomic_read_relaxed(&ofproto->backer->rt_support.explicit_drop_action, + &value); + + return value; } bool @@ -1379,6 +1390,39 @@ check_ct_timeout_policy(struct dpif_backer *backer) return !error; } +/* Tests whether backer's datapath supports the OVS_ACTION_ATTR_DROP action. */ +static bool +check_drop_action(struct dpif_backer *backer) +{ + struct odputil_keybuf keybuf; + uint8_t actbuf[NL_A_U32_SIZE]; + struct ofpbuf actions; + struct ofpbuf key; + struct flow flow; + bool supported; + + struct odp_flow_key_parms odp_parms = { + .flow = &flow, + .probe = true, + }; + + memset(&flow, 0, sizeof flow); + ofpbuf_use_stack(&key, &keybuf, sizeof keybuf); + odp_flow_key_from_flow(&odp_parms, &key); + + ofpbuf_use_stack(&actions, &actbuf, sizeof actbuf); + nl_msg_put_u32(&actions, OVS_ACTION_ATTR_DROP, XLATE_OK); + + supported = dpif_may_support_explicit_drop_action(backer->dpif) && + dpif_probe_feature(backer->dpif, "drop", &key, &actions, NULL); + + VLOG_INFO("%s: Datapath %s explicit drop action", + dpif_name(backer->dpif), + (supported) ? "supports" : "does not support"); + + return supported; +} + /* Tests whether 'backer''s datapath supports the all-zero SNAT case. */ static bool dpif_supports_ct_zero_snat(struct dpif_backer *backer) @@ -1649,8 +1693,8 @@ check_support(struct dpif_backer *backer) backer->rt_support.max_hash_alg = check_max_dp_hash_alg(backer); backer->rt_support.check_pkt_len = check_check_pkt_len(backer); backer->rt_support.ct_timeout = check_ct_timeout_policy(backer); - backer->rt_support.explicit_drop_action = - dpif_supports_explicit_drop_action(backer->dpif); + atomic_store_relaxed(&backer->rt_support.explicit_drop_action, + check_drop_action(backer)); backer->rt_support.lb_output_action = dpif_supports_lb_output_action(backer->dpif); backer->rt_support.ct_zero_snat = dpif_supports_ct_zero_snat(backer); @@ -1667,6 +1711,28 @@ check_support(struct dpif_backer *backer) backer->rt_support.odp.nd_ext = check_nd_extensions(backer); } +/* TC does not support offloading the explicit drop action. As such we need to + * re-probe the datapath if hw-offload has been modified. + * Note: We don't support true --> false transition as that requires a restart. + * See netdev_set_flow_api_enabled(). */ +static bool +recheck_support_explicit_drop_action(struct dpif_backer *backer) +{ + bool explicit_drop_action; + + atomic_read_relaxed(&backer->rt_support.explicit_drop_action, + &explicit_drop_action); + + if (explicit_drop_action + && !dpif_may_support_explicit_drop_action(backer->dpif) + && !check_drop_action(backer)) { + atomic_store_relaxed(&backer->rt_support.explicit_drop_action, false); + return true; + } + + return false; +} + static int construct(struct ofproto *ofproto_) { @@ -5714,6 +5780,7 @@ ct_zone_limit_protection_update(const char *datapath_type, bool protected) static void get_datapath_cap(const char *datapath_type, struct smap *cap) { + bool explicit_drop_action; struct dpif_backer_support *s; struct dpif_backer *backer = shash_find_data(&all_dpif_backers, datapath_type); @@ -5749,8 +5816,9 @@ get_datapath_cap(const char *datapath_type, struct smap *cap) smap_add_format(cap, "max_hash_alg", "%"PRIuSIZE, s->max_hash_alg); smap_add(cap, "check_pkt_len", s->check_pkt_len ? "true" : "false"); smap_add(cap, "ct_timeout", s->ct_timeout ? "true" : "false"); + atomic_read_relaxed(&s->explicit_drop_action, &explicit_drop_action); smap_add(cap, "explicit_drop_action", - s->explicit_drop_action ? "true" :"false"); + explicit_drop_action ? "true" :"false"); smap_add(cap, "lb_output_action", s->lb_output_action ? "true" : "false"); smap_add(cap, "ct_zero_snat", s->ct_zero_snat ? "true" : "false"); smap_add(cap, "add_mpls", s->add_mpls ? "true" : "false"); @@ -6279,7 +6347,7 @@ show_dp_feature_bool(struct ds *ds, const char *feature, const bool *b) ds_put_format(ds, "%s: %s\n", feature, *b ? "Yes" : "No"); } -static void OVS_UNUSED +static void show_dp_feature_atomic_bool(struct ds *ds, const char *feature, const atomic_bool *b) { diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index 92d33aa64709..d33f73df8aed 100644 --- a/ofproto/ofproto-dpif.h +++ b/ofproto/ofproto-dpif.h @@ -51,6 +51,7 @@ #include "hmapx.h" #include "odp-util.h" #include "id-pool.h" +#include "ovs-atomic.h" #include "ovs-thread.h" #include "ofproto-provider.h" #include "util.h" @@ -202,7 +203,8 @@ struct group_dpif *group_dpif_lookup(struct ofproto_dpif *, DPIF_SUPPORT_FIELD(bool, ct_timeout, "Conntrack timeout policy") \ \ /* True if the datapath supports explicit drop action. */ \ - DPIF_SUPPORT_FIELD(bool, explicit_drop_action, "Explicit Drop action") \ + DPIF_SUPPORT_FIELD(atomic_bool, explicit_drop_action, \ + "Explicit Drop action") \ \ /* True if the datapath supports balance_tcp optimization */ \ DPIF_SUPPORT_FIELD(bool, lb_output_action, "Optimized Balance TCP mode")\