| Message ID | 1498220902-27157-3-git-send-email-antonio.fischetti@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Jun 23, 2017 at 01:28:22PM +0100, antonio.fischetti@intel.com wrote: > From: Antonio Fischetti <antonio.fischetti@intel.com> > > 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 <antonio.fischetti@intel.com> > Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> > Co-authored-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> Thanks for the patch! checkpatch reports a few style issues: WARNING: Line length is >79-characters long #145 FILE: lib/dpctl.c:1529: dpctl_print(dpctl_p, "\n %3d..%3d | ", i, i + NUM_BKTS_PER_ROW - 1); ERROR: Inappropriate bracing around statement #147 FILE: lib/dpctl.c:1531: if (conn_per_bkts[i] > gt) Is this concept of buckets one that the kernel conntracker has too? If so, then I'm concerned about the definition of conn_per_buckets[] in dpctl_ct_bkts(), since it has a hardcoded CONNTRACK_BUCKETS size and nothing checks whether cte.bkt is in the right range. If not, then should this command be one that is limited to the userspace conntracker? I'd update NEWS to describe the new command. Thanks, Ben.
Thanks Ben for your feedback, my replies inline. > -----Original Message----- > From: Ben Pfaff [mailto:blp@ovn.org] > Sent: Tuesday, July 11, 2017 8:54 PM > To: Fischetti, Antonio <antonio.fischetti@intel.com> > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH 3/3] dpctl: Add new 'ct-bkts' command. > > On Fri, Jun 23, 2017 at 01:28:22PM +0100, antonio.fischetti@intel.com > wrote: > > From: Antonio Fischetti <antonio.fischetti@intel.com> > > > > 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 <antonio.fischetti@intel.com> > > Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> > > Co-authored-by: Bhanuprakash Bodireddy > <bhanuprakash.bodireddy@intel.com> > > Thanks for the patch! > > checkpatch reports a few style issues: > [AF] my bad, will fix > WARNING: Line length is >79-characters long > #145 FILE: lib/dpctl.c:1529: > dpctl_print(dpctl_p, "\n %3d..%3d | ", i, i + > NUM_BKTS_PER_ROW - 1); > > ERROR: Inappropriate bracing around statement > #147 FILE: lib/dpctl.c:1531: > if (conn_per_bkts[i] > gt) > > > Is this concept of buckets one that the kernel conntracker has too? If > so, then I'm concerned about the definition of conn_per_buckets[] in > dpctl_ct_bkts(), since it has a hardcoded CONNTRACK_BUCKETS size and > nothing checks whether cte.bkt is in the right range. If not, then > should this command be one that is limited to the userspace conntracker? [AF] You're right, I didn't realize the concept of buckets is in the kernel CT too, I did this implementation with the userspace ConnTracker in mind. Would it be ok to have a first implementation limited to the userspace CT like: dpctl_ct_bkts(...) { if (!dpctl_p->is_appctl) { /* Not called by ovs-appctl command => exit. */ dpctl_print(dpctl_p, "Command is available for UserSpace ConnTracker only.\n"); return 0; } < implementation for Userspace CT here > ... } so that later on it could be extended to cover also the kernel CT case? > > I'd update NEWS to describe the new command. > > Thanks, > > Ben.
On Fri, Jul 14, 2017 at 10:11:04AM +0000, Fischetti, Antonio wrote: > Thanks Ben for your feedback, my replies inline. > > > -----Original Message----- > > From: Ben Pfaff [mailto:blp@ovn.org] > > Sent: Tuesday, July 11, 2017 8:54 PM > > To: Fischetti, Antonio <antonio.fischetti@intel.com> > > Cc: dev@openvswitch.org > > Subject: Re: [ovs-dev] [PATCH 3/3] dpctl: Add new 'ct-bkts' command. > > > > On Fri, Jun 23, 2017 at 01:28:22PM +0100, antonio.fischetti@intel.com > > wrote: > > > From: Antonio Fischetti <antonio.fischetti@intel.com> > > > > > > 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 <antonio.fischetti@intel.com> > > > Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> > > > Co-authored-by: Bhanuprakash Bodireddy > > <bhanuprakash.bodireddy@intel.com> > > > > Is this concept of buckets one that the kernel conntracker has too? If > > so, then I'm concerned about the definition of conn_per_buckets[] in > > dpctl_ct_bkts(), since it has a hardcoded CONNTRACK_BUCKETS size and > > nothing checks whether cte.bkt is in the right range. If not, then > > should this command be one that is limited to the userspace conntracker? > > [AF] You're right, I didn't realize the concept of buckets > is in the kernel CT too, I did this implementation with the > userspace ConnTracker in mind. > > Would it be ok to have a first implementation limited to the userspace CT like: > > dpctl_ct_bkts(...) > { > if (!dpctl_p->is_appctl) { /* Not called by ovs-appctl command => exit. */ > dpctl_print(dpctl_p, "Command is available for UserSpace ConnTracker only.\n"); > return 0; > } > > < implementation for Userspace CT here > > ... > > } > > so that later on it could be extended to cover also the kernel CT case? Is it much work to extend it to cover the kernel? If not, it would be better to cover both. Otherwise, sure, this approach is an OK way to start.
> -----Original Message----- > From: Ben Pfaff [mailto:blp@ovn.org] > Sent: Friday, July 14, 2017 7:29 PM > To: Fischetti, Antonio <antonio.fischetti@intel.com> > Cc: CC: <dev@openvswitch.org> > Subject: Re: [ovs-dev] [PATCH 3/3] dpctl: Add new 'ct-bkts' command. > > On Fri, Jul 14, 2017 at 10:11:04AM +0000, Fischetti, Antonio wrote: > > Thanks Ben for your feedback, my replies inline. > > > > > -----Original Message----- > > > From: Ben Pfaff [mailto:blp@ovn.org] > > > Sent: Tuesday, July 11, 2017 8:54 PM > > > To: Fischetti, Antonio <antonio.fischetti@intel.com> > > > Cc: dev@openvswitch.org > > > Subject: Re: [ovs-dev] [PATCH 3/3] dpctl: Add new 'ct-bkts' command. > > > > > > On Fri, Jun 23, 2017 at 01:28:22PM +0100, antonio.fischetti@intel.com > > > wrote: > > > > From: Antonio Fischetti <antonio.fischetti@intel.com> > > > > > > > > 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 <antonio.fischetti@intel.com> > > > > Signed-off-by: Bhanuprakash Bodireddy > <bhanuprakash.bodireddy@intel.com> > > > > Co-authored-by: Bhanuprakash Bodireddy > > > <bhanuprakash.bodireddy@intel.com> > > > > > > Is this concept of buckets one that the kernel conntracker has too? > If > > > so, then I'm concerned about the definition of conn_per_buckets[] in > > > dpctl_ct_bkts(), since it has a hardcoded CONNTRACK_BUCKETS size and > > > nothing checks whether cte.bkt is in the right range. If not, then > > > should this command be one that is limited to the userspace > conntracker? > > > > [AF] You're right, I didn't realize the concept of buckets > > is in the kernel CT too, I did this implementation with the > > userspace ConnTracker in mind. > > > > Would it be ok to have a first implementation limited to the userspace > CT like: > > > > dpctl_ct_bkts(...) > > { > > if (!dpctl_p->is_appctl) { /* Not called by ovs-appctl command => > exit. */ > > dpctl_print(dpctl_p, "Command is available for UserSpace > ConnTracker only.\n"); > > return 0; > > } > > > > < implementation for Userspace CT here > > > ... > > > > } > > > > so that later on it could be extended to cover also the kernel CT case? > > Is it much work to extend it to cover the kernel? If not, it would be > better to cover both. Otherwise, sure, this approach is an OK way to > start. [AF] I'd prefer to start with an implementation for userspace only. I did some investigation on how to cover the kernel case too but - as I'm not enough familiar with the details of the kernel CT implementation - I'd limit this patch to the userspace CT for now. I'll post a v2 soon. Thanks, Antonio
diff --git a/lib/conntrack.c b/lib/conntrack.c index c62ff2a..a1ea350 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -1927,7 +1927,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) { struct ct_l4_proto *class; long long expiration; @@ -1950,6 +1950,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 @@ -1987,7 +1988,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 243aebb..fe1b21d 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) - 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 4cca9c8..3081b0e 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -1443,6 +1443,80 @@ 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; + + 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, >)) { + 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++; + conn_per_bkts[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. */ @@ -1739,6 +1813,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 cccc574..6930060 100644 --- a/lib/dpctl.man +++ b/lib/dpctl.man @@ -227,3 +227,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 ea75c0c..2707f76 100644 --- a/utilities/ovs-dpctl.c +++ b/utilities/ovs-dpctl.c @@ -183,6 +183,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"