[PATCH 0 of 5] Improvements to libxl VIF hotplug, unplug; xapi can now use these functions

[PATCH 0 of 5] Improvements to libxl VIF hotplug, unplug; xapi can now use these functions

[David Scott]

Improvements to libxl VIF hotplug/unplug; xapi can now use these functions

This patch set includes:
1. addition of VIF QoS parameters to libxl (this code is ported from xapi/
   xenops)
2. updates to the libxl ocaml bindings to use the new parameters
3. bugfixes to the "xl network-attach" command so the QoS settings can be
   more easily tested.

FYI the corresponding xapi changes are in a branch here:

https://github.com/djs55/xen-api/tree/hackathon

The only code which remains in the xapi VIF hotplug/unplug paths concerns
ethtool settings for VIFs. This could be ported separately if desired.

xapi used to have code for setting the device "protocol" field in xenstore,
depending on the PV kernel architecture. According to IanC, this was always
unnecessary for netback so it has been removed from xapi (and not added to 
libxl)

This was all work done last week at the xen hackathon. Comments, criticism 
etc. are welcome!

Signed-off-by: David Scott <dave.scott@eu.citrix.com>

[PATCH 1 of 5] libxl: fix memory management in "xl network-attach"

[David Scott]

# HG changeset patch
# User David Scott <dave.scott@eu.citrix.com>
# Date 1301314652 -3600
# Node ID 85361e3dab12827c6321931c9e09d02fb22578ee
# Parent  a65612bcbb921e98a8843157bf365e4ab16e8144
libxl: fix memory management in "xl network-attach"

The libxl_device_nic struct has strings which are initially strdup()ed and then free()ed in libxl_device_nic_destroy(). In the "network-attach" parser we need to free() the existing string and strdup((*argv) + N), rather than just copying the pointer.

Signed-off-by: David Scott <dave.scott@eu.citrix.com>

diff -r a65612bcbb92 -r 85361e3dab12 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Fri Mar 25 09:03:17 2011 +0000
+++ b/tools/libxl/xl_cmdimpl.c	Mon Mar 28 13:17:32 2011 +0100
@@ -4277,12 +4277,14 @@
                 nic.mac[i] = val;
             }
         } else if (!strncmp("bridge=", *argv, 7)) {
-            nic.bridge = (*argv) + 7;
+            free(nic.bridge);
+            nic.bridge = strdup((*argv) + 7);
         } else if (!strncmp("ip=", *argv, 3)) {
             free(nic.ip);
             nic.ip = strdup((*argv) + 3);
         } else if (!strncmp("script=", *argv, 6)) {
-            nic.script = (*argv) + 6;
+            free(nic.script);
+            nic.script = strdup((*argv) + 6);
         } else if (!strncmp("backend=", *argv, 8)) {
             if(libxl_name_to_domid(&ctx, ((*argv) + 8), &val)) {
                 fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n");
@@ -4290,9 +4292,11 @@
             }
             nic.backend_domid = val;
         } else if (!strncmp("vifname=", *argv, 8)) {
-            nic.ifname = (*argv) + 8;
+            free(nic.ifname);
+            nic.ifname = strdup((*argv) + 8);
         } else if (!strncmp("model=", *argv, 6)) {
-            nic.model = (*argv) + 6;
+            free(nic.model);
+            nic.model = strdup((*argv) + 6);
         } else if (!strncmp("rate=", *argv, 5)) {
         } else if (!strncmp("accel=", *argv, 6)) {
         } else {

Re: [PATCH 1 of 5] libxl: fix memory management in "xl network-attach"

[Stefano Stabellini]

On Mon, 28 Mar 2011, David Scott wrote:
> # HG changeset patch
> # User David Scott <dave.scott@eu.citrix.com>
> # Date 1301314652 -3600
> # Node ID 85361e3dab12827c6321931c9e09d02fb22578ee
> # Parent  a65612bcbb921e98a8843157bf365e4ab16e8144
> libxl: fix memory management in "xl network-attach"
> 
> The libxl_device_nic struct has strings which are initially strdup()ed and then free()ed in libxl_device_nic_destroy(). In the "network-attach" parser we need to free() the existing string and strdup((*argv) + N), rather than just copying the pointer.
> 
> Signed-off-by: David Scott <dave.scott@eu.citrix.com>
> 

Thanks for the patch!

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

Re: [PATCH 1 of 5] libxl: fix memory management in "xl network-attach"

[Ian Campbell]

On Mon, 2011-03-28 at 13:26 +0100, Dave Scott wrote:
> # HG changeset patch
> # User David Scott <dave.scott@eu.citrix.com>
> # Date 1301314652 -3600
> # Node ID 85361e3dab12827c6321931c9e09d02fb22578ee
> # Parent  a65612bcbb921e98a8843157bf365e4ab16e8144
> libxl: fix memory management in "xl network-attach"
> 
> The libxl_device_nic struct has strings which are initially strdup()ed
> and then free()ed in libxl_device_nic_destroy(). In the
> "network-attach" parser we need to free() the existing string and
> strdup((*argv) + N), rather than just copying the pointer.

Ideally we would have a parsing routine shared with the cfg file parser
but for now this is a definite improvement:

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> 
> Signed-off-by: David Scott <dave.scott@eu.citrix.com>
> 
> diff -r a65612bcbb92 -r 85361e3dab12 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Fri Mar 25 09:03:17 2011 +0000
> +++ b/tools/libxl/xl_cmdimpl.c	Mon Mar 28 13:17:32 2011 +0100
> @@ -4277,12 +4277,14 @@
>                  nic.mac[i] = val;
>              }
>          } else if (!strncmp("bridge=", *argv, 7)) {
> -            nic.bridge = (*argv) + 7;
> +            free(nic.bridge);
> +            nic.bridge = strdup((*argv) + 7);
>          } else if (!strncmp("ip=", *argv, 3)) {
>              free(nic.ip);
>              nic.ip = strdup((*argv) + 3);
>          } else if (!strncmp("script=", *argv, 6)) {
> -            nic.script = (*argv) + 6;
> +            free(nic.script);
> +            nic.script = strdup((*argv) + 6);
>          } else if (!strncmp("backend=", *argv, 8)) {
>              if(libxl_name_to_domid(&ctx, ((*argv) + 8), &val)) {
>                  fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n");
> @@ -4290,9 +4292,11 @@
>              }
>              nic.backend_domid = val;
>          } else if (!strncmp("vifname=", *argv, 8)) {
> -            nic.ifname = (*argv) + 8;
> +            free(nic.ifname);
> +            nic.ifname = strdup((*argv) + 8);
>          } else if (!strncmp("model=", *argv, 6)) {
> -            nic.model = (*argv) + 6;
> +            free(nic.model);
> +            nic.model = strdup((*argv) + 6);
>          } else if (!strncmp("rate=", *argv, 5)) {
>          } else if (!strncmp("accel=", *argv, 6)) {
>          } else {
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

[PATCH 2 of 5] tools: ocaml: move the nic_info record into a module to the field labels live in a separate namespace

[David Scott]

# HG changeset patch
# User David Scott <dave.scott@eu.citrix.com>
# Date 1301314652 -3600
# Node ID 45326ad6a0d396bfcd3c83d209ab7a19d6499896
# Parent  85361e3dab12827c6321931c9e09d02fb22578ee
tools: ocaml: move the nic_info record into a module to the field labels live in a separate namespace.

Otherwise the redefinition of fields like "backend_domid" "devid" et al make the records un-instantiable.

Signed-off-by: David Scott <dave.scott@eu.citrix.com>

diff -r 85361e3dab12 -r 45326ad6a0d3 tools/ocaml/libs/xl/xl.ml
--- a/tools/ocaml/libs/xl/xl.ml	Mon Mar 28 13:17:32 2011 +0100
+++ b/tools/ocaml/libs/xl/xl.ml	Mon Mar 28 13:17:32 2011 +0100
@@ -97,18 +97,20 @@
 	| NICTYPE_IOEMU
 	| NICTYPE_VIF
 
-type nic_info =
-{
-	backend_domid : domid;
-	devid : int;
-	mtu : int;
-	model : string;
-	mac : int array;
-	bridge : string;
-	ifname : string;
-	script : string;
-	nictype : nic_type;
-}
+module Nic_info = struct
+	type t =
+	{
+		backend_domid : domid;
+		devid : int;
+		mtu : int;
+		model : string;
+		mac : int array;
+		bridge : string;
+		ifname : string;
+		script : string;
+		nictype : nic_type;
+	}
+end
 
 type console_type =
 	| CONSOLETYPE_XENCONSOLED
@@ -179,7 +181,7 @@
 external disk_add : disk_info -> domid -> unit = "stub_xl_disk_add"
 external disk_remove : disk_info -> domid -> unit = "stub_xl_disk_remove"
 
-external nic_add : nic_info -> domid -> unit = "stub_xl_nic_add"
+external nic_add : Nic_info.t -> domid -> unit = "stub_xl_nic_add"
 external nic_remove : disk_info -> domid -> unit = "stub_xl_nic_remove"
 
 external console_add : console_info -> build_state -> domid -> unit = "stub_xl_console_add"
diff -r 85361e3dab12 -r 45326ad6a0d3 tools/ocaml/libs/xl/xl.mli
--- a/tools/ocaml/libs/xl/xl.mli	Mon Mar 28 13:17:32 2011 +0100
+++ b/tools/ocaml/libs/xl/xl.mli	Mon Mar 28 13:17:32 2011 +0100
@@ -97,18 +97,20 @@
 	| NICTYPE_IOEMU
 	| NICTYPE_VIF
 
-type nic_info =
-{
-	backend_domid : domid;
-	devid : int;
-	mtu : int;
-	model : string;
-	mac : int array;
-	bridge : string;
-	ifname : string;
-	script : string;
-	nictype : nic_type;
-}
+module Nic_info : sig
+	type t =
+	{
+		backend_domid : domid;
+		devid : int;
+		mtu : int;
+		model : string;
+		mac : int array;
+		bridge : string;
+		ifname : string;
+		script : string;
+		nictype : nic_type;
+	}
+end
 
 type console_type =
 	| CONSOLETYPE_XENCONSOLED
@@ -179,7 +181,7 @@
 external disk_add : disk_info -> domid -> unit = "stub_xl_disk_add"
 external disk_remove : disk_info -> domid -> unit = "stub_xl_disk_remove"
 
-external nic_add : nic_info -> domid -> unit = "stub_xl_nic_add"
+external nic_add : Nic_info.t -> domid -> unit = "stub_xl_nic_add"
 external nic_remove : disk_info -> domid -> unit = "stub_xl_nic_remove"
 
 external console_add : console_info -> build_state -> domid -> unit = "stub_xl_console_add"

Re: [PATCH 2 of 5] tools: ocaml: move the nic_info record into a module to the field labels live in a separate namespace

[Ian Campbell]

On Mon, 2011-03-28 at 13:26 +0100, Dave Scott wrote:
> # HG changeset patch
> # User David Scott <dave.scott@eu.citrix.com>
> # Date 1301314652 -3600
> # Node ID 45326ad6a0d396bfcd3c83d209ab7a19d6499896
> # Parent  85361e3dab12827c6321931c9e09d02fb22578ee
> tools: ocaml: move the nic_info record into a module to the field
> labels live in a separate namespace.
> 
> Otherwise the redefinition of fields like "backend_domid" "devid" et
> al make the records un-instantiable.

I suppose it is the case that all the defined data types ought to have
this done? Doing it for all but one of the types which clash would be
confusing...

I guess that Foo.t is an ocaml idiom, so I won't comment on it ;-)

The libxl type which this structure mirrors is called libxl_device_nic
so perhaps to aid future autogeneratability (something I was working on
at the hackathon) we could use module Device_nic? I'm assuming the full
name as external modules see it would be Xl.Device_nic, if not perhaps
Xl_device_nic?

I ran into some wrinkles with the autogenerated bindings since ocaml
appears to lack a #include style syntax which allow one to easily inject
a glob of generated code into the middle of an otherwise hand-written
source code file. I got around this with:
	sed -e '/^(\* @@LIBXL_TYPES@@ \*)$$/r_libxl_types.mli.in < xl.mli.in > xl.mli
(I'm not sure if I am proud of this or not).

Does putting the datatypes into a module (or modules) ease this at all?
It looks like we might end up with device_nic.ml, device_disk.ml etc for
each generated type, which is not ideal, and probably not better than
using sed.

Having moved the data type into a module does it also make sense to move
the relevant functions too e.g. Xl.Device_nic.add : Nic_info.t -> domid
-> unit etc?

Unless you can use the "module Foo" syntax multiple times to extend the
contents of the module (i.e. to do the autogenerated stuff separately
from the handwritten stuff so that it can all be included at once) this
looks like it would have the same issues with putting each module in a
separate file I worried about above.

> -external nic_add : nic_info -> domid -> unit = "stub_xl_nic_add"
> +external nic_add : Nic_info.t -> domid -> unit = "stub_xl_nic_add"
>  external nic_remove : disk_info -> domid -> unit = "stub_xl_nic_remove"

One of these parameters is not like the others ;-0 I see you caught that
in patch 5/5.

Ian.

[0] i..we

RE: [PATCH 2 of 5] tools: ocaml: move the nic_info record into a module to the field labels live in a separate namespace

[Dave Scott]

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

Hi Ian,

> On Mon, 2011-03-28 at 13:26 +0100, Dave Scott wrote:
> > tools: ocaml: move the nic_info record into a module to the field
> > labels live in a separate namespace.
> >
> > Otherwise the redefinition of fields like "backend_domid" "devid" et
> > al make the records un-instantiable.

Ian Campbell wrote:
> I suppose it is the case that all the defined data types ought to have
> this done? Doing it for all but one of the types which clash would be
> confusing...

Hm, there is nothing worse than seeing code which has been partially rewritten N different ways :-) I'll modify the patch to ripple the change through the rest of the file to keep it consistent...

> I guess that Foo.t is an ocaml idiom, so I won't comment on it ;-)

I think this is fairly common in ocaml, e.g. in the standard library Set and Map modules. For defining records like these there are two fairly common approaches: one to do the Foo.t thing; the other to prefix each field with "foo_". More recent ocaml compilers have a nice syntax hack where you can omit all but one of the "Foo." prefixes on fields when constructing values, the rest get inferred like this:

let x = { Foo.x = 1; y = 2; z = 3 }

there's no direct equivalent for "foo_" so you have to write

let x = { foo_x = 1; foo_y = 2; foo_z = 3 }

Maybe it just depends what you're used to :-)

> The libxl type which this structure mirrors is called libxl_device_nic
> so perhaps to aid future autogeneratability (something I was working on
> at the hackathon) we could use module Device_nic? I'm assuming the full
> name as external modules see it would be Xl.Device_nic, if not perhaps
> Xl_device_nic?

Good idea-- Xl.Device_nic would work ok.

> I ran into some wrinkles with the autogenerated bindings since ocaml
> appears to lack a #include style syntax which allow one to easily
> inject
> a glob of generated code into the middle of an otherwise hand-written
> source code file. I got around this with:
> 	sed -e '/^(\* @@LIBXL_TYPES@@ \*)$$/r_libxl_types.mli.in <
> xl.mli.in > xl.mli
> (I'm not sure if I am proud of this or not).

It does lack a general #include directive but there is one for *whole* modules and module interfaces:

$ ocaml
        Objective Caml version 3.11.2

# module M = struct
    let x = 1
  end
  module N = struct
    include M
    let y = 2
  end
  ;;
module M : sig val x : int end
module N : sig val x : int val y : int end

So perhaps you could put a "include Xl_autogen" in the middle of your hand-written bit of code?

> Does putting the datatypes into a module (or modules) ease this at all?
> It looks like we might end up with device_nic.ml, device_disk.ml etc
> for
> each generated type, which is not ideal, and probably not better than
> using sed.
> 
> Having moved the data type into a module does it also make sense to
> move
> the relevant functions too e.g. Xl.Device_nic.add : Nic_info.t -> domid
> -> unit etc?

That would make the generated ocamldoc look a bit nicer. If the plan is to stick to this kind of naming convention in libxl i.e. with a "struct xl_thing" and functions with names "xl_thing_do" then it's probably worth it.

> Unless you can use the "module Foo" syntax multiple times to extend the
> contents of the module

Although you can't extend an existing module M you can define a new module N which includes M and other stuff, as above. Perhaps that'll be good enough?

Cheers,
Dave


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

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

[PATCH 3 of 5] libxl: add NIC QoS parameters

[David Scott]

# HG changeset patch
# User David Scott <dave.scott@eu.citrix.com>
# 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@eu.citrix.com>

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));
+    }
 
     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),
     ])
 
 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);

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

[Stefano Stabellini]

On Mon, 28 Mar 2011, David Scott wrote:
> # HG changeset patch
> # User David Scott <dave.scott@eu.citrix.com>
> # 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@eu.citrix.com>
> 

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,

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

[Ian Campbell]

On Mon, 2011-03-28 at 15:37 +0100, Stefano Stabellini wrote:
> On Mon, 28 Mar 2011, David Scott wrote:
> > # HG changeset patch
> > # User David Scott <dave.scott@eu.citrix.com>
> > # 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@eu.citrix.com>
> > 
> 
> 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...

Hint: Add to ~/.hgrc:

        [diff]
        showfunc = True

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

Comment is the fourth element of the field tuple, the third is
const-ness so you would need e.g.
	("qos_kib_per_sec", uint32, False, "maximum rate in KiB/sec")
(I really should get round to supporting named fields in the IDL...)

We don't have a strict type naming convention but elsewhere we have
max_memkb and generally *kb.

Ian.

[PATCH 4 of 5] tools: ocaml: add NIC QoS parameters to the ocaml libxl interface

[David Scott]

# HG changeset patch
# User David Scott <dave.scott@eu.citrix.com>
# Date 1301314652 -3600
# Node ID 455cc73ea6268d54bc94e4c39ee954cef107f88b
# Parent  3aab79c907a2c78f4e81362944ee65ddf6f2cc6f
tools: ocaml: add NIC QoS parameters to the ocaml libxl interface

Signed-off-by: David Scott <dave.scott@eu.citrix.com>

diff -r 3aab79c907a2 -r 455cc73ea626 tools/ocaml/libs/xl/xl.ml
--- a/tools/ocaml/libs/xl/xl.ml	Mon Mar 28 13:17:32 2011 +0100
+++ b/tools/ocaml/libs/xl/xl.ml	Mon Mar 28 13:17:32 2011 +0100
@@ -109,6 +109,8 @@
 		ifname : string;
 		script : string;
 		nictype : nic_type;
+		qos_kib_per_sec : int32;
+		qos_timeslice_usec : int32;
 	}
 end
 
diff -r 3aab79c907a2 -r 455cc73ea626 tools/ocaml/libs/xl/xl.mli
--- a/tools/ocaml/libs/xl/xl.mli	Mon Mar 28 13:17:32 2011 +0100
+++ b/tools/ocaml/libs/xl/xl.mli	Mon Mar 28 13:17:32 2011 +0100
@@ -109,6 +109,8 @@
 		ifname : string;
 		script : string;
 		nictype : nic_type;
+		qos_kib_per_sec : int32;
+		qos_timeslice_usec : int32;
 	}
 end
 
diff -r 3aab79c907a2 -r 455cc73ea626 tools/ocaml/libs/xl/xl_stubs.c
--- a/tools/ocaml/libs/xl/xl_stubs.c	Mon Mar 28 13:17:32 2011 +0100
+++ b/tools/ocaml/libs/xl/xl_stubs.c	Mon Mar 28 13:17:32 2011 +0100
@@ -225,7 +225,8 @@
 	c_val->ifname = dup_String_val(gc, Field(v, 6));
 	c_val->script = dup_String_val(gc, Field(v, 7));
 	c_val->nictype = (Int_val(Field(v, 8))) + NICTYPE_IOEMU;
-
+	c_val->qos_kib_per_sec = (Int_val(Field(v, 9)));
+	c_val->qos_timeslice_usec = (Int_val(Field(v, 10)));
 out:
 	CAMLreturn(ret);
 }

Re: [PATCH 4 of 5] tools: ocaml: add NIC QoS parameters to the ocaml libxl interface

[Ian Campbell]

On Mon, 2011-03-28 at 13:26 +0100, Dave Scott wrote:
> # HG changeset patch
> # User David Scott <dave.scott@eu.citrix.com>
> # Date 1301314652 -3600
> # Node ID 455cc73ea6268d54bc94e4c39ee954cef107f88b
> # Parent  3aab79c907a2c78f4e81362944ee65ddf6f2cc6f
> tools: ocaml: add NIC QoS parameters to the ocaml libxl interface
> 
> Signed-off-by: David Scott <dave.scott@eu.citrix.com>

Looks good, modulo the impact of comments on previous patches:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

> 
> diff -r 3aab79c907a2 -r 455cc73ea626 tools/ocaml/libs/xl/xl.ml
> --- a/tools/ocaml/libs/xl/xl.ml	Mon Mar 28 13:17:32 2011 +0100
> +++ b/tools/ocaml/libs/xl/xl.ml	Mon Mar 28 13:17:32 2011 +0100
> @@ -109,6 +109,8 @@
>  		ifname : string;
>  		script : string;
>  		nictype : nic_type;
> +		qos_kib_per_sec : int32;
> +		qos_timeslice_usec : int32;
>  	}
>  end
>  
> diff -r 3aab79c907a2 -r 455cc73ea626 tools/ocaml/libs/xl/xl.mli
> --- a/tools/ocaml/libs/xl/xl.mli	Mon Mar 28 13:17:32 2011 +0100
> +++ b/tools/ocaml/libs/xl/xl.mli	Mon Mar 28 13:17:32 2011 +0100
> @@ -109,6 +109,8 @@
>  		ifname : string;
>  		script : string;
>  		nictype : nic_type;
> +		qos_kib_per_sec : int32;
> +		qos_timeslice_usec : int32;
>  	}
>  end
>  
> diff -r 3aab79c907a2 -r 455cc73ea626 tools/ocaml/libs/xl/xl_stubs.c
> --- a/tools/ocaml/libs/xl/xl_stubs.c	Mon Mar 28 13:17:32 2011 +0100
> +++ b/tools/ocaml/libs/xl/xl_stubs.c	Mon Mar 28 13:17:32 2011 +0100
> @@ -225,7 +225,8 @@
>  	c_val->ifname = dup_String_val(gc, Field(v, 6));
>  	c_val->script = dup_String_val(gc, Field(v, 7));
>  	c_val->nictype = (Int_val(Field(v, 8))) + NICTYPE_IOEMU;
> -
> +	c_val->qos_kib_per_sec = (Int_val(Field(v, 9)));
> +	c_val->qos_timeslice_usec = (Int_val(Field(v, 10)));
>  out:
>  	CAMLreturn(ret);
>  }
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

[PATCH 5 of 5] tools: ocaml: rename nic_remove to nic_del, for consistency with libxl

[David Scott]

# HG changeset patch
# User David Scott <dave.scott@eu.citrix.com>
# Date 1301314652 -3600
# Node ID bb7e56721316582cc42527e97e7651a8a369c33c
# Parent  455cc73ea6268d54bc94e4c39ee954cef107f88b
tools: ocaml: rename nic_remove to nic_del, for consistency with libxl.

Signed-off-by: David Scott <dave.scott@eu.citrix.com>

diff -r 455cc73ea626 -r bb7e56721316 tools/ocaml/libs/xl/xl.ml
--- a/tools/ocaml/libs/xl/xl.ml	Mon Mar 28 13:17:32 2011 +0100
+++ b/tools/ocaml/libs/xl/xl.ml	Mon Mar 28 13:17:32 2011 +0100
@@ -184,7 +184,7 @@
 external disk_remove : disk_info -> domid -> unit = "stub_xl_disk_remove"
 
 external nic_add : Nic_info.t -> domid -> unit = "stub_xl_nic_add"
-external nic_remove : disk_info -> domid -> unit = "stub_xl_nic_remove"
+external nic_del : Nic_info.t -> domid -> unit = "stub_xl_nic_del"
 
 external console_add : console_info -> build_state -> domid -> unit = "stub_xl_console_add"
 
diff -r 455cc73ea626 -r bb7e56721316 tools/ocaml/libs/xl/xl.mli
--- a/tools/ocaml/libs/xl/xl.mli	Mon Mar 28 13:17:32 2011 +0100
+++ b/tools/ocaml/libs/xl/xl.mli	Mon Mar 28 13:17:32 2011 +0100
@@ -184,7 +184,7 @@
 external disk_remove : disk_info -> domid -> unit = "stub_xl_disk_remove"
 
 external nic_add : Nic_info.t -> domid -> unit = "stub_xl_nic_add"
-external nic_remove : disk_info -> domid -> unit = "stub_xl_nic_remove"
+external nic_del : Nic_info.t -> domid -> unit = "stub_xl_nic_del"
 
 external console_add : console_info -> build_state -> domid -> unit = "stub_xl_console_add"
 
diff -r 455cc73ea626 -r bb7e56721316 tools/ocaml/libs/xl/xl_stubs.c
--- a/tools/ocaml/libs/xl/xl_stubs.c	Mon Mar 28 13:17:32 2011 +0100
+++ b/tools/ocaml/libs/xl/xl_stubs.c	Mon Mar 28 13:17:32 2011 +0100
@@ -416,7 +416,7 @@
 	CAMLreturn(Val_unit);
 }
 
-value stub_xl_nic_remove(value info, value domid)
+value stub_xl_nic_del(value info, value domid)
 {
 	CAMLparam2(info, domid);
 	libxl_device_nic c_info;

Re: [PATCH 5 of 5] tools: ocaml: rename nic_remove to nic_del, for consistency with libxl

[Ian Campbell]

On Mon, 2011-03-28 at 13:26 +0100, Dave Scott wrote:
> # HG changeset patch
> # User David Scott <dave.scott@eu.citrix.com>
> # Date 1301314652 -3600
> # Node ID bb7e56721316582cc42527e97e7651a8a369c33c
> # Parent  455cc73ea6268d54bc94e4c39ee954cef107f88b
> tools: ocaml: rename nic_remove to nic_del, for consistency with libxl.

At least disk has the same issue, other libxl functions use _remove and
others use _{hard,soft}_shutdown. Sigh :-(

> Signed-off-by: David Scott <dave.scott@eu.citrix.com>
> 
> diff -r 455cc73ea626 -r bb7e56721316 tools/ocaml/libs/xl/xl.ml
> --- a/tools/ocaml/libs/xl/xl.ml	Mon Mar 28 13:17:32 2011 +0100
> +++ b/tools/ocaml/libs/xl/xl.ml	Mon Mar 28 13:17:32 2011 +0100
> @@ -184,7 +184,7 @@
>  external disk_remove : disk_info -> domid -> unit = "stub_xl_disk_remove"
>  
>  external nic_add : Nic_info.t -> domid -> unit = "stub_xl_nic_add"
> -external nic_remove : disk_info -> domid -> unit = "stub_xl_nic_remove"
> +external nic_del : Nic_info.t -> domid -> unit = "stub_xl_nic_del"
>  
>  external console_add : console_info -> build_state -> domid -> unit = "stub_xl_console_add"
>  
> diff -r 455cc73ea626 -r bb7e56721316 tools/ocaml/libs/xl/xl.mli
> --- a/tools/ocaml/libs/xl/xl.mli	Mon Mar 28 13:17:32 2011 +0100
> +++ b/tools/ocaml/libs/xl/xl.mli	Mon Mar 28 13:17:32 2011 +0100
> @@ -184,7 +184,7 @@
>  external disk_remove : disk_info -> domid -> unit = "stub_xl_disk_remove"
>  
>  external nic_add : Nic_info.t -> domid -> unit = "stub_xl_nic_add"
> -external nic_remove : disk_info -> domid -> unit = "stub_xl_nic_remove"
> +external nic_del : Nic_info.t -> domid -> unit = "stub_xl_nic_del"
>  
>  external console_add : console_info -> build_state -> domid -> unit = "stub_xl_console_add"
>  
> diff -r 455cc73ea626 -r bb7e56721316 tools/ocaml/libs/xl/xl_stubs.c
> --- a/tools/ocaml/libs/xl/xl_stubs.c	Mon Mar 28 13:17:32 2011 +0100
> +++ b/tools/ocaml/libs/xl/xl_stubs.c	Mon Mar 28 13:17:32 2011 +0100
> @@ -416,7 +416,7 @@
>  	CAMLreturn(Val_unit);
>  }
>  
> -value stub_xl_nic_remove(value info, value domid)
> +value stub_xl_nic_del(value info, value domid)
>  {
>  	CAMLparam2(info, domid);
>  	libxl_device_nic c_info;
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel