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

PATCH New David Scott 3 participants 14 messages Mar 28, 2011 – Mar 30, 2011

lore

Share & tools

lore

David Scott

author

2011-03-28 12:26 UTC #0 lore raw

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

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 <[email protected]>

David Scott

author

2011-03-28 12:26 UTC #1 lore raw

PATCH 1/1 libxl: fix memory management in "xl network-attach"

# HG changeset patch
# User David Scott <[email protected]> # 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 <[email protected]>

tools/libxl/xl_cmdimpl.c +8 -4

diff -r a65612bcbb92 -r 85361e3dab12 tools/libxl/xl_cmdimpl.c
@@ -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 {

Stefano Stabellini

Acked-by on v1 1/1

2011-03-28 14:37 UTC #2 lore raw

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

Quoted 11 lines · David Scott

# HG changeset patch
# User David Scott <[email protected]> # 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 <[email protected]>

Thanks for the patch!

Acked-by Stefano Stabellini <[email protected]>

Ian Campbell

Acked-by on v1 1/1

2011-03-29 08:30 UTC #3 lore raw

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

In reply to David Scott

Quoted 11 lines · Dave Scott

# HG changeset patch
# User David Scott <[email protected]> # 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 <[email protected]>

Quoted 42 lines

Signed-off-by David Scott <[email protected]>

diff -r a65612bcbb92 -r 85361e3dab12 tools/libxl/xl_cmdimpl.c

tools/libxl/xl_cmdimpl.c +8 -4

@@ -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
[email protected]
http://lists.xensource.com/xen-devel

David Scott

author

2011-03-28 12:26 UTC #4 lore raw

PATCH 1/1 tools: ocaml: move the nic_info record into a module to the field labels live in a separate namespace

In reply to David Scott

# HG changeset patch
# User David Scott <[email protected]>
# 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 <[email protected]>

tools/ocaml/libs/xl/xl.ml +15 ~~-13~~

diff -r 85361e3dab12 -r 45326ad6a0d3 tools/ocaml/libs/xl/xl.ml
@@ -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"

tools/ocaml/libs/xl/xl.mli +15 ~~-13~~

diff -r 85361e3dab12 -r 45326ad6a0d3 tools/ocaml/libs/xl/xl.mli
@@ -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"

Ian Campbell

2011-03-29 08:42 UTC #5 lore raw

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

Quoted 10 lines · Dave Scott

# HG changeset patch
# User David Scott <[email protected]>
# 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.

tools/ocaml/libs/xl/xl.ml line 185

Quoted 3 lines

tools/ocaml/libs/xl/xl.ml +1 -1

-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

Dave Scott

author

2011-03-30 17:18 UTC #6 lore raw

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

Hi Ian,

Quoted 6 lines · Dave Scott

Quoted 5 lines
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.

Quoted 3 lines · Ian Campbell

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

Quoted 1 line

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

Quoted 5 lines

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.

Quoted 8 lines

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?

Quoted 10 lines

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.

Quoted 2 lines

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

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

David Scott

author

2011-03-28 12:26 UTC #7 lore raw

PATCH 1/1 libxl: add NIC QoS parameters

In reply to David Scott

# HG changeset patch
# User David Scott <[email protected]> # 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 <[email protected]>

tools/libxl/libxl.c +8 -0

diff -r 45326ad6a0d3 -r 3aab79c907a2 tools/libxl/libxl.c
@@ -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));

tools/libxl/libxl.idl +2 -0

diff -r 45326ad6a0d3 -r 3aab79c907a2 tools/libxl/libxl.idl
@@ -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", [

tools/libxl/xl_cmdimpl.c +8 -1

diff -r 45326ad6a0d3 -r 3aab79c907a2 tools/libxl/xl_cmdimpl.c
@@ -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);

Stefano Stabellini

2011-03-28 14:37 UTC #8 lore raw

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

Quoted 12 lines · David Scott

# HG changeset patch
# User David Scott <[email protected]> # 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

Quoted 9 lines

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 <[email protected]>

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

Quoted 21 lines

diff -r 45326ad6a0d3 -r 3aab79c907a2 tools/libxl/libxl.c

tools/libxl/libxl.c +3 -0

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

tools/libxl/libxl.c line 1273

Quoted 8 lines

tools/libxl/libxl.c +5 -0

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

tools/libxl/libxl.idl line 230

Quoted 13 lines

tools/libxl/libxl.c +0 -0

     flexarray_append(front, "backend-id");
     flexarray_append(front, libxl__sprintf(&gc, "%d", nic->backend_domid));

diff -r 45326ad6a0d3 -r 3aab79c907a2 tools/libxl/libxl.idl

tools/libxl/libxl.idl +2 -0

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

Quoted 18 lines

tools/libxl/libxl.idl +0 -0

 libxl_device_net2 = Struct("device_net2", [

diff -r 45326ad6a0d3 -r 3aab79c907a2 tools/libxl/xl_cmdimpl.c

tools/libxl/xl_cmdimpl.c +4 -1

@@ -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");
                 }

tools/libxl/xl_cmdimpl.c line 4310

Quoted 10 lines

tools/libxl/xl_cmdimpl.c +4 -0

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

Ian Campbell

2011-03-29 08:48 UTC #9 lore raw

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

Quoted 17 lines · Stefano Stabellini

Quoted 16 lines · David Scott
# HG changeset patch
# User David Scott <[email protected]> # 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

Quoted 8 lines

Quoted 5 lines
The "rate" key in the network backend is interpreted by netback. It wants:
bytes_per_interval, interval_length
Signed-off-by David Scott <[email protected]>
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

Quoted 5 lines

Quoted 5 lines
diff -r 45326ad6a0d3 -r 3aab79c907a2 tools/libxl/libxl.idl
tools/libxl/libxl.idl +0 -0
@@ -225,6 +225,8 @@
     ("ifname", string),

Quoted 8 lines

tools/libxl/libxl.idl line 230
Quoted 5 lines
tools/libxl/libxl.idl +2 -0
     ("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.

David Scott

author

2011-03-28 12:26 UTC #10 lore raw

PATCH 1/1 tools: ocaml: add NIC QoS parameters to the ocaml libxl interface

In reply to David Scott

# HG changeset patch
# User David Scott <[email protected]>
# Date 1301314652 -3600
# Node ID 455cc73ea6268d54bc94e4c39ee954cef107f88b
# Parent 3aab79c907a2c78f4e81362944ee65ddf6f2cc6f
tools: ocaml: add NIC QoS parameters to the ocaml libxl interface

Signed-off-by David Scott <[email protected]>

tools/ocaml/libs/xl/xl.ml +2 -0

diff -r 3aab79c907a2 -r 455cc73ea626 tools/ocaml/libs/xl/xl.ml
@@ -109,6 +109,8 @@
 		ifname : string;
 		script : string;
 		nictype : nic_type;
+		qos_kib_per_sec : int32;
+		qos_timeslice_usec : int32;
 	}
 end

tools/ocaml/libs/xl/xl.mli +2 -0

diff -r 3aab79c907a2 -r 455cc73ea626 tools/ocaml/libs/xl/xl.mli
@@ -109,6 +109,8 @@
 		ifname : string;
 		script : string;
 		nictype : nic_type;
+		qos_kib_per_sec : int32;
+		qos_timeslice_usec : int32;
 	}
 end

tools/ocaml/libs/xl/xl_stubs.c +2 -1

diff -r 3aab79c907a2 -r 455cc73ea626 tools/ocaml/libs/xl/xl_stubs.c
@@ -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);
 }

Ian Campbell

Acked-by on v1 1/1

2011-03-29 08:49 UTC #11 lore raw

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

Quoted 8 lines · Dave Scott

# HG changeset patch
# User David Scott <[email protected]>
# Date 1301314652 -3600
# Node ID 455cc73ea6268d54bc94e4c39ee954cef107f88b
# Parent 3aab79c907a2c78f4e81362944ee65ddf6f2cc6f
tools: ocaml: add NIC QoS parameters to the ocaml libxl interface
Signed-off-by David Scott <[email protected]>

Looks good, modulo the impact of comments on previous patches:

Acked-by Ian Campbell <[email protected]>

Quoted 43 lines

diff -r 3aab79c907a2 -r 455cc73ea626 tools/ocaml/libs/xl/xl.ml
tools/ocaml/libs/xl/xl.ml +2 -0
@@ -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
tools/ocaml/libs/xl/xl.mli +2 -0
@@ -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
tools/ocaml/libs/xl/xl_stubs.c +2 -1
@@ -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
[email protected]
http://lists.xensource.com/xen-devel

David Scott

author

2011-03-28 12:26 UTC #12 lore raw

PATCH 1/1 tools: ocaml: rename nic_remove to nic_del, for consistency with libxl

In reply to David Scott

# HG changeset patch
# User David Scott <[email protected]>
# 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 <[email protected]>

tools/ocaml/libs/xl/xl.ml +1 -1

diff -r 455cc73ea626 -r bb7e56721316 tools/ocaml/libs/xl/xl.ml
@@ -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"

tools/ocaml/libs/xl/xl.mli +1 -1

diff -r 455cc73ea626 -r bb7e56721316 tools/ocaml/libs/xl/xl.mli
@@ -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"

tools/ocaml/libs/xl/xl_stubs.c +1 -1

diff -r 455cc73ea626 -r bb7e56721316 tools/ocaml/libs/xl/xl_stubs.c
@@ -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;

Ian Campbell

2011-03-29 08:52 UTC #13 lore raw

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

Quoted 6 lines · Dave Scott

# HG changeset patch
# User David Scott <[email protected]>
# 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 :-(

Quoted 43 lines

Signed-off-by David Scott <[email protected]>

diff -r 455cc73ea626 -r bb7e56721316 tools/ocaml/libs/xl/xl.ml

tools/ocaml/libs/xl/xl.ml +1 -1

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

tools/ocaml/libs/xl/xl.mli +1 -1

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

tools/ocaml/libs/xl/xl_stubs.c +1 -1

@@ -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
[email protected]
http://lists.xensource.com/xen-devel