Thanks Darrel for your feedback, I'll post a v3 soon.

Antonio

> -----Original Message-----
> From: Darrell Ball [mailto:[email protected]]
> Sent: Monday, July 17, 2017 6:03 PM
> To: Fischetti, Antonio <[email protected]>; [email protected]
> Subject: Re: [ovs-dev] [PATCH v2] dpctl: Add new 'ct-bkts' command.
> 
> I have not tested it yet, but will do that later today or tomorrow.
> I have a few comments inline.
> 
> On 7/17/17, 6:48 AM, "[email protected] on behalf of
> [email protected]" <[email protected] on behalf of
> [email protected]> wrote:
> 
>     With the command:
>      ovs-appctl dpctl/ct-bkts
>     shows the number of connections per bucket.
> 
>     By using a threshold:
>      ovs-appctl dpctl/ct-bkts gt=N
>     for each bucket shows the number of connections when they
>     are greater than N.
> 
>     Signed-off-by: Antonio Fischetti <[email protected]>
>     Signed-off-by: Bhanuprakash Bodireddy
> <[email protected]>
>     Co-authored-by: Bhanuprakash Bodireddy
> <[email protected]>
>     ---
>      lib/conntrack.c       |  5 +--
>      lib/conntrack.h       |  4 +--
>      lib/ct-dpif.h         |  4 +++
>      lib/dpctl.c           | 88
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>      lib/dpctl.man         |  8 +++++
>      utilities/ovs-dpctl.c |  1 +
>      6 files changed, 105 insertions(+), 5 deletions(-)
> 
>     diff --git a/lib/conntrack.c b/lib/conntrack.c
>     index de46a6b..e986115 100644
>     --- a/lib/conntrack.c
>     +++ b/lib/conntrack.c
>     @@ -1931,7 +1931,7 @@ conn_key_to_tuple(const struct conn_key *key,
> struct ct_dpif_tuple *tuple)
> 
>      static void
>      conn_to_ct_dpif_entry(const struct conn *conn, struct ct_dpif_entry
> *entry,
>     -                      long long now)
>     +                      long long now, int bkt)
> 
> The parameters can also include the number of buckets.
> 
> 
>      {
>          struct ct_l4_proto *class;
>          long long expiration;
>     @@ -1954,6 +1954,7 @@ conn_to_ct_dpif_entry(const struct conn *conn,
> struct ct_dpif_entry *entry,
>          if (class->conn_get_protoinfo) {
>              class->conn_get_protoinfo(conn, &entry->protoinfo);
>          }
>     +    entry->bkt = bkt;
>      }
> 
>      int
>     @@ -1991,7 +1992,7 @@ conntrack_dump_next(struct conntrack_dump *dump,
> struct ct_dpif_entry *entry)
>                  INIT_CONTAINER(conn, node, node);
>                  if ((!dump->filter_zone || conn->key.zone == dump->zone)
> &&
>                       (conn->conn_type != CT_CONN_TYPE_UN_NAT)) {
>     -                conn_to_ct_dpif_entry(conn, entry, now);
>     +                conn_to_ct_dpif_entry(conn, entry, now, dump-
> >bucket);
>                      break;
>                  }
>                  /* Else continue, until we find an entry in the
> appropriate zone
>     diff --git a/lib/conntrack.h b/lib/conntrack.h
>     index defde4c..81eb9df 100644
>     --- a/lib/conntrack.h
>     +++ b/lib/conntrack.h
>     @@ -28,6 +28,7 @@
>      #include "ovs-atomic.h"
>      #include "ovs-thread.h"
>      #include "packets.h"
>     +#include "ct-dpif.h"
> 
>      /* Userspace connection tracker
>       * ============================
>     @@ -242,9 +243,6 @@ struct conntrack_bucket {
>          long long next_cleanup OVS_GUARDED;
>      };
> 
>     -#define CONNTRACK_BUCKETS_SHIFT 8
>     -#define CONNTRACK_BUCKETS (1 << CONNTRACK_BUCKETS_SHIFT)
> 
> Please don’t move these defines from the Userspace conntrack module
> itself.
> The associated datastructures are in the Userspace conntrack module itself
> and hence
> the defines belong here.
> 
> The API conn_to_ct_dpif_entry() can also return the number of buckets.
> 
> 
>     -
>      struct conntrack {
>          /* Independent buckets containing the connections */
>          struct conntrack_bucket buckets[CONNTRACK_BUCKETS];
>     diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
>     index cd35f3e..b2a2f9e 100644
>     --- a/lib/ct-dpif.h
>     +++ b/lib/ct-dpif.h
>     @@ -20,6 +20,9 @@
>      #include "openvswitch/types.h"
>      #include "packets.h"
> 
>     +#define CONNTRACK_BUCKETS_SHIFT 8
>     +#define CONNTRACK_BUCKETS (1 << CONNTRACK_BUCKETS_SHIFT)
>     +
>      union ct_dpif_inet_addr {
>          ovs_be32 ip;
>          ovs_be32 ip6[4];
>     @@ -169,6 +172,7 @@ struct ct_dpif_entry {
>          /* Timeout for this entry in seconds */
>          uint32_t timeout;
>          uint32_t mark;
>     +    int bkt; /* Number of CT bucket. */
>      };
> 
>      enum {
>     diff --git a/lib/dpctl.c b/lib/dpctl.c
>     index 6aa6c8e..c3efc5b 100644
>     --- a/lib/dpctl.c
>     +++ b/lib/dpctl.c
>     @@ -1464,6 +1464,93 @@ dpctl_ct_stats_show(int argc, const char
> *argv[],
>          dpif_close(dpif);
>          return error;
>      }
>     +
>     +#define CT_BKTS_GT "gt="
>     +static int
>     +dpctl_ct_bkts(int argc, const char *argv[],
>     +                     struct dpctl_params *dpctl_p)
>     +{
>     +    struct dpif *dpif;
>     +    char *name;
>     +
>     +    struct ct_dpif_dump_state *dump;
>     +    struct ct_dpif_entry cte;
>     +    uint16_t gt = 0; /* Threshold: display value when greater than
> gt. */
>     +    uint16_t *pzone = NULL;
>     +    int lastargc = 0;
>     +
>     +    int conn_per_bkts[CONNTRACK_BUCKETS];
>     +    int error;
>     +
>     +    if (!dpctl_p->is_appctl) {
>     +        /* Not called by ovs-appctl command then exit. */
>     +        dpctl_print(dpctl_p,
>     +            "Command is available for UserSpace ConnTracker
> only.\n");
>     +        return 0;
>     +    }
>     +
>     +    while (argc > 1 && lastargc != argc) {
>     +        lastargc = argc;
>     +        if (!strncmp(argv[argc - 1], CT_BKTS_GT, strlen(CT_BKTS_GT)))
> {
>     +            if (ovs_scan(argv[argc - 1], CT_BKTS_GT"%"SCNu16, &gt)) {
>     +                argc--;
>     +            }
>     +        }
>     +    }
>     +
>     +    name = (argc > 1) ? xstrdup(argv[1]) : get_one_dp(dpctl_p);
>     +    if (!name) {
>     +        return EINVAL;
>     +    }
>     +
>     +    error = parsed_dpif_open(name, false, &dpif);
>     +    free(name);
>     +    if (error) {
>     +        dpctl_error(dpctl_p, error, "opening datapath");
>     +        return error;
>     +    }
>     +
>     +    error = ct_dpif_dump_start(dpif, &dump, pzone);
>     +    if (error) {
>     +        dpctl_error(dpctl_p, error, "starting conntrack dump");
>     +        dpif_close(dpif);
>     +        return error;
>     +    }
>     +
>     +    memset(conn_per_bkts, 0, sizeof(conn_per_bkts));
>     +    int tot_conn = 0;
>     +    while (!ct_dpif_dump_next(dump, &cte)) {
>     +        ct_dpif_entry_uninit(&cte);
>     +        tot_conn++;
>     +        if (cte.bkt < CONNTRACK_BUCKETS) {
>     +            conn_per_bkts[cte.bkt]++;
>     +        } else {
>     +            dpctl_print(dpctl_p, "Bucket nr out of range: %d\n",
> cte.bkt);
>     +        }
>     +    }
>     +
>     +    dpctl_print(dpctl_p, "Current Connections: %d\n", tot_conn);
>     +    dpctl_print(dpctl_p, "|  Buckets  |"
>     +            "         Connections per Buckets         |\n");
>     +    dpctl_print(dpctl_p, "+-----------+"
>     +            "-----------------------------------------+");
>     +#define NUM_BKTS_PER_ROW 8
>     +    for (int i = 0; i < CONNTRACK_BUCKETS; i++) {
>     +        if (i % NUM_BKTS_PER_ROW == 0) {
>     +             dpctl_print(dpctl_p, "\n %3d..%3d   | ",
>     +                     i, i + NUM_BKTS_PER_ROW - 1);
>     +        }
>     +        if (conn_per_bkts[i] > gt) {
>     +            dpctl_print(dpctl_p, "%5d", conn_per_bkts[i]);
>     +        } else {
>     +            dpctl_print(dpctl_p, "%5s", ".");
>     +        }
>     +    }
>     +    dpctl_print(dpctl_p, "\n\n");
>     +    ct_dpif_dump_done(dump);
>     +    dpif_close(dpif);
>     +    return error;
>     +}
>      ?
>      /* Undocumented commands for unit testing. */
> 
>     @@ -1760,6 +1847,7 @@ static const struct dpctl_command all_commands[]
> = {
>          { "flush-conntrack", "[dp] [zone=N]", 0, 2,
> dpctl_flush_conntrack, DP_RW },
>          { "ct-stats-show", "[dp] [zone=N] [verbose]",
>            0, 3, dpctl_ct_stats_show, DP_RO },
>     +    { "ct-bkts", "[dp] [gt=N]", 0, 2, dpctl_ct_bkts, 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 f95d54a..1c77085 100644
>     --- a/lib/dpctl.man
>     +++ b/lib/dpctl.man
>     @@ -228,3 +228,11 @@ Displays the number of connections grouped by
> protocol used by \fIdp\fR.
>      If \fBzone=\fIzone\fR is specified, numbers refer to the connections
> in
>      \fBzone\fR. The \fBverbose\fR option allows to group by connection
> state
>      for each protocol.
>     +.
>     +.TP
>     +\*(DX\fBct\-bkts\fR [\fIdp\fR] [\fBgt=\fIThreshold\fR]
>     +For each ConnTracker bucket, displays the number of connections used
>     +by \fIdp\fR.
>     +If \fBgt=\fIThreshold\fR is specified, bucket numbers are displayed
> when
>     +greater than \fIThreshold\fR.
>     +
>     diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
>     index 7292fca..7b005ac 100644
>     --- a/utilities/ovs-dpctl.c
>     +++ b/utilities/ovs-dpctl.c
>     @@ -202,6 +202,7 @@ usage(void *userdata OVS_UNUSED)
>                     "delete all conntrack entries in ZONE\n"
>                 "  ct-stats-show [DP] [zone=ZONE] [verbose] " \
>                     "CT connections grouped by protocol\n"
>     +           "  ct-bkts [DP] [gt=N] display connections per CT
> bucket\n"
>                 "Each IFACE on add-dp, add-if, and set-if may be followed
> by\n"
>                 "comma-separated options.  See ovs-dpctl(8) for syntax, or
> the\n"
>                 "Interface table in ovs-vswitchd.conf.db(5) for an options
> list.\n"
>     --
>     2.4.11
> 
>     _______________________________________________
>     dev mailing list
>     [email protected]
>     https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__mail.openvswitch.org_mailman_listinfo_ovs-
> 2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-
> uZnsw&m=mpxzw0ihtwMc63o5o0OOvPxnPY4M4Wy_F9JJ_3A5QGo&s=Molb6s0sAKhqp9Id74o8
> zTklq4M-O2e38H5OoddYP_s&e=
> 

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to