[ovs-dev] [PATCH v2 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support

Justin Pettit jpettit at ovn.org
Tue Aug 6 05:27:42 UTC 2019


> On Aug 1, 2019, at 3:07 PM, Yi-Hung Wei <yihung.wei at gmail.com> wrote:
> 
> This patch also adds missing symbols in the windows datapath so
> that the build on windows can pass.

Alin, can you do a quick sanity-check on the Windows part?  Thanks.

Yi-Hung, it looks good to me.  Just mostly some minor stuff below.

> diff --git a/datapath-windows/ovsext/Netlink/NetlinkProto.h b/datapath-windows/ovsext/Netlink/NetlinkProto.h
> index 59b56565c1dc..db1fa2bacae8 100644
> --- a/datapath-windows/ovsext/Netlink/NetlinkProto.h
> +++ b/datapath-windows/ovsext/Netlink/NetlinkProto.h
> @@ -51,6 +51,7 @@
> #define NLM_F_ECHO              0x008
> 
> #define NLM_F_ROOT              0x100
> +#define NLM_F_REPLACE           0x100
> #define NLM_F_MATCH             0x200
> #define NLM_F_EXCL              0x200
> #define NLM_F_ATOMIC            0x400

Below, I mention reorganizing the similar Linux header file based on request type.  I'd recommend that here, too.

> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index 2f4906817946..8dacb1c7c253 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> ...
> +struct ct_dpif_timeout_policy {
> +    uint32_t    id;         /* An unique identifier of a timeout policy in
> +                             * the datapath. */
> +    uint32_t    present;    /* If a timeout attribute is present set the
> +                             * corresponding bit. */

This is a minor nit, but I think this would be clearer if it were less abstract.  For example:

    uint32_t    id;         /* Unique identifier for the timeout policy in
                             * the datapath. */
    uint32_t    present;    /* If a timeout attribute is present set the
                             * corresponding CT_DPIF_TP_ATTR_* mapping bit. */

> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index d0a1c58adace..2079e368fb52 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -7529,6 +7529,12 @@ const struct dpif_class dpif_netdev_class = {
>     NULL,                       /* ct_set_limits */
>     NULL,                       /* ct_get_limits */
>     NULL,                       /* ct_del_limits */
> +    NULL,                       /* ct_set_timeout_policy */
> +    NULL,                       /* ct_get_timeout_policy */
> +    NULL,                       /* ct_del_timeout_policy */
> +    NULL,                       /* ct_timeout_policy_dump_start */
> +    NULL,                       /* ct_timeout_policy_dump_next */
> +    NULL,                       /* ct_timeout_policy_dump_done */

Is there a plan to add timeout policies to the userspace datapath?  It's probably worth adding a line to "Documentation/faq/releases.rst".

> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 7bc71d6d19d7..b859508f718a 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> 
> +enum OVS_PACKED_ENUM dpif_netlink_support_timeout_policy_protocol {
> +    DPIF_NL_TP_AF_INET_TCP,
> +    DPIF_NL_TP_AF_INET_UDP,
> +    DPIF_NL_TP_AF_INET_ICMP,
> +    DPIF_NL_TP_AF_INET6_TCP,
> +    DPIF_NL_TP_AF_INET6_UDP,
> +    DPIF_NL_TP_AF_INET6_ICMPV6,
> +    DPIF_NL_TP_MAX
> +};
> +
> +#define DPIF_NL_ALL_TP 0x3F

Not that it will be changing all that much, but I think it's always nice to auto-generate these sorts of values:

	#define DPIF_NL_ALL_TP ((1UL << DPIF_NL_TP_MAX) - 1)

> +static int
> +dpif_netlink_ct_set_timeout_policy(struct dpif *dpif OVS_UNUSED,
> +                                   const struct ct_dpif_timeout_policy *tp)
> +{
> ...
> +out:
> +    ds_destroy(&nl_tp_name);
> +    return  err;

There's an extra space before 'err'.

> +struct dpif_netlink_tp_dump_node {
> +    struct      hmap_node hmap_node;      /* node in tp_dump_map. */
> +    struct      ct_dpif_timeout_policy *tp;
> +    uint32_t    present;

I think having this called 'present' is a little confusing since 'tp' has a different member called 'present'.  How about something like "l3_l4_present"?

> +static int
> +dpif_netlink_ct_timeout_policy_dump_next(struct dpif *dpif OVS_UNUSED,
> +                                         void *state,
> +                                         struct ct_dpif_timeout_policy *tp)
> +{
> ...
> +    struct nl_ct_timeout_policy nl_tp;
> +    uint32_t tp_id;

These two variables could be declared in a tighter scope.

> +static int
> +dpif_netlink_ct_timeout_policy_dump_done(struct dpif *dpif OVS_UNUSED,
> +                                         void *state)
> +{
> +    struct dpif_netlink_ct_timeout_policy_dump_state *dump_state = state;
> +    int err;
> +
> +    err = nl_ct_timeout_policy_dump_done(dump_state->nl_dump_state);
> +    hmap_destroy(&dump_state->tp_dump_map);
> +    free(dump_state);
> +    return err;
> +}

If someone calls "_dump_done()" before they've called "_dump_next()" until it returned EOF, it seems like we could leak nodes in the dump state.  Should those be cleaned up?  If not, it should probably at least be documented in this function and the prototype description.  (I didn't propose a patch here, since I didn't know how you want to handle it.)

> diff --git a/lib/dpif-netlink.h b/lib/dpif-netlink.h
> index 0a9628088275..1e753994d747 100644
> --- a/lib/dpif-netlink.h
> +++ b/lib/dpif-netlink.h
> @@ -23,6 +23,7 @@
> #include "odp-netlink.h"
> 
> #include "flow.h"
> +#include "netlink-conntrack.h"

Why is this included?  I was able to build fine without it.

> diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
> index 7631ba5d5d31..9bc0ddb66248 100644
> --- a/lib/netlink-conntrack.c
> +++ b/lib/netlink-conntrack.c
> ...
> +int
> +nl_ct_set_default_timeout_policy(const struct nl_ct_timeout_policy *nl_tp)
> +{
> ...
> +}

I don't think this function is needed anymore.

> +int
> +nl_ct_get_default_timeout_policy(uint16_t l3num, uint8_t l4num,
> +                                 struct nl_ct_timeout_policy *nl_tp)
> +{
> ...
> +}

Same with this.

> diff --git a/lib/netlink-conntrack.h b/lib/netlink-conntrack.h
> index 8b536fd65ba8..ae6e428e0929 100644
> --- a/lib/netlink-conntrack.h
> +++ b/lib/netlink-conntrack.h
> @@ -17,9 +17,12 @@
> ...
> +#include "netlink-socket.h"

Is this needed?  It seems like it builds fine without it.

> diff --git a/lib/netlink-protocol.h b/lib/netlink-protocol.h
> index c0617dfad21f..bf631b1a14d0 100644
> --- a/lib/netlink-protocol.h
> +++ b/lib/netlink-protocol.h
> @@ -48,6 +48,7 @@
> #define NLM_F_ECHO              0x008
> 
> #define NLM_F_ROOT              0x100
> +#define NLM_F_REPLACE           0x100

I know it's not new to your changes, but it looks odd to have two different definitions with the same value.  I think it would be clearer if they were grouped and labeled with the request type.

Thanks,

--Justin


-=-=-=-=-=-=-=-=-=-=-

diff --git a/Documentation/faq/releases.rst b/Documentation/faq/releases.rst
index 8daa23bb2d0c..985e20067dba 100644
--- a/Documentation/faq/releases.rst
+++ b/Documentation/faq/releases.rst
@@ -110,8 +110,9 @@ Q: Are all features available with all datapaths?
     ========================== ============== ============== ========= =======
     Connection tracking             4.3            YES          YES      YES
     Conntrack Fragment Reass.       4.3            YES          YES      YES
+    Conntrack Timeout Policies      4.3            YES          NO       NO
+    Conntrack Zone Limit            4.18           YES          NO       YES
     NAT                             4.6            YES          YES      YES
-    Conntrack zone limit            4.18           YES          NO       YES
     Tunnel - LISP                   NO             YES          NO       NO
     Tunnel - STT                    NO             YES          NO       YES
     Tunnel - GRE                    3.11           YES          YES      YES
diff --git a/datapath-windows/ovsext/Netlink/NetlinkProto.h b/datapath-windows/ovsext/Netlink/NetlinkProto.h
index db1fa2bacae8..b32f6f7fb114 100644
--- a/datapath-windows/ovsext/Netlink/NetlinkProto.h
+++ b/datapath-windows/ovsext/Netlink/NetlinkProto.h
@@ -50,14 +50,17 @@
 #define NLM_F_ACK               0x004
 #define NLM_F_ECHO              0x008
 
+/* GET request flag.*/
 #define NLM_F_ROOT              0x100
-#define NLM_F_REPLACE           0x100
 #define NLM_F_MATCH             0x200
-#define NLM_F_EXCL              0x200
 #define NLM_F_ATOMIC            0x400
-#define NLM_F_CREATE            0x400
 #define NLM_F_DUMP              (NLM_F_ROOT | NLM_F_MATCH)
 
+/* NEW request flags. */
+#define NLM_F_REPLACE           0x100
+#define NLM_F_EXCL              0x200
+#define NLM_F_CREATE            0x400
+
 /* nlmsg_type values. */
 #define NLMSG_NOOP              1
 #define NLMSG_ERROR             2
diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
index 8dacb1c7c253..aabd6962f2c0 100644
--- a/lib/ct-dpif.h
+++ b/lib/ct-dpif.h
@@ -261,10 +261,10 @@ enum OVS_PACKED_ENUM ct_dpif_tp_attr {
 };
 
 struct ct_dpif_timeout_policy {
-    uint32_t    id;         /* An unique identifier of a timeout policy in
+    uint32_t    id;         /* Unique identifier for the timeout policy in
                              * the datapath. */
     uint32_t    present;    /* If a timeout attribute is present set the
-                             * corresponding bit. */
+                             * corresponding CT_DPIF_TP_ATTR_* mapping bit. */
     uint32_t    attrs[CT_DPIF_TP_ATTR_MAX];     /* An array that specifies
                                                  * timeout attribute values */
 };
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index b859508f718a..08073a6f6a56 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -3042,7 +3042,8 @@ enum OVS_PACKED_ENUM dpif_netlink_support_timeout_policy_protocol {
     DPIF_NL_TP_MAX
 };
 
-#define DPIF_NL_ALL_TP 0x3F
+#define DPIF_NL_ALL_TP ((1UL << DPIF_NL_TP_MAX) - 1)
+
 
 static struct dpif_netlink_timeout_policy_protocol tp_protos[] = {
     [DPIF_NL_TP_AF_INET_TCP] = { .l3num = AF_INET, .l4num = IPPROTO_TCP },
@@ -3202,7 +3203,7 @@ dpif_netlink_set_ct_dpif_tp_icmpv6_attrs(
 
 static void
 dpif_netlink_set_ct_dpif_tp_attrs(const struct nl_ct_timeout_policy *nl_tp,
-                               struct ct_dpif_timeout_policy *tp)
+                                  struct ct_dpif_timeout_policy *tp)
 {
     if (nl_tp->l4num == IPPROTO_TCP) {
         dpif_netlink_set_ct_dpif_tp_tcp_attrs(nl_tp, tp);
@@ -3285,7 +3286,7 @@ dpif_netlink_ct_set_timeout_policy(struct dpif *dpif OVS_UNUSED,
 
 out:
     ds_destroy(&nl_tp_name);
-    return  err;
+    return err;
 }
 
 static int
@@ -3314,7 +3315,7 @@ dpif_netlink_ct_get_timeout_policy(struct dpif *dpif OVS_UNUSED,
 
 out:
     ds_destroy(&nl_tp_name);
-    return  err;
+    return err;
 }
 
 /* Returns 0 if all the sub timeout policies are deleted or
@@ -3353,7 +3354,7 @@ struct dpif_netlink_ct_timeout_policy_dump_state {
 struct dpif_netlink_tp_dump_node {
     struct      hmap_node hmap_node;      /* node in tp_dump_map. */
     struct      ct_dpif_timeout_policy *tp;
-    uint32_t    present;
+    uint32_t    l3_l4_present;
 };
 
 static struct dpif_netlink_tp_dump_node *
@@ -3363,7 +3364,7 @@ get_dpif_netlink_tp_dump_node_by_tp_id(uint32_t tp_id,
     struct dpif_netlink_tp_dump_node *tp_dump_node;
 
     HMAP_FOR_EACH_WITH_HASH (tp_dump_node, hmap_node, hash_int(tp_id, 0),
-                            tp_dump_map) {
+                             tp_dump_map) {
         if (tp_dump_node->tp->id == tp_id) {
             return tp_dump_node;
         }
@@ -3382,7 +3383,7 @@ update_dpif_netlink_tp_dump_node(
     for (i = 0; i < DPIF_NL_TP_MAX; ++i) {
         if (nl_tp->l3num == tp_protos[i].l3num &&
             nl_tp->l4num == tp_protos[i].l4num) {
-            tp_dump_node->present |= 1 << i;
+            tp_dump_node->l3_l4_present |= 1 << i;
             break;
         }
     }
@@ -3412,11 +3413,12 @@ dpif_netlink_ct_timeout_policy_dump_next(struct dpif *dpif OVS_UNUSED,
 {
     struct dpif_netlink_ct_timeout_policy_dump_state *dump_state = state;
     struct dpif_netlink_tp_dump_node *tp_dump_node;
-    struct nl_ct_timeout_policy nl_tp;
-    uint32_t tp_id;
     int err;
 
     do {
+        struct nl_ct_timeout_policy nl_tp;
+        uint32_t tp_id;
+
         err =  nl_ct_timeout_policy_dump_next(dump_state->nl_dump_state,
                                               &nl_tp);
         if (err) {
@@ -3438,7 +3440,7 @@ dpif_netlink_ct_timeout_policy_dump_next(struct dpif *dpif OVS_UNUSED,
         }
 
         update_dpif_netlink_tp_dump_node(&nl_tp, tp_dump_node);
-        if (tp_dump_node->present == DPIF_NL_ALL_TP) {
+        if (tp_dump_node->l3_l4_present == DPIF_NL_ALL_TP) {
             hmap_remove(&dump_state->tp_dump_map, &tp_dump_node->hmap_node);
             *tp = *tp_dump_node->tp;
             free(tp_dump_node->tp);
diff --git a/lib/dpif-netlink.h b/lib/dpif-netlink.h
index 1e753994d747..d2689029c3e0 100644
--- a/lib/dpif-netlink.h
+++ b/lib/dpif-netlink.h
@@ -20,7 +20,6 @@
 #include <stdbool.h>
 #include <stddef.h>
 #include <stdint.h>
-#include "odp-netlink.h"
 
 #include "flow.h"
 #include "netlink-conntrack.h"
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 79a2314500cf..fb56d91eafe9 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -502,16 +502,16 @@ struct dpif_class {
     /* Connection tracking timeout policy */
 
     /* A connection tracking timeout policy contains a list of timeout
-     * attributes that specifies timeout values on various connection states.
-     * In a datapath, the timeout policy is identified by a 4 bytes unsigned
-     * integer, and the unsupported timeout attributes are ignored.
-     * When a connection is committed it can be associated with a timeout
+     * attributes that specify timeout values on various connection states.
+     * In a datapath, the timeout policy is identified by a 4-byte unsigned
+     * integer.  Unsupported timeout attributes are ignored.  When a
+     * connection is committed it can be associated with a timeout
      * policy, or it defaults to the datapath's default timeout policy. */
 
     /* Sets timeout policy '*tp' into the datapath. */
     int (*ct_set_timeout_policy)(struct dpif *,
                                  const struct ct_dpif_timeout_policy *tp);
-    /* Gets a timeout policy specified by tp_id and stores it into '*tp'.*/
+    /* Gets a timeout policy specified by tp_id and stores it into '*tp'. */
     int (*ct_get_timeout_policy)(struct dpif *, uint32_t tp_id,
                                  struct ct_dpif_timeout_policy *tp);
     /* Deletes a timeout policy identified by 'tp_id'. */
diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
index 9bc0ddb66248..828e4a5a84c1 100644
--- a/lib/netlink-conntrack.c
+++ b/lib/netlink-conntrack.c
@@ -1060,34 +1060,6 @@ nl_ct_set_timeout_policy(const struct nl_ct_timeout_policy *nl_tp)
     return err;
 }
 
-int
-nl_ct_set_default_timeout_policy(const struct nl_ct_timeout_policy *nl_tp)
-{
-    struct ofpbuf buf;
-    size_t offset;
-    int i, err;
-
-    ofpbuf_init(&buf, 512);
-    nl_msg_put_nfgenmsg(&buf, 0, AF_UNSPEC, NFNL_SUBSYS_CTNETLINK_TIMEOUT,
-                        IPCTNL_MSG_TIMEOUT_DEFAULT_SET, NLM_F_REQUEST
-                        | NLM_F_ACK | NLM_F_REPLACE);
-
-    nl_msg_put_be16(&buf, CTA_TIMEOUT_L3PROTO, htons(nl_tp->l3num));
-    nl_msg_put_u8(&buf, CTA_TIMEOUT_L4PROTO, nl_tp->l4num);
-
-    offset = nl_msg_start_nested(&buf, CTA_TIMEOUT_DATA);
-    for (i = 1; i <= nl_ct_timeout_policy_max_attr[nl_tp->l4num]; ++i) {
-        if (nl_tp->present & 1 << i) {
-            nl_msg_put_be32(&buf, i, htonl(nl_tp->attrs[i]));
-        }
-    }
-    nl_msg_end_nested(&buf, offset);
-
-    err = nl_transact(NETLINK_NETFILTER, &buf, NULL);
-    ofpbuf_uninit(&buf);
-    return err;
-}
-
 int
 nl_ct_get_timeout_policy(const char *tp_name,
                          struct nl_ct_timeout_policy *nl_tp)
@@ -1112,33 +1084,6 @@ out:
     return err;
 }
 
-int
-nl_ct_get_default_timeout_policy(uint16_t l3num, uint8_t l4num,
-                                 struct nl_ct_timeout_policy *nl_tp)
-{
-    struct ofpbuf request, *reply;
-    int err;
-
-    ofpbuf_init(&request, 512);
-    nl_msg_put_nfgenmsg(&request, 0, AF_UNSPEC, NFNL_SUBSYS_CTNETLINK_TIMEOUT,
-                        IPCTNL_MSG_TIMEOUT_DEFAULT_GET,
-                        NLM_F_REQUEST | NLM_F_ACK);
-
-    nl_msg_put_be16(&request, CTA_TIMEOUT_L3PROTO, htons(l3num));
-    nl_msg_put_u8(&request, CTA_TIMEOUT_L4PROTO, l4num);
-    err = nl_transact(NETLINK_NETFILTER, &request, &reply);
-    if (err) {
-        goto out;
-    }
-
-    err = nl_ct_timeout_policy_from_ofpbuf(reply, nl_tp, true);
-
-out:
-    ofpbuf_uninit(&request);
-    ofpbuf_delete(reply);
-    return err;
-}
-
 int
 nl_ct_del_timeout_policy(const char *tp_name)
 {
diff --git a/lib/netlink-conntrack.h b/lib/netlink-conntrack.h
index ae6e428e0929..81c74549bd16 100644
--- a/lib/netlink-conntrack.h
+++ b/lib/netlink-conntrack.h
@@ -22,7 +22,6 @@
 #include "byte-order.h"
 #include "compiler.h"
 #include "ct-dpif.h"
-#include "netlink-socket.h"
 #include "openvswitch/dynamic-string.h"
 #include "openvswitch/hmap.h"
 #include "openvswitch/ofpbuf.h"
@@ -50,7 +49,7 @@ struct nl_ct_dump_state;
 struct nl_ct_timeout_policy_dump_state;
 
 int nl_ct_dump_start(struct nl_ct_dump_state **, const uint16_t *zone,
-        int *ptot_bkts);
+                     int *ptot_bkts);
 int nl_ct_dump_next(struct nl_ct_dump_state *, struct ct_dpif_entry *);
 int nl_ct_dump_done(struct nl_ct_dump_state *);
 
@@ -59,11 +58,8 @@ int nl_ct_flush_zone(uint16_t zone);
 int nl_ct_flush_tuple(const struct ct_dpif_tuple *, uint16_t zone);
 
 int nl_ct_set_timeout_policy(const struct nl_ct_timeout_policy *nl_tp);
-int nl_ct_set_default_timeout_policy(const struct nl_ct_timeout_policy *nl_tp);
 int nl_ct_get_timeout_policy(const char *tp_name,
                              struct nl_ct_timeout_policy *nl_tp);
-int nl_ct_get_default_timeout_policy(uint16_t l3num, uint8_t l4num,
-                                     struct nl_ct_timeout_policy *nl_tp);
 int nl_ct_del_timeout_policy(const char *tp_name);
 int nl_ct_timeout_policy_dump_start(
     struct nl_ct_timeout_policy_dump_state **statep);
diff --git a/lib/netlink-protocol.h b/lib/netlink-protocol.h
index bf631b1a14d0..ceded7915ef8 100644
--- a/lib/netlink-protocol.h
+++ b/lib/netlink-protocol.h
@@ -47,14 +47,17 @@
 #define NLM_F_ACK               0x004
 #define NLM_F_ECHO              0x008
 
+/* GET request flag.*/
 #define NLM_F_ROOT              0x100
-#define NLM_F_REPLACE           0x100
 #define NLM_F_MATCH             0x200
-#define NLM_F_EXCL              0x200
 #define NLM_F_ATOMIC            0x400
-#define NLM_F_CREATE            0x400
 #define NLM_F_DUMP              (NLM_F_ROOT | NLM_F_MATCH)
 
+/* NEW request flags. */
+#define NLM_F_REPLACE           0x100
+#define NLM_F_EXCL              0x200
+#define NLM_F_CREATE            0x400
+
 /* nlmsg_type values. */
 #define NLMSG_NOOP              1
 #define NLMSG_ERROR             2










More information about the dev mailing list