WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH 3 of 5] libxl: add NIC QoS parameters

To: David Scott <dave.scott@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH 3 of 5] libxl: add NIC QoS parameters
From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Date: Mon, 28 Mar 2011 15:37:34 +0100
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Mon, 28 Mar 2011 07:38:51 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <3aab79c907a2c78f4e81.1301315197@ely>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <patchbomb.1301315194@ely> <3aab79c907a2c78f4e81.1301315197@ely>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Alpine 2.00 (DEB 1167 2008-08-23)
On Mon, 28 Mar 2011, David Scott wrote:
> # HG changeset patch
> # User David Scott <dave.scott@xxxxxxxxxxxxx>
> # Date 1301314652 -3600
> # Node ID 3aab79c907a2c78f4e81362944ee65ddf6f2cc6f
> # Parent  45326ad6a0d396bfcd3c83d209ab7a19d6499896
> libxl: add NIC QoS parameters
> 
> The parameters are:
>   qos_kib_per_sec:    maximum rate in KiB/sec
>   qos_timeslice_usec: time period over which the average rate is enforced in
>                       usec
> 
> One can now execute commands like
>   xl network-attach ... rate=1024,50000
> which should impose an average limit of 1MiB/sec, over intervals of 50ms
> 
> The "rate" key in the network backend is interpreted by netback. It wants:
>   bytes_per_interval, interval_length
> 
> Signed-off-by: David Scott <dave.scott@xxxxxxxxxxxxx>
> 

Thanks for the patch! Next time could you please generate the diff with
function names (diff -p)? It would make it much easier to read...


> diff -r 45326ad6a0d3 -r 3aab79c907a2 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c     Mon Mar 28 13:17:32 2011 +0100
> +++ b/tools/libxl/libxl.c     Mon Mar 28 13:17:32 2011 +0100
> @@ -1194,6 +1194,8 @@
>                 libxl_xen_script_dir_path()) < 0 )
>          return ERROR_FAIL;
>      nic_info->nictype = NICTYPE_IOEMU;
> +    nic_info->qos_kib_per_sec = 0;
> +    nic_info->qos_timeslice_usec = 0;
>      return 0;
>  }
>  
> @@ -1205,6 +1207,7 @@
>      libxl__device device;
>      char *dompath, **l;
>      unsigned int nb, rc;
> +    uint32_t bytes_per_interval; 
>  
>      front = flexarray_make(16, 1);
>      if (!front) {
> @@ -1263,6 +1266,11 @@
>      flexarray_append(back, libxl__strdup(&gc, nic->bridge));
>      flexarray_append(back, "handle");
>      flexarray_append(back, libxl__sprintf(&gc, "%d", nic->devid));
> +    if (nic->qos_timeslice_usec > 0) {
> +        bytes_per_interval = (uint32_t) (((uint64_t)nic->qos_kib_per_sec * 
> 1024L * (uint64_t)nic->qos_timeslice_usec) / 1000000L);
> +        flexarray_append(back, "rate");
> +        flexarray_append(back, libxl__sprintf(&gc, "%u,%u", 
> bytes_per_interval, nic->qos_timeslice_usec));
> +    }

Could you please use 80 characters columns? This line is very very long...


>  
>      flexarray_append(front, "backend-id");
>      flexarray_append(front, libxl__sprintf(&gc, "%d", nic->backend_domid));
> diff -r 45326ad6a0d3 -r 3aab79c907a2 tools/libxl/libxl.idl
> --- a/tools/libxl/libxl.idl   Mon Mar 28 13:17:32 2011 +0100
> +++ b/tools/libxl/libxl.idl   Mon Mar 28 13:17:32 2011 +0100
> @@ -225,6 +225,8 @@
>      ("ifname", string),
>      ("script", string),
>      ("nictype", libxl_nic_type),
> +    ("qos_kib_per_sec", uint32),
> +    ("qos_timeslice_usec", uint32),
>      ])

it is probably worth adding a comment here to explain what these
parameters are, like you did in the commit message


>  
>  libxl_device_net2 = Struct("device_net2", [
> diff -r 45326ad6a0d3 -r 3aab79c907a2 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c        Mon Mar 28 13:17:32 2011 +0100
> +++ b/tools/libxl/xl_cmdimpl.c        Mon Mar 28 13:17:32 2011 +0100
> @@ -880,7 +880,10 @@
>                          nic->backend_domid = 0;
>                      }
>                  } else if (!strcmp(p, "rate")) {
> -                    fprintf(stderr, "the rate parameter for vifs is 
> currently not supported\n");
> +                    if (sscanf(p2 + 1, "%u,%u", &(nic->qos_kib_per_sec), 
> &(nic->qos_timeslice_usec)) != 2) {
> +                        fprintf(stderr, "Specified rate parameter needs to 
> take the form: kib_per_sec,timeslice_usec\n");
> +                        break;
> +                    }
>                  } else if (!strcmp(p, "accel")) {
>                      fprintf(stderr, "the accel parameter for vifs is 
> currently not supported\n");
>                  }
> @@ -4298,6 +4301,10 @@
>              free(nic.model);
>              nic.model = strdup((*argv) + 6);
>          } else if (!strncmp("rate=", *argv, 5)) {
> +            if (sscanf((*argv) + 5, "%u,%u", &(nic.qos_kib_per_sec), 
> &(nic.qos_timeslice_usec)) != 2) {
> +                fprintf(stderr, "Specified rate parameter needs to take the 
> form: kib_per_sec,timeslice_usec\n");
> +                return 1;
> +            }
>          } else if (!strncmp("accel=", *argv, 6)) {
>          } else {
>              fprintf(stderr, "unrecognized argument `%s'\n", *argv);

considering that qos_kib_per_sec and qos_timeslice_usec are uint32_t,
sscanf should be called using PRIu32:

sscanf(p2 + 1, "%"PRIu32",%"PRIu32,

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>