|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [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@xxxxxxxx>
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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |