diff mbox series

[ovs-dev,v12,4/6] dpif: Probe support for OVS_ACTION_ATTR_DROP.

Message ID 20240315195508.95099-5-eric@garver.life
State Changes Requested
Delegated to: Eelco Chaudron
Headers show
Series dpif: probe support for OVS_ACTION_ATTR_DROP | expand

Checks

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 fail test: fail

Commit Message

Eric Garver March 15, 2024, 7:55 p.m. UTC
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      | 78 ++++++++++++++++++++++++++++++++++---
 ofproto/ofproto-dpif.h      |  4 +-
 5 files changed, 82 insertions(+), 10 deletions(-)

Comments

Eelco Chaudron March 22, 2024, 9:49 a.m. UTC | #1
On 15 Mar 2024, at 20:55, Eric Garver wrote:

> Kernel support has been added for this action. As such, we need to probe
> the datapath for support.
>
> Signed-off-by: Eric Garver <eric@garver.life>

Thanks for following up. This change looks good to me.

Acked-by: Eelco Chaudron <echaudro@redhat.com>
diff mbox series

Patch

diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index a265e05ad253..fce6456f47c8 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -1086,11 +1086,11 @@  enum ovs_action_attr {
 	OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
 	OVS_ACTION_ATTR_ADD_MPLS,     /* struct ovs_action_add_mpls. */
 	OVS_ACTION_ATTR_DEC_TTL,      /* Nested OVS_DEC_TTL_ATTR_*. */
+	OVS_ACTION_ATTR_DROP,         /* u32 xlate_error. */
 
 #ifndef __KERNEL__
 	OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
 	OVS_ACTION_ATTR_TUNNEL_POP,    /* u32 port number. */
-	OVS_ACTION_ATTR_DROP,          /* u32 xlate_error. */
 	OVS_ACTION_ATTR_LB_OUTPUT,     /* u32 bond-id. */
 #endif
 	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
diff --git a/lib/dpif.c b/lib/dpif.c
index 0f480bec48d0..23eb18495a66 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -28,6 +28,7 @@ 
 #include "dpctl.h"
 #include "dpif-netdev.h"
 #include "flow.h"
+#include "netdev-offload.h"
 #include "netdev-provider.h"
 #include "netdev.h"
 #include "netlink.h"
@@ -1935,9 +1936,10 @@  dpif_supports_tnl_push_pop(const struct dpif *dpif)
 }
 
 bool
-dpif_supports_explicit_drop_action(const struct dpif *dpif)
+dpif_may_support_explicit_drop_action(const struct dpif *dpif)
 {
-    return dpif_is_netdev(dpif);
+    /* TC does not support offloading this action. */
+    return dpif_is_netdev(dpif) || !netdev_is_flow_api_enabled();
 }
 
 bool
diff --git a/lib/dpif.h b/lib/dpif.h
index 0f2dc2ef3c55..a764e8a592bd 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -940,7 +940,7 @@  int dpif_get_pmds_for_port(const struct dpif * dpif, odp_port_t port_no,
 
 char *dpif_get_dp_version(const struct dpif *);
 bool dpif_supports_tnl_push_pop(const struct dpif *);
-bool dpif_supports_explicit_drop_action(const struct dpif *);
+bool dpif_may_support_explicit_drop_action(const struct dpif *);
 bool dpif_synced_dp_layers(struct dpif *);
 
 /* Log functions. */
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 2ec1cd1854ab..dcef9ade5c2e 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_)
 {
@@ -5708,6 +5774,7 @@  ct_zone_limit_protection_update(const char *datapath_type, bool protected)
 static void
 get_datapath_cap(const char *datapath_type, struct smap *cap)
 {
+    bool explicit_drop_action;
     struct dpif_backer_support *s;
     struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
                                                  datapath_type);
@@ -5743,8 +5810,9 @@  get_datapath_cap(const char *datapath_type, struct smap *cap)
     smap_add_format(cap, "max_hash_alg", "%"PRIuSIZE, s->max_hash_alg);
     smap_add(cap, "check_pkt_len", s->check_pkt_len ? "true" : "false");
     smap_add(cap, "ct_timeout", s->ct_timeout ? "true" : "false");
+    atomic_read_relaxed(&s->explicit_drop_action, &explicit_drop_action);
     smap_add(cap, "explicit_drop_action",
-             s->explicit_drop_action ? "true" :"false");
+             explicit_drop_action ? "true" :"false");
     smap_add(cap, "lb_output_action", s->lb_output_action ? "true" : "false");
     smap_add(cap, "ct_zero_snat", s->ct_zero_snat ? "true" : "false");
     smap_add(cap, "add_mpls", s->add_mpls ? "true" : "false");
@@ -6273,7 +6341,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")\