| Message ID | 20240403143533.61730-5-eric@garver.life |
|---|---|
| State | Accepted |
| Commit | 3c8d069b9bc2307ff2740d45a61d8d9504d65a80 |
| 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/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On 4/3/24 16:35, 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 | 2 +- > ofproto/ofproto-dpif.c | 77 ++++++++++++++++++++++++++++++++++--- > ofproto/ofproto-dpif.h | 4 +- > 5 files changed, 81 insertions(+), 10 deletions(-) Recheck-request: github-robot
On 3 Apr 2024, at 16:35, 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 | 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 d18754c84f62..d9fb991ef234 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..32d037be607c 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; > @@ -855,7 +860,11 @@ 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,40 @@ 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; > + bool supported; > + > + struct flow flow = { > + .dl_type = CONSTANT_HTONS(0x1234), /* bogus */ > + }; > + struct odp_flow_key_parms odp_parms = { > + .flow = &flow, > + .probe = true, > + }; > + > + 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 +1692,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 +1710,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)) { > + ovs_assert(!check_drop_action(backer)); If we only call this for the log message, would just adding the log message here not be better? It avoids the installation of a flow, and I have not thought this through, but it might confuse the revalidator. > + atomic_store_relaxed(&backer->rt_support.explicit_drop_action, false); > + return true; > + } > + > + return false; > +} > + > static int > construct(struct ofproto *ofproto_) > { > @@ -5714,6 +5779,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 +5815,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 +6346,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
On Fri, Apr 05, 2024 at 11:39:58AM +0200, Eelco Chaudron wrote: > > > On 3 Apr 2024, at 16:35, 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 | 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 d18754c84f62..d9fb991ef234 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..32d037be607c 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; > > @@ -855,7 +860,11 @@ 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,40 @@ 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; > > + bool supported; > > + > > + struct flow flow = { > > + .dl_type = CONSTANT_HTONS(0x1234), /* bogus */ > > + }; > > + struct odp_flow_key_parms odp_parms = { > > + .flow = &flow, > > + .probe = true, > > + }; > > + > > + 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 +1692,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 +1710,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)) { > > + ovs_assert(!check_drop_action(backer)); > > If we only call this for the log message, would just adding the log message here not be better? > It avoids the installation of a flow, and I have not thought this through, but it might confuse the revalidator. So you and Ilya would rather I insert a VLOG_INFO() here? That's fine with me. Can we agree on the message before I post another revision? VLOG_INFO("%s: Datapath supports explicit drop action, " "but disabled due to hw-offload=true.", dpif_name(backer->dpif)) Is that agreeable?
On 4/5/24 15:01, Eric Garver wrote: > On Fri, Apr 05, 2024 at 11:39:58AM +0200, Eelco Chaudron wrote: >> >> >> On 3 Apr 2024, at 16:35, 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 | 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 d18754c84f62..d9fb991ef234 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..32d037be607c 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; >>> @@ -855,7 +860,11 @@ 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,40 @@ 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; >>> + bool supported; >>> + >>> + struct flow flow = { >>> + .dl_type = CONSTANT_HTONS(0x1234), /* bogus */ >>> + }; >>> + struct odp_flow_key_parms odp_parms = { >>> + .flow = &flow, >>> + .probe = true, >>> + }; >>> + >>> + 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 +1692,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 +1710,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)) { >>> + ovs_assert(!check_drop_action(backer)); >> >> If we only call this for the log message, would just adding the log message here not be better? >> It avoids the installation of a flow, We're not going to install the flow, because there is a check for dpif_may_support_explicit_drop_action() in the check_drop_action(). >> and I have not thought this through, but it might confuse the revalidator. So, the revalidator will not be confused. > > So you and Ilya would rather I insert a VLOG_INFO() here? That's fine > with me. I'd keep as is, unless Eelco has a strong opinion towards the log. > > Can we agree on the message before I post another revision? > > VLOG_INFO("%s: Datapath supports explicit drop action, " > "but disabled due to hw-offload=true.", > dpif_name(backer->dpif)) > > Is that agreeable? >
On 4/5/24 15:08, Ilya Maximets wrote: > On 4/5/24 15:01, Eric Garver wrote: >> On Fri, Apr 05, 2024 at 11:39:58AM +0200, Eelco Chaudron wrote: >>> >>> >>> On 3 Apr 2024, at 16:35, 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 | 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 d18754c84f62..d9fb991ef234 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..32d037be607c 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; >>>> @@ -855,7 +860,11 @@ 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,40 @@ 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; >>>> + bool supported; >>>> + >>>> + struct flow flow = { >>>> + .dl_type = CONSTANT_HTONS(0x1234), /* bogus */ >>>> + }; >>>> + struct odp_flow_key_parms odp_parms = { >>>> + .flow = &flow, >>>> + .probe = true, >>>> + }; >>>> + >>>> + 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 +1692,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 +1710,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)) { >>>> + ovs_assert(!check_drop_action(backer)); >>> >>> If we only call this for the log message, would just adding the log message here not be better? >>> It avoids the installation of a flow, > > We're not going to install the flow, because there is a check for > dpif_may_support_explicit_drop_action() in the check_drop_action(). > >>> and I have not thought this through, but it might confuse the revalidator. > > So, the revalidator will not be confused. > >> >> So you and Ilya would rather I insert a VLOG_INFO() here? That's fine >> with me. > > I'd keep as is, unless Eelco has a strong opinion towards the log. > >> >> Can we agree on the message before I post another revision? >> >> VLOG_INFO("%s: Datapath supports explicit drop action, " >> "but disabled due to hw-offload=true.", >> dpif_name(backer->dpif)) >> >> Is that agreeable? In case Eelco prefers a solution with the log, I'd suggest to just log "%s: Datapath no longer supports explicit drop action". The reasoning is the same as before, this module doesn't know why exactly dpif_may_support_explicit_drop_action() now returns 'false' and shouldn't make assumptions that it is because of hardware offload. And it should be simple enough to correlate this message with enabling of the hardware offload. Best regards, Ilya Maximets.
On 5 Apr 2024, at 15:43, Ilya Maximets wrote: > On 4/5/24 15:08, Ilya Maximets wrote: >> On 4/5/24 15:01, Eric Garver wrote: >>> On Fri, Apr 05, 2024 at 11:39:58AM +0200, Eelco Chaudron wrote: >>>> >>>> >>>> On 3 Apr 2024, at 16:35, 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 | 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 d18754c84f62..d9fb991ef234 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..32d037be607c 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; >>>>> @@ -855,7 +860,11 @@ 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,40 @@ 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; >>>>> + bool supported; >>>>> + >>>>> + struct flow flow = { >>>>> + .dl_type = CONSTANT_HTONS(0x1234), /* bogus */ >>>>> + }; >>>>> + struct odp_flow_key_parms odp_parms = { >>>>> + .flow = &flow, >>>>> + .probe = true, >>>>> + }; >>>>> + >>>>> + 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 +1692,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 +1710,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)) { >>>>> + ovs_assert(!check_drop_action(backer)); >>>> >>>> If we only call this for the log message, would just adding the log message here not be better? >>>> It avoids the installation of a flow, >> >> We're not going to install the flow, because there is a check for >> dpif_may_support_explicit_drop_action() in the check_drop_action(). >>>> and I have not thought this through, but it might confuse the revalidator. >> >> So, the revalidator will not be confused. >> >>> >>> So you and Ilya would rather I insert a VLOG_INFO() here? That's fine >>> with me. >> >> I'd keep as is, unless Eelco has a strong opinion towards the log. >> >>> >>> Can we agree on the message before I post another revision? >>> >>> VLOG_INFO("%s: Datapath supports explicit drop action, " >>> "but disabled due to hw-offload=true.", >>> dpif_name(backer->dpif)) >>> >>> Is that agreeable? > > In case Eelco prefers a solution with the log, I'd suggest to > just log "%s: Datapath no longer supports explicit drop action". > The reasoning is the same as before, this module doesn't know > why exactly dpif_may_support_explicit_drop_action() now returns > 'false' and shouldn't make assumptions that it is because of > hardware offload. And it should be simple enough to correlate > this message with enabling of the hardware offload. I once again forgot this is for enabling TC only (not disabling), so let’s keep the patch as is. Acked-by: Eelco Chaudron <echaudro@redhat.com> Cheers, Eelco
diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h index d18754c84f62..d9fb991ef234 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..32d037be607c 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; @@ -855,7 +860,11 @@ 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,40 @@ 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; + bool supported; + + struct flow flow = { + .dl_type = CONSTANT_HTONS(0x1234), /* bogus */ + }; + struct odp_flow_key_parms odp_parms = { + .flow = &flow, + .probe = true, + }; + + 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 +1692,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 +1710,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)) { + ovs_assert(!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 +5779,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 +5815,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 +6346,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(-)