[ovs-dev,v8,9/9] ipf: Add fragmentation status reporting.

Message ID	1531795191-58140-10-git-send-email-dlu998@gmail.com
State	Changes Requested
Headers	show Return-Path: <ovs-dev-bounces@openvswitch.org> From: Darrell Ball <dlu998@gmail.com> To: dlu998@gmail.com, dev@openvswitch.org, jpettit@ovn.org Date: Mon, 16 Jul 2018 19:39:51 -0700 Message-Id: <1531795191-58140-10-git-send-email-dlu998@gmail.com> In-Reply-To: <1531795191-58140-1-git-send-email-dlu998@gmail.com> References: <1531795191-58140-1-git-send-email-dlu998@gmail.com> Subject: [ovs-dev] [patch v8 9/9] ipf: Add fragmentation status reporting. Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org
Series	Userspace datapath: Add fragmentation support. \| expand [ovs-dev,v8,0/9] Userspace datapath: Add fragmentation support. [ovs-dev,v8,1/9] dp-packet: Add const qualifiers for checksum apis. [ovs-dev,v8,2/9] flow: Enhance parse_ipv6_ext_hdrs. [ovs-dev,v8,3/9] tests: Add missed local stack checks. [ovs-dev,v8,4/9] conntrack: Reword conntrack_execute() description. [ovs-dev,v8,5/9] Userspace datapath: Add fragmentation handling. [ovs-dev,v8,6/9] ipf: Add command to disable fragmentation handling. [ovs-dev,v8,7/9] ipf: Add set minimum fragment size command. [ovs-dev,v8,8/9] ipf: Add set maximum fragments supported command. [ovs-dev,v8,9/9] ipf: Add fragmentation status reporting.

Message ID

1531795191-58140-10-git-send-email-dlu998@gmail.com

State

Changes Requested

Headers

From: Darrell Ball <dlu998@gmail.com>
To: dlu998@gmail.com,
	dev@openvswitch.org,
	jpettit@ovn.org
Date: Mon, 16 Jul 2018 19:39:51 -0700
Message-Id: <1531795191-58140-10-git-send-email-dlu998@gmail.com>
In-Reply-To: <1531795191-58140-1-git-send-email-dlu998@gmail.com>
References: <1531795191-58140-1-git-send-email-dlu998@gmail.com>
Subject: [ovs-dev] [patch v8 9/9] ipf: Add fragmentation status reporting.
Precedence: list
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Sender: ovs-dev-bounces@openvswitch.org
Errors-To: ovs-dev-bounces@openvswitch.org

Series

Userspace datapath: Add fragmentation support. | expand

Commit Message

Darrell Ball July 17, 2018, 2:39 a.m. UTC

A new command "ovs-appctl dpctl/ipf-get-status" is added
for userspace datapath conntrack fragmentation support.
The command shows the configuration status, fragment counters and
ipf lists state.

Signed-off-by: Darrell Ball <dlu998@gmail.com>
---
 NEWS                             |   2 +
 lib/ct-dpif.c                    |  45 +++++++++++++++
 lib/ct-dpif.h                    |   9 +++
 lib/dpctl.c                      | 107 ++++++++++++++++++++++++++++++++++
 lib/dpctl.man                    |   6 ++
 lib/dpif-netdev.c                |  58 +++++++++++++++++++
 lib/dpif-netlink.c               |   4 ++
 lib/dpif-provider.h              |  16 +++++
 lib/ipf.c                        | 107 ++++++++++++++++++++++++++++++++++
 lib/ipf.h                        |  10 ++++
 tests/system-kmod-macros.at      |  32 ++++++++++
 tests/system-traffic.at          |  40 +++++++++----
 tests/system-userspace-macros.at | 122 +++++++++++++++++++++++++++++++++++++++
 13 files changed, 548 insertions(+), 10 deletions(-)

Comments

Justin Pettit Aug. 11, 2018, 3 a.m. UTC | #1

> On Jul 16, 2018, at 4:39 PM, Darrell Ball <dlu998@gmail.com> wrote:
> 
> void
> ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
> {
> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index f886ab9..2ff7e26 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -204,6 +204,15 @@ int ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns);
> int ct_dpif_ipf_set_enabled(struct dpif *, bool v6, bool enable);
> int ct_dpif_ipf_set_min_frag(struct dpif *, bool, uint32_t);
> int ct_dpif_ipf_set_max_nfrags(struct dpif *, uint32_t);
> +int ct_dpif_ipf_get_status(struct dpif *dpif, bool *, unsigned int *,
> +                           unsigned int *, unsigned int *, unsigned int *,
> +                           unsigned int *, unsigned int *, unsigned int *,
> +                           unsigned int *, bool *, unsigned int *,
> +                           unsigned int *, unsigned int *, unsigned int *,
> +                           unsigned int *, unsigned int *);
> +int ct_dpif_ipf_dump_start(struct dpif *dpif, struct ipf_dump_ctx **);

On my system, I needed to declare "ipf_dump_ctx" for the build to finish.

> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 5ec36bd..b3e7ce7 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> ..
> +static int
> +dpctl_ct_ipf_get_status(int argc, const char *argv[],
> +                        struct dpctl_params *dpctl_p)
> +{
> +    struct dpif *dpif;
> +    int error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif);
> +    if (!error) {
> +        bool ipf_v4_enabled;
> +        unsigned int min_v4_frag_size;
> +        unsigned int nfrag_max;
> +        unsigned int nfrag;
> +        unsigned int n4frag_accepted;
> +        unsigned int n4frag_completed_sent;
> +        unsigned int n4frag_expired_sent;
> +        unsigned int n4frag_too_small;
> +        unsigned int n4frag_overlap;
> +        unsigned int min_v6_frag_size;
> +        bool ipf_v6_enabled;
> +        unsigned int n6frag_accepted;
> +        unsigned int n6frag_completed_sent;
> +        unsigned int n6frag_expired_sent;
> +        unsigned int n6frag_too_small;
> +        unsigned int n6frag_overlap;
> +        error = ct_dpif_ipf_get_status(dpif, &ipf_v4_enabled,
> +            &min_v4_frag_size, &nfrag_max, &nfrag, &n4frag_accepted,
> +            &n4frag_completed_sent, &n4frag_expired_sent, &n4frag_too_small,
> +            &n4frag_overlap, &ipf_v6_enabled, &min_v6_frag_size,
> +            &n6frag_accepted, &n6frag_completed_sent, &n6frag_expired_sent,
> +            &n6frag_too_small, &n6frag_overlap);
> +

If we just use 'ipf_status', we can delete a lot of these variable declarations.

> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 76bc1d9..db551ea 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -6553,6 +6553,60 @@ dpif_netdev_ipf_set_max_nfrags(struct dpif *dpif OVS_UNUSED,
>     return ipf_set_max_nfrags(max_frags);
> }
> 
> +static int
> +dpif_netdev_ipf_get_status(struct dpif *dpif OVS_UNUSED,
> +    bool *ipf_v4_enabled, unsigned int *min_v4_frag_size,
> +    unsigned int *nfrag_max, unsigned int *nfrag,
> +    unsigned int *n4frag_accepted, unsigned int *n4frag_completed_sent,
> +    unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
> +    unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> +    unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> +    unsigned int *n6frag_completed_sent, unsigned int *n6frag_expired_sent,
> +    unsigned int *n6frag_too_small, unsigned int *n6frag_overlap)
> +{
> +    struct ipf_status ipf_status;
> +    ipf_get_status(&ipf_status);
> +    *ipf_v4_enabled = ipf_status.ifp_v4_enabled;
> +    *min_v4_frag_size = ipf_status.min_v4_frag_size;
> +    *nfrag_max = ipf_status.nfrag_max;
> +    *nfrag = ipf_status.nfrag;
> +    *n4frag_accepted = ipf_status.n4frag_accepted;
> +    *n4frag_completed_sent = ipf_status.n4frag_completed_sent;
> +    *n4frag_expired_sent = ipf_status.n4frag_expired_sent;
> +    *n4frag_too_small = ipf_status.n4frag_too_small;
> +    *n4frag_overlap = ipf_status.n4frag_overlap;
> +    *ipf_v6_enabled = ipf_status.ifp_v6_enabled;
> +    *min_v6_frag_size = ipf_status.min_v6_frag_size;
> +    *n6frag_accepted = ipf_status.n6frag_accepted;
> +    *n6frag_completed_sent = ipf_status.n6frag_completed_sent;
> +    *n6frag_expired_sent = ipf_status.n6frag_expired_sent;
> +    *n6frag_too_small = ipf_status.n6frag_too_small;
> +    *n6frag_overlap = ipf_status.n6frag_overlap;
> +    return 0;
> +}

As, I'll suggest later, this can be reduced to just a couple of lines if 'ipf_status' is passed into this function.

> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 214f570..2e0d596 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -451,6 +451,22 @@ struct dpif_class {
>     int (*ipf_set_min_frag)(struct dpif *, bool v6, uint32_t min_frag);
>     /* Set maximum number of fragments tracked. */
>     int (*ipf_set_max_nfrags)(struct dpif *, uint32_t max_nfrags);
> +    /* Get fragmentation configuration status and counters. */
> +    int (*ipf_get_status)(struct dpif *, bool *ipf_v4_enabled,
> +        unsigned int *min_v4_frag_size,
> +        unsigned int *nfrag_max, unsigned int *nfrag,
> +        unsigned int *n4frag_accepted, unsigned int *n4frag_completed_sent,
> +        unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
> +        unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> +        unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> +        unsigned int *n6frag_completed_sent,
> +        unsigned int *n6frag_expired_sent, unsigned int *n6frag_too_small,
> +        unsigned int *n6frag_overlap);

I'd suggest passing in 'ipf_status', since it makes this prototype much smaller and has all the information you need.

> +    int (*ipf_dump_start)(struct dpif *, struct ipf_dump_ctx **ipf_dump_ctx);
> +    /* Finds the next ipf list and creates a string representation of the
> +     * state of an ipf list, to which 'dump' is pointed to. */

This comment should probably go above the declaration of ipf_dump_start().

> +    int (*ipf_dump_next)(struct dpif *, void *, char **dump);
> +    int (*ipf_dump_done)(struct dpif *, void *ipf_dump_ctx);

> diff --git a/lib/ipf.h b/lib/ipf.h
> index 4289e5e..36a182a 100644
> --- a/lib/ipf.h
> +++ b/lib/ipf.h
> @@ -63,4 +63,14 @@ int ipf_set_min_frag(bool v6, uint32_t value);
> 
> int ipf_set_max_nfrags(uint32_t value);
> 

We need to reintroduce 'ipf_status', since this is where it's first used:

> +struct ipf_status {
> +   bool ifp_v4_enabled;
> +   unsigned int min_v4_frag_size;
> +   unsigned int nfrag_max;
> +   unsigned int nfrag;
> +   unsigned int n4frag_accepted;
> +   unsigned int n4frag_completed_sent;
> +   unsigned int n4frag_expired_sent;
> +   unsigned int n4frag_too_small;
> +   unsigned int n4frag_overlap;
> +   bool ifp_v6_enabled;
> +   unsigned int min_v6_frag_size;
> +   unsigned int n6frag_accepted;
> +   unsigned int n6frag_completed_sent;
> +   unsigned int n6frag_expired_sent;
> +   unsigned int n6frag_too_small;
> +   unsigned int n6frag_overlap;
> +};

I think it's a little cleaner if we create another struct that consolidates the common elements between IPv4 and IPv6.  Something like the following:

-=-=-=-=-=-=-=-
struct proto_status {
    bool enabled;
    unsigned int min_frag_size;

    /* Fragment statistics */
    unsigned int accepted;
    unsigned int completed_sent;
    unsigned int expired_sent;
    unsigned int too_small;
    unsigned int overlap;
};

struct ipf_status {
    unsigned int nfrag;
    unsigned int nfrag_max;

    struct proto_status v4;
    struct proto_status v6;
};
-=-=-=-=-=-=-=-

I worry a bit about wrapping 32-bit counters.  I see that the current atomic_count is an unsigned int, but It might be worth creating a new 64-bit type so that we can handle things like "accepted", which may be fairly high.

> diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at
> index 40b7567..fcae3cc 100644
> --- a/tests/system-userspace-macros.at
> +++ b/tests/system-userspace-macros.at
> ...
> +# FORMAT_FRAG_LIST([])
> +#
> +# Strip content from the piped input which can differ from test to test; recirc_id
> +# and ip_id fields in an ipf_list vary from test to test and hence are cleared.
> +m4_define([FORMAT_FRAG_LIST],
> +    [[sed -e 's/ip_id=[0-9]*/ip_id=<cleared>/g' -e 's/recirc_id=[0-9]*/recirc_id=<cleared>/g']])
> +
> +# DPCTL_CHECK_FRAGMENTATION_FAIL()
> +#
> +# Used to check fragmentation counters for some fragmentation tests using
> +# the userspace datapath, when failure to transmit fragments is expected.
> +m4_define([DPCTL_CHECK_FRAGMENTATION_FAIL],
> +[
> +AT_CHECK([ovs-appctl dpctl/ipf-get-status -m | FORMAT_FRAG_LIST()], [], [dnl
> +        Fragmentation Module Status
> +        ---------------------------
> +        v4 enabled: 1
> +        v6 enabled: 1
> +        max num frags (v4/v6): 500
> +        num frag: 7
> +        min v4 frag size: 1000
> +        v4 frags accepted: 7
> +        v4 frags completed: 0
> +        v4 frags expired: 0
> +        v4 frags too small: 0
> +        v4 frags overlapped: 0
> +        min v6 frag size: 1280
> +        v6 frags accepted: 0
> +        v6 frags completed: 0
> +        v6 frags expired: 0
> +        v6 frags too small: 0
> +        v6 frags overlapped: 0
> +
> +        Fragment Lists:
> +
> +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<cleared>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<cleared>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<cleared>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<cleared>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<cleared>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<cleared>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<cleared>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)

If it's not too difficult, it might be nice to add some IPv6 stats here, too.

I've appended an incremental with some of the suggestions I made above as well as a few minor documentation/comment changes.

--Justin


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

diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index a59bc1e930d4..f5387c6a0678 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -20,6 +20,7 @@
 #include <errno.h>
 
 #include "ct-dpif.h"
+#include "ipf.h"
 #include "openvswitch/ofp-parse.h"
 #include "openvswitch/vlog.h"
 
@@ -188,24 +189,10 @@ ct_dpif_ipf_set_max_nfrags(struct dpif *dpif, uint32_t max
_frags)
             : EOPNOTSUPP);
 }
 
-int ct_dpif_ipf_get_status(struct dpif *dpif, bool *ipf_v4_enabled,
-    unsigned int *min_v4_frag_size, unsigned int *nfrag_max,
-    unsigned int *nfrag, unsigned int *n4frag_accepted,
-    unsigned int *n4frag_completed_sent,
-    unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
-    unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
-    unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
-    unsigned int *n6frag_completed_sent,
-    unsigned int *n6frag_expired_sent, unsigned int *n6frag_too_small,
-    unsigned int *n6frag_overlap)
+int ct_dpif_ipf_get_status(struct dpif *dpif, struct ipf_status *status)
 {
     return (dpif->dpif_class->ipf_get_status
-            ? dpif->dpif_class->ipf_get_status(dpif, ipf_v4_enabled,
-            min_v4_frag_size, nfrag_max, nfrag, n4frag_accepted,
-            n4frag_completed_sent, n4frag_expired_sent, n4frag_too_small,
-            n4frag_overlap, ipf_v6_enabled, min_v6_frag_size, n6frag_accepted,
-            n6frag_completed_sent, n6frag_expired_sent, n6frag_too_small,
-            n6frag_overlap)
+            ? dpif->dpif_class->ipf_get_status(dpif, status)
             : EOPNOTSUPP);
 }
 
diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
index 57d375bc1e82..603dbcf2d14a 100644
--- a/lib/ct-dpif.h
+++ b/lib/ct-dpif.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015 Nicira, Inc.
+ * Copyright (c) 2015, 2018 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,6 +20,9 @@
 #include "openvswitch/types.h"
 #include "packets.h"
 
+struct ipf_dump_ctx;
+struct ipf_status;
+
 union ct_dpif_inet_addr {
     ovs_be32 ip;
     ovs_be32 ip6[4];
@@ -203,12 +206,7 @@ int ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns)
;
 int ct_dpif_ipf_set_enabled(struct dpif *, bool v6, bool enable);
 int ct_dpif_ipf_set_min_frag(struct dpif *, bool, uint32_t);
 int ct_dpif_ipf_set_max_nfrags(struct dpif *, uint32_t);
-int ct_dpif_ipf_get_status(struct dpif *dpif, bool *, unsigned int *,
-                           unsigned int *, unsigned int *, unsigned int *,
-                           unsigned int *, unsigned int *, unsigned int *,
-                           unsigned int *, bool *, unsigned int *,
-                           unsigned int *, unsigned int *, unsigned int *,
-                           unsigned int *, unsigned int *);
+int ct_dpif_ipf_get_status(struct dpif *dpif, struct ipf_status *);
 int ct_dpif_ipf_dump_start(struct dpif *dpif, struct ipf_dump_ctx **);
 int ct_dpif_ipf_dump_next(struct dpif *dpif, void *, char **);
 int ct_dpif_ipf_dump_done(struct dpif *dpif, void *);
diff --git a/lib/dpctl.c b/lib/dpctl.c
index f650b37dd901..832220e5102c 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1799,61 +1799,41 @@ dpctl_ct_ipf_get_status(int argc, const char *argv[],
     struct dpif *dpif;
     int error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif);
     if (!error) {
-        bool ipf_v4_enabled;
-        unsigned int min_v4_frag_size;
-        unsigned int nfrag_max;
-        unsigned int nfrag;
-        unsigned int n4frag_accepted;
-        unsigned int n4frag_completed_sent;
-        unsigned int n4frag_expired_sent;
-        unsigned int n4frag_too_small;
-        unsigned int n4frag_overlap;
-        unsigned int min_v6_frag_size;
-        bool ipf_v6_enabled;
-        unsigned int n6frag_accepted;
-        unsigned int n6frag_completed_sent;
-        unsigned int n6frag_expired_sent;
-        unsigned int n6frag_too_small;
-        unsigned int n6frag_overlap;
-        error = ct_dpif_ipf_get_status(dpif, &ipf_v4_enabled,
-            &min_v4_frag_size, &nfrag_max, &nfrag, &n4frag_accepted,
-            &n4frag_completed_sent, &n4frag_expired_sent, &n4frag_too_small,
-            &n4frag_overlap, &ipf_v6_enabled, &min_v6_frag_size,
-            &n6frag_accepted, &n6frag_completed_sent, &n6frag_expired_sent,
-            &n6frag_too_small, &n6frag_overlap);
+        struct ipf_status status;
+        error = ct_dpif_ipf_get_status(dpif, &status);
 
         if (!error) {
             dpctl_print(dpctl_p, "        Fragmentation Module Status\n");
             dpctl_print(dpctl_p, "        ---------------------------\n");
-            dpctl_print(dpctl_p, "        v4 enabled: %u\n", ipf_v4_enabled);
-            dpctl_print(dpctl_p, "        v6 enabled: %u\n", ipf_v6_enabled);
+            dpctl_print(dpctl_p, "        v4 enabled: %u\n", status.v4.enabled);
+            dpctl_print(dpctl_p, "        v6 enabled: %u\n", status.v6.enabled);
             dpctl_print(dpctl_p, "        max num frags (v4/v6): %u\n",
-                        nfrag_max);
-            dpctl_print(dpctl_p, "        num frag: %u\n", nfrag);
+                        status.nfrag_max);
+            dpctl_print(dpctl_p, "        num frag: %u\n", status.nfrag);
             dpctl_print(dpctl_p, "        min v4 frag size: %u\n",
-                        min_v4_frag_size);
+                        status.v4.min_frag_size);
             dpctl_print(dpctl_p, "        v4 frags accepted: %u\n",
-                        n4frag_accepted);
+                        status.v4.accepted);
             dpctl_print(dpctl_p, "        v4 frags completed: %u\n",
-                        n4frag_completed_sent);
+                        status.v4.completed_sent);
             dpctl_print(dpctl_p, "        v4 frags expired: %u\n",
-                        n4frag_expired_sent);
+                        status.v4.expired_sent);
             dpctl_print(dpctl_p, "        v4 frags too small: %u\n",
-                        n4frag_too_small);
+                        status.v4.too_small);
             dpctl_print(dpctl_p, "        v4 frags overlapped: %u\n",
-                        n4frag_overlap);
+                        status.v4.overlap);
             dpctl_print(dpctl_p, "        min v6 frag size: %u\n",
-                        min_v6_frag_size);
+                        status.v6.min_frag_size);
             dpctl_print(dpctl_p, "        v6 frags accepted: %u\n",
-                        n6frag_accepted);
+                        status.v6.accepted);
             dpctl_print(dpctl_p, "        v6 frags completed: %u\n",
-                        n6frag_completed_sent);
+                        status.v6.completed_sent);
             dpctl_print(dpctl_p, "        v6 frags expired: %u\n",
-                        n6frag_expired_sent);
+                        status.v6.expired_sent);
             dpctl_print(dpctl_p, "        v6 frags too small: %u\n",
-                        n6frag_too_small);
+                        status.v6.too_small);
             dpctl_print(dpctl_p, "        v6 frags overlapped: %u\n",
-                        n6frag_overlap);
+                        status.v6.overlap);
         } else {
             dpctl_error(dpctl_p, error,
                         "ipf status could not be retrieved");
diff --git a/lib/dpctl.man b/lib/dpctl.man
index fc53094d2dc4..db2b7f8bcc4b 100644
--- a/lib/dpctl.man
+++ b/lib/dpctl.man
@@ -304,5 +304,6 @@ fragmentation is enabled.  Only supported for userspace datapath.
 .TP
 .DO "[\fB\-m\fR | \fB\-\-more\fR]" "\*(DX\fBipf\-get\-status\fR [\fIdp\fR]"
 Gets the configuration settings and fragment counters associated with the
-fragmentation handling of the userspace datapath connection tracker.  With
-\fB\-m\fR or \fB\-\-more\fR, also dumps the ipf lists.
+fragmentation handling of the userspace datapath connection tracker.
+With \fB\-m\fR or \fB\-\-more\fR, also dumps the IP fragment lists.
+Only supported for userspace datapath.
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 73ab632b969e..d2cd0cbfe2ec 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -6801,33 +6801,9 @@ dpif_netdev_ipf_set_max_nfrags(struct dpif *dpif OVS_UNUSED,
 
 static int
 dpif_netdev_ipf_get_status(struct dpif *dpif OVS_UNUSED,
-    bool *ipf_v4_enabled, unsigned int *min_v4_frag_size,
-    unsigned int *nfrag_max, unsigned int *nfrag,
-    unsigned int *n4frag_accepted, unsigned int *n4frag_completed_sent,
-    unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
-    unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
-    unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
-    unsigned int *n6frag_completed_sent, unsigned int *n6frag_expired_sent,
-    unsigned int *n6frag_too_small, unsigned int *n6frag_overlap)
-{
-    struct ipf_status ipf_status;
-    ipf_get_status(&ipf_status);
-    *ipf_v4_enabled = ipf_status.ifp_v4_enabled;
-    *min_v4_frag_size = ipf_status.min_v4_frag_size;
-    *nfrag_max = ipf_status.nfrag_max;
-    *nfrag = ipf_status.nfrag;
-    *n4frag_accepted = ipf_status.n4frag_accepted;
-    *n4frag_completed_sent = ipf_status.n4frag_completed_sent;
-    *n4frag_expired_sent = ipf_status.n4frag_expired_sent;
-    *n4frag_too_small = ipf_status.n4frag_too_small;
-    *n4frag_overlap = ipf_status.n4frag_overlap;
-    *ipf_v6_enabled = ipf_status.ifp_v6_enabled;
-    *min_v6_frag_size = ipf_status.min_v6_frag_size;
-    *n6frag_accepted = ipf_status.n6frag_accepted;
-    *n6frag_completed_sent = ipf_status.n6frag_completed_sent;
-    *n6frag_expired_sent = ipf_status.n6frag_expired_sent;
-    *n6frag_too_small = ipf_status.n6frag_too_small;
-    *n6frag_overlap = ipf_status.n6frag_overlap;
+                           struct ipf_status *status)
+{
+    ipf_get_status(status);
     return 0;
 }
 
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 2e0d5968cea6..457911b7277a 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -452,19 +452,11 @@ struct dpif_class {
     /* Set maximum number of fragments tracked. */
     int (*ipf_set_max_nfrags)(struct dpif *, uint32_t max_nfrags);
     /* Get fragmentation configuration status and counters. */
-    int (*ipf_get_status)(struct dpif *, bool *ipf_v4_enabled,
-        unsigned int *min_v4_frag_size,
-        unsigned int *nfrag_max, unsigned int *nfrag,
-        unsigned int *n4frag_accepted, unsigned int *n4frag_completed_sent,
-        unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
-        unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
-        unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
-        unsigned int *n6frag_completed_sent,
-        unsigned int *n6frag_expired_sent, unsigned int *n6frag_too_small,
-        unsigned int *n6frag_overlap);
-    int (*ipf_dump_start)(struct dpif *, struct ipf_dump_ctx **ipf_dump_ctx);
+    int (*ipf_get_status)(struct dpif *, struct ipf_status *);
+
     /* Finds the next ipf list and creates a string representation of the
      * state of an ipf list, to which 'dump' is pointed to. */
+    int (*ipf_dump_start)(struct dpif *, struct ipf_dump_ctx **ipf_dump_ctx);
     int (*ipf_dump_next)(struct dpif *, void *, char **dump);
     int (*ipf_dump_done)(struct dpif *, void *ipf_dump_ctx);
 
diff --git a/lib/ipf.c b/lib/ipf.c
index ff08f5933e96..ff4de524abf2 100644
--- a/lib/ipf.c
+++ b/lib/ipf.c
@@ -1347,26 +1347,22 @@ ipf_set_max_nfrags(uint32_t value)
 int
 ipf_get_status(struct ipf_status *ipf_status)
 {
-    atomic_read_relaxed(&ifp_v4_enabled, &ipf_status->ifp_v4_enabled);
-    atomic_read_relaxed(&min_v4_frag_size, &ipf_status->min_v4_frag_size);
+    atomic_read_relaxed(&ifp_v4_enabled, &ipf_status->v4.enabled);
+    atomic_read_relaxed(&min_v4_frag_size, &ipf_status->v4.min_frag_size);
     atomic_read_relaxed(&nfrag_max, &ipf_status->nfrag_max);
     ipf_status->nfrag = atomic_count_get(&nfrag);
-    ipf_status->n4frag_accepted = atomic_count_get(&n4frag_accepted);
-    ipf_status->n4frag_completed_sent =
-        atomic_count_get(&n4frag_completed_sent);
-    ipf_status->n4frag_expired_sent =
-        atomic_count_get(&n4frag_expired_sent);
-    ipf_status->n4frag_too_small = atomic_count_get(&n4frag_too_small);
-    ipf_status->n4frag_overlap = atomic_count_get(&n4frag_overlap);
-    atomic_read_relaxed(&ifp_v6_enabled, &ipf_status->ifp_v6_enabled);
-    atomic_read_relaxed(&min_v6_frag_size, &ipf_status->min_v6_frag_size);
-    ipf_status->n6frag_accepted = atomic_count_get(&n6frag_accepted);
-    ipf_status->n6frag_completed_sent =
-        atomic_count_get(&n6frag_completed_sent);
-    ipf_status->n6frag_expired_sent =
-        atomic_count_get(&n6frag_expired_sent);
-    ipf_status->n6frag_too_small = atomic_count_get(&n6frag_too_small);
-    ipf_status->n6frag_overlap = atomic_count_get(&n6frag_overlap);
+    ipf_status->v4.accepted = atomic_count_get(&n4frag_accepted);
+    ipf_status->v4.completed_sent = atomic_count_get(&n4frag_completed_sent);
+    ipf_status->v4.expired_sent = atomic_count_get(&n4frag_expired_sent);
+    ipf_status->v4.too_small = atomic_count_get(&n4frag_too_small);
+    ipf_status->v4.overlap = atomic_count_get(&n4frag_overlap);
+    atomic_read_relaxed(&ifp_v6_enabled, &ipf_status->v6.enabled);
+    atomic_read_relaxed(&min_v6_frag_size, &ipf_status->v6.min_frag_size);
+    ipf_status->v6.accepted = atomic_count_get(&n6frag_accepted);
+    ipf_status->v6.completed_sent = atomic_count_get(&n6frag_completed_sent);
+    ipf_status->v6.expired_sent = atomic_count_get(&n6frag_expired_sent);
+    ipf_status->v6.too_small = atomic_count_get(&n6frag_too_small);
+    ipf_status->v6.overlap = atomic_count_get(&n6frag_overlap);
     return 0;
 }
 
@@ -1374,7 +1370,8 @@ struct ipf_dump_ctx {
     struct hmap_position bucket_pos;
 };
 
-/* Allocates an 'ipf_dump_ctx' to keep track of an hmap position. */
+/* Allocates an 'ipf_dump_ctx' to keep track of an hmap position. The
+ * caller must call ipf_dump_done() when dumping is finished. */
 int
 ipf_dump_start(struct ipf_dump_ctx **ipf_dump_ctx)
 {
@@ -1415,7 +1412,7 @@ ipf_dump_create(const struct ipf_list *ipf_list, struct ds *ds)
 
 /* Finds the next ipf list starting from 'ipf_dump_ctx->bucket_pos' and uses
  * ipf_dump_create() to create a string representation of the state of an
- * ipf list, to which 'dump' is pointed to. */
+ * ipf list, to which 'dump' is pointed to.  Return EOF when */
 int
 ipf_dump_next(struct ipf_dump_ctx *ipf_dump_ctx, char **dump)
 {
@@ -1439,7 +1436,7 @@ ipf_dump_next(struct ipf_dump_ctx *ipf_dump_ctx, char **dump)
     }
 }
 
-/* Frees an ipf_dump_ctx allocated by ipf_dump_start. */
+/* Frees 'ipf_dump_ctx' allocated by ipf_dump_start(). */
 int
 ipf_dump_done(struct ipf_dump_ctx *ipf_dump_ctx)
 {
diff --git a/lib/ipf.h b/lib/ipf.h
index 635988b2fcd8..8fa58ab75851 100644
--- a/lib/ipf.h
+++ b/lib/ipf.h
@@ -20,6 +20,26 @@
 #include "dp-packet.h"
 #include "openvswitch/types.h"
 
+struct proto_status {
+    bool enabled;
+    unsigned int min_frag_size;
+
+    /* Fragment statistics */
+    unsigned int accepted;
+    unsigned int completed_sent;
+    unsigned int expired_sent;
+    unsigned int too_small;
+    unsigned int overlap;
+};
+
+struct ipf_status {
+    unsigned int nfrag;
+    unsigned int nfrag_max;
+
+    struct proto_status v4;
+    struct proto_status v6;
+};
+
 void ipf_preprocess_conntrack(struct dp_packet_batch *pb, long long now,
                               ovs_be16 dl_type, uint16_t zone,
                               uint32_t hash_basis);

Darrell Ball Aug. 13, 2018, 9:19 p.m. UTC | #2

Thanks.

Darrell

On Fri, Aug 10, 2018 at 8:00 PM, Justin Pettit <jpettit@ovn.org> wrote:

>
> > On Jul 16, 2018, at 4:39 PM, Darrell Ball <dlu998@gmail.com> wrote:
> >
> > void
> > ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
> > {
> > diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> > index f886ab9..2ff7e26 100644
> > --- a/lib/ct-dpif.h
> > +++ b/lib/ct-dpif.h
> > @@ -204,6 +204,15 @@ int ct_dpif_get_nconns(struct dpif *dpif, uint32_t
> *nconns);
> > int ct_dpif_ipf_set_enabled(struct dpif *, bool v6, bool enable);
> > int ct_dpif_ipf_set_min_frag(struct dpif *, bool, uint32_t);
> > int ct_dpif_ipf_set_max_nfrags(struct dpif *, uint32_t);
> > +int ct_dpif_ipf_get_status(struct dpif *dpif, bool *, unsigned int *,
> > +                           unsigned int *, unsigned int *, unsigned int
> *,
> > +                           unsigned int *, unsigned int *, unsigned int
> *,
> > +                           unsigned int *, bool *, unsigned int *,
> > +                           unsigned int *, unsigned int *, unsigned int
> *,
> > +                           unsigned int *, unsigned int *);
> > +int ct_dpif_ipf_dump_start(struct dpif *dpif, struct ipf_dump_ctx **);
>
> On my system, I needed to declare "ipf_dump_ctx" for the build to finish.
>
> > diff --git a/lib/dpctl.c b/lib/dpctl.c
> > index 5ec36bd..b3e7ce7 100644
> > --- a/lib/dpctl.c
> > +++ b/lib/dpctl.c
> > ..
> > +static int
> > +dpctl_ct_ipf_get_status(int argc, const char *argv[],
> > +                        struct dpctl_params *dpctl_p)
> > +{
> > +    struct dpif *dpif;
> > +    int error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif);
> > +    if (!error) {
> > +        bool ipf_v4_enabled;
> > +        unsigned int min_v4_frag_size;
> > +        unsigned int nfrag_max;
> > +        unsigned int nfrag;
> > +        unsigned int n4frag_accepted;
> > +        unsigned int n4frag_completed_sent;
> > +        unsigned int n4frag_expired_sent;
> > +        unsigned int n4frag_too_small;
> > +        unsigned int n4frag_overlap;
> > +        unsigned int min_v6_frag_size;
> > +        bool ipf_v6_enabled;
> > +        unsigned int n6frag_accepted;
> > +        unsigned int n6frag_completed_sent;
> > +        unsigned int n6frag_expired_sent;
> > +        unsigned int n6frag_too_small;
> > +        unsigned int n6frag_overlap;
> > +        error = ct_dpif_ipf_get_status(dpif, &ipf_v4_enabled,
> > +            &min_v4_frag_size, &nfrag_max, &nfrag, &n4frag_accepted,
> > +            &n4frag_completed_sent, &n4frag_expired_sent,
> &n4frag_too_small,
> > +            &n4frag_overlap, &ipf_v6_enabled, &min_v6_frag_size,
> > +            &n6frag_accepted, &n6frag_completed_sent,
> &n6frag_expired_sent,
> > +            &n6frag_too_small, &n6frag_overlap);
> > +
>
> If we just use 'ipf_status', we can delete a lot of these variable
> declarations.
>


I understand; I commented earlier about this. The reason for not using
ipf_status is not because I like typing lots.

The reasoning is:

1/ ipf_status is specific to the userspace datapath. The kernel datapath
and various hardware offload
datapaths may not use or even be able to use the same datatype.
Note that dpctl and dpif code is intended to be generic across datapaths.

2/ Using ipf_status exposes what is intended to be private internal
datastructures to external modules
I was trying to avoid that, if possible, and use base types in common code.

Perhaps, the easiest way to address this to simply have a dpif_ipf_status,
which is defined similarly to ipf_status, at
least initially ? These can diverge later if needed,



>
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 76bc1d9..db551ea 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -6553,6 +6553,60 @@ dpif_netdev_ipf_set_max_nfrags(struct dpif *dpif
> OVS_UNUSED,
> >     return ipf_set_max_nfrags(max_frags);
> > }
> >
> > +static int
> > +dpif_netdev_ipf_get_status(struct dpif *dpif OVS_UNUSED,
> > +    bool *ipf_v4_enabled, unsigned int *min_v4_frag_size,
> > +    unsigned int *nfrag_max, unsigned int *nfrag,
> > +    unsigned int *n4frag_accepted, unsigned int *n4frag_completed_sent,
> > +    unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
> > +    unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> > +    unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> > +    unsigned int *n6frag_completed_sent, unsigned int
> *n6frag_expired_sent,
> > +    unsigned int *n6frag_too_small, unsigned int *n6frag_overlap)
> > +{
> > +    struct ipf_status ipf_status;
> > +    ipf_get_status(&ipf_status);
> > +    *ipf_v4_enabled = ipf_status.ifp_v4_enabled;
> > +    *min_v4_frag_size = ipf_status.min_v4_frag_size;
> > +    *nfrag_max = ipf_status.nfrag_max;
> > +    *nfrag = ipf_status.nfrag;
> > +    *n4frag_accepted = ipf_status.n4frag_accepted;
> > +    *n4frag_completed_sent = ipf_status.n4frag_completed_sent;
> > +    *n4frag_expired_sent = ipf_status.n4frag_expired_sent;
> > +    *n4frag_too_small = ipf_status.n4frag_too_small;
> > +    *n4frag_overlap = ipf_status.n4frag_overlap;
> > +    *ipf_v6_enabled = ipf_status.ifp_v6_enabled;
> > +    *min_v6_frag_size = ipf_status.min_v6_frag_size;
> > +    *n6frag_accepted = ipf_status.n6frag_accepted;
> > +    *n6frag_completed_sent = ipf_status.n6frag_completed_sent;
> > +    *n6frag_expired_sent = ipf_status.n6frag_expired_sent;
> > +    *n6frag_too_small = ipf_status.n6frag_too_small;
> > +    *n6frag_overlap = ipf_status.n6frag_overlap;
> > +    return 0;
> > +}
>
> As, I'll suggest later, this can be reduced to just a couple of lines if
> 'ipf_status' is passed into this function.
>

same comment above; I can use a dpif_ipf_status.




>
> > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> > index 214f570..2e0d596 100644
> > --- a/lib/dpif-provider.h
> > +++ b/lib/dpif-provider.h
> > @@ -451,6 +451,22 @@ struct dpif_class {
> >     int (*ipf_set_min_frag)(struct dpif *, bool v6, uint32_t min_frag);
> >     /* Set maximum number of fragments tracked. */
> >     int (*ipf_set_max_nfrags)(struct dpif *, uint32_t max_nfrags);
> > +    /* Get fragmentation configuration status and counters. */
> > +    int (*ipf_get_status)(struct dpif *, bool *ipf_v4_enabled,
> > +        unsigned int *min_v4_frag_size,
> > +        unsigned int *nfrag_max, unsigned int *nfrag,
> > +        unsigned int *n4frag_accepted, unsigned int
> *n4frag_completed_sent,
> > +        unsigned int *n4frag_expired_sent, unsigned int
> *n4frag_too_small,
> > +        unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> > +        unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> > +        unsigned int *n6frag_completed_sent,
> > +        unsigned int *n6frag_expired_sent, unsigned int
> *n6frag_too_small,
> > +        unsigned int *n6frag_overlap);
>
> I'd suggest passing in 'ipf_status', since it makes this prototype much
> smaller and has all the information you need.
>


same comment as above.


>
> > +    int (*ipf_dump_start)(struct dpif *, struct ipf_dump_ctx
> **ipf_dump_ctx);
> > +    /* Finds the next ipf list and creates a string representation of
> the
> > +     * state of an ipf list, to which 'dump' is pointed to. */
>
> This comment should probably go above the declaration of ipf_dump_start().
>


yep


>
> > +    int (*ipf_dump_next)(struct dpif *, void *, char **dump);
> > +    int (*ipf_dump_done)(struct dpif *, void *ipf_dump_ctx);
>
> > diff --git a/lib/ipf.h b/lib/ipf.h
> > index 4289e5e..36a182a 100644
> > --- a/lib/ipf.h
> > +++ b/lib/ipf.h
> > @@ -63,4 +63,14 @@ int ipf_set_min_frag(bool v6, uint32_t value);
> >
> > int ipf_set_max_nfrags(uint32_t value);
> >
>
> We need to reintroduce 'ipf_status', since this is where it's first used:
>


same comment as comment 1.



>
> > +struct ipf_status {
> > +   bool ifp_v4_enabled;
> > +   unsigned int min_v4_frag_size;
> > +   unsigned int nfrag_max;
> > +   unsigned int nfrag;
> > +   unsigned int n4frag_accepted;
> > +   unsigned int n4frag_completed_sent;
> > +   unsigned int n4frag_expired_sent;
> > +   unsigned int n4frag_too_small;
> > +   unsigned int n4frag_overlap;
> > +   bool ifp_v6_enabled;
> > +   unsigned int min_v6_frag_size;
> > +   unsigned int n6frag_accepted;
> > +   unsigned int n6frag_completed_sent;
> > +   unsigned int n6frag_expired_sent;
> > +   unsigned int n6frag_too_small;
> > +   unsigned int n6frag_overlap;
> > +};
>
> I think it's a little cleaner if we create another struct that
> consolidates the common elements between IPv4 and IPv6.  Something like the
> following:
>
> -=-=-=-=-=-=-=-
> struct proto_status {
>     bool enabled;
>     unsigned int min_frag_size;
>
>     /* Fragment statistics */
>     unsigned int accepted;
>     unsigned int completed_sent;
>     unsigned int expired_sent;
>     unsigned int too_small;
>     unsigned int overlap;
> };
>
> struct ipf_status {
>     unsigned int nfrag;
>     unsigned int nfrag_max;
>
>     struct proto_status v4;
>     struct proto_status v6;
>


Thats fine; it is a bit clearer and simple to integrate.



> };
> -=-=-=-=-=-=-=-
>
> I worry a bit about wrapping 32-bit counters.  I see that the current
> atomic_count is an unsigned int, but It might be worth creating a new
> 64-bit type so that we can handle things like "accepted", which may be
> fairly high.
>


This is really a bug and something I intended to revisit. All these
counters should be 64 bit.



>
> > diff --git a/tests/system-userspace-macros.at b/tests/
> system-userspace-macros.at
> > index 40b7567..fcae3cc 100644
> > --- a/tests/system-userspace-macros.at
> > +++ b/tests/system-userspace-macros.at
> > ...
> > +# FORMAT_FRAG_LIST([])
> > +#
> > +# Strip content from the piped input which can differ from test to
> test; recirc_id
> > +# and ip_id fields in an ipf_list vary from test to test and hence are
> cleared.
> > +m4_define([FORMAT_FRAG_LIST],
> > +    [[sed -e 's/ip_id=[0-9]*/ip_id=<cleared>/g' -e
> 's/recirc_id=[0-9]*/recirc_id=<cleared>/g']])
> > +
> > +# DPCTL_CHECK_FRAGMENTATION_FAIL()
> > +#
> > +# Used to check fragmentation counters for some fragmentation tests
> using
> > +# the userspace datapath, when failure to transmit fragments is
> expected.
> > +m4_define([DPCTL_CHECK_FRAGMENTATION_FAIL],
> > +[
> > +AT_CHECK([ovs-appctl dpctl/ipf-get-status -m | FORMAT_FRAG_LIST()], [],
> [dnl
> > +        Fragmentation Module Status
> > +        ---------------------------
> > +        v4 enabled: 1
> > +        v6 enabled: 1
> > +        max num frags (v4/v6): 500
> > +        num frag: 7
> > +        min v4 frag size: 1000
> > +        v4 frags accepted: 7
> > +        v4 frags completed: 0
> > +        v4 frags expired: 0
> > +        v4 frags too small: 0
> > +        v4 frags overlapped: 0
> > +        min v6 frag size: 1280
> > +        v6 frags accepted: 0
> > +        v6 frags completed: 0
> > +        v6 frags expired: 0
> > +        v6 frags too small: 0
> > +        v6 frags overlapped: 0
> > +
> > +        Fragment Lists:
> > +
> > +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<clear
> ed>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> > +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<clear
> ed>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> > +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<clear
> ed>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> > +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<clear
> ed>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> > +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<clear
> ed>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> > +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<clear
> ed>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> > +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<clear
> ed>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
>
> If it's not too difficult, it might be nice to add some IPv6 stats here,
> too.
>


It is easy to add; I was just lazy.



>
> I've appended an incremental with some of the suggestions I made above as
> well as a few minor documentation/comment changes.
>


I looked at those carefully.
I completed one sentence based on what I think you wanted to say.




>
> --Justin
>
>
> -=-=-=-=-=-=-=-=-
>
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index a59bc1e930d4..f5387c6a0678 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -20,6 +20,7 @@
>  #include <errno.h>
>
>  #include "ct-dpif.h"
> +#include "ipf.h"
>  #include "openvswitch/ofp-parse.h"
>  #include "openvswitch/vlog.h"
>
> @@ -188,24 +189,10 @@ ct_dpif_ipf_set_max_nfrags(struct dpif *dpif,
> uint32_t max
> _frags)
>              : EOPNOTSUPP);
>  }
>
> -int ct_dpif_ipf_get_status(struct dpif *dpif, bool *ipf_v4_enabled,
> -    unsigned int *min_v4_frag_size, unsigned int *nfrag_max,
> -    unsigned int *nfrag, unsigned int *n4frag_accepted,
> -    unsigned int *n4frag_completed_sent,
> -    unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
> -    unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> -    unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> -    unsigned int *n6frag_completed_sent,
> -    unsigned int *n6frag_expired_sent, unsigned int *n6frag_too_small,
> -    unsigned int *n6frag_overlap)
> +int ct_dpif_ipf_get_status(struct dpif *dpif, struct ipf_status *status)
>  {
>      return (dpif->dpif_class->ipf_get_status
> -            ? dpif->dpif_class->ipf_get_status(dpif, ipf_v4_enabled,
> -            min_v4_frag_size, nfrag_max, nfrag, n4frag_accepted,
> -            n4frag_completed_sent, n4frag_expired_sent, n4frag_too_small,
> -            n4frag_overlap, ipf_v6_enabled, min_v6_frag_size,
> n6frag_accepted,
> -            n6frag_completed_sent, n6frag_expired_sent, n6frag_too_small,
> -            n6frag_overlap)
> +            ? dpif->dpif_class->ipf_get_status(dpif, status)
>              : EOPNOTSUPP);
>  }
>
> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index 57d375bc1e82..603dbcf2d14a 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2015 Nicira, Inc.
> + * Copyright (c) 2015, 2018 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -20,6 +20,9 @@
>  #include "openvswitch/types.h"
>  #include "packets.h"
>
> +struct ipf_dump_ctx;
> +struct ipf_status;
> +
>  union ct_dpif_inet_addr {
>      ovs_be32 ip;
>      ovs_be32 ip6[4];
> @@ -203,12 +206,7 @@ int ct_dpif_get_nconns(struct dpif *dpif, uint32_t
> *nconns)
> ;
>  int ct_dpif_ipf_set_enabled(struct dpif *, bool v6, bool enable);
>  int ct_dpif_ipf_set_min_frag(struct dpif *, bool, uint32_t);
>  int ct_dpif_ipf_set_max_nfrags(struct dpif *, uint32_t);
> -int ct_dpif_ipf_get_status(struct dpif *dpif, bool *, unsigned int *,
> -                           unsigned int *, unsigned int *, unsigned int *,
> -                           unsigned int *, unsigned int *, unsigned int *,
> -                           unsigned int *, bool *, unsigned int *,
> -                           unsigned int *, unsigned int *, unsigned int *,
> -                           unsigned int *, unsigned int *);
> +int ct_dpif_ipf_get_status(struct dpif *dpif, struct ipf_status *);
>  int ct_dpif_ipf_dump_start(struct dpif *dpif, struct ipf_dump_ctx **);
>  int ct_dpif_ipf_dump_next(struct dpif *dpif, void *, char **);
>  int ct_dpif_ipf_dump_done(struct dpif *dpif, void *);
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index f650b37dd901..832220e5102c 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1799,61 +1799,41 @@ dpctl_ct_ipf_get_status(int argc, const char
> *argv[],
>      struct dpif *dpif;
>      int error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif);
>      if (!error) {
> -        bool ipf_v4_enabled;
> -        unsigned int min_v4_frag_size;
> -        unsigned int nfrag_max;
> -        unsigned int nfrag;
> -        unsigned int n4frag_accepted;
> -        unsigned int n4frag_completed_sent;
> -        unsigned int n4frag_expired_sent;
> -        unsigned int n4frag_too_small;
> -        unsigned int n4frag_overlap;
> -        unsigned int min_v6_frag_size;
> -        bool ipf_v6_enabled;
> -        unsigned int n6frag_accepted;
> -        unsigned int n6frag_completed_sent;
> -        unsigned int n6frag_expired_sent;
> -        unsigned int n6frag_too_small;
> -        unsigned int n6frag_overlap;
> -        error = ct_dpif_ipf_get_status(dpif, &ipf_v4_enabled,
> -            &min_v4_frag_size, &nfrag_max, &nfrag, &n4frag_accepted,
> -            &n4frag_completed_sent, &n4frag_expired_sent,
> &n4frag_too_small,
> -            &n4frag_overlap, &ipf_v6_enabled, &min_v6_frag_size,
> -            &n6frag_accepted, &n6frag_completed_sent,
> &n6frag_expired_sent,
> -            &n6frag_too_small, &n6frag_overlap);
> +        struct ipf_status status;
> +        error = ct_dpif_ipf_get_status(dpif, &status);
>
>          if (!error) {
>              dpctl_print(dpctl_p, "        Fragmentation Module Status\n");
>              dpctl_print(dpctl_p, "        ---------------------------\n"
> );
> -            dpctl_print(dpctl_p, "        v4 enabled: %u\n",
> ipf_v4_enabled);
> -            dpctl_print(dpctl_p, "        v6 enabled: %u\n",
> ipf_v6_enabled);
> +            dpctl_print(dpctl_p, "        v4 enabled: %u\n",
> status.v4.enabled);
> +            dpctl_print(dpctl_p, "        v6 enabled: %u\n",
> status.v6.enabled);
>              dpctl_print(dpctl_p, "        max num frags (v4/v6): %u\n",
> -                        nfrag_max);
> -            dpctl_print(dpctl_p, "        num frag: %u\n", nfrag);
> +                        status.nfrag_max);
> +            dpctl_print(dpctl_p, "        num frag: %u\n", status.nfrag);
>              dpctl_print(dpctl_p, "        min v4 frag size: %u\n",
> -                        min_v4_frag_size);
> +                        status.v4.min_frag_size);
>              dpctl_print(dpctl_p, "        v4 frags accepted: %u\n",
> -                        n4frag_accepted);
> +                        status.v4.accepted);
>              dpctl_print(dpctl_p, "        v4 frags completed: %u\n",
> -                        n4frag_completed_sent);
> +                        status.v4.completed_sent);
>              dpctl_print(dpctl_p, "        v4 frags expired: %u\n",
> -                        n4frag_expired_sent);
> +                        status.v4.expired_sent);
>              dpctl_print(dpctl_p, "        v4 frags too small: %u\n",
> -                        n4frag_too_small);
> +                        status.v4.too_small);
>              dpctl_print(dpctl_p, "        v4 frags overlapped: %u\n",
> -                        n4frag_overlap);
> +                        status.v4.overlap);
>              dpctl_print(dpctl_p, "        min v6 frag size: %u\n",
> -                        min_v6_frag_size);
> +                        status.v6.min_frag_size);
>              dpctl_print(dpctl_p, "        v6 frags accepted: %u\n",
> -                        n6frag_accepted);
> +                        status.v6.accepted);
>              dpctl_print(dpctl_p, "        v6 frags completed: %u\n",
> -                        n6frag_completed_sent);
> +                        status.v6.completed_sent);
>              dpctl_print(dpctl_p, "        v6 frags expired: %u\n",
> -                        n6frag_expired_sent);
> +                        status.v6.expired_sent);
>              dpctl_print(dpctl_p, "        v6 frags too small: %u\n",
> -                        n6frag_too_small);
> +                        status.v6.too_small);
>              dpctl_print(dpctl_p, "        v6 frags overlapped: %u\n",
> -                        n6frag_overlap);
> +                        status.v6.overlap);
>          } else {
>              dpctl_error(dpctl_p, error,
>                          "ipf status could not be retrieved");
> diff --git a/lib/dpctl.man b/lib/dpctl.man
> index fc53094d2dc4..db2b7f8bcc4b 100644
> --- a/lib/dpctl.man
> +++ b/lib/dpctl.man
> @@ -304,5 +304,6 @@ fragmentation is enabled.  Only supported for
> userspace datapath.
>  .TP
>  .DO "[\fB\-m\fR | \fB\-\-more\fR]" "\*(DX\fBipf\-get\-status\fR
> [\fIdp\fR]"
>  Gets the configuration settings and fragment counters associated with the
> -fragmentation handling of the userspace datapath connection tracker.  With
> -\fB\-m\fR or \fB\-\-more\fR, also dumps the ipf lists.
> +fragmentation handling of the userspace datapath connection tracker.
> +With \fB\-m\fR or \fB\-\-more\fR, also dumps the IP fragment lists.
> +Only supported for userspace datapath.
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 73ab632b969e..d2cd0cbfe2ec 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -6801,33 +6801,9 @@ dpif_netdev_ipf_set_max_nfrags(struct dpif *dpif
> OVS_UNUSED,
>
>  static int
>  dpif_netdev_ipf_get_status(struct dpif *dpif OVS_UNUSED,
> -    bool *ipf_v4_enabled, unsigned int *min_v4_frag_size,
> -    unsigned int *nfrag_max, unsigned int *nfrag,
> -    unsigned int *n4frag_accepted, unsigned int *n4frag_completed_sent,
> -    unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
> -    unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> -    unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> -    unsigned int *n6frag_completed_sent, unsigned int
> *n6frag_expired_sent,
> -    unsigned int *n6frag_too_small, unsigned int *n6frag_overlap)
> -{
> -    struct ipf_status ipf_status;
> -    ipf_get_status(&ipf_status);
> -    *ipf_v4_enabled = ipf_status.ifp_v4_enabled;
> -    *min_v4_frag_size = ipf_status.min_v4_frag_size;
> -    *nfrag_max = ipf_status.nfrag_max;
> -    *nfrag = ipf_status.nfrag;
> -    *n4frag_accepted = ipf_status.n4frag_accepted;
> -    *n4frag_completed_sent = ipf_status.n4frag_completed_sent;
> -    *n4frag_expired_sent = ipf_status.n4frag_expired_sent;
> -    *n4frag_too_small = ipf_status.n4frag_too_small;
> -    *n4frag_overlap = ipf_status.n4frag_overlap;
> -    *ipf_v6_enabled = ipf_status.ifp_v6_enabled;
> -    *min_v6_frag_size = ipf_status.min_v6_frag_size;
> -    *n6frag_accepted = ipf_status.n6frag_accepted;
> -    *n6frag_completed_sent = ipf_status.n6frag_completed_sent;
> -    *n6frag_expired_sent = ipf_status.n6frag_expired_sent;
> -    *n6frag_too_small = ipf_status.n6frag_too_small;
> -    *n6frag_overlap = ipf_status.n6frag_overlap;
> +                           struct ipf_status *status)
> +{
> +    ipf_get_status(status);
>      return 0;
>  }
>
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 2e0d5968cea6..457911b7277a 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -452,19 +452,11 @@ struct dpif_class {
>      /* Set maximum number of fragments tracked. */
>      int (*ipf_set_max_nfrags)(struct dpif *, uint32_t max_nfrags);
>      /* Get fragmentation configuration status and counters. */
> -    int (*ipf_get_status)(struct dpif *, bool *ipf_v4_enabled,
> -        unsigned int *min_v4_frag_size,
> -        unsigned int *nfrag_max, unsigned int *nfrag,
> -        unsigned int *n4frag_accepted, unsigned int
> *n4frag_completed_sent,
> -        unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
> -        unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> -        unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> -        unsigned int *n6frag_completed_sent,
> -        unsigned int *n6frag_expired_sent, unsigned int *n6frag_too_small,
> -        unsigned int *n6frag_overlap);
> -    int (*ipf_dump_start)(struct dpif *, struct ipf_dump_ctx
> **ipf_dump_ctx);
> +    int (*ipf_get_status)(struct dpif *, struct ipf_status *);
> +
>      /* Finds the next ipf list and creates a string representation of the
>       * state of an ipf list, to which 'dump' is pointed to. */
> +    int (*ipf_dump_start)(struct dpif *, struct ipf_dump_ctx
> **ipf_dump_ctx);
>      int (*ipf_dump_next)(struct dpif *, void *, char **dump);
>      int (*ipf_dump_done)(struct dpif *, void *ipf_dump_ctx);
>
> diff --git a/lib/ipf.c b/lib/ipf.c
> index ff08f5933e96..ff4de524abf2 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -1347,26 +1347,22 @@ ipf_set_max_nfrags(uint32_t value)
>  int
>  ipf_get_status(struct ipf_status *ipf_status)
>  {
> -    atomic_read_relaxed(&ifp_v4_enabled, &ipf_status->ifp_v4_enabled);
> -    atomic_read_relaxed(&min_v4_frag_size, &ipf_status->min_v4_frag_size)
> ;
> +    atomic_read_relaxed(&ifp_v4_enabled, &ipf_status->v4.enabled);
> +    atomic_read_relaxed(&min_v4_frag_size, &ipf_status->v4.min_frag_size)
> ;
>      atomic_read_relaxed(&nfrag_max, &ipf_status->nfrag_max);
>      ipf_status->nfrag = atomic_count_get(&nfrag);
> -    ipf_status->n4frag_accepted = atomic_count_get(&n4frag_accepted);
> -    ipf_status->n4frag_completed_sent =
> -        atomic_count_get(&n4frag_completed_sent);
> -    ipf_status->n4frag_expired_sent =
> -        atomic_count_get(&n4frag_expired_sent);
> -    ipf_status->n4frag_too_small = atomic_count_get(&n4frag_too_small);
> -    ipf_status->n4frag_overlap = atomic_count_get(&n4frag_overlap);
> -    atomic_read_relaxed(&ifp_v6_enabled, &ipf_status->ifp_v6_enabled);
> -    atomic_read_relaxed(&min_v6_frag_size, &ipf_status->min_v6_frag_size)
> ;
> -    ipf_status->n6frag_accepted = atomic_count_get(&n6frag_accepted);
> -    ipf_status->n6frag_completed_sent =
> -        atomic_count_get(&n6frag_completed_sent);
> -    ipf_status->n6frag_expired_sent =
> -        atomic_count_get(&n6frag_expired_sent);
> -    ipf_status->n6frag_too_small = atomic_count_get(&n6frag_too_small);
> -    ipf_status->n6frag_overlap = atomic_count_get(&n6frag_overlap);
> +    ipf_status->v4.accepted = atomic_count_get(&n4frag_accepted);
> +    ipf_status->v4.completed_sent = atomic_count_get(&n4frag_compl
> eted_sent);
> +    ipf_status->v4.expired_sent = atomic_count_get(&n4frag_expired_sent);
> +    ipf_status->v4.too_small = atomic_count_get(&n4frag_too_small);
> +    ipf_status->v4.overlap = atomic_count_get(&n4frag_overlap);
> +    atomic_read_relaxed(&ifp_v6_enabled, &ipf_status->v6.enabled);
> +    atomic_read_relaxed(&min_v6_frag_size, &ipf_status->v6.min_frag_size)
> ;
> +    ipf_status->v6.accepted = atomic_count_get(&n6frag_accepted);
> +    ipf_status->v6.completed_sent = atomic_count_get(&n6frag_compl
> eted_sent);
> +    ipf_status->v6.expired_sent = atomic_count_get(&n6frag_expired_sent);
> +    ipf_status->v6.too_small = atomic_count_get(&n6frag_too_small);
> +    ipf_status->v6.overlap = atomic_count_get(&n6frag_overlap);
>      return 0;
>  }
>
> @@ -1374,7 +1370,8 @@ struct ipf_dump_ctx {
>      struct hmap_position bucket_pos;
>  };
>
> -/* Allocates an 'ipf_dump_ctx' to keep track of an hmap position. */
> +/* Allocates an 'ipf_dump_ctx' to keep track of an hmap position. The
> + * caller must call ipf_dump_done() when dumping is finished. */
>  int
>  ipf_dump_start(struct ipf_dump_ctx **ipf_dump_ctx)
>  {
> @@ -1415,7 +1412,7 @@ ipf_dump_create(const struct ipf_list *ipf_list,
> struct ds *ds)
>
>  /* Finds the next ipf list starting from 'ipf_dump_ctx->bucket_pos' and
> uses
>   * ipf_dump_create() to create a string representation of the state of an
> - * ipf list, to which 'dump' is pointed to. */
> + * ipf list, to which 'dump' is pointed to.  Return EOF when */
>


I think you meant to say:
"Returns EOF when there are no more ipf_lists."



>  int
>  ipf_dump_next(struct ipf_dump_ctx *ipf_dump_ctx, char **dump)
>  {
> @@ -1439,7 +1436,7 @@ ipf_dump_next(struct ipf_dump_ctx *ipf_dump_ctx,
> char **dump)
>      }
>  }
>
> -/* Frees an ipf_dump_ctx allocated by ipf_dump_start. */
> +/* Frees 'ipf_dump_ctx' allocated by ipf_dump_start(). */
>  int
>  ipf_dump_done(struct ipf_dump_ctx *ipf_dump_ctx)
>  {
> diff --git a/lib/ipf.h b/lib/ipf.h
> index 635988b2fcd8..8fa58ab75851 100644
> --- a/lib/ipf.h
> +++ b/lib/ipf.h
> @@ -20,6 +20,26 @@
>  #include "dp-packet.h"
>  #include "openvswitch/types.h"
>
> +struct proto_status {
> +    bool enabled;
> +    unsigned int min_frag_size;
> +
> +    /* Fragment statistics */
> +    unsigned int accepted;
> +    unsigned int completed_sent;
> +    unsigned int expired_sent;
> +    unsigned int too_small;
> +    unsigned int overlap;
> +};
> +
> +struct ipf_status {
> +    unsigned int nfrag;
> +    unsigned int nfrag_max;
> +
> +    struct proto_status v4;
> +    struct proto_status v6;
> +};
> +
>  void ipf_preprocess_conntrack(struct dp_packet_batch *pb, long long now,
>                                ovs_be16 dl_type, uint16_t zone,
>                                uint32_t hash_basis);
>
>
>
>

Darrell Ball Aug. 16, 2018, 6:44 a.m. UTC | #3

Thanks Justin

By the way, I just noticed that you reviewed most patches on Aug 1.
Since the company e-mail is filtering some OVS list e-mails and I have
relied on
it just for notifications and was otherwise busy, I missed those. I am going
through those carefully now.

One outstanding comment inline that I wanted to double check.


On Fri, Aug 10, 2018 at 8:00 PM, Justin Pettit <jpettit@ovn.org> wrote:

>
> > On Jul 16, 2018, at 4:39 PM, Darrell Ball <dlu998@gmail.com> wrote:
> >
> > void
> > ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
> > {
> > diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> > index f886ab9..2ff7e26 100644
> > --- a/lib/ct-dpif.h
> > +++ b/lib/ct-dpif.h
> > @@ -204,6 +204,15 @@ int ct_dpif_get_nconns(struct dpif *dpif, uint32_t
> *nconns);
> > int ct_dpif_ipf_set_enabled(struct dpif *, bool v6, bool enable);
> > int ct_dpif_ipf_set_min_frag(struct dpif *, bool, uint32_t);
> > int ct_dpif_ipf_set_max_nfrags(struct dpif *, uint32_t);
> > +int ct_dpif_ipf_get_status(struct dpif *dpif, bool *, unsigned int *,
> > +                           unsigned int *, unsigned int *, unsigned int
> *,
> > +                           unsigned int *, unsigned int *, unsigned int
> *,
> > +                           unsigned int *, bool *, unsigned int *,
> > +                           unsigned int *, unsigned int *, unsigned int
> *,
> > +                           unsigned int *, unsigned int *);
> > +int ct_dpif_ipf_dump_start(struct dpif *dpif, struct ipf_dump_ctx **);
>
> On my system, I needed to declare "ipf_dump_ctx" for the build to finish.
>


I re-ran Travis

Bssically the same code as V8

Here is the link:

Travis:
https://travis-ci.org/darball/ovs_master

The corresponding public git repo branch:
https://github.com/darball/ovs_master/commits/conntrack

Possibility, the build issue you saw is related to your proposed change in
removing #include "ipf.h"
from patch 7.

diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
index 6eb55b4b0197..f8a31921e16a 100644
--- a/lib/ct-dpif.h
+++ b/lib/ct-dpif.h
@@ -17,7 +17,6 @@
 #ifndef CT_DPIF_H
 #define CT_DPIF_H

-#include "ipf.h"
 #include "openvswitch/types.h"
 #include "packets.h"

ipf.h has the declaration for ipf_dump_ctx.





>
> > diff --git a/lib/dpctl.c b/lib/dpctl.c
> > index 5ec36bd..b3e7ce7 100644
> > --- a/lib/dpctl.c
> > +++ b/lib/dpctl.c
> > ..
> > +static int
> > +dpctl_ct_ipf_get_status(int argc, const char *argv[],
> > +                        struct dpctl_params *dpctl_p)
> > +{
> > +    struct dpif *dpif;
> > +    int error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif);
> > +    if (!error) {
> > +        bool ipf_v4_enabled;
> > +        unsigned int min_v4_frag_size;
> > +        unsigned int nfrag_max;
> > +        unsigned int nfrag;
> > +        unsigned int n4frag_accepted;
> > +        unsigned int n4frag_completed_sent;
> > +        unsigned int n4frag_expired_sent;
> > +        unsigned int n4frag_too_small;
> > +        unsigned int n4frag_overlap;
> > +        unsigned int min_v6_frag_size;
> > +        bool ipf_v6_enabled;
> > +        unsigned int n6frag_accepted;
> > +        unsigned int n6frag_completed_sent;
> > +        unsigned int n6frag_expired_sent;
> > +        unsigned int n6frag_too_small;
> > +        unsigned int n6frag_overlap;
> > +        error = ct_dpif_ipf_get_status(dpif, &ipf_v4_enabled,
> > +            &min_v4_frag_size, &nfrag_max, &nfrag, &n4frag_accepted,
> > +            &n4frag_completed_sent, &n4frag_expired_sent,
> &n4frag_too_small,
> > +            &n4frag_overlap, &ipf_v6_enabled, &min_v6_frag_size,
> > +            &n6frag_accepted, &n6frag_completed_sent,
> &n6frag_expired_sent,
> > +            &n6frag_too_small, &n6frag_overlap);
> > +
>
> If we just use 'ipf_status', we can delete a lot of these variable
> declarations.
>
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 76bc1d9..db551ea 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -6553,6 +6553,60 @@ dpif_netdev_ipf_set_max_nfrags(struct dpif *dpif
> OVS_UNUSED,
> >     return ipf_set_max_nfrags(max_frags);
> > }
> >
> > +static int
> > +dpif_netdev_ipf_get_status(struct dpif *dpif OVS_UNUSED,
> > +    bool *ipf_v4_enabled, unsigned int *min_v4_frag_size,
> > +    unsigned int *nfrag_max, unsigned int *nfrag,
> > +    unsigned int *n4frag_accepted, unsigned int *n4frag_completed_sent,
> > +    unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
> > +    unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> > +    unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> > +    unsigned int *n6frag_completed_sent, unsigned int
> *n6frag_expired_sent,
> > +    unsigned int *n6frag_too_small, unsigned int *n6frag_overlap)
> > +{
> > +    struct ipf_status ipf_status;
> > +    ipf_get_status(&ipf_status);
> > +    *ipf_v4_enabled = ipf_status.ifp_v4_enabled;
> > +    *min_v4_frag_size = ipf_status.min_v4_frag_size;
> > +    *nfrag_max = ipf_status.nfrag_max;
> > +    *nfrag = ipf_status.nfrag;
> > +    *n4frag_accepted = ipf_status.n4frag_accepted;
> > +    *n4frag_completed_sent = ipf_status.n4frag_completed_sent;
> > +    *n4frag_expired_sent = ipf_status.n4frag_expired_sent;
> > +    *n4frag_too_small = ipf_status.n4frag_too_small;
> > +    *n4frag_overlap = ipf_status.n4frag_overlap;
> > +    *ipf_v6_enabled = ipf_status.ifp_v6_enabled;
> > +    *min_v6_frag_size = ipf_status.min_v6_frag_size;
> > +    *n6frag_accepted = ipf_status.n6frag_accepted;
> > +    *n6frag_completed_sent = ipf_status.n6frag_completed_sent;
> > +    *n6frag_expired_sent = ipf_status.n6frag_expired_sent;
> > +    *n6frag_too_small = ipf_status.n6frag_too_small;
> > +    *n6frag_overlap = ipf_status.n6frag_overlap;
> > +    return 0;
> > +}
>
> As, I'll suggest later, this can be reduced to just a couple of lines if
> 'ipf_status' is passed into this function.
>
> > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> > index 214f570..2e0d596 100644
> > --- a/lib/dpif-provider.h
> > +++ b/lib/dpif-provider.h
> > @@ -451,6 +451,22 @@ struct dpif_class {
> >     int (*ipf_set_min_frag)(struct dpif *, bool v6, uint32_t min_frag);
> >     /* Set maximum number of fragments tracked. */
> >     int (*ipf_set_max_nfrags)(struct dpif *, uint32_t max_nfrags);
> > +    /* Get fragmentation configuration status and counters. */
> > +    int (*ipf_get_status)(struct dpif *, bool *ipf_v4_enabled,
> > +        unsigned int *min_v4_frag_size,
> > +        unsigned int *nfrag_max, unsigned int *nfrag,
> > +        unsigned int *n4frag_accepted, unsigned int
> *n4frag_completed_sent,
> > +        unsigned int *n4frag_expired_sent, unsigned int
> *n4frag_too_small,
> > +        unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> > +        unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> > +        unsigned int *n6frag_completed_sent,
> > +        unsigned int *n6frag_expired_sent, unsigned int
> *n6frag_too_small,
> > +        unsigned int *n6frag_overlap);
>
> I'd suggest passing in 'ipf_status', since it makes this prototype much
> smaller and has all the information you need.
>
> > +    int (*ipf_dump_start)(struct dpif *, struct ipf_dump_ctx
> **ipf_dump_ctx);
> > +    /* Finds the next ipf list and creates a string representation of
> the
> > +     * state of an ipf list, to which 'dump' is pointed to. */
>
> This comment should probably go above the declaration of ipf_dump_start().
>
> > +    int (*ipf_dump_next)(struct dpif *, void *, char **dump);
> > +    int (*ipf_dump_done)(struct dpif *, void *ipf_dump_ctx);
>
> > diff --git a/lib/ipf.h b/lib/ipf.h
> > index 4289e5e..36a182a 100644
> > --- a/lib/ipf.h
> > +++ b/lib/ipf.h
> > @@ -63,4 +63,14 @@ int ipf_set_min_frag(bool v6, uint32_t value);
> >
> > int ipf_set_max_nfrags(uint32_t value);
> >
>
> We need to reintroduce 'ipf_status', since this is where it's first used:
>
> > +struct ipf_status {
> > +   bool ifp_v4_enabled;
> > +   unsigned int min_v4_frag_size;
> > +   unsigned int nfrag_max;
> > +   unsigned int nfrag;
> > +   unsigned int n4frag_accepted;
> > +   unsigned int n4frag_completed_sent;
> > +   unsigned int n4frag_expired_sent;
> > +   unsigned int n4frag_too_small;
> > +   unsigned int n4frag_overlap;
> > +   bool ifp_v6_enabled;
> > +   unsigned int min_v6_frag_size;
> > +   unsigned int n6frag_accepted;
> > +   unsigned int n6frag_completed_sent;
> > +   unsigned int n6frag_expired_sent;
> > +   unsigned int n6frag_too_small;
> > +   unsigned int n6frag_overlap;
> > +};
>
> I think it's a little cleaner if we create another struct that
> consolidates the common elements between IPv4 and IPv6.  Something like the
> following:
>
> -=-=-=-=-=-=-=-
> struct proto_status {
>     bool enabled;
>     unsigned int min_frag_size;
>
>     /* Fragment statistics */
>     unsigned int accepted;
>     unsigned int completed_sent;
>     unsigned int expired_sent;
>     unsigned int too_small;
>     unsigned int overlap;
> };
>
> struct ipf_status {
>     unsigned int nfrag;
>     unsigned int nfrag_max;
>
>     struct proto_status v4;
>     struct proto_status v6;
> };
> -=-=-=-=-=-=-=-
>
> I worry a bit about wrapping 32-bit counters.  I see that the current
> atomic_count is an unsigned int, but It might be worth creating a new
> 64-bit type so that we can handle things like "accepted", which may be
> fairly high.
>
> > diff --git a/tests/system-userspace-macros.at b/tests/
> system-userspace-macros.at
> > index 40b7567..fcae3cc 100644
> > --- a/tests/system-userspace-macros.at
> > +++ b/tests/system-userspace-macros.at
> > ...
> > +# FORMAT_FRAG_LIST([])
> > +#
> > +# Strip content from the piped input which can differ from test to
> test; recirc_id
> > +# and ip_id fields in an ipf_list vary from test to test and hence are
> cleared.
> > +m4_define([FORMAT_FRAG_LIST],
> > +    [[sed -e 's/ip_id=[0-9]*/ip_id=<cleared>/g' -e
> 's/recirc_id=[0-9]*/recirc_id=<cleared>/g']])
> > +
> > +# DPCTL_CHECK_FRAGMENTATION_FAIL()
> > +#
> > +# Used to check fragmentation counters for some fragmentation tests
> using
> > +# the userspace datapath, when failure to transmit fragments is
> expected.
> > +m4_define([DPCTL_CHECK_FRAGMENTATION_FAIL],
> > +[
> > +AT_CHECK([ovs-appctl dpctl/ipf-get-status -m | FORMAT_FRAG_LIST()], [],
> [dnl
> > +        Fragmentation Module Status
> > +        ---------------------------
> > +        v4 enabled: 1
> > +        v6 enabled: 1
> > +        max num frags (v4/v6): 500
> > +        num frag: 7
> > +        min v4 frag size: 1000
> > +        v4 frags accepted: 7
> > +        v4 frags completed: 0
> > +        v4 frags expired: 0
> > +        v4 frags too small: 0
> > +        v4 frags overlapped: 0
> > +        min v6 frag size: 1280
> > +        v6 frags accepted: 0
> > +        v6 frags completed: 0
> > +        v6 frags expired: 0
> > +        v6 frags too small: 0
> > +        v6 frags overlapped: 0
> > +
> > +        Fragment Lists:
> > +
> > +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<clear
> ed>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> > +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<clear
> ed>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> > +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<clear
> ed>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> > +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<clear
> ed>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> > +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<clear
> ed>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> > +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<clear
> ed>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
> > +(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<clear
> ed>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
>
> If it's not too difficult, it might be nice to add some IPv6 stats here,
> too.
>
> I've appended an incremental with some of the suggestions I made above as
> well as a few minor documentation/comment changes.
>
> --Justin
>
>
> -=-=-=-=-=-=-=-=-
>
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index a59bc1e930d4..f5387c6a0678 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -20,6 +20,7 @@
>  #include <errno.h>
>
>  #include "ct-dpif.h"
> +#include "ipf.h"
>  #include "openvswitch/ofp-parse.h"
>  #include "openvswitch/vlog.h"
>
> @@ -188,24 +189,10 @@ ct_dpif_ipf_set_max_nfrags(struct dpif *dpif,
> uint32_t max
> _frags)
>              : EOPNOTSUPP);
>  }
>
> -int ct_dpif_ipf_get_status(struct dpif *dpif, bool *ipf_v4_enabled,
> -    unsigned int *min_v4_frag_size, unsigned int *nfrag_max,
> -    unsigned int *nfrag, unsigned int *n4frag_accepted,
> -    unsigned int *n4frag_completed_sent,
> -    unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
> -    unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> -    unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> -    unsigned int *n6frag_completed_sent,
> -    unsigned int *n6frag_expired_sent, unsigned int *n6frag_too_small,
> -    unsigned int *n6frag_overlap)
> +int ct_dpif_ipf_get_status(struct dpif *dpif, struct ipf_status *status)
>  {
>      return (dpif->dpif_class->ipf_get_status
> -            ? dpif->dpif_class->ipf_get_status(dpif, ipf_v4_enabled,
> -            min_v4_frag_size, nfrag_max, nfrag, n4frag_accepted,
> -            n4frag_completed_sent, n4frag_expired_sent, n4frag_too_small,
> -            n4frag_overlap, ipf_v6_enabled, min_v6_frag_size,
> n6frag_accepted,
> -            n6frag_completed_sent, n6frag_expired_sent, n6frag_too_small,
> -            n6frag_overlap)
> +            ? dpif->dpif_class->ipf_get_status(dpif, status)
>              : EOPNOTSUPP);
>  }
>
> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index 57d375bc1e82..603dbcf2d14a 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2015 Nicira, Inc.
> + * Copyright (c) 2015, 2018 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -20,6 +20,9 @@
>  #include "openvswitch/types.h"
>  #include "packets.h"
>
> +struct ipf_dump_ctx;
> +struct ipf_status;
> +
>  union ct_dpif_inet_addr {
>      ovs_be32 ip;
>      ovs_be32 ip6[4];
> @@ -203,12 +206,7 @@ int ct_dpif_get_nconns(struct dpif *dpif, uint32_t
> *nconns)
> ;
>  int ct_dpif_ipf_set_enabled(struct dpif *, bool v6, bool enable);
>  int ct_dpif_ipf_set_min_frag(struct dpif *, bool, uint32_t);
>  int ct_dpif_ipf_set_max_nfrags(struct dpif *, uint32_t);
> -int ct_dpif_ipf_get_status(struct dpif *dpif, bool *, unsigned int *,
> -                           unsigned int *, unsigned int *, unsigned int *,
> -                           unsigned int *, unsigned int *, unsigned int *,
> -                           unsigned int *, bool *, unsigned int *,
> -                           unsigned int *, unsigned int *, unsigned int *,
> -                           unsigned int *, unsigned int *);
> +int ct_dpif_ipf_get_status(struct dpif *dpif, struct ipf_status *);
>  int ct_dpif_ipf_dump_start(struct dpif *dpif, struct ipf_dump_ctx **);
>  int ct_dpif_ipf_dump_next(struct dpif *dpif, void *, char **);
>  int ct_dpif_ipf_dump_done(struct dpif *dpif, void *);
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index f650b37dd901..832220e5102c 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1799,61 +1799,41 @@ dpctl_ct_ipf_get_status(int argc, const char
> *argv[],
>      struct dpif *dpif;
>      int error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif);
>      if (!error) {
> -        bool ipf_v4_enabled;
> -        unsigned int min_v4_frag_size;
> -        unsigned int nfrag_max;
> -        unsigned int nfrag;
> -        unsigned int n4frag_accepted;
> -        unsigned int n4frag_completed_sent;
> -        unsigned int n4frag_expired_sent;
> -        unsigned int n4frag_too_small;
> -        unsigned int n4frag_overlap;
> -        unsigned int min_v6_frag_size;
> -        bool ipf_v6_enabled;
> -        unsigned int n6frag_accepted;
> -        unsigned int n6frag_completed_sent;
> -        unsigned int n6frag_expired_sent;
> -        unsigned int n6frag_too_small;
> -        unsigned int n6frag_overlap;
> -        error = ct_dpif_ipf_get_status(dpif, &ipf_v4_enabled,
> -            &min_v4_frag_size, &nfrag_max, &nfrag, &n4frag_accepted,
> -            &n4frag_completed_sent, &n4frag_expired_sent,
> &n4frag_too_small,
> -            &n4frag_overlap, &ipf_v6_enabled, &min_v6_frag_size,
> -            &n6frag_accepted, &n6frag_completed_sent,
> &n6frag_expired_sent,
> -            &n6frag_too_small, &n6frag_overlap);
> +        struct ipf_status status;
> +        error = ct_dpif_ipf_get_status(dpif, &status);
>
>          if (!error) {
>              dpctl_print(dpctl_p, "        Fragmentation Module Status\n");
>              dpctl_print(dpctl_p, "        ---------------------------\n"
> );
> -            dpctl_print(dpctl_p, "        v4 enabled: %u\n",
> ipf_v4_enabled);
> -            dpctl_print(dpctl_p, "        v6 enabled: %u\n",
> ipf_v6_enabled);
> +            dpctl_print(dpctl_p, "        v4 enabled: %u\n",
> status.v4.enabled);
> +            dpctl_print(dpctl_p, "        v6 enabled: %u\n",
> status.v6.enabled);
>              dpctl_print(dpctl_p, "        max num frags (v4/v6): %u\n",
> -                        nfrag_max);
> -            dpctl_print(dpctl_p, "        num frag: %u\n", nfrag);
> +                        status.nfrag_max);
> +            dpctl_print(dpctl_p, "        num frag: %u\n", status.nfrag);
>              dpctl_print(dpctl_p, "        min v4 frag size: %u\n",
> -                        min_v4_frag_size);
> +                        status.v4.min_frag_size);
>              dpctl_print(dpctl_p, "        v4 frags accepted: %u\n",
> -                        n4frag_accepted);
> +                        status.v4.accepted);
>              dpctl_print(dpctl_p, "        v4 frags completed: %u\n",
> -                        n4frag_completed_sent);
> +                        status.v4.completed_sent);
>              dpctl_print(dpctl_p, "        v4 frags expired: %u\n",
> -                        n4frag_expired_sent);
> +                        status.v4.expired_sent);
>              dpctl_print(dpctl_p, "        v4 frags too small: %u\n",
> -                        n4frag_too_small);
> +                        status.v4.too_small);
>              dpctl_print(dpctl_p, "        v4 frags overlapped: %u\n",
> -                        n4frag_overlap);
> +                        status.v4.overlap);
>              dpctl_print(dpctl_p, "        min v6 frag size: %u\n",
> -                        min_v6_frag_size);
> +                        status.v6.min_frag_size);
>              dpctl_print(dpctl_p, "        v6 frags accepted: %u\n",
> -                        n6frag_accepted);
> +                        status.v6.accepted);
>              dpctl_print(dpctl_p, "        v6 frags completed: %u\n",
> -                        n6frag_completed_sent);
> +                        status.v6.completed_sent);
>              dpctl_print(dpctl_p, "        v6 frags expired: %u\n",
> -                        n6frag_expired_sent);
> +                        status.v6.expired_sent);
>              dpctl_print(dpctl_p, "        v6 frags too small: %u\n",
> -                        n6frag_too_small);
> +                        status.v6.too_small);
>              dpctl_print(dpctl_p, "        v6 frags overlapped: %u\n",
> -                        n6frag_overlap);
> +                        status.v6.overlap);
>          } else {
>              dpctl_error(dpctl_p, error,
>                          "ipf status could not be retrieved");
> diff --git a/lib/dpctl.man b/lib/dpctl.man
> index fc53094d2dc4..db2b7f8bcc4b 100644
> --- a/lib/dpctl.man
> +++ b/lib/dpctl.man
> @@ -304,5 +304,6 @@ fragmentation is enabled.  Only supported for
> userspace datapath.
>  .TP
>  .DO "[\fB\-m\fR | \fB\-\-more\fR]" "\*(DX\fBipf\-get\-status\fR
> [\fIdp\fR]"
>  Gets the configuration settings and fragment counters associated with the
> -fragmentation handling of the userspace datapath connection tracker.  With
> -\fB\-m\fR or \fB\-\-more\fR, also dumps the ipf lists.
> +fragmentation handling of the userspace datapath connection tracker.
> +With \fB\-m\fR or \fB\-\-more\fR, also dumps the IP fragment lists.
> +Only supported for userspace datapath.
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 73ab632b969e..d2cd0cbfe2ec 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -6801,33 +6801,9 @@ dpif_netdev_ipf_set_max_nfrags(struct dpif *dpif
> OVS_UNUSED,
>
>  static int
>  dpif_netdev_ipf_get_status(struct dpif *dpif OVS_UNUSED,
> -    bool *ipf_v4_enabled, unsigned int *min_v4_frag_size,
> -    unsigned int *nfrag_max, unsigned int *nfrag,
> -    unsigned int *n4frag_accepted, unsigned int *n4frag_completed_sent,
> -    unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
> -    unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> -    unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> -    unsigned int *n6frag_completed_sent, unsigned int
> *n6frag_expired_sent,
> -    unsigned int *n6frag_too_small, unsigned int *n6frag_overlap)
> -{
> -    struct ipf_status ipf_status;
> -    ipf_get_status(&ipf_status);
> -    *ipf_v4_enabled = ipf_status.ifp_v4_enabled;
> -    *min_v4_frag_size = ipf_status.min_v4_frag_size;
> -    *nfrag_max = ipf_status.nfrag_max;
> -    *nfrag = ipf_status.nfrag;
> -    *n4frag_accepted = ipf_status.n4frag_accepted;
> -    *n4frag_completed_sent = ipf_status.n4frag_completed_sent;
> -    *n4frag_expired_sent = ipf_status.n4frag_expired_sent;
> -    *n4frag_too_small = ipf_status.n4frag_too_small;
> -    *n4frag_overlap = ipf_status.n4frag_overlap;
> -    *ipf_v6_enabled = ipf_status.ifp_v6_enabled;
> -    *min_v6_frag_size = ipf_status.min_v6_frag_size;
> -    *n6frag_accepted = ipf_status.n6frag_accepted;
> -    *n6frag_completed_sent = ipf_status.n6frag_completed_sent;
> -    *n6frag_expired_sent = ipf_status.n6frag_expired_sent;
> -    *n6frag_too_small = ipf_status.n6frag_too_small;
> -    *n6frag_overlap = ipf_status.n6frag_overlap;
> +                           struct ipf_status *status)
> +{
> +    ipf_get_status(status);
>      return 0;
>  }
>
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 2e0d5968cea6..457911b7277a 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -452,19 +452,11 @@ struct dpif_class {
>      /* Set maximum number of fragments tracked. */
>      int (*ipf_set_max_nfrags)(struct dpif *, uint32_t max_nfrags);
>      /* Get fragmentation configuration status and counters. */
> -    int (*ipf_get_status)(struct dpif *, bool *ipf_v4_enabled,
> -        unsigned int *min_v4_frag_size,
> -        unsigned int *nfrag_max, unsigned int *nfrag,
> -        unsigned int *n4frag_accepted, unsigned int
> *n4frag_completed_sent,
> -        unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
> -        unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> -        unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> -        unsigned int *n6frag_completed_sent,
> -        unsigned int *n6frag_expired_sent, unsigned int *n6frag_too_small,
> -        unsigned int *n6frag_overlap);
> -    int (*ipf_dump_start)(struct dpif *, struct ipf_dump_ctx
> **ipf_dump_ctx);
> +    int (*ipf_get_status)(struct dpif *, struct ipf_status *);
> +
>      /* Finds the next ipf list and creates a string representation of the
>       * state of an ipf list, to which 'dump' is pointed to. */
> +    int (*ipf_dump_start)(struct dpif *, struct ipf_dump_ctx
> **ipf_dump_ctx);
>      int (*ipf_dump_next)(struct dpif *, void *, char **dump);
>      int (*ipf_dump_done)(struct dpif *, void *ipf_dump_ctx);
>
> diff --git a/lib/ipf.c b/lib/ipf.c
> index ff08f5933e96..ff4de524abf2 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -1347,26 +1347,22 @@ ipf_set_max_nfrags(uint32_t value)
>  int
>  ipf_get_status(struct ipf_status *ipf_status)
>  {
> -    atomic_read_relaxed(&ifp_v4_enabled, &ipf_status->ifp_v4_enabled);
> -    atomic_read_relaxed(&min_v4_frag_size, &ipf_status->min_v4_frag_size)
> ;
> +    atomic_read_relaxed(&ifp_v4_enabled, &ipf_status->v4.enabled);
> +    atomic_read_relaxed(&min_v4_frag_size, &ipf_status->v4.min_frag_size)
> ;
>      atomic_read_relaxed(&nfrag_max, &ipf_status->nfrag_max);
>      ipf_status->nfrag = atomic_count_get(&nfrag);
> -    ipf_status->n4frag_accepted = atomic_count_get(&n4frag_accepted);
> -    ipf_status->n4frag_completed_sent =
> -        atomic_count_get(&n4frag_completed_sent);
> -    ipf_status->n4frag_expired_sent =
> -        atomic_count_get(&n4frag_expired_sent);
> -    ipf_status->n4frag_too_small = atomic_count_get(&n4frag_too_small);
> -    ipf_status->n4frag_overlap = atomic_count_get(&n4frag_overlap);
> -    atomic_read_relaxed(&ifp_v6_enabled, &ipf_status->ifp_v6_enabled);
> -    atomic_read_relaxed(&min_v6_frag_size, &ipf_status->min_v6_frag_size)
> ;
> -    ipf_status->n6frag_accepted = atomic_count_get(&n6frag_accepted);
> -    ipf_status->n6frag_completed_sent =
> -        atomic_count_get(&n6frag_completed_sent);
> -    ipf_status->n6frag_expired_sent =
> -        atomic_count_get(&n6frag_expired_sent);
> -    ipf_status->n6frag_too_small = atomic_count_get(&n6frag_too_small);
> -    ipf_status->n6frag_overlap = atomic_count_get(&n6frag_overlap);
> +    ipf_status->v4.accepted = atomic_count_get(&n4frag_accepted);
> +    ipf_status->v4.completed_sent = atomic_count_get(&n4frag_compl
> eted_sent);
> +    ipf_status->v4.expired_sent = atomic_count_get(&n4frag_expired_sent);
> +    ipf_status->v4.too_small = atomic_count_get(&n4frag_too_small);
> +    ipf_status->v4.overlap = atomic_count_get(&n4frag_overlap);
> +    atomic_read_relaxed(&ifp_v6_enabled, &ipf_status->v6.enabled);
> +    atomic_read_relaxed(&min_v6_frag_size, &ipf_status->v6.min_frag_size)
> ;
> +    ipf_status->v6.accepted = atomic_count_get(&n6frag_accepted);
> +    ipf_status->v6.completed_sent = atomic_count_get(&n6frag_compl
> eted_sent);
> +    ipf_status->v6.expired_sent = atomic_count_get(&n6frag_expired_sent);
> +    ipf_status->v6.too_small = atomic_count_get(&n6frag_too_small);
> +    ipf_status->v6.overlap = atomic_count_get(&n6frag_overlap);
>      return 0;
>  }
>
> @@ -1374,7 +1370,8 @@ struct ipf_dump_ctx {
>      struct hmap_position bucket_pos;
>  };
>
> -/* Allocates an 'ipf_dump_ctx' to keep track of an hmap position. */
> +/* Allocates an 'ipf_dump_ctx' to keep track of an hmap position. The
> + * caller must call ipf_dump_done() when dumping is finished. */
>  int
>  ipf_dump_start(struct ipf_dump_ctx **ipf_dump_ctx)
>  {
> @@ -1415,7 +1412,7 @@ ipf_dump_create(const struct ipf_list *ipf_list,
> struct ds *ds)
>
>  /* Finds the next ipf list starting from 'ipf_dump_ctx->bucket_pos' and
> uses
>   * ipf_dump_create() to create a string representation of the state of an
> - * ipf list, to which 'dump' is pointed to. */
> + * ipf list, to which 'dump' is pointed to.  Return EOF when */
>  int
>  ipf_dump_next(struct ipf_dump_ctx *ipf_dump_ctx, char **dump)
>  {
> @@ -1439,7 +1436,7 @@ ipf_dump_next(struct ipf_dump_ctx *ipf_dump_ctx,
> char **dump)
>      }
>  }
>
> -/* Frees an ipf_dump_ctx allocated by ipf_dump_start. */
> +/* Frees 'ipf_dump_ctx' allocated by ipf_dump_start(). */
>  int
>  ipf_dump_done(struct ipf_dump_ctx *ipf_dump_ctx)
>  {
> diff --git a/lib/ipf.h b/lib/ipf.h
> index 635988b2fcd8..8fa58ab75851 100644
> --- a/lib/ipf.h
> +++ b/lib/ipf.h
> @@ -20,6 +20,26 @@
>  #include "dp-packet.h"
>  #include "openvswitch/types.h"
>
> +struct proto_status {
> +    bool enabled;
> +    unsigned int min_frag_size;
> +
> +    /* Fragment statistics */
> +    unsigned int accepted;
> +    unsigned int completed_sent;
> +    unsigned int expired_sent;
> +    unsigned int too_small;
> +    unsigned int overlap;
> +};
> +
> +struct ipf_status {
> +    unsigned int nfrag;
> +    unsigned int nfrag_max;
> +
> +    struct proto_status v4;
> +    struct proto_status v6;
> +};
> +
>  void ipf_preprocess_conntrack(struct dp_packet_batch *pb, long long now,
>                                ovs_be16 dl_type, uint16_t zone,
>                                uint32_t hash_basis);
>
>
>
>

Justin Pettit Aug. 17, 2018, 6:26 p.m. UTC | #4

> On Aug 13, 2018, at 2:19 PM, Darrell Ball <dlu998@gmail.com> wrote:
> 
> > diff --git a/lib/dpctl.c b/lib/dpctl.c
> > index 5ec36bd..b3e7ce7 100644
> > --- a/lib/dpctl.c
> > +++ b/lib/dpctl.c
> > ..
> > +static int
> > +dpctl_ct_ipf_get_status(int argc, const char *argv[],
> > +                        struct dpctl_params *dpctl_p)
> > +{
> > +    struct dpif *dpif;
> > +    int error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif);
> > +    if (!error) {
> > +        bool ipf_v4_enabled;
> > +        unsigned int min_v4_frag_size;
> > +        unsigned int nfrag_max;
> > +        unsigned int nfrag;
> > +        unsigned int n4frag_accepted;
> > +        unsigned int n4frag_completed_sent;
> > +        unsigned int n4frag_expired_sent;
> > +        unsigned int n4frag_too_small;
> > +        unsigned int n4frag_overlap;
> > +        unsigned int min_v6_frag_size;
> > +        bool ipf_v6_enabled;
> > +        unsigned int n6frag_accepted;
> > +        unsigned int n6frag_completed_sent;
> > +        unsigned int n6frag_expired_sent;
> > +        unsigned int n6frag_too_small;
> > +        unsigned int n6frag_overlap;
> > +        error = ct_dpif_ipf_get_status(dpif, &ipf_v4_enabled,
> > +            &min_v4_frag_size, &nfrag_max, &nfrag, &n4frag_accepted,
> > +            &n4frag_completed_sent, &n4frag_expired_sent, &n4frag_too_small,
> > +            &n4frag_overlap, &ipf_v6_enabled, &min_v6_frag_size,
> > +            &n6frag_accepted, &n6frag_completed_sent, &n6frag_expired_sent,
> > +            &n6frag_too_small, &n6frag_overlap);
> > +
> 
> If we just use 'ipf_status', we can delete a lot of these variable declarations.
> 
> 
> I understand; I commented earlier about this. The reason for not using ipf_status is not because I like typing lots.
> 
> The reasoning is:
> 
> 1/ ipf_status is specific to the userspace datapath. The kernel datapath and various hardware offload
> datapaths may not use or even be able to use the same datatype.
> Note that dpctl and dpif code is intended to be generic across datapaths.
> 
> 2/ Using ipf_status exposes what is intended to be private internal datastructures to external modules
> I was trying to avoid that, if possible, and use base types in common code.
> 
> Perhaps, the easiest way to address this to simply have a dpif_ipf_status, which is defined similarly to ipf_status, at
> least initially ? These can diverge later if needed,

Yes, I had those same thoughts, so I think just creating a separate "dpif_ipf_status" would be a good solutions.

--Justin

diff --git a/NEWS b/NEWS
index 6b82ebb..b9f0959 100644
--- a/NEWS
+++ b/NEWS
@@ -24,6 +24,8 @@  Post-v2.9.0
        datapath conntrack fragmentation support.
      * New "ovs-appctl dpctl/ipf-set-max-nfrags" command for userspace datapath
        conntrack fragmentation support.
+     * New "ovs-appctl dpctl/ipf-get-status" command for userspace datapath
+       conntrack fragmentation support.
    - ovs-vsctl: New commands "add-bond-iface" and "del-bond-iface".
    - OpenFlow:
      * OFPT_ROLE_STATUS is now available in OpenFlow 1.3.
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index ee23a4d..a59bc1e 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -188,6 +188,51 @@  ct_dpif_ipf_set_max_nfrags(struct dpif *dpif, uint32_t max_frags)
             : EOPNOTSUPP);
 }
 
+int ct_dpif_ipf_get_status(struct dpif *dpif, bool *ipf_v4_enabled,
+    unsigned int *min_v4_frag_size, unsigned int *nfrag_max,
+    unsigned int *nfrag, unsigned int *n4frag_accepted,
+    unsigned int *n4frag_completed_sent,
+    unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
+    unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
+    unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
+    unsigned int *n6frag_completed_sent,
+    unsigned int *n6frag_expired_sent, unsigned int *n6frag_too_small,
+    unsigned int *n6frag_overlap)
+{
+    return (dpif->dpif_class->ipf_get_status
+            ? dpif->dpif_class->ipf_get_status(dpif, ipf_v4_enabled,
+            min_v4_frag_size, nfrag_max, nfrag, n4frag_accepted,
+            n4frag_completed_sent, n4frag_expired_sent, n4frag_too_small,
+            n4frag_overlap, ipf_v6_enabled, min_v6_frag_size, n6frag_accepted,
+            n6frag_completed_sent, n6frag_expired_sent, n6frag_too_small,
+            n6frag_overlap)
+            : EOPNOTSUPP);
+}
+
+int
+ct_dpif_ipf_dump_start(struct dpif *dpif, struct ipf_dump_ctx **dump_ctx)
+{
+    return (dpif->dpif_class->ipf_dump_start
+           ? dpif->dpif_class->ipf_dump_start(dpif, dump_ctx)
+           : EOPNOTSUPP);
+}
+
+int
+ct_dpif_ipf_dump_next(struct dpif *dpif, void *dump_ctx,  char **dump)
+{
+    return (dpif->dpif_class->ipf_dump_next
+            ? dpif->dpif_class->ipf_dump_next(dpif, dump_ctx, dump)
+            : EOPNOTSUPP);
+}
+
+int
+ct_dpif_ipf_dump_done(struct dpif *dpif, void *dump_ctx)
+{
+    return (dpif->dpif_class->ipf_dump_done
+            ? dpif->dpif_class->ipf_dump_done(dpif, dump_ctx)
+            : EOPNOTSUPP);
+}
+
 void
 ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
 {
diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
index f886ab9..2ff7e26 100644
--- a/lib/ct-dpif.h
+++ b/lib/ct-dpif.h
@@ -204,6 +204,15 @@  int ct_dpif_get_nconns(struct dpif *dpif, uint32_t *nconns);
 int ct_dpif_ipf_set_enabled(struct dpif *, bool v6, bool enable);
 int ct_dpif_ipf_set_min_frag(struct dpif *, bool, uint32_t);
 int ct_dpif_ipf_set_max_nfrags(struct dpif *, uint32_t);
+int ct_dpif_ipf_get_status(struct dpif *dpif, bool *, unsigned int *,
+                           unsigned int *, unsigned int *, unsigned int *,
+                           unsigned int *, unsigned int *, unsigned int *,
+                           unsigned int *, bool *, unsigned int *,
+                           unsigned int *, unsigned int *, unsigned int *,
+                           unsigned int *, unsigned int *);
+int ct_dpif_ipf_dump_start(struct dpif *dpif, struct ipf_dump_ctx **);
+int ct_dpif_ipf_dump_next(struct dpif *dpif, void *, char **);
+int ct_dpif_ipf_dump_done(struct dpif *dpif, void *);
 void ct_dpif_entry_uninit(struct ct_dpif_entry *);
 void ct_dpif_format_entry(const struct ct_dpif_entry *, struct ds *,
                           bool verbose, bool print_stats);
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 5ec36bd..b3e7ce7 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1792,6 +1792,111 @@  dpctl_ipf_set_max_nfrags(int argc, const char *argv[],
     return error;
 }
 
+static void
+dpctl_dump_ipf(struct dpif *dpif, struct dpctl_params *dpctl_p)
+{
+    struct ipf_dump_ctx *dump_ctx;
+    char *dump;
+
+    int error = ct_dpif_ipf_dump_start(dpif, &dump_ctx);
+    if (error) {
+        dpctl_error(dpctl_p, error, "starting ipf list dump");
+        /* Nothing to clean up, just return. */
+        return;
+    }
+
+    dpctl_print(dpctl_p, "\n        Fragment Lists:\n\n");
+    while (!(error = ct_dpif_ipf_dump_next(dpif, dump_ctx, &dump))) {
+        dpctl_print(dpctl_p, "%s\n", dump);
+        free(dump);
+    }
+
+    if (error && error != EOF) {
+        dpctl_error(dpctl_p, error, "dumping ipf lists failed");
+    }
+
+    ct_dpif_ipf_dump_done(dpif, dump_ctx);
+}
+
+static int
+dpctl_ct_ipf_get_status(int argc, const char *argv[],
+                        struct dpctl_params *dpctl_p)
+{
+    struct dpif *dpif;
+    int error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif);
+    if (!error) {
+        bool ipf_v4_enabled;
+        unsigned int min_v4_frag_size;
+        unsigned int nfrag_max;
+        unsigned int nfrag;
+        unsigned int n4frag_accepted;
+        unsigned int n4frag_completed_sent;
+        unsigned int n4frag_expired_sent;
+        unsigned int n4frag_too_small;
+        unsigned int n4frag_overlap;
+        unsigned int min_v6_frag_size;
+        bool ipf_v6_enabled;
+        unsigned int n6frag_accepted;
+        unsigned int n6frag_completed_sent;
+        unsigned int n6frag_expired_sent;
+        unsigned int n6frag_too_small;
+        unsigned int n6frag_overlap;
+        error = ct_dpif_ipf_get_status(dpif, &ipf_v4_enabled,
+            &min_v4_frag_size, &nfrag_max, &nfrag, &n4frag_accepted,
+            &n4frag_completed_sent, &n4frag_expired_sent, &n4frag_too_small,
+            &n4frag_overlap, &ipf_v6_enabled, &min_v6_frag_size,
+            &n6frag_accepted, &n6frag_completed_sent, &n6frag_expired_sent,
+            &n6frag_too_small, &n6frag_overlap);
+
+        if (!error) {
+            dpctl_print(dpctl_p, "        Fragmentation Module Status\n");
+            dpctl_print(dpctl_p, "        ---------------------------\n");
+            dpctl_print(dpctl_p, "        v4 enabled: %u\n", ipf_v4_enabled);
+            dpctl_print(dpctl_p, "        v6 enabled: %u\n", ipf_v6_enabled);
+            dpctl_print(dpctl_p, "        max num frags (v4/v6): %u\n",
+                        nfrag_max);
+            dpctl_print(dpctl_p, "        num frag: %u\n", nfrag);
+            dpctl_print(dpctl_p, "        min v4 frag size: %u\n",
+                        min_v4_frag_size);
+            dpctl_print(dpctl_p, "        v4 frags accepted: %u\n",
+                        n4frag_accepted);
+            dpctl_print(dpctl_p, "        v4 frags completed: %u\n",
+                        n4frag_completed_sent);
+            dpctl_print(dpctl_p, "        v4 frags expired: %u\n",
+                        n4frag_expired_sent);
+            dpctl_print(dpctl_p, "        v4 frags too small: %u\n",
+                        n4frag_too_small);
+            dpctl_print(dpctl_p, "        v4 frags overlapped: %u\n",
+                        n4frag_overlap);
+            dpctl_print(dpctl_p, "        min v6 frag size: %u\n",
+                        min_v6_frag_size);
+            dpctl_print(dpctl_p, "        v6 frags accepted: %u\n",
+                        n6frag_accepted);
+            dpctl_print(dpctl_p, "        v6 frags completed: %u\n",
+                        n6frag_completed_sent);
+            dpctl_print(dpctl_p, "        v6 frags expired: %u\n",
+                        n6frag_expired_sent);
+            dpctl_print(dpctl_p, "        v6 frags too small: %u\n",
+                        n6frag_too_small);
+            dpctl_print(dpctl_p, "        v6 frags overlapped: %u\n",
+                        n6frag_overlap);
+        } else {
+            dpctl_error(dpctl_p, error,
+                        "ipf status could not be retrieved");
+            return error;
+        }
+
+        if (argc > 0 && (!strncmp(argv[argc - 1], "-m", 2)
+            || !strncmp(argv[argc - 1], "--more", 6))) {
+            dpctl_dump_ipf(dpif, dpctl_p);
+        }
+
+        dpif_close(dpif);
+    }
+
+    return error;
+}
+
 /* Undocumented commands for unit testing. */
 
 static int
@@ -2099,6 +2204,8 @@  static const struct dpctl_command all_commands[] = {
        dpctl_ipf_set_min_frag, DP_RW },
     { "ipf-set-max-nfrags", "[dp] maxfrags", 1, 2,
        dpctl_ipf_set_max_nfrags, DP_RW },
+    { "ipf-get-status", "[dp] [-m | --more]", 0, 2, dpctl_ct_ipf_get_status,
+       DP_RO },
     { "help", "", 0, INT_MAX, dpctl_help, DP_RO },
     { "list-commands", "", 0, INT_MAX, dpctl_list_commands, DP_RO },
 
diff --git a/lib/dpctl.man b/lib/dpctl.man
index f5a36b0..1074963 100644
--- a/lib/dpctl.man
+++ b/lib/dpctl.man
@@ -304,3 +304,9 @@  connection tracker.  The default value is 1000 and the clamped maximum
 is 5000.  Note that packet buffers can be held by the fragmentation
 module while fragments are incomplete, but will timeout after 15 seconds.
 Memory pool sizing should be set accordingly when fragmentation is enabled.
+.
+.TP
+.DO "[\fB\-m\fR | \fB\-\-more\fR]" "\*(DX\fBipf\-get\-status\fR [\fIdp\fR]"
+Gets the configuration settings and fragment counters associated with the
+fragmentation handling of the userspace datapath connection tracker.  With
+\fB\-m\fR or \fB\-\-more\fR, also dumps the ipf lists.
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 76bc1d9..db551ea 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -6553,6 +6553,60 @@  dpif_netdev_ipf_set_max_nfrags(struct dpif *dpif OVS_UNUSED,
     return ipf_set_max_nfrags(max_frags);
 }
 
+static int
+dpif_netdev_ipf_get_status(struct dpif *dpif OVS_UNUSED,
+    bool *ipf_v4_enabled, unsigned int *min_v4_frag_size,
+    unsigned int *nfrag_max, unsigned int *nfrag,
+    unsigned int *n4frag_accepted, unsigned int *n4frag_completed_sent,
+    unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
+    unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
+    unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
+    unsigned int *n6frag_completed_sent, unsigned int *n6frag_expired_sent,
+    unsigned int *n6frag_too_small, unsigned int *n6frag_overlap)
+{
+    struct ipf_status ipf_status;
+    ipf_get_status(&ipf_status);
+    *ipf_v4_enabled = ipf_status.ifp_v4_enabled;
+    *min_v4_frag_size = ipf_status.min_v4_frag_size;
+    *nfrag_max = ipf_status.nfrag_max;
+    *nfrag = ipf_status.nfrag;
+    *n4frag_accepted = ipf_status.n4frag_accepted;
+    *n4frag_completed_sent = ipf_status.n4frag_completed_sent;
+    *n4frag_expired_sent = ipf_status.n4frag_expired_sent;
+    *n4frag_too_small = ipf_status.n4frag_too_small;
+    *n4frag_overlap = ipf_status.n4frag_overlap;
+    *ipf_v6_enabled = ipf_status.ifp_v6_enabled;
+    *min_v6_frag_size = ipf_status.min_v6_frag_size;
+    *n6frag_accepted = ipf_status.n6frag_accepted;
+    *n6frag_completed_sent = ipf_status.n6frag_completed_sent;
+    *n6frag_expired_sent = ipf_status.n6frag_expired_sent;
+    *n6frag_too_small = ipf_status.n6frag_too_small;
+    *n6frag_overlap = ipf_status.n6frag_overlap;
+    return 0;
+}
+
+static int
+dpif_netdev_ipf_dump_start(struct dpif *dpif OVS_UNUSED,
+                           struct ipf_dump_ctx **ipf_dump_ctx)
+{
+    return ipf_dump_start(ipf_dump_ctx);
+}
+
+static int
+dpif_netdev_ipf_dump_next(struct dpif *dpif OVS_UNUSED,
+                          void *ipf_dump_ctx, char **dump)
+{
+    return ipf_dump_next(ipf_dump_ctx, dump);
+}
+
+static int
+dpif_netdev_ipf_dump_done(struct dpif *dpif OVS_UNUSED,
+                          void *ipf_dump_ctx)
+{
+    return ipf_dump_done(ipf_dump_ctx);
+
+}
+
 const struct dpif_class dpif_netdev_class = {
     "netdev",
     dpif_netdev_init,
@@ -6604,6 +6658,10 @@  const struct dpif_class dpif_netdev_class = {
     dpif_netdev_ipf_set_enabled,
     dpif_netdev_ipf_set_min_frag,
     dpif_netdev_ipf_set_max_nfrags,
+    dpif_netdev_ipf_get_status,
+    dpif_netdev_ipf_dump_start,
+    dpif_netdev_ipf_dump_next,
+    dpif_netdev_ipf_dump_done,
     dpif_netdev_meter_get_features,
     dpif_netdev_meter_set,
     dpif_netdev_meter_get,
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 80c54f5..42ac01d 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -3009,6 +3009,10 @@  const struct dpif_class dpif_netlink_class = {
     NULL,                       /* ipf_set_enabled */
     NULL,                       /* ipf_set_min_frag */
     NULL,                       /* ipf_set_max_nfrags */
+    NULL,                       /* ipf_get_status */
+    NULL,                       /* ipf_dump_start */
+    NULL,                       /* ipf_dump_next */
+    NULL,                       /* ipf_dump_done */
     dpif_netlink_meter_get_features,
     dpif_netlink_meter_set,
     dpif_netlink_meter_get,
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 214f570..2e0d596 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -451,6 +451,22 @@  struct dpif_class {
     int (*ipf_set_min_frag)(struct dpif *, bool v6, uint32_t min_frag);
     /* Set maximum number of fragments tracked. */
     int (*ipf_set_max_nfrags)(struct dpif *, uint32_t max_nfrags);
+    /* Get fragmentation configuration status and counters. */
+    int (*ipf_get_status)(struct dpif *, bool *ipf_v4_enabled,
+        unsigned int *min_v4_frag_size,
+        unsigned int *nfrag_max, unsigned int *nfrag,
+        unsigned int *n4frag_accepted, unsigned int *n4frag_completed_sent,
+        unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
+        unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
+        unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
+        unsigned int *n6frag_completed_sent,
+        unsigned int *n6frag_expired_sent, unsigned int *n6frag_too_small,
+        unsigned int *n6frag_overlap);
+    int (*ipf_dump_start)(struct dpif *, struct ipf_dump_ctx **ipf_dump_ctx);
+    /* Finds the next ipf list and creates a string representation of the
+     * state of an ipf list, to which 'dump' is pointed to. */
+    int (*ipf_dump_next)(struct dpif *, void *, char **dump);
+    int (*ipf_dump_done)(struct dpif *, void *ipf_dump_ctx);
 
     /* Meters */
     /* Queries 'dpif' for supported meter features.
diff --git a/lib/ipf.c b/lib/ipf.c
index b1d9a11..44f2cd4 100644
--- a/lib/ipf.c
+++ b/lib/ipf.c
@@ -52,6 +52,10 @@  enum ipf_list_state {
     IPF_LIST_STATE_NUM,
 };
 
+static char *ipf_state_name[IPF_LIST_STATE_NUM] =
+    {"unused", "reassemble fail", "other frag", "first frag", "last frag",
+     "first/last frag", "complete"};
+
 enum ipf_list_type {
     IPF_FRAG_COMPLETED_LIST,
     IPF_FRAG_EXPIRY_LIST,
@@ -1337,3 +1341,106 @@  ipf_set_max_nfrags(uint32_t value)
     atomic_store_relaxed(&nfrag_max, value);
     return 0;
 }
+
+int
+ipf_get_status(struct ipf_status *ipf_status)
+{
+    atomic_read_relaxed(&ifp_v4_enabled, &ipf_status->ifp_v4_enabled);
+    atomic_read_relaxed(&min_v4_frag_size, &ipf_status->min_v4_frag_size);
+    atomic_read_relaxed(&nfrag_max, &ipf_status->nfrag_max);
+    ipf_status->nfrag = atomic_count_get(&nfrag);
+    ipf_status->n4frag_accepted = atomic_count_get(&n4frag_accepted);
+    ipf_status->n4frag_completed_sent =
+        atomic_count_get(&n4frag_completed_sent);
+    ipf_status->n4frag_expired_sent =
+        atomic_count_get(&n4frag_expired_sent);
+    ipf_status->n4frag_too_small = atomic_count_get(&n4frag_too_small);
+    ipf_status->n4frag_overlap = atomic_count_get(&n4frag_overlap);
+    atomic_read_relaxed(&ifp_v6_enabled, &ipf_status->ifp_v6_enabled);
+    atomic_read_relaxed(&min_v6_frag_size, &ipf_status->min_v6_frag_size);
+    ipf_status->n6frag_accepted = atomic_count_get(&n6frag_accepted);
+    ipf_status->n6frag_completed_sent =
+        atomic_count_get(&n6frag_completed_sent);
+    ipf_status->n6frag_expired_sent =
+        atomic_count_get(&n6frag_expired_sent);
+    ipf_status->n6frag_too_small = atomic_count_get(&n6frag_too_small);
+    ipf_status->n6frag_overlap = atomic_count_get(&n6frag_overlap);
+    return 0;
+}
+
+struct ipf_dump_ctx {
+    struct hmap_position bucket_pos;
+};
+
+/* Allocates an 'ipf_dump_ctx' to keep track of an hmap position. */
+int
+ipf_dump_start(struct ipf_dump_ctx **ipf_dump_ctx)
+{
+    *ipf_dump_ctx = xzalloc(sizeof **ipf_dump_ctx);
+    return 0;
+}
+
+/* Creates a string representation of the state of an 'ipf_list' and puts
+ * it in  'ds'. */
+static void
+ipf_dump_create(const struct ipf_list *ipf_list, struct ds *ds)
+{
+
+    ds_put_cstr(ds, "(");
+    if (ipf_list->key.dl_type == htons(ETH_TYPE_IP)) {
+        ds_put_format(ds, "src="IP_FMT",dst="IP_FMT",",
+                      IP_ARGS(ipf_list->key.src_addr.ipv4_aligned),
+                      IP_ARGS(ipf_list->key.dst_addr.ipv4_aligned));
+    } else {
+        ds_put_cstr(ds, "src=");
+        ipv6_format_addr(&ipf_list->key.src_addr.ipv6_aligned, ds);
+        ds_put_cstr(ds, ",dst=");
+        ipv6_format_addr(&ipf_list->key.dst_addr.ipv6_aligned, ds);
+        ds_put_cstr(ds, ",");
+    }
+
+    ds_put_format(ds, "recirc_id=%u,ip_id=%u,dl_type=0x%x,zone=%u,nw_proto=%u",
+                  ipf_list->key.recirc_id, ntohl(ipf_list->key.ip_id),
+                  ntohs(ipf_list->key.dl_type), ipf_list->key.zone,
+                  ipf_list->key.nw_proto);
+
+    ds_put_format(ds, ",num_fragments=%u,state=%s",
+                  ipf_list->last_inuse_idx + 1,
+                  ipf_state_name[ipf_list->state]);
+
+    ds_put_cstr(ds, ")");
+}
+
+/* Finds the next ipf list starting from 'ipf_dump_ctx->bucket_pos' and uses
+ * ipf_dump_create() to create a string representation of the state of an
+ * ipf list, to which 'dump' is pointed to. */
+int
+ipf_dump_next(struct ipf_dump_ctx *ipf_dump_ctx, char **dump)
+{
+    ipf_lock_lock(&ipf_lock);
+
+    struct hmap_node *node = hmap_at_position(&frag_lists,
+                                              &ipf_dump_ctx->bucket_pos);
+    if (!node) {
+        ipf_lock_unlock(&ipf_lock);
+        return EOF;
+    } else {
+        struct ipf_list *ipf_list_;
+        INIT_CONTAINER(ipf_list_, node, node);
+        struct ipf_list ipf_list = *ipf_list_;
+        ipf_lock_unlock(&ipf_lock);
+        struct ds ds = DS_EMPTY_INITIALIZER;
+        ipf_dump_create(&ipf_list, &ds);
+        *dump = xstrdup(ds.string);
+        ds_destroy(&ds);
+        return 0;
+    }
+}
+
+/* Frees an ipf_dump_ctx allocated by ipf_dump_start. */
+int
+ipf_dump_done(struct ipf_dump_ctx *ipf_dump_ctx)
+{
+    free(ipf_dump_ctx);
+    return 0;
+}
diff --git a/lib/ipf.h b/lib/ipf.h
index 4289e5e..36a182a 100644
--- a/lib/ipf.h
+++ b/lib/ipf.h
@@ -63,4 +63,14 @@  int ipf_set_min_frag(bool v6, uint32_t value);
 
 int ipf_set_max_nfrags(uint32_t value);
 
+int ipf_get_status(struct ipf_status *ipf_status);
+
+struct ipf_dump_ctx;
+
+int ipf_dump_start(struct ipf_dump_ctx **ipf_dump_ctx);
+
+int ipf_dump_next(struct ipf_dump_ctx *ipf_dump_ctx, char **dump);
+
+int ipf_dump_done(struct ipf_dump_ctx *ipf_dump_ctx);
+
 #endif /* ipf.h */
diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at
index 3ea5b87..736a643 100644
--- a/tests/system-kmod-macros.at
+++ b/tests/system-kmod-macros.at
@@ -130,3 +130,35 @@  m4_define([CHECK_CT_DPIF_GET_NCONNS],
 [
     AT_SKIP_IF([:])
 ])
+
+# DPCTL_SET_MIN_FRAG_SIZE()
+#
+# The kernel does not support this command.
+m4_define([DPCTL_SET_MIN_FRAG_SIZE],
+[
+
+])
+
+# DPCTL_MODIFY_FRAGMENTATION()
+#
+# The kernel does not support this command.
+m4_define([DPCTL_MODIFY_FRAGMENTATION],
+[
+
+])
+
+# DPCTL_CHECK_FRAGMENTATION_PASS()
+#
+# The kernel does not support this command.
+m4_define([DPCTL_CHECK_FRAGMENTATION_PASS],
+[
+
+])
+
+# DPCTL_CHECK_FRAGMENTATION_FAIL()
+#
+# The kernel does not support this command.
+m4_define([DPCTL_CHECK_FRAGMENTATION_FAIL],
+[
+
+])
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 556a6a0..2a5bdb6 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -1923,6 +1923,9 @@  priority=100,in_port=2,ct_state=+trk+est-new,icmp,action=1
 
 AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
 
+dnl Modify userspace conntrack fragmentation handling.
+DPCTL_MODIFY_FRAGMENTATION()
+
 dnl Ipv4 fragmentation connectivity check.
 NS_CHECK_EXEC([at_ns0], [ping -s 1600 -q -c 3 -i 0.3 -w 2 10.1.1.2 | FORMAT_PING], [0], [dnl
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
@@ -1933,6 +1936,9 @@  NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w 2 10.1.1.2 | FORMAT_PING
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
 ])
 
+dnl Check userspace conntrack fragmentation counters.
+DPCTL_CHECK_FRAGMENTATION_PASS()
+
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
@@ -1958,11 +1964,17 @@  priority=100,in_port=2,ct_state=+trk+est-new,icmp,action=1
 
 AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
 
+dnl Modify userspace conntrack fragmentation handling.
+DPCTL_MODIFY_FRAGMENTATION()
+
 dnl Ipv4 fragmentation connectivity check.
 NS_CHECK_EXEC([at_ns0], [ping -s 1600 -q -c 1 -i 0.3 -w 2 10.1.1.2 | FORMAT_PING], [0], [dnl
 7 packets transmitted, 0 received, 100% packet loss, time 0ms
 ])
 
+dnl Check userspace conntrack fragmentation counters.
+DPCTL_CHECK_FRAGMENTATION_FAIL()
+
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
@@ -1988,6 +2000,9 @@  priority=100,in_port=2,ct_state=+trk+est-new,icmp,action=1
 
 AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
 
+dnl Modify userspace conntrack fragmentation handling.
+DPCTL_MODIFY_FRAGMENTATION()
+
 dnl Ipv4 fragmentation connectivity check.
 NS_CHECK_EXEC([at_ns0], [ping -s 1600 -q -c 3 -i 0.3 -w 2 10.2.2.2 | FORMAT_PING], [0], [dnl
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
@@ -1998,6 +2013,9 @@  NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w 2 10.2.2.2 | FORMAT_PING
 3 packets transmitted, 3 received, 0% packet loss, time 0ms
 ])
 
+dnl Check userspace conntrack fragmentation counters.
+DPCTL_CHECK_FRAGMENTATION_PASS()
+
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
@@ -2056,6 +2074,7 @@  AT_CLEANUP
 AT_SETUP([conntrack - IPv4 fragmentation incomplete reassembled packet])
 CHECK_CONNTRACK()
 OVS_TRAFFIC_VSWITCHD_START()
+DPCTL_SET_MIN_FRAG_SIZE()
 
 ADD_NAMESPACES(at_ns0, at_ns1)
 
@@ -2077,8 +2096,8 @@  AT_CLEANUP
 dnl Uses same first fragment as above 'incomplete reassembled packet' test.
 AT_SETUP([conntrack - IPv4 fragmentation with fragments specified])
 CHECK_CONNTRACK()
-CHECK_CONNTRACK_SMALL_FRAG()
 OVS_TRAFFIC_VSWITCHD_START()
+DPCTL_SET_MIN_FRAG_SIZE()
 
 ADD_NAMESPACES(at_ns0, at_ns1)
 
@@ -2101,8 +2120,8 @@  AT_CLEANUP
 
 AT_SETUP([conntrack - IPv4 fragmentation out of order])
 CHECK_CONNTRACK()
-CHECK_CONNTRACK_SMALL_FRAG()
 OVS_TRAFFIC_VSWITCHD_START()
+DPCTL_SET_MIN_FRAG_SIZE()
 
 ADD_NAMESPACES(at_ns0, at_ns1)
 
@@ -2125,9 +2144,9 @@  AT_CLEANUP
 
 AT_SETUP([conntrack - IPv4 fragmentation overlapping fragments by 1 octet])
 CHECK_CONNTRACK()
-CHECK_CONNTRACK_SMALL_FRAG()
 CHECK_CONNTRACK_FRAG_OVERLAP()
 OVS_TRAFFIC_VSWITCHD_START()
+DPCTL_SET_MIN_FRAG_SIZE()
 
 ADD_NAMESPACES(at_ns0, at_ns1)
 
@@ -2149,9 +2168,9 @@  AT_CLEANUP
 
 AT_SETUP([conntrack - IPv4 fragmentation overlapping fragments by 1 octet out of order])
 CHECK_CONNTRACK()
-CHECK_CONNTRACK_SMALL_FRAG()
 CHECK_CONNTRACK_FRAG_OVERLAP()
 OVS_TRAFFIC_VSWITCHD_START()
+DPCTL_SET_MIN_FRAG_SIZE()
 
 ADD_NAMESPACES(at_ns0, at_ns1)
 
@@ -2348,6 +2367,7 @@  AT_CLEANUP
 AT_SETUP([conntrack - IPv6 fragmentation incomplete reassembled packet])
 CHECK_CONNTRACK()
 OVS_TRAFFIC_VSWITCHD_START()
+DPCTL_SET_MIN_FRAG_SIZE()
 
 ADD_NAMESPACES(at_ns0, at_ns1)
 
@@ -2368,8 +2388,8 @@  AT_CLEANUP
 
 AT_SETUP([conntrack - IPv6 fragmentation with fragments specified])
 CHECK_CONNTRACK()
-CHECK_CONNTRACK_SMALL_FRAG()
 OVS_TRAFFIC_VSWITCHD_START()
+DPCTL_SET_MIN_FRAG_SIZE()
 
 ADD_NAMESPACES(at_ns0, at_ns1)
 
@@ -2392,8 +2412,8 @@  AT_CLEANUP
 
 AT_SETUP([conntrack - IPv6 fragmentation out of order])
 CHECK_CONNTRACK()
-CHECK_CONNTRACK_SMALL_FRAG()
 OVS_TRAFFIC_VSWITCHD_START()
+DPCTL_SET_MIN_FRAG_SIZE()
 
 ADD_NAMESPACES(at_ns0, at_ns1)
 
@@ -2416,9 +2436,9 @@  AT_CLEANUP
 
 AT_SETUP([conntrack - IPv6 fragmentation, multiple extension headers])
 CHECK_CONNTRACK()
-CHECK_CONNTRACK_SMALL_FRAG()
 CHECK_CONNTRACK_FRAG_IPV6_MULT_EXTEN()
 OVS_TRAFFIC_VSWITCHD_START()
+DPCTL_SET_MIN_FRAG_SIZE()
 
 ADD_NAMESPACES(at_ns0, at_ns1)
 
@@ -2442,9 +2462,9 @@  AT_CLEANUP
 
 AT_SETUP([conntrack - IPv6 fragmentation, multiple extension headers + out of order])
 CHECK_CONNTRACK()
-CHECK_CONNTRACK_SMALL_FRAG()
 CHECK_CONNTRACK_FRAG_IPV6_MULT_EXTEN()
 OVS_TRAFFIC_VSWITCHD_START()
+DPCTL_SET_MIN_FRAG_SIZE()
 
 ADD_NAMESPACES(at_ns0, at_ns1)
 
@@ -2468,9 +2488,9 @@  AT_CLEANUP
 
 AT_SETUP([conntrack - IPv6 fragmentation, multiple extension headers 2])
 CHECK_CONNTRACK()
-CHECK_CONNTRACK_SMALL_FRAG()
 CHECK_CONNTRACK_FRAG_IPV6_MULT_EXTEN()
 OVS_TRAFFIC_VSWITCHD_START()
+DPCTL_SET_MIN_FRAG_SIZE()
 
 ADD_NAMESPACES(at_ns0, at_ns1)
 
@@ -2494,9 +2514,9 @@  AT_CLEANUP
 
 AT_SETUP([conntrack - IPv6 fragmentation, multiple extension headers 2 + out of order])
 CHECK_CONNTRACK()
-CHECK_CONNTRACK_SMALL_FRAG()
 CHECK_CONNTRACK_FRAG_IPV6_MULT_EXTEN()
 OVS_TRAFFIC_VSWITCHD_START()
+DPCTL_SET_MIN_FRAG_SIZE()
 
 ADD_NAMESPACES(at_ns0, at_ns1)
 
diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at
index 40b7567..fcae3cc 100644
--- a/tests/system-userspace-macros.at
+++ b/tests/system-userspace-macros.at
@@ -119,3 +119,125 @@  m4_define([CHECK_CT_DPIF_SET_GET_MAXCONNS])
 # Perform requirements checks for running ovs-dpctl ct-get-nconns. The
 # userspace datapath does support this feature.
 m4_define([CHECK_CT_DPIF_GET_NCONNS])
+
+# DPCTL_SET_MIN_FRAG_SIZE()
+#
+# The userspace datapath supports this command.
+m4_define([DPCTL_SET_MIN_FRAG_SIZE],
+[
+AT_CHECK([ovs-appctl dpctl/ipf-set-min-frag v4 400], [], [dnl
+setting minimum fragment size successful
+])
+AT_CHECK([ovs-appctl dpctl/ipf-set-min-frag v6 400], [], [dnl
+setting minimum fragment size successful
+])
+])
+
+# DPCTL_MODIFY_FRAGMENTATION()
+#
+# The userspace datapath supports this command.
+m4_define([DPCTL_MODIFY_FRAGMENTATION],
+[
+AT_CHECK([ovs-appctl dpctl/ipf-set-min-frag v4 1000], [], [dnl
+setting minimum fragment size successful
+])
+AT_CHECK([ovs-appctl dpctl/ipf-set-max-nfrags 500], [], [dnl
+setting maximum fragments successful
+])
+AT_CHECK([ovs-appctl dpctl/ipf-get-status], [], [dnl
+        Fragmentation Module Status
+        ---------------------------
+        v4 enabled: 1
+        v6 enabled: 1
+        max num frags (v4/v6): 500
+        num frag: 0
+        min v4 frag size: 1000
+        v4 frags accepted: 0
+        v4 frags completed: 0
+        v4 frags expired: 0
+        v4 frags too small: 0
+        v4 frags overlapped: 0
+        min v6 frag size: 1280
+        v6 frags accepted: 0
+        v6 frags completed: 0
+        v6 frags expired: 0
+        v6 frags too small: 0
+        v6 frags overlapped: 0
+])
+])
+
+# DPCTL_CHECK_FRAGMENTATION_PASS()
+#
+# Used to check fragmentation counters for some fragmentation tests using
+# the userspace datapath.
+m4_define([DPCTL_CHECK_FRAGMENTATION_PASS],
+[
+AT_CHECK([ovs-appctl dpctl/ipf-get-status --more], [], [dnl
+        Fragmentation Module Status
+        ---------------------------
+        v4 enabled: 1
+        v6 enabled: 1
+        max num frags (v4/v6): 500
+        num frag: 0
+        min v4 frag size: 1000
+        v4 frags accepted: 30
+        v4 frags completed: 30
+        v4 frags expired: 0
+        v4 frags too small: 0
+        v4 frags overlapped: 0
+        min v6 frag size: 1280
+        v6 frags accepted: 0
+        v6 frags completed: 0
+        v6 frags expired: 0
+        v6 frags too small: 0
+        v6 frags overlapped: 0
+
+        Fragment Lists:
+
+])
+])
+
+# FORMAT_FRAG_LIST([])
+#
+# Strip content from the piped input which can differ from test to test; recirc_id
+# and ip_id fields in an ipf_list vary from test to test and hence are cleared.
+m4_define([FORMAT_FRAG_LIST],
+    [[sed -e 's/ip_id=[0-9]*/ip_id=<cleared>/g' -e 's/recirc_id=[0-9]*/recirc_id=<cleared>/g']])
+
+# DPCTL_CHECK_FRAGMENTATION_FAIL()
+#
+# Used to check fragmentation counters for some fragmentation tests using
+# the userspace datapath, when failure to transmit fragments is expected.
+m4_define([DPCTL_CHECK_FRAGMENTATION_FAIL],
+[
+AT_CHECK([ovs-appctl dpctl/ipf-get-status -m | FORMAT_FRAG_LIST()], [], [dnl
+        Fragmentation Module Status
+        ---------------------------
+        v4 enabled: 1
+        v6 enabled: 1
+        max num frags (v4/v6): 500
+        num frag: 7
+        min v4 frag size: 1000
+        v4 frags accepted: 7
+        v4 frags completed: 0
+        v4 frags expired: 0
+        v4 frags too small: 0
+        v4 frags overlapped: 0
+        min v6 frag size: 1280
+        v6 frags accepted: 0
+        v6 frags completed: 0
+        v6 frags expired: 0
+        v6 frags too small: 0
+        v6 frags overlapped: 0
+
+        Fragment Lists:
+
+(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<cleared>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
+(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<cleared>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
+(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<cleared>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
+(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<cleared>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
+(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<cleared>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
+(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<cleared>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
+(src=10.1.1.1,dst=10.1.1.2,recirc_id=<cleared>,ip_id=<cleared>,dl_type=0x800,zone=9,nw_proto=1,num_fragments=1,state=first frag)
+])
+])

[ovs-dev,v8,9/9] ipf: Add fragmentation status reporting.

Commit Message

Comments

Patch