Hi, This patch series implements the required plumbering for vif rate limiting in libxl/xl. The first patch fixes trivial indentation issues and introduces no functional changes. The second patch implements dry-run for `xl network-attach` for debugging and testing purposes. The third patch adds the required plumbering in libxl/xl to add vif rate limiting support. It uses the `rate` option syntax from xm/xend, ensuring full backward compatiblity. The final patch adds the "check-xl-vif-parse" test script which tests various `rate` syntax. This is my first patch to Xen. Any comments or feedbacks are welcomed and appreciated. Thanks! -- Mathieu
Signed-off-by: Mathieu Gagné <mgagne@iweb.com> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -841,10 +841,10 @@ static void parse_config_data(const char nic->script = strdup(default_vifscript); } - if (default_bridge) { - free(nic->bridge); - nic->bridge = strdup(default_bridge); - } + if (default_bridge) { + free(nic->bridge); + nic->bridge = strdup(default_bridge); + } p = strtok(buf2, ","); if (!p) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Mathieu Gagné
2012-Mar-20 01:28 UTC
[PATCH 2 of 4] xl: xl network-attach -N (dry run) option
Add dryrun for testing and debugging purposes. Signed-off-by: Mathieu Gagné <mgagne@iweb.com> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -4756,6 +4756,16 @@ int main_networkattach(int argc, char ** return 1; } } + + if (dryrun_only) { + char *json = libxl_device_nic_to_json(ctx, &nic); + printf("vif: %s\n", json); + free(json); + libxl_device_nic_dispose(&nic); + if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-1); } + return 0; + } + if (libxl_device_nic_add(ctx, domid, &nic)) { fprintf(stderr, "libxl_device_nic_add failed.\n"); return 1; diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c --- a/tools/libxl/xl_cmdtable.c +++ b/tools/libxl/xl_cmdtable.c @@ -279,7 +279,7 @@ struct cmd_spec cmd_table[] = { "", }, { "network-attach", - &main_networkattach, 0, + &main_networkattach, 1, "Create a new virtual network device", "<Domain> [type=<type>] [mac=<mac>] [bridge=<bridge>] " "[ip=<ip>] [script=<script>] [backend=<BackDomain>] [vifname=<name>] " _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
The `rate` parameter specifies the rate at which the outgoing traffic will be limited to. This defaults to unlimited. The `rate` keyword supports an optional time window parameter for specifying the granularity of credit replenishment. It determines the frequency at which the vif transmission credit is replenished. The default window is 50ms. For example: 'rate=10Mb/s' 'rate=250KB/s' 'rate=1MB/s@20ms' Signed-off-by: Mathieu Gagné <mgagne@iweb.com> diff --git a/docs/misc/xl-network-configuration.markdown b/docs/misc/xl-network-configuration.markdown --- a/docs/misc/xl-network-configuration.markdown +++ b/docs/misc/xl-network-configuration.markdown @@ -122,5 +122,21 @@ Specifies the backend domain which this defaults to domain 0. Specifying another domain requires setting up a driver domain which is outside the scope of this document. +### rate + +Specifies the rate at which the outgoing traffic will be limited to. This +defaults to unlimited. + +The `rate` keyword supports an optional time window parameter for specifying +the granularity of credit replenishment. It determines the frequency at which +the vif transmission credit is replenished. The default window is 50ms. + +For example: + + 'rate=10Mb/s' + 'rate=250KB/s' + 'rate=1MB/s@20ms' + + [oui]: http://en.wikipedia.org/wiki/Organizationally_Unique_Identifier [net]: http://wiki.xen.org/wiki/HostConfiguration/Networking diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile --- a/tools/libxl/Makefile +++ b/tools/libxl/Makefile @@ -57,7 +57,7 @@ LIBXL_OBJS += _libxl_types.o libxl_flask AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \ - libxlu_disk_l.o libxlu_disk.o + libxlu_disk_l.o libxlu_disk.o libxlu_vif.o $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h CLIENTS = xl testidl diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -1834,6 +1834,13 @@ int libxl_device_nic_add(libxl_ctx *ctx, flexarray_append(back, libxl__strdup(gc, nic->ip)); } + if (nic->rate_interval_usecs > 0) { + flexarray_append(back, "rate"); + flexarray_append(back, libxl__sprintf(gc, "%u,%u", + nic->rate_bytes_per_interval, + nic->rate_interval_usecs)); + } + flexarray_append(back, "bridge"); flexarray_append(back, libxl__strdup(gc, nic->bridge)); flexarray_append(back, "handle"); diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -341,6 +341,8 @@ libxl_device_nic = Struct("device_nic", ("ifname", string), ("script", string), ("nictype", libxl_nic_type), + ("rate_bytes_per_interval", uint32), + ("rate_interval_usecs", uint32), ]) libxl_device_pci = Struct("device_pci", [ diff --git a/tools/libxl/libxlu_internal.h b/tools/libxl/libxlu_internal.h --- a/tools/libxl/libxlu_internal.h +++ b/tools/libxl/libxlu_internal.h @@ -17,9 +17,11 @@ #define LIBXLU_INTERNAL_H #include <stdio.h> +#include <stdlib.h> #include <errno.h> #include <string.h> #include <assert.h> +#include <regex.h> #define XLU_ConfigList XLU_ConfigSetting diff --git a/tools/libxl/libxlu_vif.c b/tools/libxl/libxlu_vif.c new file mode 100644 --- /dev/null +++ b/tools/libxl/libxlu_vif.c @@ -0,0 +1,91 @@ +#include "libxl_osdeps.h" /* must come before any other headers */ +#include "libxlu_internal.h" + +static const char *vif_bytes_per_sec_re = "^([0-9]+)([GMK]?)([Bb])/s$"; +static const char *vif_internal_usec_re = "^([0-9]+)([mu]?)s?$"; + +static void vif_parse_rate_bytes_per_sec(const char *bytes, uint32_t *bytes_per_sec) +{ + regex_t rec; + uint64_t tmp_bytes_per_sec = 0; + + regcomp(&rec, vif_bytes_per_sec_re, REG_EXTENDED); + if (regexec(&rec, bytes, 0, NULL, 0) == 0) { + const char *p = bytes; + tmp_bytes_per_sec = strtoul(p, (char**)&p, 0); + if (*p == 'G' || *p == '\0') + tmp_bytes_per_sec *= 1000 * 1000 * 1000; + else if (*p == 'M') + tmp_bytes_per_sec *= 1000 * 1000; + else if (*p == 'K') + tmp_bytes_per_sec *= 1000; + if (*p == 'b' || *(p+1) == 'b') + tmp_bytes_per_sec /= 8; + } + regfree(&rec); + + if (tmp_bytes_per_sec > UINT32_MAX || tmp_bytes_per_sec == 0) + tmp_bytes_per_sec = UINT32_MAX; + *bytes_per_sec = tmp_bytes_per_sec; +} + +static void vif_parse_rate_interval_usecs(const char *interval, + uint32_t *interval_usecs) +{ + regex_t rec; + uint64_t tmp_interval_usecs = 0; + + regcomp(&rec, vif_internal_usec_re, REG_EXTENDED); + if (regexec(&rec, interval, 0, NULL, 0) == 0) { + const char *p = interval; + tmp_interval_usecs = strtoul(p, (char**)&p, 10); + if (*p == 's' || *p == '\0') + tmp_interval_usecs *= 1000 * 1000; + else if (*p == 'm') + tmp_interval_usecs *= 1000; + } + regfree(&rec); + + if (tmp_interval_usecs > UINT32_MAX || tmp_interval_usecs == 0) + tmp_interval_usecs = UINT32_MAX; + *interval_usecs = tmp_interval_usecs; +} + +void xlu_vif_parse_rate(const char *rate, uint32_t *bytes_per_interval, + uint32_t *interval_usecs) +{ + uint32_t bytes_per_sec = 0; + uint64_t tmp_bytes_per_interval = 0; + char *tmprate, *ratetok; + + tmprate = strdup(rate); + + ratetok = strtok(tmprate, "@"); + vif_parse_rate_bytes_per_sec(ratetok, &bytes_per_sec); + + ratetok = strtok(NULL, "@"); + if (ratetok != NULL) + vif_parse_rate_interval_usecs(ratetok, interval_usecs); + else + *interval_usecs = 50000; /* Default to 50ms */ + + free(tmprate); + + tmp_bytes_per_interval + (((uint64_t) bytes_per_sec * (uint64_t) *interval_usecs) / 1000000L); + + /* overflow checking: default to unlimited rate */ + if ((tmp_bytes_per_interval == 0) || (tmp_bytes_per_interval > UINT32_MAX)) { + tmp_bytes_per_interval = UINT32_MAX; + *interval_usecs = 0; + } + *bytes_per_interval = tmp_bytes_per_interval; +} + +/* + * Local variables: + * mode: C + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h --- a/tools/libxl/libxlutil.h +++ b/tools/libxl/libxlutil.h @@ -88,6 +88,12 @@ int xlu_disk_parse(XLU_Config *cfg, int * resulting disk struct is used with libxl. */ +/* + * Vif rate parsing. + */ + +void xlu_vif_parse_rate(const char *rate, uint32_t *bytes_per_interval, + uint32_t *interval_usecs); #endif /* LIBXLUTIL_H */ diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -900,7 +900,8 @@ static void parse_config_data(const char nic->backend_domid = 0; } } else if (!strcmp(p, "rate")) { - fprintf(stderr, "the rate parameter for vifs is currently not supported\n"); + xlu_vif_parse_rate((p2 + 4), &(nic->rate_bytes_per_interval), + &(nic->rate_interval_usecs)); } else if (!strcmp(p, "accel")) { fprintf(stderr, "the accel parameter for vifs is currently not supported\n"); } @@ -4750,6 +4751,8 @@ int main_networkattach(int argc, char ** } else if (MATCH_OPTION("model", *argv, oparg)) { replace_string(&nic.model, oparg); } else if (MATCH_OPTION("rate", *argv, oparg)) { + xlu_vif_parse_rate(oparg, &(nic.rate_bytes_per_interval), + &(nic.rate_interval_usecs)); } else if (MATCH_OPTION("accel", *argv, oparg)) { } else { fprintf(stderr, "unrecognized argument `%s'\n", *argv); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Mathieu Gagné
2012-Mar-20 01:28 UTC
[PATCH 4 of 4] xl: add "check-xl-vif-parse" test script
This test script runs "xl -N network-attach 0 <foobar>" against various rate syntax and checks that the output is as expected. Signed-off-by: Mathieu Gagné <mgagne@iweb.com> diff --git a/tools/libxl/check-xl-vif-parse b/tools/libxl/check-xl-vif-parse new file mode 100755 --- /dev/null +++ b/tools/libxl/check-xl-vif-parse @@ -0,0 +1,233 @@ +#!/bin/bash + +set -e + +if [ -x ./xl ] ; then + export LD_LIBRARY_PATH=. + XL=./xl +else + XL=xl +fi + +fprefix=tmp.check-xl-vif-parse + +expected () { + cat >$fprefix.expected +} + +failures=0 + +one () { + expected_rc=$1; shift + printf "test case %s...\n" "$*" + set +e + ${XL} -N network-attach 0 "$@" </dev/null >$fprefix.actual 2>/dev/null + actual_rc=$? + diff -u $fprefix.expected $fprefix.actual + diff_rc=$? + set -e + if [ $actual_rc != $expected_rc ] || [ $diff_rc != 0 ]; then + echo >&2 "test case \`$*' failed ($actual_rc $diff_rc)" + failures=$(( $failures + 1 )) + fi +} + +complete () { + if [ "$failures" = 0 ]; then + echo all ok.; exit 0 + else + echo "$failures tests failed."; exit 1 + fi +} + +e=1 + + +#---------- test data ---------- + +expected </dev/null +one $e foo + +# test invalid rate +expected <<END +vif: { + "backend_domid": 0, + "devid": 0, + "mtu": 0, + "model": null, + "mac": "00:00:00:00:00:00", + "ip": null, + "bridge": null, + "ifname": null, + "script": null, + "nictype": null, + "rate_bytes_per_interval": 214748364, + "rate_interval_usecs": 50000 +} + +END +one 0 rate=foo +one 0 rate=10MB +one 0 rate=10MB/m +one 0 rate=10ZB +one 0 rate=10ZB/s +one 0 rate=10ZB/m + +# test Kb/s unit +expected <<END +vif: { + "backend_domid": 0, + "devid": 0, + "mtu": 0, + "model": null, + "mac": "00:00:00:00:00:00", + "ip": null, + "bridge": null, + "ifname": null, + "script": null, + "nictype": null, + "rate_bytes_per_interval": 100, + "rate_interval_usecs": 50000 +} + +END +one 0 rate=16Kb/s +one 0 rate=16Kb/s@50ms +one 0 rate=2KB/s +one 0 rate=2KB/s@50ms + +# test Mb/s unit +expected <<END +vif: { + "backend_domid": 0, + "devid": 0, + "mtu": 0, + "model": null, + "mac": "00:00:00:00:00:00", + "ip": null, + "bridge": null, + "ifname": null, + "script": null, + "nictype": null, + "rate_bytes_per_interval": 100000, + "rate_interval_usecs": 50000 +} + +END +one 0 rate=16Mb/s +one 0 rate=16Mb/s@50ms +one 0 rate=2MB/s +one 0 rate=2MB/s@50ms + +# test Gb/s unit +expected <<END +vif: { + "backend_domid": 0, + "devid": 0, + "mtu": 0, + "model": null, + "mac": "00:00:00:00:00:00", + "ip": null, + "bridge": null, + "ifname": null, + "script": null, + "nictype": null, + "rate_bytes_per_interval": 50000000, + "rate_interval_usecs": 50000 +} + +END +one 0 rate=8Gb/s +one 0 rate=8Gb/s@50ms +one 0 rate=1GB/s +one 0 rate=1GB/s@50ms + +# test bytes/s overflow +expected <<END +vif: { + "backend_domid": 0, + "devid": 0, + "mtu": 0, + "model": null, + "mac": "00:00:00:00:00:00", + "ip": null, + "bridge": null, + "ifname": null, + "script": null, + "nictype": null, + "rate_bytes_per_interval": 214748364, + "rate_interval_usecs": 50000 +} + +END +one 0 rate=40Gb/s +one 0 rate=40Gb/s@50ms +one 0 rate=40GB/s +one 0 rate=40GB/s@50ms + +# test bytes/s and interval +expected <<END +vif: { + "backend_domid": 0, + "devid": 0, + "mtu": 0, + "model": null, + "mac": "00:00:00:00:00:00", + "ip": null, + "bridge": null, + "ifname": null, + "script": null, + "nictype": null, + "rate_bytes_per_interval": 10000000, + "rate_interval_usecs": 1000000 +} + +END +one 0 rate=80Mb/s@1s +one 0 rate=10MB/s@1s + +# test invalid interval +expected <<END +vif: { + "backend_domid": 0, + "devid": 0, + "mtu": 0, + "model": null, + "mac": "00:00:00:00:00:00", + "ip": null, + "bridge": null, + "ifname": null, + "script": null, + "nictype": null, + "rate_bytes_per_interval": 4294967295, + "rate_interval_usecs": 0 +} + +END +one 0 rate=10Mb/s@foo +one 0 rate=10Mb/s@10h +one 0 rate=10MB/s@foo +one 0 rate=10MB/s@10h + +# test bytes/s with interval overflow +expected <<END +vif: { + "backend_domid": 0, + "devid": 0, + "mtu": 0, + "model": null, + "mac": "00:00:00:00:00:00", + "ip": null, + "bridge": null, + "ifname": null, + "script": null, + "nictype": null, + "rate_bytes_per_interval": 4294967295, + "rate_interval_usecs": 0 +} + +END +one 0 rate=200Gb/s@10s +one 0 rate=25GB/s@10s + +complete _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Tue, 2012-03-20 at 01:28 +0000, Mathieu Gagné wrote:> Signed-off-by: Mathieu Gagné <mgagne@iweb.com>Acked-by: Ian Campbell <ian.campbell@citrix.com>> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -841,10 +841,10 @@ static void parse_config_data(const char > nic->script = strdup(default_vifscript); > } > > - if (default_bridge) { > - free(nic->bridge); > - nic->bridge = strdup(default_bridge); > - } > + if (default_bridge) { > + free(nic->bridge); > + nic->bridge = strdup(default_bridge); > + } > > p = strtok(buf2, ","); > if (!p) > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2012-Mar-20 10:58 UTC
Re: [PATCH 2 of 4] xl: xl network-attach -N (dry run) option
On Tue, 2012-03-20 at 01:28 +0000, Mathieu Gagné wrote:> Add dryrun for testing and debugging purposes. > > Signed-off-by: Mathieu Gagné <mgagne@iweb.com>Acked-by: Ian Campbell <ian.campbell@citrix.com>> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -4756,6 +4756,16 @@ int main_networkattach(int argc, char ** > return 1; > } > } > + > + if (dryrun_only) { > + char *json = libxl_device_nic_to_json(ctx, &nic); > + printf("vif: %s\n", json); > + free(json); > + libxl_device_nic_dispose(&nic); > + if (ferror(stdout) || fflush(stdout)) { perror("stdout"); exit(-1); } > + return 0; > + } > + > if (libxl_device_nic_add(ctx, domid, &nic)) { > fprintf(stderr, "libxl_device_nic_add failed.\n"); > return 1; > diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c > --- a/tools/libxl/xl_cmdtable.c > +++ b/tools/libxl/xl_cmdtable.c > @@ -279,7 +279,7 @@ struct cmd_spec cmd_table[] = { > "", > }, > { "network-attach", > - &main_networkattach, 0, > + &main_networkattach, 1, > "Create a new virtual network device", > "<Domain> [type=<type>] [mac=<mac>] [bridge=<bridge>] " > "[ip=<ip>] [script=<script>] [backend=<BackDomain>] [vifname=<name>] " > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Tue, 20 Mar 2012, Mathieu Gagné wrote:> Signed-off-by: Mathieu Gagné <mgagne@iweb.com>Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -841,10 +841,10 @@ static void parse_config_data(const char > nic->script = strdup(default_vifscript); > } > > - if (default_bridge) { > - free(nic->bridge); > - nic->bridge = strdup(default_bridge); > - } > + if (default_bridge) { > + free(nic->bridge); > + nic->bridge = strdup(default_bridge); > + } > > p = strtok(buf2, ","); > if (!p) >--8323329-1682964974-1332241695=:923 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --8323329-1682964974-1332241695=:923--
Stefano Stabellini
2012-Mar-20 11:09 UTC
Re: [PATCH 2 of 4] xl: xl network-attach -N (dry run) option
On Tue, 20 Mar 2012, Mathieu Gagné wrote:> Add dryrun for testing and debugging purposes.Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --8323329-1995199105-1332241780=:923 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --8323329-1995199105-1332241780=:923--
Stefano Stabellini
2012-Mar-20 11:16 UTC
Re: [PATCH 3 of 4] xl: add support for vif rate limiting
On Tue, 20 Mar 2012, Mathieu Gagné wrote:> The `rate` parameter specifies the rate at which the outgoing traffic > will be limited to. This defaults to unlimited. > > The `rate` keyword supports an optional time window parameter for specifying > the granularity of credit replenishment. It determines the frequency at which > the vif transmission credit is replenished. The default window is 50ms. > > For example: > > ''rate=10Mb/s'' > ''rate=250KB/s'' > ''rate=1MB/s@20ms'' > > Signed-off-by: Mathieu Gagné <mgagne@iweb.com> > > diff --git a/docs/misc/xl-network-configuration.markdown b/docs/misc/xl-network-configuration.markdown > --- a/docs/misc/xl-network-configuration.markdown > +++ b/docs/misc/xl-network-configuration.markdown > @@ -122,5 +122,21 @@ Specifies the backend domain which this > defaults to domain 0. Specifying another domain requires setting up a > driver domain which is outside the scope of this document. > > +### rate > + > +Specifies the rate at which the outgoing traffic will be limited to. This > +defaults to unlimited. > + > +The `rate` keyword supports an optional time window parameter for specifying > +the granularity of credit replenishment. It determines the frequency at which > +the vif transmission credit is replenished. The default window is 50ms. > + > +For example: > + > + ''rate=10Mb/s'' > + ''rate=250KB/s'' > + ''rate=1MB/s@20ms'' > + > + > [oui]: http://en.wikipedia.org/wiki/Organizationally_Unique_Identifier > [net]: http://wiki.xen.org/wiki/HostConfiguration/Networking > diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile > --- a/tools/libxl/Makefile > +++ b/tools/libxl/Makefile > @@ -57,7 +57,7 @@ LIBXL_OBJS += _libxl_types.o libxl_flask > AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h > AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c > LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \ > - libxlu_disk_l.o libxlu_disk.o > + libxlu_disk_l.o libxlu_disk.o libxlu_vif.o > $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h > > CLIENTS = xl testidl > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -1834,6 +1834,13 @@ int libxl_device_nic_add(libxl_ctx *ctx, > flexarray_append(back, libxl__strdup(gc, nic->ip)); > } > > + if (nic->rate_interval_usecs > 0) { > + flexarray_append(back, "rate"); > + flexarray_append(back, libxl__sprintf(gc, "%u,%u", > + nic->rate_bytes_per_interval, > + nic->rate_interval_usecs)); > + } > + > flexarray_append(back, "bridge"); > flexarray_append(back, libxl__strdup(gc, nic->bridge)); > flexarray_append(back, "handle"); > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -341,6 +341,8 @@ libxl_device_nic = Struct("device_nic", > ("ifname", string), > ("script", string), > ("nictype", libxl_nic_type), > + ("rate_bytes_per_interval", uint32), > + ("rate_interval_usecs", uint32), > ]) > > libxl_device_pci = Struct("device_pci", [ > diff --git a/tools/libxl/libxlu_internal.h b/tools/libxl/libxlu_internal.h > --- a/tools/libxl/libxlu_internal.h > +++ b/tools/libxl/libxlu_internal.h > @@ -17,9 +17,11 @@ > #define LIBXLU_INTERNAL_H > > #include <stdio.h> > +#include <stdlib.h> > #include <errno.h> > #include <string.h> > #include <assert.h> > +#include <regex.h> > > #define XLU_ConfigList XLU_ConfigSetting > > diff --git a/tools/libxl/libxlu_vif.c b/tools/libxl/libxlu_vif.c > new file mode 100644 > --- /dev/null > +++ b/tools/libxl/libxlu_vif.c > @@ -0,0 +1,91 @@ > +#include "libxl_osdeps.h" /* must come before any other headers */ > +#include "libxlu_internal.h" > + > +static const char *vif_bytes_per_sec_re = "^([0-9]+)([GMK]?)([Bb])/s$"; > +static const char *vif_internal_usec_re = "^([0-9]+)([mu]?)s?$"; > + > +static void vif_parse_rate_bytes_per_sec(const char *bytes, uint32_t *bytes_per_sec) > +{ > + regex_t rec; > + uint64_t tmp_bytes_per_sec = 0; > + > + regcomp(&rec, vif_bytes_per_sec_re, REG_EXTENDED); > + if (regexec(&rec, bytes, 0, NULL, 0) == 0) { > + const char *p = bytes; > + tmp_bytes_per_sec = strtoul(p, (char**)&p, 0); > + if (*p == ''G'' || *p == ''\0'') > + tmp_bytes_per_sec *= 1000 * 1000 * 1000; > + else if (*p == ''M'') > + tmp_bytes_per_sec *= 1000 * 1000; > + else if (*p == ''K'') > + tmp_bytes_per_sec *= 1000; > + if (*p == ''b'' || *(p+1) == ''b'') > + tmp_bytes_per_sec /= 8; > + } > + regfree(&rec); > + > + if (tmp_bytes_per_sec > UINT32_MAX || tmp_bytes_per_sec == 0) > + tmp_bytes_per_sec = UINT32_MAX; > + *bytes_per_sec = tmp_bytes_per_sec; > +} > + > +static void vif_parse_rate_interval_usecs(const char *interval, > + uint32_t *interval_usecs) > +{ > + regex_t rec; > + uint64_t tmp_interval_usecs = 0; > + > + regcomp(&rec, vif_internal_usec_re, REG_EXTENDED); > + if (regexec(&rec, interval, 0, NULL, 0) == 0) { > + const char *p = interval; > + tmp_interval_usecs = strtoul(p, (char**)&p, 10); > + if (*p == ''s'' || *p == ''\0'') > + tmp_interval_usecs *= 1000 * 1000; > + else if (*p == ''m'') > + tmp_interval_usecs *= 1000; > + } > + regfree(&rec); > + > + if (tmp_interval_usecs > UINT32_MAX || tmp_interval_usecs == 0) > + tmp_interval_usecs = UINT32_MAX; > + *interval_usecs = tmp_interval_usecs; > +} > + > +void xlu_vif_parse_rate(const char *rate, uint32_t *bytes_per_interval, > + uint32_t *interval_usecs) > +{ > + uint32_t bytes_per_sec = 0; > + uint64_t tmp_bytes_per_interval = 0; > + char *tmprate, *ratetok; > + > + tmprate = strdup(rate); > + > + ratetok = strtok(tmprate, "@"); > + vif_parse_rate_bytes_per_sec(ratetok, &bytes_per_sec); > + > + ratetok = strtok(NULL, "@"); > + if (ratetok != NULL) > + vif_parse_rate_interval_usecs(ratetok, interval_usecs); > + else > + *interval_usecs = 50000; /* Default to 50ms */ > + > + free(tmprate); > + > + tmp_bytes_per_interval > + (((uint64_t) bytes_per_sec * (uint64_t) *interval_usecs) / 1000000L); > + > + /* overflow checking: default to unlimited rate */ > + if ((tmp_bytes_per_interval == 0) || (tmp_bytes_per_interval > UINT32_MAX)) { > + tmp_bytes_per_interval = UINT32_MAX; > + *interval_usecs = 0; > + } > + *bytes_per_interval = tmp_bytes_per_interval; > +}wouldn''t it make sense to just use a uint64_t for rate_bytes_per_interval and rate_interval_usecs? --8323329-796474145-1332242213=:923 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --8323329-796474145-1332242213=:923--
Ian Campbell
2012-Mar-20 11:21 UTC
Re: [PATCH 3 of 4] xl: add support for vif rate limiting
On Tue, 2012-03-20 at 01:28 +0000, Mathieu Gagné wrote:> The `rate` parameter specifies the rate at which the outgoing traffic > will be limited to. This defaults to unlimited. > > The `rate` keyword supports an optional time window parameter for specifying > the granularity of credit replenishment. It determines the frequency at which > the vif transmission credit is replenished. The default window is 50ms. > > For example: > > 'rate=10Mb/s' > 'rate=250KB/s' > 'rate=1MB/s@20ms' > > Signed-off-by: Mathieu Gagné <mgagne@iweb.com>Looks pretty good, thanks. I have a few comments below.> diff --git a/docs/misc/xl-network-configuration.markdown b/docs/misc/xl-network-configuration.markdown > --- a/docs/misc/xl-network-configuration.markdown > +++ b/docs/misc/xl-network-configuration.markdown > @@ -122,5 +122,21 @@ Specifies the backend domain which this > defaults to domain 0. Specifying another domain requires setting up a > driver domain which is outside the scope of this document. > > +### rate > + > +Specifies the rate at which the outgoing traffic will be limited to. This > +defaults to unlimited. > + > +The `rate` keyword supports an optional time window parameter for specifying > +the granularity of credit replenishment. It determines the frequency at which > +the vif transmission credit is replenished. The default window is 50ms.I think this needs to be more explicit/formal about what the syntax is. e.g.: The rate may be specified as "<RATE>/s" or optionally "<RATE>/s@<WINDOW>". <RATE> is in bytes and can accept suffixes Mb, Kb (list here) <WINDOW> is in microseconds and can accept suffixes <list>. The default is 50ms. Is the "/s" formally required? Does it take any modifiers (e.g. ms) Having both "/s" and "@<something>" in a single specification (e.g. "1MB/s@20ms") seems a bit odd, is that right? What does it actually mean What are the semantics of window? If I say "1MB/s@500ms" then do I get 0.5MB of credit every half second? The code and xend both seem to use "interval" rather than "window".> + > +For example:Should spell out the meaning of the examples:> + 'rate=10Mb/s'meaning up to 10 megabits every second> + 'rate=250KB/s' > + 'rate=1MB/s@20ms'meaning 1 megabyte in every 20 millisecond period> + > + > [oui]: http://en.wikipedia.org/wiki/Organizationally_Unique_Identifier > [net]: http://wiki.xen.org/wiki/HostConfiguration/Networking > diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile > --- a/tools/libxl/Makefile > +++ b/tools/libxl/Makefile > @@ -57,7 +57,7 @@ LIBXL_OBJS += _libxl_types.o libxl_flask > AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h > AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c > LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \ > - libxlu_disk_l.o libxlu_disk.o > + libxlu_disk_l.o libxlu_disk.o libxlu_vif.o > $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h > > CLIENTS = xl testidl > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -1834,6 +1834,13 @@ int libxl_device_nic_add(libxl_ctx *ctx, > flexarray_append(back, libxl__strdup(gc, nic->ip)); > } > > + if (nic->rate_interval_usecs > 0) { > + flexarray_append(back, "rate"); > + flexarray_append(back, libxl__sprintf(gc, "%u,%u", > + nic->rate_bytes_per_interval, > + nic->rate_interval_usecs)); > + } > + > flexarray_append(back, "bridge"); > flexarray_append(back, libxl__strdup(gc, nic->bridge)); > flexarray_append(back, "handle"); > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -341,6 +341,8 @@ libxl_device_nic = Struct("device_nic", > ("ifname", string), > ("script", string), > ("nictype", libxl_nic_type), > + ("rate_bytes_per_interval", uint32), > + ("rate_interval_usecs", uint32), > ]) > > libxl_device_pci = Struct("device_pci", [ > diff --git a/tools/libxl/libxlu_internal.h b/tools/libxl/libxlu_internal.h > --- a/tools/libxl/libxlu_internal.h > +++ b/tools/libxl/libxlu_internal.h > @@ -17,9 +17,11 @@ > #define LIBXLU_INTERNAL_H > > #include <stdio.h> > +#include <stdlib.h> > #include <errno.h> > #include <string.h> > #include <assert.h> > +#include <regex.h> > > #define XLU_ConfigList XLU_ConfigSetting > > diff --git a/tools/libxl/libxlu_vif.c b/tools/libxl/libxlu_vif.c > new file mode 100644 > --- /dev/null > +++ b/tools/libxl/libxlu_vif.c > @@ -0,0 +1,91 @@ > +#include "libxl_osdeps.h" /* must come before any other headers */ > +#include "libxlu_internal.h" > + > +static const char *vif_bytes_per_sec_re = "^([0-9]+)([GMK]?)([Bb])/s$"; > +static const char *vif_internal_usec_re = "^([0-9]+)([mu]?)s?$";These seem to match xend, which is good.> +static void vif_parse_rate_bytes_per_sec(const char *bytes, uint32_t *bytes_per_sec) > +{ > + regex_t rec; > + uint64_t tmp_bytes_per_sec = 0; > + > + regcomp(&rec, vif_bytes_per_sec_re, REG_EXTENDED);It seems that you use the regex only to check the syntax and then open code the parsing? That strikes me as odd, if you are going to use a regex parser you might as well use the matches returned from it. You could also combine the parsing of rate and interval into a single regex and avoid the use of strtok etc in the outermost function. You should return a syntax error if the string is invalid.> + if (regexec(&rec, bytes, 0, NULL, 0) == 0) { > + const char *p = bytes; > + tmp_bytes_per_sec = strtoul(p, (char**)&p, 0); > + if (*p == 'G' || *p == '\0') > + tmp_bytes_per_sec *= 1000 * 1000 * 1000; > + else if (*p == 'M') > + tmp_bytes_per_sec *= 1000 * 1000; > + else if (*p == 'K') > + tmp_bytes_per_sec *= 1000; > + if (*p == 'b' || *(p+1) == 'b') > + tmp_bytes_per_sec /= 8; > + } > + regfree(&rec); > + > + if (tmp_bytes_per_sec > UINT32_MAX || tmp_bytes_per_sec == 0) > + tmp_bytes_per_sec = UINT32_MAX; > + *bytes_per_sec = tmp_bytes_per_sec; > +} > + > +static void vif_parse_rate_interval_usecs(const char *interval, > + uint32_t *interval_usecs) > +{ > + regex_t rec; > + uint64_t tmp_interval_usecs = 0; > + > + regcomp(&rec, vif_internal_usec_re, REG_EXTENDED); > + if (regexec(&rec, interval, 0, NULL, 0) == 0) { > + const char *p = interval; > + tmp_interval_usecs = strtoul(p, (char**)&p, 10); > + if (*p == 's' || *p == '\0') > + tmp_interval_usecs *= 1000 * 1000; > + else if (*p == 'm') > + tmp_interval_usecs *= 1000; > + } > + regfree(&rec); > + > + if (tmp_interval_usecs > UINT32_MAX || tmp_interval_usecs == 0) > + tmp_interval_usecs = UINT32_MAX; > + *interval_usecs = tmp_interval_usecs; > +} > + > +void xlu_vif_parse_rate(const char *rate, uint32_t *bytes_per_interval, > + uint32_t *interval_usecs) > +{ > + uint32_t bytes_per_sec = 0; > + uint64_t tmp_bytes_per_interval = 0; > + char *tmprate, *ratetok; > + > + tmprate = strdup(rate); > + > + ratetok = strtok(tmprate, "@"); > + vif_parse_rate_bytes_per_sec(ratetok, &bytes_per_sec); > + > + ratetok = strtok(NULL, "@"); > + if (ratetok != NULL) > + vif_parse_rate_interval_usecs(ratetok, interval_usecs); > + else > + *interval_usecs = 50000; /* Default to 50ms */ > + > + free(tmprate); > + > + tmp_bytes_per_interval > + (((uint64_t) bytes_per_sec * (uint64_t) *interval_usecs) / 1000000L); > + > + /* overflow checking: default to unlimited rate */ > + if ((tmp_bytes_per_interval == 0) || (tmp_bytes_per_interval > UINT32_MAX)) { > + tmp_bytes_per_interval = UINT32_MAX; > + *interval_usecs = 0; > + } > + *bytes_per_interval = tmp_bytes_per_interval; > +} > + > +/* > + * Local variables: > + * mode: C > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h > --- a/tools/libxl/libxlutil.h > +++ b/tools/libxl/libxlutil.h > @@ -88,6 +88,12 @@ int xlu_disk_parse(XLU_Config *cfg, int > * resulting disk struct is used with libxl. > */ > > +/* > + * Vif rate parsing. > + */ > + > +void xlu_vif_parse_rate(const char *rate, uint32_t *bytes_per_interval, > + uint32_t *interval_usecs); > > #endif /* LIBXLUTIL_H */ > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -900,7 +900,8 @@ static void parse_config_data(const char > nic->backend_domid = 0; > } > } else if (!strcmp(p, "rate")) { > - fprintf(stderr, "the rate parameter for vifs is currently not supported\n"); > + xlu_vif_parse_rate((p2 + 4), &(nic->rate_bytes_per_interval), > + &(nic->rate_interval_usecs)); > } else if (!strcmp(p, "accel")) { > fprintf(stderr, "the accel parameter for vifs is currently not supported\n"); > } > @@ -4750,6 +4751,8 @@ int main_networkattach(int argc, char ** > } else if (MATCH_OPTION("model", *argv, oparg)) { > replace_string(&nic.model, oparg); > } else if (MATCH_OPTION("rate", *argv, oparg)) { > + xlu_vif_parse_rate(oparg, &(nic.rate_bytes_per_interval), > + &(nic.rate_interval_usecs)); > } else if (MATCH_OPTION("accel", *argv, oparg)) { > } else { > fprintf(stderr, "unrecognized argument `%s'\n", *argv); > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2012-Mar-20 11:26 UTC
Re: [PATCH 3 of 4] xl: add support for vif rate limiting
On Tue, 2012-03-20 at 11:16 +0000, Stefano Stabellini wrote:> On Tue, 20 Mar 2012, Mathieu Gagné wrote:[...]> > +void xlu_vif_parse_rate(const char *rate, uint32_t *bytes_per_interval, > > + uint32_t *interval_usecs) > > +{ > > + uint32_t bytes_per_sec = 0; > > + uint64_t tmp_bytes_per_interval = 0; > > + char *tmprate, *ratetok; > > + > > + tmprate = strdup(rate); > > + > > + ratetok = strtok(tmprate, "@"); > > + vif_parse_rate_bytes_per_sec(ratetok, &bytes_per_sec); > > + > > + ratetok = strtok(NULL, "@"); > > + if (ratetok != NULL) > > + vif_parse_rate_interval_usecs(ratetok, interval_usecs); > > + else > > + *interval_usecs = 50000; /* Default to 50ms */ > > + > > + free(tmprate); > > + > > + tmp_bytes_per_interval > > + (((uint64_t) bytes_per_sec * (uint64_t) *interval_usecs) / 1000000L); > > + > > + /* overflow checking: default to unlimited rate */ > > + if ((tmp_bytes_per_interval == 0) || (tmp_bytes_per_interval > UINT32_MAX)) { > > + tmp_bytes_per_interval = UINT32_MAX; > > + *interval_usecs = 0; > > + } > > + *bytes_per_interval = tmp_bytes_per_interval; > > +} > > wouldn't it make sense to just use a uint64_t for > rate_bytes_per_interval and rate_interval_usecs?It needn't be a barrier to this but FWIW Linux parses these into "unsigned long", but it does at least appear to do proper overflow checking. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2012-Mar-20 11:29 UTC
Re: [PATCH 4 of 4] xl: add "check-xl-vif-parse" test script
On Tue, 2012-03-20 at 01:28 +0000, Mathieu Gagné wrote:> This test script runs "xl -N network-attach 0 <foobar>" against various > rate syntax and checks that the output is as expected. > > Signed-off-by: Mathieu Gagné <mgagne@iweb.com>Great to see a testsuite! Thanks.> [...]+ > +# test invalid rate > +expected <<END > +vif: { > + "backend_domid": 0, > + "devid": 0, > + "mtu": 0, > + "model": null, > + "mac": "00:00:00:00:00:00", > + "ip": null, > + "bridge": null, > + "ifname": null, > + "script": null, > + "nictype": null, > + "rate_bytes_per_interval": 214748364, > + "rate_interval_usecs": 50000 > +} > + > +END > +one 0 rate=fooI think the result for all these ought to be a syntax error rather than success with a default value?> +one 0 rate=10MB > +one 0 rate=10MB/m > +one 0 rate=10ZB > +one 0 rate=10ZB/s > +one 0 rate=10ZB/m > + > +# test Kb/s unit > +expected <<END > +vif: { > + "backend_domid": 0, > + "devid": 0, > + "mtu": 0, > + "model": null, > + "mac": "00:00:00:00:00:00", > + "ip": null, > + "bridge": null, > + "ifname": null, > + "script": null, > + "nictype": null, > + "rate_bytes_per_interval": 100, > + "rate_interval_usecs": 50000 > +} > + > +END > +one 0 rate=16Kb/s > +one 0 rate=16Kb/s@50ms > +one 0 rate=2KB/s > +one 0 rate=2KB/s@50msTook me a while to spot b vs B but this looks good.> +[...] > +# test bytes/s overflow > +expected <<END > +vif: { > + "backend_domid": 0, > + "devid": 0, > + "mtu": 0, > + "model": null, > + "mac": "00:00:00:00:00:00", > + "ip": null, > + "bridge": null, > + "ifname": null, > + "script": null, > + "nictype": null, > + "rate_bytes_per_interval": 214748364, > + "rate_interval_usecs": 50000 > +} > + > +END > +one 0 rate=40Gb/s > +one 0 rate=40Gb/s@50ms > +one 0 rate=40GB/s > +one 0 rate=40GB/s@50ms40Gb is not beyond the realms of current hardware (although its a bit of a stretch for the PV net protocol). Stefano suggested using a 64 but type which might be a good idea for that reason. The kernel appears to do the right error handling with overflow. I suppose it would be worth mentining in the documentation that the enforcement of these limits is dependent on netback implementing it (at least Linux does, don't know bout *BSD) and the limitations are kernel specific. [...]> +# test invalid interval > +expected <<END > +vif: { > + "backend_domid": 0, > + "devid": 0, > + "mtu": 0, > + "model": null, > + "mac": "00:00:00:00:00:00", > + "ip": null, > + "bridge": null, > + "ifname": null, > + "script": null, > + "nictype": null, > + "rate_bytes_per_interval": 4294967295, > + "rate_interval_usecs": 0 > +} > + > +END > +one 0 rate=10Mb/s@fooThis ought to be a syntax error. Ian.> +one 0 rate=10Mb/s@10h > +one 0 rate=10MB/s@foo > +one 0 rate=10MB/s@10h > + > +# test bytes/s with interval overflow > +expected <<END > +vif: { > + "backend_domid": 0, > + "devid": 0, > + "mtu": 0, > + "model": null, > + "mac": "00:00:00:00:00:00", > + "ip": null, > + "bridge": null, > + "ifname": null, > + "script": null, > + "nictype": null, > + "rate_bytes_per_interval": 4294967295, > + "rate_interval_usecs": 0 > +} > + > +END > +one 0 rate=200Gb/s@10s > +one 0 rate=25GB/s@10s > + > +complete > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2012-Mar-20 11:30 UTC
Re: [PATCH 0 of 4] xl: add support for vif rate limiting
On Tue, 2012-03-20 at 01:28 +0000, Mathieu Gagné wrote:> This is my first patch to Xen. Any comments or feedbacks are welcomed > and appreciated. Thanks!Thank you for the nicely structured and clear patch series, I'd say it was near enough perfect in its presentation. I had a few comments on the individual patches which I've made in replies to those mails. Thanks, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Mathieu Gagné
2012-Mar-20 17:58 UTC
Re: [PATCH 3 of 4] xl: add support for vif rate limiting
On 3/20/12 7:21 AM, Ian Campbell wrote:> On Tue, 2012-03-20 at 01:28 +0000, Mathieu Gagné wrote: >> >> +### rate >> + >> +Specifies the rate at which the outgoing traffic will be limited to. This >> +defaults to unlimited. >> + >> +The `rate` keyword supports an optional time window parameter for specifying >> +the granularity of credit replenishment. It determines the frequency at which >> +the vif transmission credit is replenished. The default window is 50ms. > > I think this needs to be more explicit/formal about what the syntax is. > e.g.: > > The rate may be specified as "<RATE>/s" or optionally > "<RATE>/s@<WINDOW>". > > <RATE> is in bytes and can accept suffixes Mb, Kb (list here) > > <WINDOW> is in microseconds and can accept suffixes<list>. The > default is 50ms. > > Is the "/s" formally required? Does it take any modifiers (e.g. ms)It is formally required according to the original code within Xend/xm. Wanting to be 100% backward compatible, I did not change this part. <RATE> corresponds to a rate and "/s" is required to correctly express a rate unit. I would therefore disagree about making it optional. Should we add support for new modifiers? Would it make sense? Rate speed is mostly expressed in bits per second (bytes in our case), not us or ms. Would there be any use case?> Having both "/s" and "@<something>" in a single specification (e.g. > "1MB/s@20ms") seems a bit odd, is that right? What does it actually meanVif rate limiting is credit-based. It means that for "1MB/s@20ms", the available credit will be equivalent of the traffic you would have done at "1MB/s" during 20ms. Your credit will be replenished every 20ms. For your example: 1MB/s => 1,000,000 bytes per sec 20ms => 20,000 us This will results in a credit of 20,000 bytes replenished every 20,000 us. Should I find a way to explain this concept in the documentation? English is not my mother tongue, I'll have difficulty to explain the concept is a clear and concise way.> What are the semantics of window? If I say "1MB/s@500ms" then do I get > 0.5MB of credit every half second?See above.> The code and xend both seem to use "interval" rather than "window".The wording was retaken from the original commit [1] but I forgot to adjust it to its new context. What would be the correct wording? The original patch used "window", xend used "interval", another proposed patch used "timeslice", "interval" and "time period".> >> + >> +For example: > > Should spell out the meaning of the examples: >Good idea. Will do. [1] http://lists.xen.org/archives/html/xen-changelog/2006-03/msg00504.html -- Mathieu _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Mathieu Gagné
2012-Mar-20 18:25 UTC
Re: [PATCH 3 of 4] xl: add support for vif rate limiting
Forgot to reply to some comments. See below. On 3/20/12 7:21 AM, Ian Campbell wrote:> On Tue, 2012-03-20 at 01:28 +0000, Mathieu Gagné wrote: > >> +static void vif_parse_rate_bytes_per_sec(const char *bytes, uint32_t *bytes_per_sec) >> +{ >> + regex_t rec; >> + uint64_t tmp_bytes_per_sec = 0; >> + >> + regcomp(&rec, vif_bytes_per_sec_re, REG_EXTENDED); > > It seems that you use the regex only to check the syntax and then open > code the parsing? That strikes me as odd, if you are going to use a > regex parser you might as well use the matches returned from it.I originally used the matches but faced a myriad of problems and challenges. I not only had one problem (this patch) but two (this patch and the regex/matches). After much headache, I modified my approach to use regex for validation purpose only which simplified my code by a huge factor. IMO, the code was much more complex and difficult to read when I used matches. I'm open to modify it if someone can help me with this.> You could also combine the parsing of rate and interval into a single > regex and avoid the use of strtok etc in the outermost function.The original regex used sub-matches which I didn't know how or if it was possible to do in C: ^([0-9]+)([GMK]?)([Bb])/s(@([0-9]+)([mu]?)s)?$> You should return a syntax error if the string is invalid.The original code did not bailout on error and default to unlimited. Should we change this behavior in libxl/xl?>> + const char *p = bytes; >> + tmp_bytes_per_sec = strtoul(p, (char**)&p, 0); >> + if (*p == 'G' || *p == '\0') >> + tmp_bytes_per_sec *= 1000 * 1000 * 1000; >> + else if (*p == 'M') >> + tmp_bytes_per_sec *= 1000 * 1000; >> + else if (*p == 'K') >> + tmp_bytes_per_sec *= 1000; >> + if (*p == 'b' || *(p+1) == 'b') >> + tmp_bytes_per_sec /= 8;Minor optimization to do here. It's impossible to fall in the second case of the first condition: *p == '\0'. I'll fix it in v2. -- Mathieu _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Mathieu Gagné
2012-Mar-20 18:40 UTC
Re: [PATCH 3 of 4] xl: add support for vif rate limiting
On 3/20/12 7:26 AM, Ian Campbell wrote:> On Tue, 2012-03-20 at 11:16 +0000, Stefano Stabellini wrote: >> >> wouldn''t it make sense to just use a uint64_t for >> rate_bytes_per_interval and rate_interval_usecs? > > It needn''t be a barrier to this but FWIW Linux parses these into > "unsigned long", but it does at least appear to do proper overflow > checking.Xend/xm defaults to unlimited when bytes_per_interval or interval_usecs overflows a 32bit unsigned long. Internally, xen-netback will parse those values into "unsigned long" which would otherwise overflow/wrap if we used uint64_t: http://lxr.linux.no/#linux+v3.3/drivers/net/xen-netback/common.h#L86 -- Mathieu
Ian Campbell
2012-Mar-21 10:04 UTC
Re: [PATCH 3 of 4] xl: add support for vif rate limiting
On Tue, 2012-03-20 at 17:58 +0000, Mathieu Gagné wrote:> On 3/20/12 7:21 AM, Ian Campbell wrote: > > On Tue, 2012-03-20 at 01:28 +0000, Mathieu Gagné wrote: > >> > >> +### rate > >> + > >> +Specifies the rate at which the outgoing traffic will be limited to. This > >> +defaults to unlimited. > >> + > >> +The `rate` keyword supports an optional time window parameter for specifying > >> +the granularity of credit replenishment. It determines the frequency at which > >> +the vif transmission credit is replenished. The default window is 50ms. > > > > I think this needs to be more explicit/formal about what the syntax is. > > e.g.: > > > > The rate may be specified as "<RATE>/s" or optionally > > "<RATE>/s@<WINDOW>". > > > > <RATE> is in bytes and can accept suffixes Mb, Kb (list here) > > > > <WINDOW> is in microseconds and can accept suffixes<list>. The > > default is 50ms. > > > > Is the "/s" formally required? Does it take any modifiers (e.g. ms) > > It is formally required according to the original code within Xend/xm. > Wanting to be 100% backward compatible, I did not change this part. > > <RATE> corresponds to a rate and "/s" is required to correctly express a > rate unit. I would therefore disagree about making it optional.That makes sense to me.> Should we add support for new modifiers? Would it make sense? Rate speed > is mostly expressed in bits per second (bytes in our case), not us or > ms. Would there be any use case?If there are modifiers which make sense then we can add them.> > > Having both "/s" and "@<something>" in a single specification (e.g. > > "1MB/s@20ms") seems a bit odd, is that right? What does it actually mean > > Vif rate limiting is credit-based. It means that for "1MB/s@20ms", the > available credit will be equivalent of the traffic you would have done > at "1MB/s" during 20ms. Your credit will be replenished every 20ms. > > For your example: > 1MB/s => 1,000,000 bytes per sec > 20ms => 20,000 us > > This will results in a credit of 20,000 bytes replenished every 20,000 us. > > Should I find a way to explain this concept in the documentation? > English is not my mother tongue, I'll have difficulty to explain the > concept is a clear and concise way.Trust me, being a native speaker isn't helping me to come up with a way to express it either ;-) I think the paragraphs above, including the example, is a pretty good way of putting it. You can pretty much just copy that into the docs I think.> > The code and xend both seem to use "interval" rather than "window". > > The wording was retaken from the original commit [1] but I forgot to > adjust it to its new context. > > What would be the correct wording? The original patch used "window", > xend used "interval", another proposed patch used "timeslice", > "interval" and "time period".Personally I prefer interval. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2012-Mar-21 10:12 UTC
Re: [PATCH 3 of 4] xl: add support for vif rate limiting
On Tue, 2012-03-20 at 18:25 +0000, Mathieu Gagné wrote:> Forgot to reply to some comments. See below. > > On 3/20/12 7:21 AM, Ian Campbell wrote: > > On Tue, 2012-03-20 at 01:28 +0000, Mathieu Gagné wrote: > > > >> +static void vif_parse_rate_bytes_per_sec(const char *bytes, uint32_t *bytes_per_sec) > >> +{ > >> + regex_t rec; > >> + uint64_t tmp_bytes_per_sec = 0; > >> + > >> + regcomp(&rec, vif_bytes_per_sec_re, REG_EXTENDED); > > > > It seems that you use the regex only to check the syntax and then open > > code the parsing? That strikes me as odd, if you are going to use a > > regex parser you might as well use the matches returned from it. > > I originally used the matches but faced a myriad of problems and > challenges. I not only had one problem (this patch) but two (this patch > and the regex/matches). > > After much headache, I modified my approach to use regex for validation > purpose only which simplified my code by a huge factor. IMO, the code > was much more complex and difficult to read when I used matches. > > I'm open to modify it if someone can help me with this.OK, the code does seem reasonably simple and you've written and tested it so I'm inclined to suggest we go with it.> > You could also combine the parsing of rate and interval into a single > > regex and avoid the use of strtok etc in the outermost function. > > The original regex used sub-matches which I didn't know how or if it was > possible to do in C: > > ^([0-9]+)([GMK]?)([Bb])/s(@([0-9]+)([mu]?)s)?$ > > > > You should return a syntax error if the string is invalid. > > The original code did not bailout on error and default to unlimited. > Should we change this behavior in libxl/xl?I think so. While we strive to be compatible with xm in the common and valid cases I think it is reasonable for xl to differ (i.e. improve) in the handling of erroneous input or obscure corner cases. Ian.> >> + const char *p = bytes; > >> + tmp_bytes_per_sec = strtoul(p, (char**)&p, 0); > >> + if (*p == 'G' || *p == '\0') > >> + tmp_bytes_per_sec *= 1000 * 1000 * 1000; > >> + else if (*p == 'M') > >> + tmp_bytes_per_sec *= 1000 * 1000; > >> + else if (*p == 'K') > >> + tmp_bytes_per_sec *= 1000; > >> + if (*p == 'b' || *(p+1) == 'b') > >> + tmp_bytes_per_sec /= 8; > > Minor optimization to do here. It's impossible to fall in the second > case of the first condition: *p == '\0'. I'll fix it in v2. >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2012-Mar-21 10:23 UTC
Re: [PATCH 3 of 4] xl: add support for vif rate limiting
On Tue, 2012-03-20 at 18:40 +0000, Mathieu Gagné wrote:> On 3/20/12 7:26 AM, Ian Campbell wrote: > > On Tue, 2012-03-20 at 11:16 +0000, Stefano Stabellini wrote: > >> > >> wouldn't it make sense to just use a uint64_t for > >> rate_bytes_per_interval and rate_interval_usecs? > > > > It needn't be a barrier to this but FWIW Linux parses these into > > "unsigned long", but it does at least appear to do proper overflow > > checking. > > Xend/xm defaults to unlimited when bytes_per_interval or interval_usecs > overflows a 32bit unsigned long. > > Internally, xen-netback will parse those values into "unsigned long" > which would otherwise overflow/wrap if we used uint64_t: > > http://lxr.linux.no/#linux+v3.3/drivers/net/xen-netback/common.h#L86 >Right, the overflow handling of simply_strtoul turns out to leave something to be desired (it basically returns simple_strtoull without overflow checking). I guess it does say simple ;-) I think we should use 64 bit values in xl/libxl and document that the actual underlying limits are dependent on the underlying netback implementation. This will allow us to improve the kernel should we even need to without leaving us with a compatibility issue. Realistically no one is going to be asking for 40GB/s at the moment (at least not while expecting it to do anything meaningful...) Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel