[PATCH 0 of 4] xl: add support for vif rate limiting

[PATCH 0 of 4] xl: add support for vif rate limiting

[Mathieu Gagné]

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

[PATCH 1 of 4] xl: cleanup indentation

[Mathieu Gagné]

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

Re: [PATCH 1 of 4] xl: cleanup indentation

[Ian Campbell]

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

Re: [PATCH 1 of 4] xl: cleanup indentation

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 762 bytes --]

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)
> 

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 2 of 4] xl: xl network-attach -N (dry run) option

[Mathieu Gagné]

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

Re: [PATCH 2 of 4] xl: xl network-attach -N (dry run) option

[Ian Campbell]

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

Re: [PATCH 2 of 4] xl: xl network-attach -N (dry run) option

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 157 bytes --]

On Tue, 20 Mar 2012, Mathieu Gagné wrote:
> Add dryrun for testing and debugging purposes.

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 3 of 4] xl: add support for vif rate limiting

[Mathieu Gagné]

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

Re: [PATCH 3 of 4] xl: add support for vif rate limiting

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 6869 bytes --]

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?

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3 of 4] xl: add support for vif rate limiting

[Ian Campbell]

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

Re: [PATCH 3 of 4] xl: add support for vif rate limiting

[Mathieu Gagné]

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

Re: [PATCH 3 of 4] xl: add support for vif rate limiting

[Ian Campbell]

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

Re: [PATCH 3 of 4] xl: add support for vif rate limiting

[Ian Campbell]

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

Re: [PATCH 3 of 4] xl: add support for vif rate limiting

[Mathieu Gagné]

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 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.


> 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

Re: [PATCH 3 of 4] xl: add support for vif rate limiting

[Ian Campbell]

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

Re: [PATCH 3 of 4] xl: add support for vif rate limiting

[Mathieu Gagné]

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

Re: [PATCH 3 of 4] xl: add support for vif rate limiting

[Ian Campbell]

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

[PATCH 4 of 4] xl: add "check-xl-vif-parse" test script

[Mathieu Gagné]

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

Re: [PATCH 4 of 4] xl: add "check-xl-vif-parse" test script

[Ian Campbell]

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=foo

I 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@50ms

Took 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@50ms

40Gb 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@foo

This 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

Re: [PATCH 0 of 4] xl: add support for vif rate limiting

[Ian Campbell]

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