| Message ID | 1534272992-31756-11-git-send-email-yihung.wei@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | conntrack zone limitation | expand |
Bleep bloop. Greetings Yi-Hung Wei, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 115 characters long (recommended limit is 79) #331 FILE: lib/dpctl.man:277: \*(DX\fBct\-set\-limits\fR [\fIdp\fR] [\fBdefault=\fIdefault_limit\fR] [\fBzone=\fIzone\fR,\fBlimit=\fIlimit\fR]... Lines checked: 350, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@bytheb.org Thanks, 0-day Robot
> On Aug 14, 2018, at 11:56 AM, Yi-Hung Wei <yihung.wei@gmail.com> wrote: > > > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c > index a772799fe347..bb809d9920b5 100644 > --- a/lib/ct-dpif.c > +++ b/lib/ct-dpif.c > @@ -629,3 +629,70 @@ ct_dpif_free_zone_limits(struct ovs_list *zone_limits) > ... > +/* Parses a specification of a conntrack zone limit from 's' into '*pzone' > + * and '*plimit'. Returns true on success. Otherwise, returns false and > + * and puts the error message in 'ds'. */ > +bool > +ct_dpif_parse_zone_limit_tuple(const char *s, uint16_t *pzone, > + uint32_t *plimit, struct ds *ds) > +{ > ... > + if (parsed_zone == false || parsed_limit == false) { This is extremely minor, but I think we would more commonly write this as: if (!parsed_zone || !parsed_limit) { > +void > +ct_dpif_format_zone_limits(uint32_t default_limit, > + const struct ovs_list *zone_limits, struct ds *ds) > +{ > + struct ct_dpif_zone_limit *zone_limit; > + > + ds_put_format(ds, "default_limit=%"PRIu32, default_limit); I would just drop the "_", since this is meant to be human-readable. (If you intend to have the output readable as an argument to "ct-set-limits", then the formats are different, since that just takes "default=".) > + > + LIST_FOR_EACH (zone_limit, node, zone_limits) { > + ds_put_format(ds, " zone=%"PRIu16, zone_limit->zone); > + ds_put_format(ds, ",limit=%"PRIu32, zone_limit->limit); > + ds_put_format(ds, ",count=%"PRIu32, zone_limit->count); > + } I find it hard to parse all of this on a single line. What about putting each zone on its own line? > diff --git a/lib/dpctl.c b/lib/dpctl.c > index 63cc6eb0a289..076703157eae 100644 > --- a/lib/dpctl.c > +++ b/lib/dpctl.c > @@ -199,7 +199,7 @@ parsed_dpif_open(const char *arg_, bool create, struct dpif **dpifp) > * to be parsed in '*indexp'. */ > static int > opt_dpif_open(int argc, const char *argv[], struct dpctl_params *dpctl_p, > - uint8_t max_args, struct dpif **dpifp, bool multi_opt, > + int max_args, struct dpif **dpifp, bool multi_opt, > int *indexp) > { Assuming you still need the previous patch, I suspect this change should go in there. > diff --git a/lib/dpctl.c b/lib/dpctl.c > index 63cc6eb0a289..076703157eae 100644 > --- a/lib/dpctl.c > +++ b/lib/dpctl.c > > +static int > +dpctl_ct_set_limits(int argc, const char *argv[], > + struct dpctl_params *dpctl_p) > +{ > + struct dpif *dpif; > + struct ds ds = DS_EMPTY_INITIALIZER; > + int error, i = 1; > + uint32_t default_limit, *p_default_limit = NULL; > + struct ovs_list list = OVS_LIST_INITIALIZER(&list); Minor, but I think "zone_limits" would be a more consistent and clearer name than "list". > +static int > +parse_ct_limit_zones(const char *argv, struct ovs_list *list, struct ds *ds) > +{ > + char *save_ptr = NULL, *argcopy, *next_zone; > + uint16_t zone; > + > + if (strncmp(argv, "zone=", 5)) { > + ds_put_format(ds, "invalid argument %s", argv); > + return EINVAL; > + } > + > + argcopy = xstrdup(argv + 5); > + next_zone = strtok_r(argcopy, ",", &save_ptr); > + > + do { > + if (ovs_scan(next_zone, "%"SCNu16, &zone)) { > + ct_dpif_push_zone_limit(list, zone, 0, 0); > + } else { > + ds_put_cstr(ds, "invalid zone"); > + return EINVAL; Do you need to free "argcopy"? > diff --git a/lib/dpctl.man b/lib/dpctl.man > index 5d987e62daaa..deb1bd32a34b 100644 > --- a/lib/dpctl.man > +++ b/lib/dpctl.man > @@ -272,3 +272,21 @@ Only supported for userspace datapath. > \*(DX\fBct\-get\-nconns\fR [\fIdp\fR] > Prints the current number of connection tracker entries on \fIdp\fR. > Only supported for userspace datapath. > +. > +.TP These are in the "CONNECTION TRACKING TABLE DEBUGGING COMMANDS" section, which doesn't seem quite right. It may be worth dropping the "DEBUGGING" word, since these commands and "ct-set-maxconns" aren't just for debugging. > +\*(DX\fBct\-set\-limits\fR [\fIdp\fR] [\fBdefault=\fIdefault_limit\fR] [\fBzone=\fIzone\fR,\fBlimit=\fIlimit\fR]... > +Sets the maximum allowed number of connections in connection tracking zones. > +If a per zone limit for a particular zone is not available in the datapath, > +it defaults to the default per zone limit. Initially, the default per zone > +limit is unlimited(0). > +. > +.TP > +\*(DX\fBct\-del\-limits\fR [\fIdp\fR] \fBzone=\fIzone[,zone]\fR... > +Deletes the per zone limit on particular zones. The \fIzone\fR must be a > +comma-separated list. > +. > +.TP > +\*(DX\fBct\-get\-limits\fR [\fIdp\fR] [\fBzone=\fIzone[,zone]\fR...] > +Retrieves the maximum allowed number of connections. The \fIzone\fR must > +be a comma-separated list. Prints all zone limits if no \fIzone\fR is > +provided. I think these could use a bit more description. I took a stab at it with the incremental at the end. Let me know what you think. By the way, I think it's worth update the FAQ (Documentation/faq/releases.rst) to indicate on what datapaths and kernel versions this is supported. Thanks, --Justin -=-=-=-=-=-=-=- diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c index d3e8c936d7e6..4d6cdb5951c5 100644 --- a/lib/ct-dpif.c +++ b/lib/ct-dpif.c @@ -657,7 +657,7 @@ ct_dpif_parse_zone_limit_tuple(const char *s, uint16_t *pzone, } } - if (parsed_zone == false || parsed_limit == false) { + if (!parsed_zone || !parsed_limit) { ds_put_format(ds, "failed to parse zone limit"); goto error; } diff --git a/lib/dpctl.c b/lib/dpctl.c index 46634f67c6aa..8c625c2a9b2a 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -1712,11 +1712,11 @@ dpctl_ct_set_limits(int argc, const char *argv[], { struct dpif *dpif; struct ds ds = DS_EMPTY_INITIALIZER; - int error, i = 1; + int i = 1; uint32_t default_limit, *p_default_limit = NULL; - struct ovs_list list = OVS_LIST_INITIALIZER(&list); + struct ovs_list zone_limits = OVS_LIST_INITIALIZER(&zone_limits); - error = opt_dpif_open(argc, argv, dpctl_p, INT_MAX, &dpif, true, &i); + int error = opt_dpif_open(argc, argv, dpctl_p, INT_MAX, &dpif, true, &i); if (error) { return error; } @@ -1741,12 +1741,12 @@ dpctl_ct_set_limits(int argc, const char *argv[], error = EINVAL; goto error; } - ct_dpif_push_zone_limit(&list, zone, limit, 0); + ct_dpif_push_zone_limit(&zone_limits, zone, limit, 0); } - error = ct_dpif_set_limits(dpif, p_default_limit, &list); + error = ct_dpif_set_limits(dpif, p_default_limit, &zone_limits); if (!error) { - ct_dpif_free_zone_limits(&list); + ct_dpif_free_zone_limits(&zone_limits); dpif_close(dpif); return 0; } else { @@ -1756,13 +1756,14 @@ dpctl_ct_set_limits(int argc, const char *argv[], error: dpctl_error(dpctl_p, error, "%s", ds_cstr(&ds)); ds_destroy(&ds); - ct_dpif_free_zone_limits(&list); + ct_dpif_free_zone_limits(&zone_limits); dpif_close(dpif); return error; } static int -parse_ct_limit_zones(const char *argv, struct ovs_list *list, struct ds *ds) +parse_ct_limit_zones(const char *argv, struct ovs_list *zone_limits, + struct ds *ds) { char *save_ptr = NULL, *argcopy, *next_zone; uint16_t zone; @@ -1777,9 +1778,10 @@ parse_ct_limit_zones(const char *argv, struct ovs_list *list, struct ds *ds) do { if (ovs_scan(next_zone, "%"SCNu16, &zone)) { - ct_dpif_push_zone_limit(list, zone, 0, 0); + ct_dpif_push_zone_limit(zone_limits, zone, 0, 0); } else { ds_put_cstr(ds, "invalid zone"); + free(argcopy); return EINVAL; } } while ((next_zone = strtok_r(NULL, ",", &save_ptr)) != NULL); @@ -1795,19 +1797,19 @@ dpctl_ct_del_limits(int argc, const char *argv[], struct dpif *dpif; struct ds ds = DS_EMPTY_INITIALIZER; int error, i = 1; - struct ovs_list list = OVS_LIST_INITIALIZER(&list); + struct ovs_list zone_limits = OVS_LIST_INITIALIZER(&zone_limits); error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif, true, &i); if (error) { return error; } - error = parse_ct_limit_zones(argv[i], &list, &ds); + error = parse_ct_limit_zones(argv[i], &zone_limits, &ds); if (error) { goto error; } - error = ct_dpif_del_limits(dpif, &list); + error = ct_dpif_del_limits(dpif, &zone_limits); if (!error) { goto out; } else { @@ -1818,7 +1820,7 @@ error: dpctl_error(dpctl_p, error, "%s", ds_cstr(&ds)); ds_destroy(&ds); out: - ct_dpif_free_zone_limits(&list); + ct_dpif_free_zone_limits(&zone_limits); dpif_close(dpif); return error; } @@ -1830,11 +1832,11 @@ dpctl_ct_get_limits(int argc, const char *argv[], struct dpif *dpif; struct ds ds = DS_EMPTY_INITIALIZER; uint32_t default_limit; - int error, i = 1; + int i = 1; struct ovs_list list_query = OVS_LIST_INITIALIZER(&list_query); struct ovs_list list_reply = OVS_LIST_INITIALIZER(&list_reply); - error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif, true, &i); + int error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif, true, &i); if (error) { return error; } diff --git a/lib/dpctl.man b/lib/dpctl.man index deb1bd32a34b..eb59d1209999 100644 --- a/lib/dpctl.man +++ b/lib/dpctl.man @@ -275,18 +275,25 @@ Only supported for userspace datapath. . .TP \*(DX\fBct\-set\-limits\fR [\fIdp\fR] [\fBdefault=\fIdefault_limit\fR] [\fBzone=\fIzone\fR,\fBlimit=\fIlimit\fR]... -Sets the maximum allowed number of connections in connection tracking zones. -If a per zone limit for a particular zone is not available in the datapath, -it defaults to the default per zone limit. Initially, the default per zone -limit is unlimited(0). +Sets the maximum allowed number of connections in a connection tracking +zone. A specific \fIzone\fR may be set to \fIlimit\fR, and multiple zones +may be specified with a comma-separated list. If a per-zone limit for a +particular zone is not specified in the datapath, it defaults to the +default per-zone limit. A default zone may be specified with the +\fBdefault=\fIdefault_limit\fR argument. Initially, the default +per-zone limit is unlimited. An unlimited number of entries may be set +with \fB0\fR limit. Only supported for Linux kernel datapath. . .TP \*(DX\fBct\-del\-limits\fR [\fIdp\fR] \fBzone=\fIzone[,zone]\fR... -Deletes the per zone limit on particular zones. The \fIzone\fR must be a -comma-separated list. +Deletes the connection tracking limit for \fIzone\fR. Multiple zones may +be specified with a comma-separated list. Only supported for Linux +kernel datapath. . .TP -\*(DX\fBct\-get\-limits\fR [\fIdp\fR] [\fBzone=\fIzone[,zone]\fR...] -Retrieves the maximum allowed number of connections. The \fIzone\fR must -be a comma-separated list. Prints all zone limits if no \fIzone\fR is -provided. +\*(DX\fBct\-get\-limits\fR [\fIdp\fR] [\fBzone=\fIzone\fR[\fB,\fIzone\fR]...] +Retrieves the maximum allowed number of connections and current +counts per-zone. If \fIzone\fR is given, only the specified zone(s) are +printed. If no zones are specified, all the zone limits and counts are +provided. The command always displays the default zone limit. Only +supported for Linux kernel datapath.
On Thu, Aug 16, 2018 at 6:24 PM Justin Pettit <jpettit@ovn.org> wrote: > > On Aug 14, 2018, at 11:56 AM, Yi-Hung Wei <yihung.wei@gmail.com> wrote: > > > > > > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c > > index a772799fe347..bb809d9920b5 100644 > > --- a/lib/ct-dpif.c > > +++ b/lib/ct-dpif.c > > +void > > +ct_dpif_format_zone_limits(uint32_t default_limit, > > + const struct ovs_list *zone_limits, struct ds *ds) > > +{ > > + struct ct_dpif_zone_limit *zone_limit; > > + > > + ds_put_format(ds, "default_limit=%"PRIu32, default_limit); > > I would just drop the "_", since this is meant to be human-readable. (If you intend to have the output readable as an argument to "ct-set-limits", then the formats are different, since that just takes "default=".) Thanks, I will drop the "_" in v4. > > + > > + LIST_FOR_EACH (zone_limit, node, zone_limits) { > > + ds_put_format(ds, " zone=%"PRIu16, zone_limit->zone); > > + ds_put_format(ds, ",limit=%"PRIu32, zone_limit->limit); > > + ds_put_format(ds, ",count=%"PRIu32, zone_limit->count); > > + } > > I find it hard to parse all of this on a single line. What about putting each zone on its own line? Sure. I will put default limit, and every zone in different lines. > > diff --git a/lib/dpctl.c b/lib/dpctl.c > > index 63cc6eb0a289..076703157eae 100644 > > --- a/lib/dpctl.c > > +++ b/lib/dpctl.c > > @@ -199,7 +199,7 @@ parsed_dpif_open(const char *arg_, bool create, struct dpif **dpifp) > > * to be parsed in '*indexp'. */ > > static int > > opt_dpif_open(int argc, const char *argv[], struct dpctl_params *dpctl_p, > > - uint8_t max_args, struct dpif **dpifp, bool multi_opt, > > + int max_args, struct dpif **dpifp, bool multi_opt, > > int *indexp) > > { > > Assuming you still need the previous patch, I suspect this change should go in there. Yes, I fold in this change in the previous patch. > > diff --git a/lib/dpctl.man b/lib/dpctl.man > > index 5d987e62daaa..deb1bd32a34b 100644 > > --- a/lib/dpctl.man > > +++ b/lib/dpctl.man > > @@ -272,3 +272,21 @@ Only supported for userspace datapath. > > \*(DX\fBct\-get\-nconns\fR [\fIdp\fR] > > Prints the current number of connection tracker entries on \fIdp\fR. > > Only supported for userspace datapath. > > +. > > +.TP > > These are in the "CONNECTION TRACKING TABLE DEBUGGING COMMANDS" section, which doesn't seem quite right. It may be worth dropping the "DEBUGGING" word, since these commands and "ct-set-maxconns" aren't just for debugging. That make sense. I make the following change in v4. --- a/lib/dpctl.man +++ b/lib/dpctl.man @@ -196,9 +196,9 @@ Fetches the flow from \fIdp\fR's flow table with unique identifier \fIufid\fR. . .IP "\*(DX\fBdel\-flows\fR [\fIdp\fR]" Deletes all flow entries from datapath \fIdp\fR's flow table. -.SS "CONNECTION TRACKING TABLE DEBUGGING COMMANDS" -The following commands are primarily useful for debugging the connection -tracking entries in the datapath. +.SS "CONNECTION TRACKING TABLE COMMANDS" +The following commands are useful for debugging and configuring +the connection tracking table in the datapath. . .PP > By the way, I think it's worth update the FAQ (Documentation/faq/releases.rst) to indicate on what datapaths and kernel versions this is supported. Sure, I add the following line in FAQ. diff --git a/Documentation/faq/releases.rst b/Documentation/faq/releases.rst index 54c4b54b1ea0..e64fa22c4034 100644 --- a/Documentation/faq/releases.rst +++ b/Documentation/faq/releases.rst @@ -125,6 +125,7 @@ Q: Are all features available with all datapaths? NIC Bonding YES YES YES YES Multiple VTEPs YES YES YES YES Meters 4.15 YES YES NO + Conntrack zone limit 4.18 YES NO NO ===================== ============== ============== ========= ======= Do note, however: > > Thanks, > > --Justin Thanks. I fold in all the following changes and updates in v4. > > > -=-=-=-=-=-=-=- > > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c > index d3e8c936d7e6..4d6cdb5951c5 100644 > --- a/lib/ct-dpif.c > +++ b/lib/ct-dpif.c > @@ -657,7 +657,7 @@ ct_dpif_parse_zone_limit_tuple(const char *s, uint16_t *pzone, > } > } > > - if (parsed_zone == false || parsed_limit == false) { > + if (!parsed_zone || !parsed_limit) { > ds_put_format(ds, "failed to parse zone limit"); > goto error; > } > diff --git a/lib/dpctl.c b/lib/dpctl.c > index 46634f67c6aa..8c625c2a9b2a 100644 > --- a/lib/dpctl.c > +++ b/lib/dpctl.c > @@ -1712,11 +1712,11 @@ dpctl_ct_set_limits(int argc, const char *argv[], > { > struct dpif *dpif; > struct ds ds = DS_EMPTY_INITIALIZER; > - int error, i = 1; > + int i = 1; > uint32_t default_limit, *p_default_limit = NULL; > - struct ovs_list list = OVS_LIST_INITIALIZER(&list); > + struct ovs_list zone_limits = OVS_LIST_INITIALIZER(&zone_limits); > > - error = opt_dpif_open(argc, argv, dpctl_p, INT_MAX, &dpif, true, &i); > + int error = opt_dpif_open(argc, argv, dpctl_p, INT_MAX, &dpif, true, &i); > if (error) { > return error; > } > @@ -1741,12 +1741,12 @@ dpctl_ct_set_limits(int argc, const char *argv[], > error = EINVAL; > goto error; > } > - ct_dpif_push_zone_limit(&list, zone, limit, 0); > + ct_dpif_push_zone_limit(&zone_limits, zone, limit, 0); > } > > - error = ct_dpif_set_limits(dpif, p_default_limit, &list); > + error = ct_dpif_set_limits(dpif, p_default_limit, &zone_limits); > if (!error) { > - ct_dpif_free_zone_limits(&list); > + ct_dpif_free_zone_limits(&zone_limits); > dpif_close(dpif); > return 0; > } else { > @@ -1756,13 +1756,14 @@ dpctl_ct_set_limits(int argc, const char *argv[], > error: > dpctl_error(dpctl_p, error, "%s", ds_cstr(&ds)); > ds_destroy(&ds); > - ct_dpif_free_zone_limits(&list); > + ct_dpif_free_zone_limits(&zone_limits); > dpif_close(dpif); > return error; > } > > static int > -parse_ct_limit_zones(const char *argv, struct ovs_list *list, struct ds *ds) > +parse_ct_limit_zones(const char *argv, struct ovs_list *zone_limits, > + struct ds *ds) > { > char *save_ptr = NULL, *argcopy, *next_zone; > uint16_t zone; > @@ -1777,9 +1778,10 @@ parse_ct_limit_zones(const char *argv, struct ovs_list *list, struct ds *ds) > > do { > if (ovs_scan(next_zone, "%"SCNu16, &zone)) { > - ct_dpif_push_zone_limit(list, zone, 0, 0); > + ct_dpif_push_zone_limit(zone_limits, zone, 0, 0); > } else { > ds_put_cstr(ds, "invalid zone"); > + free(argcopy); > return EINVAL; > } > } while ((next_zone = strtok_r(NULL, ",", &save_ptr)) != NULL); > @@ -1795,19 +1797,19 @@ dpctl_ct_del_limits(int argc, const char *argv[], > struct dpif *dpif; > struct ds ds = DS_EMPTY_INITIALIZER; > int error, i = 1; > - struct ovs_list list = OVS_LIST_INITIALIZER(&list); > + struct ovs_list zone_limits = OVS_LIST_INITIALIZER(&zone_limits); > > error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif, true, &i); > if (error) { > return error; > } > > - error = parse_ct_limit_zones(argv[i], &list, &ds); > + error = parse_ct_limit_zones(argv[i], &zone_limits, &ds); > if (error) { > goto error; > } > > - error = ct_dpif_del_limits(dpif, &list); > + error = ct_dpif_del_limits(dpif, &zone_limits); > if (!error) { > goto out; > } else { > @@ -1818,7 +1820,7 @@ error: > dpctl_error(dpctl_p, error, "%s", ds_cstr(&ds)); > ds_destroy(&ds); > out: > - ct_dpif_free_zone_limits(&list); > + ct_dpif_free_zone_limits(&zone_limits); > dpif_close(dpif); > return error; > } > @@ -1830,11 +1832,11 @@ dpctl_ct_get_limits(int argc, const char *argv[], > struct dpif *dpif; > struct ds ds = DS_EMPTY_INITIALIZER; > uint32_t default_limit; > - int error, i = 1; > + int i = 1; > struct ovs_list list_query = OVS_LIST_INITIALIZER(&list_query); > struct ovs_list list_reply = OVS_LIST_INITIALIZER(&list_reply); > > - error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif, true, &i); > + int error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif, true, &i); > if (error) { > return error; > } > diff --git a/lib/dpctl.man b/lib/dpctl.man > index deb1bd32a34b..eb59d1209999 100644 > --- a/lib/dpctl.man > +++ b/lib/dpctl.man > @@ -275,18 +275,25 @@ Only supported for userspace datapath. > . > .TP > \*(DX\fBct\-set\-limits\fR [\fIdp\fR] [\fBdefault=\fIdefault_limit\fR] [\fBzone=\fIzone\fR,\fBlimit=\fIlimit\fR]... > -Sets the maximum allowed number of connections in connection tracking zones. > -If a per zone limit for a particular zone is not available in the datapath, > -it defaults to the default per zone limit. Initially, the default per zone > -limit is unlimited(0). > +Sets the maximum allowed number of connections in a connection tracking > +zone. A specific \fIzone\fR may be set to \fIlimit\fR, and multiple zones > +may be specified with a comma-separated list. If a per-zone limit for a > +particular zone is not specified in the datapath, it defaults to the > +default per-zone limit. A default zone may be specified with the > +\fBdefault=\fIdefault_limit\fR argument. Initially, the default > +per-zone limit is unlimited. An unlimited number of entries may be set > +with \fB0\fR limit. Only supported for Linux kernel datapath. > . > .TP > \*(DX\fBct\-del\-limits\fR [\fIdp\fR] \fBzone=\fIzone[,zone]\fR... > -Deletes the per zone limit on particular zones. The \fIzone\fR must be a > -comma-separated list. > +Deletes the connection tracking limit for \fIzone\fR. Multiple zones may > +be specified with a comma-separated list. Only supported for Linux > +kernel datapath. > . > .TP > -\*(DX\fBct\-get\-limits\fR [\fIdp\fR] [\fBzone=\fIzone[,zone]\fR...] > -Retrieves the maximum allowed number of connections. The \fIzone\fR must > -be a comma-separated list. Prints all zone limits if no \fIzone\fR is > -provided. > +\*(DX\fBct\-get\-limits\fR [\fIdp\fR] [\fBzone=\fIzone\fR[\fB,\fIzone\fR]...] > +Retrieves the maximum allowed number of connections and current > +counts per-zone. If \fIzone\fR is given, only the specified zone(s) are > +printed. If no zones are specified, all the zone limits and counts are > +provided. The command always displays the default zone limit. Only > +supported for Linux kernel datapath.
diff --git a/NEWS b/NEWS index 94a7e9e32c09..a9a1026901f8 100644 --- a/NEWS +++ b/NEWS @@ -21,6 +21,8 @@ v2.10.0 - xx xxx xxxx default it always accepts names and in interactive use it displays them; use --names or --no-names to override. See ovs-ofctl(8) for details. - ovs-vsctl: New commands "add-bond-iface" and "del-bond-iface". + - ovs-dpctl: + * New commands "ct-set-limits", "ct-del-limits", and "ct-get-limits". - OpenFlow: * OFPT_ROLE_STATUS is now available in OpenFlow 1.3. * OpenFlow 1.5 extensible statistics (OXS) now implemented. diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c index a772799fe347..bb809d9920b5 100644 --- a/lib/ct-dpif.c +++ b/lib/ct-dpif.c @@ -629,3 +629,70 @@ ct_dpif_free_zone_limits(struct ovs_list *zone_limits) free(p); } } + +/* Parses a specification of a conntrack zone limit from 's' into '*pzone' + * and '*plimit'. Returns true on success. Otherwise, returns false and + * and puts the error message in 'ds'. */ +bool +ct_dpif_parse_zone_limit_tuple(const char *s, uint16_t *pzone, + uint32_t *plimit, struct ds *ds) +{ + char *pos, *key, *value, *copy, *err; + bool parsed_limit = false, parsed_zone = false; + + pos = copy = xstrdup(s); + while (ofputil_parse_key_value(&pos, &key, &value)) { + if (!*value) { + ds_put_format(ds, "field %s missing value", key); + goto error; + } + + if (!strcmp(key, "zone")) { + err = str_to_u16(value, key, pzone); + if (err) { + free(err); + goto error_with_msg; + } + parsed_zone = true; + } else if (!strcmp(key, "limit")) { + err = str_to_u32(value, plimit); + if (err) { + free(err); + goto error_with_msg; + } + parsed_limit = true; + } else { + ds_put_format(ds, "invalid zone limit field: %s", key); + goto error; + } + } + + if (parsed_zone == false || parsed_limit == false) { + ds_put_format(ds, "failed to parse zone limit"); + goto error; + } + + free(copy); + return true; + +error_with_msg: + ds_put_format(ds, "failed to parse field %s", key); +error: + free(copy); + return false; +} + +void +ct_dpif_format_zone_limits(uint32_t default_limit, + const struct ovs_list *zone_limits, struct ds *ds) +{ + struct ct_dpif_zone_limit *zone_limit; + + ds_put_format(ds, "default_limit=%"PRIu32, default_limit); + + LIST_FOR_EACH (zone_limit, node, zone_limits) { + ds_put_format(ds, " zone=%"PRIu16, zone_limit->zone); + ds_put_format(ds, ",limit=%"PRIu32, zone_limit->limit); + ds_put_format(ds, ",count=%"PRIu32, zone_limit->count); + } +} diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h index c80e18b72b56..c9cfb258b133 100644 --- a/lib/ct-dpif.h +++ b/lib/ct-dpif.h @@ -223,5 +223,9 @@ void ct_dpif_push_zone_limit(struct ovs_list *, uint16_t zone, uint32_t limit, uint32_t count); struct ct_dpif_zone_limit * ct_dpif_pop_zone_limit(struct ovs_list *); void ct_dpif_free_zone_limits(struct ovs_list *); +bool ct_dpif_parse_zone_limit_tuple(const char *s, uint16_t *pzone, + uint32_t *plimit, struct ds *); +void ct_dpif_format_zone_limits(uint32_t default_limit, + const struct ovs_list *, struct ds *); #endif /* CT_DPIF_H */ diff --git a/lib/dpctl.c b/lib/dpctl.c index 63cc6eb0a289..076703157eae 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -199,7 +199,7 @@ parsed_dpif_open(const char *arg_, bool create, struct dpif **dpifp) * to be parsed in '*indexp'. */ static int opt_dpif_open(int argc, const char *argv[], struct dpctl_params *dpctl_p, - uint8_t max_args, struct dpif **dpifp, bool multi_opt, + int max_args, struct dpif **dpifp, bool multi_opt, int *indexp) { char *dpname; @@ -1680,6 +1680,167 @@ dpctl_ct_get_nconns(int argc, const char *argv[], return error; } +static int +dpctl_ct_set_limits(int argc, const char *argv[], + struct dpctl_params *dpctl_p) +{ + struct dpif *dpif; + struct ds ds = DS_EMPTY_INITIALIZER; + int error, i = 1; + uint32_t default_limit, *p_default_limit = NULL; + struct ovs_list list = OVS_LIST_INITIALIZER(&list); + + error = opt_dpif_open(argc, argv, dpctl_p, INT_MAX, &dpif, true, &i); + if (error) { + return error; + } + + /* Parse default limit */ + if (!strncmp(argv[i], "default=", 8)) { + if (ovs_scan(argv[i], "default=%"SCNu32, &default_limit)) { + p_default_limit = &default_limit; + i++; + } else { + ds_put_cstr(&ds, "invalid default limit"); + error = EINVAL; + goto error; + } + } + + /* Parse ct zone limit tuples */ + while (i < argc) { + uint16_t zone; + uint32_t limit; + if (!ct_dpif_parse_zone_limit_tuple(argv[i++], &zone, &limit, &ds)) { + error = EINVAL; + goto error; + } + ct_dpif_push_zone_limit(&list, zone, limit, 0); + } + + error = ct_dpif_set_limits(dpif, p_default_limit, &list); + if (!error) { + ct_dpif_free_zone_limits(&list); + dpif_close(dpif); + return 0; + } else { + ds_put_cstr(&ds, "failed to set conntrack limit"); + } + +error: + dpctl_error(dpctl_p, error, "%s", ds_cstr(&ds)); + ds_destroy(&ds); + ct_dpif_free_zone_limits(&list); + dpif_close(dpif); + return error; +} + +static int +parse_ct_limit_zones(const char *argv, struct ovs_list *list, struct ds *ds) +{ + char *save_ptr = NULL, *argcopy, *next_zone; + uint16_t zone; + + if (strncmp(argv, "zone=", 5)) { + ds_put_format(ds, "invalid argument %s", argv); + return EINVAL; + } + + argcopy = xstrdup(argv + 5); + next_zone = strtok_r(argcopy, ",", &save_ptr); + + do { + if (ovs_scan(next_zone, "%"SCNu16, &zone)) { + ct_dpif_push_zone_limit(list, zone, 0, 0); + } else { + ds_put_cstr(ds, "invalid zone"); + return EINVAL; + } + } while ((next_zone = strtok_r(NULL, ",", &save_ptr)) != NULL); + + free(argcopy); + return 0; +} + +static int +dpctl_ct_del_limits(int argc, const char *argv[], + struct dpctl_params *dpctl_p) +{ + struct dpif *dpif; + struct ds ds = DS_EMPTY_INITIALIZER; + int error, i = 1; + struct ovs_list list = OVS_LIST_INITIALIZER(&list); + + error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif, true, &i); + if (error) { + return error; + } + + error = parse_ct_limit_zones(argv[i], &list, &ds); + if (error) { + goto error; + } + + error = ct_dpif_del_limits(dpif, &list); + if (!error) { + goto out; + } else { + ds_put_cstr(&ds, "failed to delete conntrack limit"); + } + +error: + dpctl_error(dpctl_p, error, "%s", ds_cstr(&ds)); + ds_destroy(&ds); +out: + ct_dpif_free_zone_limits(&list); + dpif_close(dpif); + return error; +} + +static int +dpctl_ct_get_limits(int argc, const char *argv[], + struct dpctl_params *dpctl_p) +{ + struct dpif *dpif; + struct ds ds = DS_EMPTY_INITIALIZER; + uint32_t default_limit; + int error, i = 1; + struct ovs_list list_query = OVS_LIST_INITIALIZER(&list_query); + struct ovs_list list_reply = OVS_LIST_INITIALIZER(&list_reply); + + error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif, true, &i); + if (error) { + return error; + } + + if (argc > i) { + error = parse_ct_limit_zones(argv[i], &list_query, &ds); + if (error) { + goto error; + } + } + + error = ct_dpif_get_limits(dpif, &default_limit, &list_query, + &list_reply); + if (!error) { + ct_dpif_format_zone_limits(default_limit, &list_reply, &ds); + dpctl_print(dpctl_p, "%s\n", ds_cstr(&ds)); + goto out; + } else { + ds_put_format(&ds, "failed to get conntrack limit %s", + ovs_strerror(error)); + } + +error: + dpctl_error(dpctl_p, error, "%s", ds_cstr(&ds)); +out: + ds_destroy(&ds); + ct_dpif_free_zone_limits(&list_query); + ct_dpif_free_zone_limits(&list_reply); + dpif_close(dpif); + return error; +} + /* Undocumented commands for unit testing. */ static int @@ -1979,6 +2140,12 @@ static const struct dpctl_command all_commands[] = { { "ct-set-maxconns", "[dp] maxconns", 1, 2, dpctl_ct_set_maxconns, DP_RW }, { "ct-get-maxconns", "[dp]", 0, 1, dpctl_ct_get_maxconns, DP_RO }, { "ct-get-nconns", "[dp]", 0, 1, dpctl_ct_get_nconns, DP_RO }, + { "ct-set-limits", "[dp] [default=L] [zone=N,limit=L]...", 1, INT_MAX, + dpctl_ct_set_limits, DP_RO }, + { "ct-del-limits", "[dp] zone=N1[,N2]...", 1, 2, dpctl_ct_del_limits, + DP_RO }, + { "ct-get-limits", "[dp] [zone=N1[,N2]...]", 0, 2, dpctl_ct_get_limits, + 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 5d987e62daaa..deb1bd32a34b 100644 --- a/lib/dpctl.man +++ b/lib/dpctl.man @@ -272,3 +272,21 @@ Only supported for userspace datapath. \*(DX\fBct\-get\-nconns\fR [\fIdp\fR] Prints the current number of connection tracker entries on \fIdp\fR. Only supported for userspace datapath. +. +.TP +\*(DX\fBct\-set\-limits\fR [\fIdp\fR] [\fBdefault=\fIdefault_limit\fR] [\fBzone=\fIzone\fR,\fBlimit=\fIlimit\fR]... +Sets the maximum allowed number of connections in connection tracking zones. +If a per zone limit for a particular zone is not available in the datapath, +it defaults to the default per zone limit. Initially, the default per zone +limit is unlimited(0). +. +.TP +\*(DX\fBct\-del\-limits\fR [\fIdp\fR] \fBzone=\fIzone[,zone]\fR... +Deletes the per zone limit on particular zones. The \fIzone\fR must be a +comma-separated list. +. +.TP +\*(DX\fBct\-get\-limits\fR [\fIdp\fR] [\fBzone=\fIzone[,zone]\fR...] +Retrieves the maximum allowed number of connections. The \fIzone\fR must +be a comma-separated list. Prints all zone limits if no \fIzone\fR is +provided.
This patch implments the following three commands on dpctl so that users can use ovs-dpctl or ovs-appctl to set, delete, and get the per zone limit. For example, $ ovs-appctl dpctl/ct-set-limits default=10 zone=0,limit=5 zone=1,limit=3 $ ovs-appctl dpct/ct-del-limits zone=0 $ ovs-appctl dpct/ct-get-limits zone=1,2,3 Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> --- NEWS | 2 + lib/ct-dpif.c | 67 +++++++++++++++++++++++ lib/ct-dpif.h | 4 ++ lib/dpctl.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++- lib/dpctl.man | 18 +++++++ 5 files changed, 259 insertions(+), 1 deletion(-)