diff mbox series

[ovs-dev,v4,1/3] dpif: Add support for OVS_ACTION_ATTR_CT_CLEAR

Message ID 20180119192153.4660-2-e@erig.me
State Accepted
Headers show
Series Add dpif support for ct_clear action | expand

Commit Message

Eric Garver Jan. 19, 2018, 7:21 p.m. UTC 
This supports using the ct_clear action in the kernel datapath. To
preserve compatibility with current ct_clear behavior on old kernels, we
only pass this action down to the datapath if a probe reveals the
datapath actually supports it.

Signed-off-by: Eric Garver <e@erig.me>
Acked-by: William Tu <u9012063@gmail.com>
Acked-by: Flavio Leitner <fbl@sysclose.org>
---
 NEWS                                              |  5 ++--
 datapath/linux/compat/include/linux/openvswitch.h |  2 ++
 lib/conntrack.c                                   | 10 +++++++
 lib/conntrack.h                                   |  1 +
 lib/dpif-netdev.c                                 |  1 +
 lib/dpif.c                                        |  1 +
 lib/odp-execute.c                                 |  7 +++++
 lib/odp-util.c                                    | 11 ++++++++
 lib/ofp-actions.c                                 |  1 +
 ofproto/ofproto-dpif-ipfix.c                      |  1 +
 ofproto/ofproto-dpif-sflow.c                      |  1 +
 ofproto/ofproto-dpif-xlate.c                      | 13 ++++++++-
 ofproto/ofproto-dpif.c                            | 32 +++++++++++++++++++++++
 ofproto/ofproto-dpif.h                            |  5 +++-
 tests/odp.at                                      |  1 +
 15 files changed, 88 insertions(+), 4 deletions(-)

Comments

Justin Pettit Jan. 20, 2018, 7:22 p.m. UTC | #1 
> On Jan 19, 2018, at 11:21 AM, Eric Garver <e@erig.me> wrote:
> 
> This supports using the ct_clear action in the kernel datapath. To
> preserve compatibility with current ct_clear behavior on old kernels, we
> only pass this action down to the datapath if a probe reveals the
> datapath actually supports it.

It looks like it hasn't been added to the out-of-tree kernel module in the OVS repo.  Is there a reason for that?

> +int
> +conntrack_clear(struct dp_packet *packet)
> +{
> +    /* According to pkt_metadata_init(), ct_state == 0 is enough to make all of
> +     * the conntrack fields invalid. */
> +    packet->md.ct_state = 0;
> +
> +    return 0;
> +}

I made this void since it always returns success and nothing checks its return value.

> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 6157b70aee60..675b3a697641 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1255,6 +1255,37 @@ check_ct_eventmask(struct dpif_backer *backer)
>     return !error;
> }
> 
> +static bool
> +check_ct_clear(struct dpif_backer *backer)

I added a comment to this function:

+/* Tests whether 'backer''s datapath supports the OVS_ACTION_ATTR_CT_CLEAR
+ * action. */

I pushed the series with those changes to master.  Did you want me to backport it to branch-2.9?

--Justin
Eric Garver Jan. 22, 2018, 2:34 p.m. UTC | #2 
On Sat, Jan 20, 2018 at 11:22:06AM -0800, Justin Pettit wrote:
> 
> > On Jan 19, 2018, at 11:21 AM, Eric Garver <e@erig.me> wrote:
> > 
> > This supports using the ct_clear action in the kernel datapath. To
> > preserve compatibility with current ct_clear behavior on old kernels, we
> > only pass this action down to the datapath if a probe reveals the
> > datapath actually supports it.
> 
> It looks like it hasn't been added to the out-of-tree kernel module in the OVS repo.  Is there a reason for that?

No reason - just an oversight.
I will post a follow up series for the module backport.

> 
> > +int
> > +conntrack_clear(struct dp_packet *packet)
> > +{
> > +    /* According to pkt_metadata_init(), ct_state == 0 is enough to make all of
> > +     * the conntrack fields invalid. */
> > +    packet->md.ct_state = 0;
> > +
> > +    return 0;
> > +}
> 
> I made this void since it always returns success and nothing checks its return value.

Thanks.

> 
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 6157b70aee60..675b3a697641 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -1255,6 +1255,37 @@ check_ct_eventmask(struct dpif_backer *backer)
> >     return !error;
> > }
> > 
> > +static bool
> > +check_ct_clear(struct dpif_backer *backer)
> 
> I added a comment to this function:
> 
> +/* Tests whether 'backer''s datapath supports the OVS_ACTION_ATTR_CT_CLEAR
> + * action. */
> 
> I pushed the series with those changes to master.  Did you want me to backport it to branch-2.9?

Yes. I would appreciate it going to 2.9 as well.

> 
> --Justin
Justin Pettit Jan. 22, 2018, 7:31 p.m. UTC | #3 
> On Jan 22, 2018, at 6:34 AM, Eric Garver <e@erig.me> wrote:
> 
> On Sat, Jan 20, 2018 at 11:22:06AM -0800, Justin Pettit wrote:
>> 
>>> On Jan 19, 2018, at 11:21 AM, Eric Garver <e@erig.me> wrote:
>>> 
>>> This supports using the ct_clear action in the kernel datapath. To
>>> preserve compatibility with current ct_clear behavior on old kernels, we
>>> only pass this action down to the datapath if a probe reveals the
>>> datapath actually supports it.
>> 
>> It looks like it hasn't been added to the out-of-tree kernel module in the OVS repo.  Is there a reason for that?
> 
> No reason - just an oversight.
> I will post a follow up series for the module backport.

Great.  Thank you.

>> I pushed the series with those changes to master.  Did you want me to backport it to branch-2.9?
> 
> Yes. I would appreciate it going to 2.9 as well.

Done.

Thanks for the patches.

--Justin
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index c067b9462f2d..32c5bfce00e0 100644
--- a/NEWS
+++ b/NEWS
@@ -1,7 +1,8 @@ 
 Post-v2.9.0
 --------------------
-    - Nothing yet.
-
+   - OpenFlow:
+     * ct_clear action is now backed by kernel datapath. Support is probed for
+       when OVS starts.
 
 v2.9.0 - xx xxx xxxx
 --------------------
diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
index c7142b604c21..3deaba6f98a7 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -883,6 +883,7 @@  enum ovs_nat_attr {
  * @OVS_ACTION_ATTR_PUSH_ETH: Push a new outermost Ethernet header onto the
  * packet.
  * @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the packet.
+ * @OVS_ACTION_ATTR_CT_CLEAR: Clear conntrack state from the packet.
  * @OVS_ACTION_ATTR_PUSH_NSH: push NSH header to the packet.
  * @OVS_ACTION_ATTR_POP_NSH: pop the outermost NSH header off the packet.
  *
@@ -920,6 +921,7 @@  enum ovs_action_attr {
 	OVS_ACTION_ATTR_TRUNC,        /* u32 struct ovs_action_trunc. */
 	OVS_ACTION_ATTR_PUSH_ETH,     /* struct ovs_action_push_eth. */
 	OVS_ACTION_ATTR_POP_ETH,      /* No argument. */
+	OVS_ACTION_ATTR_CT_CLEAR,     /* No argument. */
 
 #ifndef __KERNEL__
 	OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
diff --git a/lib/conntrack.c b/lib/conntrack.c
index c89ac43ad1e6..b0dba1c92922 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -1325,6 +1325,16 @@  conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
     return 0;
 }
 
+int
+conntrack_clear(struct dp_packet *packet)
+{
+    /* According to pkt_metadata_init(), ct_state == 0 is enough to make all of
+     * the conntrack fields invalid. */
+    packet->md.ct_state = 0;
+
+    return 0;
+}
+
 static void
 set_mark(struct dp_packet *pkt, struct conn *conn, uint32_t val, uint32_t mask)
 {
diff --git a/lib/conntrack.h b/lib/conntrack.h
index 419bcc3e3996..915f8d6b8787 100644
--- a/lib/conntrack.h
+++ b/lib/conntrack.h
@@ -98,6 +98,7 @@  int conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
                       ovs_be16 tp_src, ovs_be16 tp_dst, const char *helper,
                       const struct nat_action_info_t *nat_action_info,
                       long long now);
+int conntrack_clear(struct dp_packet *packet);
 
 struct conntrack_dump {
     struct conntrack *ct;
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index c7d157ab6403..a8f389bb3568 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -5762,6 +5762,7 @@  dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
     case OVS_ACTION_ATTR_CLONE:
     case OVS_ACTION_ATTR_PUSH_NSH:
     case OVS_ACTION_ATTR_POP_NSH:
+    case OVS_ACTION_ATTR_CT_CLEAR:
     case __OVS_ACTION_ATTR_MAX:
         OVS_NOT_REACHED();
     }
diff --git a/lib/dpif.c b/lib/dpif.c
index ab2e2329b536..d3ea4613fd21 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1275,6 +1275,7 @@  dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_,
     case OVS_ACTION_ATTR_CLONE:
     case OVS_ACTION_ATTR_PUSH_NSH:
     case OVS_ACTION_ATTR_POP_NSH:
+    case OVS_ACTION_ATTR_CT_CLEAR:
     case OVS_ACTION_ATTR_UNSPEC:
     case __OVS_ACTION_ATTR_MAX:
         OVS_NOT_REACHED();
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index ebaea3182c85..20f33eb57acb 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -35,6 +35,7 @@ 
 #include "unaligned.h"
 #include "util.h"
 #include "csum.h"
+#include "conntrack.h"
 
 /* Masked copy of an ethernet address. 'src' is already properly masked. */
 static void
@@ -674,6 +675,7 @@  requires_datapath_assistance(const struct nlattr *a)
     case OVS_ACTION_ATTR_CLONE:
     case OVS_ACTION_ATTR_PUSH_NSH:
     case OVS_ACTION_ATTR_POP_NSH:
+    case OVS_ACTION_ATTR_CT_CLEAR:
         return false;
 
     case OVS_ACTION_ATTR_UNSPEC:
@@ -874,6 +876,11 @@  odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
             }
             break;
         }
+        case OVS_ACTION_ATTR_CT_CLEAR:
+            DP_PACKET_BATCH_FOR_EACH (packet, batch) {
+                conntrack_clear(packet);
+            }
+            break;
 
         case OVS_ACTION_ATTR_OUTPUT:
         case OVS_ACTION_ATTR_TUNNEL_PUSH:
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 8a5e9e82eaa8..6a29a76de5cd 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -118,6 +118,7 @@  odp_action_len(uint16_t type)
     case OVS_ACTION_ATTR_SET_MASKED: return ATTR_LEN_VARIABLE;
     case OVS_ACTION_ATTR_SAMPLE: return ATTR_LEN_VARIABLE;
     case OVS_ACTION_ATTR_CT: return ATTR_LEN_VARIABLE;
+    case OVS_ACTION_ATTR_CT_CLEAR: return 0;
     case OVS_ACTION_ATTR_PUSH_ETH: return sizeof(struct ovs_action_push_eth);
     case OVS_ACTION_ATTR_POP_ETH: return 0;
     case OVS_ACTION_ATTR_CLONE: return ATTR_LEN_VARIABLE;
@@ -1131,6 +1132,9 @@  format_odp_action(struct ds *ds, const struct nlattr *a,
     case OVS_ACTION_ATTR_CT:
         format_odp_conntrack_action(ds, a);
         break;
+    case OVS_ACTION_ATTR_CT_CLEAR:
+        ds_put_cstr(ds, "ct_clear");
+        break;
     case OVS_ACTION_ATTR_CLONE:
         format_odp_clone_action(ds, a, portno_names);
         break;
@@ -2285,6 +2289,13 @@  parse_odp_action(const char *s, const struct simap *port_names,
     }
 
     {
+        if (!strncmp(s, "ct_clear", 8)) {
+            nl_msg_put_flag(actions, OVS_ACTION_ATTR_CT_CLEAR);
+            return 8;
+        }
+    }
+
+    {
         int retval;
 
         retval = parse_conntrack_action(s, actions);
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index db933634bf8b..012494976600 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -7400,6 +7400,7 @@  ofpacts_execute_action_set(struct ofpbuf *action_list,
     if (!ofpacts_copy_last(action_list, action_set, OFPACT_GROUP) &&
         !ofpacts_copy_last(action_list, action_set, OFPACT_OUTPUT) &&
         !ofpacts_copy_last(action_list, action_set, OFPACT_RESUBMIT) &&
+        !ofpacts_copy_last(action_list, action_set, OFPACT_CT_CLEAR) &&
         !ofpacts_copy_last(action_list, action_set, OFPACT_CT)) {
         ofpbuf_clear(action_list);
     }
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index a420903e9dd4..97dbc3d29fe1 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -2983,6 +2983,7 @@  dpif_ipfix_read_actions(const struct flow *flow,
         case OVS_ACTION_ATTR_TRUNC:
         case OVS_ACTION_ATTR_HASH:
         case OVS_ACTION_ATTR_CT:
+        case OVS_ACTION_ATTR_CT_CLEAR:
         case OVS_ACTION_ATTR_METER:
         case OVS_ACTION_ATTR_SET_MASKED:
         case OVS_ACTION_ATTR_SET:
diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
index e30a411f5a69..fb7589fda070 100644
--- a/ofproto/ofproto-dpif-sflow.c
+++ b/ofproto/ofproto-dpif-sflow.c
@@ -1160,6 +1160,7 @@  dpif_sflow_read_actions(const struct flow *flow,
 	case OVS_ACTION_ATTR_RECIRC:
 	case OVS_ACTION_ATTR_HASH:
         case OVS_ACTION_ATTR_CT:
+    case OVS_ACTION_ATTR_CT_CLEAR:
         case OVS_ACTION_ATTR_METER:
 	    break;
 
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 896d5e290a46..6c55545cb291 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5687,6 +5687,17 @@  compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc,
 }
 
 static void
+compose_ct_clear_action(struct xlate_ctx *ctx)
+{
+    clear_conntrack(ctx);
+    /* This action originally existed without dpif support. So to preserve
+     * compatibility, only append it if the dpif supports it. */
+    if (ctx->xbridge->support.ct_clear) {
+        nl_msg_put_flag(ctx->odp_actions,  OVS_ACTION_ATTR_CT_CLEAR);
+    }
+}
+
+static void
 rewrite_flow_encap_ethernet(struct xlate_ctx *ctx,
                             struct flow *flow,
                             struct flow_wildcards *wc)
@@ -6442,7 +6453,7 @@  do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
 
         case OFPACT_CT_CLEAR:
-            clear_conntrack(ctx);
+            compose_ct_clear_action(ctx);
             break;
 
         case OFPACT_NAT:
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 6157b70aee60..675b3a697641 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1255,6 +1255,37 @@  check_ct_eventmask(struct dpif_backer *backer)
     return !error;
 }
 
+static bool
+check_ct_clear(struct dpif_backer *backer)
+{
+    struct odputil_keybuf keybuf;
+    uint8_t actbuf[NL_A_FLAG_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_flag(&actions, OVS_ACTION_ATTR_CT_CLEAR);
+
+    supported = dpif_probe_feature(backer->dpif, "ct_clear", &key,
+                                   &actions, NULL);
+
+    VLOG_INFO("%s: Datapath %s ct_clear action",
+              dpif_name(backer->dpif), (supported) ? "supports"
+                                                   : "does not support");
+    return supported;
+}
+
 #define CHECK_FEATURE__(NAME, SUPPORT, FIELD, VALUE, ETHTYPE)               \
 static bool                                                                 \
 check_##NAME(struct dpif_backer *backer)                                    \
@@ -1316,6 +1347,7 @@  check_support(struct dpif_backer *backer)
     backer->rt_support.clone = check_clone(backer);
     backer->rt_support.sample_nesting = check_max_sample_nesting(backer);
     backer->rt_support.ct_eventmask = check_ct_eventmask(backer);
+    backer->rt_support.ct_clear = check_ct_clear(backer);
 
     /* Flow fields. */
     backer->rt_support.odp.ct_state = check_ct_state(backer);
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index c96e00e6a2ab..6443cc5b2d63 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -173,7 +173,10 @@  struct group_dpif *group_dpif_lookup(struct ofproto_dpif *,
     DPIF_SUPPORT_FIELD(size_t, sample_nesting, "Sample nesting")            \
                                                                             \
     /* OVS_CT_ATTR_EVENTMASK supported by OVS_ACTION_ATTR_CT action. */     \
-    DPIF_SUPPORT_FIELD(bool, ct_eventmask, "Conntrack eventmask")
+    DPIF_SUPPORT_FIELD(bool, ct_eventmask, "Conntrack eventmask")           \
+                                                                            \
+    /* True if the datapath supports OVS_ACTION_ATTR_CT_CLEAR action. */    \
+    DPIF_SUPPORT_FIELD(bool, ct_clear, "Conntrack clear")
 
 /* Stores the various features which the corresponding backer supports. */
 struct dpif_backer_support {
diff --git a/tests/odp.at b/tests/odp.at
index 270b9ff7a82e..ea8f40ede78b 100644
--- a/tests/odp.at
+++ b/tests/odp.at
@@ -354,6 +354,7 @@  ct(force_commit,nat(src=fe80::20c:29ff:fe88:a18b,random))
 ct(force_commit,nat(src=fe80::20c:29ff:fe88:1-fe80::20c:29ff:fe88:a18b,random))
 ct(force_commit,nat(src=[[fe80::20c:29ff:fe88:1]]-[[fe80::20c:29ff:fe88:a18b]]:255-4096,random))
 ct(force_commit,helper=ftp,nat(src=10.1.1.240-10.1.1.255))
+ct_clear
 trunc(100)
 clone(1)
 clone(clone(push_vlan(vid=12,pcp=0),2),1)