| Message ID | 20230809144258.1608770-3-eric@garver.life |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dpif: probe support for OVS_ACTION_ATTR_DROP | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Wed, Aug 09, 2023 at 10:42:57AM -0400, Eric Garver wrote: > Kernel support is being added for this action. As such, we need to probe > the datapath for support. > > Signed-off-by: Eric Garver <eric@garver.life> Acked-by: Simon Horman <horms@ovn.org>
On 9 Aug 2023, at 16:42, Eric Garver wrote: > Kernel support is being added for this action. As such, we need to probe > the datapath for support. > > Signed-off-by: Eric Garver <eric@garver.life> Some comments below… Cheers, Eelco > --- > include/linux/openvswitch.h | 2 +- > lib/dpif.c | 6 ------ > lib/dpif.h | 1 - > ofproto/ofproto-dpif.c | 41 +++++++++++++++++++++++++++++++++++-- > 4 files changed, 40 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..28fa40879655 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > @@ -1934,12 +1934,6 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif) > return dpif_is_netdev(dpif); > } > > -bool > -dpif_supports_explicit_drop_action(const struct dpif *dpif) > -{ > - return dpif_is_netdev(dpif); > -} > - > bool > dpif_supports_lb_output_action(const struct dpif *dpif) > { > diff --git a/lib/dpif.h b/lib/dpif.h > index 0f2dc2ef3c55..8dc5da45b3ef 100644 > --- a/lib/dpif.h > +++ b/lib/dpif.h > @@ -940,7 +940,6 @@ int dpif_get_pmds_for_port(const struct dpif * dpif, odp_port_t port_no, > > char *dpif_get_dp_version(const struct dpif *); > bool dpif_supports_tnl_push_pop(const struct dpif *); > -bool dpif_supports_explicit_drop_action(const struct dpif *); > bool dpif_synced_dp_layers(struct dpif *); > > /* Log functions. */ > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index e22ca757ac35..e4de2becef2e 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -72,6 +72,8 @@ > #include "util.h" > #include "uuid.h" > #include "vlan-bitmap.h" > +#include "dpif-netdev.h" > +#include "netdev-offload.h" > > VLOG_DEFINE_THIS_MODULE(ofproto_dpif); > > @@ -1375,6 +1377,42 @@ 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, > + }; > + > + /* TC does not support offloading this action. */ > + if (netdev_is_flow_api_enabled() && !dpif_is_netdev(backer->dpif)) { I think we need a log message here to indicate TC is enabled. Maybe we should even delay this test top below, so we can have a log message like: “Datapath %s drop action supported, but not enabled due to hardware offload being enabled” > + return false; > + } > + > + memset(&flow, 0, sizeof flow); > + ofpbuf_use_stack(&key, &keybuf, sizeof keybuf); > + odp_flow_key_from_flow(&odp_parms, &key); > + > + ofpbuf_use_stack(&actions, &actbuf, sizeof actbuf); > + nl_msg_put_u32(&actions, OVS_ACTION_ATTR_DROP, XLATE_OK); > + > + supported = dpif_probe_feature(backer->dpif, "drop", &key, &actions, NULL); > + > + VLOG_INFO("%s: Datapath %s drop action", > + dpif_name(backer->dpif), (supported) ? "supports" > + : "does not support"); > + return supported; > +} > + What about when HW offload gets enabled/disabled later on? We still have the DROP_ACTION enabled which might cause problems/confusion, same when HW offload gets disabled. Do we want to re-test when hw offload is enabled/disabled? > /* Tests whether 'backer''s datapath supports the all-zero SNAT case. */ > static bool > dpif_supports_ct_zero_snat(struct dpif_backer *backer) > @@ -1627,8 +1665,7 @@ 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); > + 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); > -- > 2.39.0 > > _______________________________________________ > 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..28fa40879655 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -1934,12 +1934,6 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif) return dpif_is_netdev(dpif); } -bool -dpif_supports_explicit_drop_action(const struct dpif *dpif) -{ - return dpif_is_netdev(dpif); -} - bool dpif_supports_lb_output_action(const struct dpif *dpif) { diff --git a/lib/dpif.h b/lib/dpif.h index 0f2dc2ef3c55..8dc5da45b3ef 100644 --- a/lib/dpif.h +++ b/lib/dpif.h @@ -940,7 +940,6 @@ int dpif_get_pmds_for_port(const struct dpif * dpif, odp_port_t port_no, char *dpif_get_dp_version(const struct dpif *); bool dpif_supports_tnl_push_pop(const struct dpif *); -bool dpif_supports_explicit_drop_action(const struct dpif *); bool dpif_synced_dp_layers(struct dpif *); /* Log functions. */ diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index e22ca757ac35..e4de2becef2e 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -72,6 +72,8 @@ #include "util.h" #include "uuid.h" #include "vlan-bitmap.h" +#include "dpif-netdev.h" +#include "netdev-offload.h" VLOG_DEFINE_THIS_MODULE(ofproto_dpif); @@ -1375,6 +1377,42 @@ 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, + }; + + /* TC does not support offloading this action. */ + if (netdev_is_flow_api_enabled() && !dpif_is_netdev(backer->dpif)) { + return false; + } + + memset(&flow, 0, sizeof flow); + ofpbuf_use_stack(&key, &keybuf, sizeof keybuf); + odp_flow_key_from_flow(&odp_parms, &key); + + ofpbuf_use_stack(&actions, &actbuf, sizeof actbuf); + nl_msg_put_u32(&actions, OVS_ACTION_ATTR_DROP, XLATE_OK); + + supported = dpif_probe_feature(backer->dpif, "drop", &key, &actions, NULL); + + VLOG_INFO("%s: Datapath %s 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) @@ -1627,8 +1665,7 @@ 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); + 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);
Kernel support is being added for this action. As such, we need to probe the datapath for support. Signed-off-by: Eric Garver <eric@garver.life> --- include/linux/openvswitch.h | 2 +- lib/dpif.c | 6 ------ lib/dpif.h | 1 - ofproto/ofproto-dpif.c | 41 +++++++++++++++++++++++++++++++++++-- 4 files changed, 40 insertions(+), 10 deletions(-)