diff mbox series

[ovs-dev,v3,2/4] dpif: Probe support for OVS_ACTION_ATTR_DROP.

Message ID 20230728000446.436067-3-eric@garver.life
State Changes Requested
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 success test: success

Commit Message

Eric Garver July 28, 2023, 12:04 a.m. UTC 
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      | 34 ++++++++++++++++++++++++++++++++--
 4 files changed, 33 insertions(+), 10 deletions(-)

Comments

Ilya Maximets Aug. 3, 2023, 11:43 a.m. UTC | #1 
On 7/28/23 02:04, 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>

Hi, Eric.  Thanks for the update.

IIRC, there was a discussion that this feature has to be mutually exclusive
with the TC hardware offload.  Otherwise, we will constantly treat flows
dumped from TC as invalid.

Best regards, Ilya Maximets.
Eric Garver Aug. 3, 2023, 2:05 p.m. UTC | #2 
On Thu, Aug 03, 2023 at 01:43:40PM +0200, Ilya Maximets wrote:
> On 7/28/23 02:04, 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>
> 
> Hi, Eric.  Thanks for the update.
> 
> IIRC, there was a discussion that this feature has to be mutually exclusive
> with the TC hardware offload.  Otherwise, we will constantly treat flows
> dumped from TC as invalid.

I think I misunderstood the conversation. I thought it landed on "let's
ignore TC for now", but really it landed on "let's ignore the case of
enabling HW/TC offload at runtime" (since docs say a restart is
required).

I'll work on a v4 then which sets
ofproto->backer->rt_support.explicit_drop_action with regards to TC.
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 aff6ef4ba455..b8a411bf32e8 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1928,12 +1928,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 9e9d0aa1b0a8..44f1d75839c1 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 fad7342b0b02..c490a3c1da48 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1375,6 +1375,37 @@  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_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 +1658,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);