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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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