Re: [libvirt] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
Paolo Bonzini pbonz...@redhat.com writes:
Il 26/02/2013 20:35, Anthony Liguori ha scritto:
See also discussion on multi-valued keys in command line option
arguments and config files in v1 thread. Hopefully we can reach a
conclusion soon, and then we'll see whether this patch is what we want.
Yeah, let's drop this patch by now. I am starting to be convinced that
cpus=A,cpus=B,cpus=C is the best approach. It is not pretty, but at
least it uses generic parser code instead of yet another ad-hoc
parser.
No, we cannot rely on this behavior. We had to do it to support
backwards compat with netdev but it should not be used anywhere else.
We chose a config format (git's) because it was a good match for
QemuOpts, and multiple-valued operations are well supported by that
config format; see git-config(1). There is no reason to consider it a
backwards-compatibility hack.
There's such thing as list support in QemuOpts. The only way
QemuOptsVisitor was able to implement it was to expose QemuOpts publicly
via options_int.h and rely on a implementation detail.
There are fixed types supported by QemuOpts. It just so happens that
whenever qemu_opt_set() is called, instead of replacing the last
instance, the value is either prepended or appended in order to
implement a replace or set-if-unset behavior.
All values are treated this way. So the above list would only be:
{
.name = cpus,
.type = QEMU_OPT_STRING
}
Which is indistinguishable from a straight string property. This means
it's impossible to introspect because the type is context-sensitive.
What's more, there is no API outside of QemuOptsVisitor that can
actually work with lists of QemuOpts values.
If we want to have list syntax, we need to introduce first class support
for it. Here's a simple example of how to do this. Obviously we would
want to introduce some better error checking so we can catch if someone
tries to access a list with a non-list accessor. We also would need
list accessor functions.
But it's really not hard at all.
(Only compile tested)
From 4ee7ed36d597f0defd78baac7480ecb29e11e1c1 Mon Sep 17 00:00:00 2001
From: Anthony Liguori aligu...@us.ibm.com
Date: Wed, 27 Feb 2013 09:32:09 -0600
Subject: [PATCH] qemu-opts: support lists
Signed-off-by: Anthony Liguori aligu...@us.ibm.com
---
include/qemu/option.h | 2 ++
util/qemu-option.c| 12
2 files changed, 14 insertions(+)
diff --git a/include/qemu/option.h b/include/qemu/option.h
index ba197cd..e4808c3 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -41,6 +41,7 @@ enum QEMUOptionParType {
typedef struct QEMUOptionParameter {
const char *name;
enum QEMUOptionParType type;
+bool list;
union {
uint64_t n;
char* s;
@@ -95,6 +96,7 @@ enum QemuOptType {
typedef struct QemuOptDesc {
const char *name;
enum QemuOptType type;
+bool list;
const char *help;
} QemuOptDesc;
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 5a1d03c..6b1ae6e 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -636,6 +636,18 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
return;
}
+if (desc-list strchr(value, ':')) {
+gchar **tokens = g_strsplit(value, :, 0);
+int i;
+for (i = 0; tokens[i]; i++) {
+opt_set(opts, name, tokens[i], prepend, errp);
+if (error_is_set(errp)) {
+return;
+}
+}
+g_strfreev(tokens);
+}
+
opt = g_malloc0(sizeof(*opt));
opt-name = g_strdup(name);
opt-opts = opts;
--
1.8.0
Regards,
Anthony Liguori
Paolo
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
Il 27/02/2013 16:42, Anthony Liguori ha scritto: There's such thing as list support in QemuOpts. The only way QemuOptsVisitor was able to implement it was to expose QemuOpts publicly via options_int.h and rely on a implementation detail. There are fixed types supported by QemuOpts. It just so happens that whenever qemu_opt_set() is called, instead of replacing the last instance, the value is either prepended or appended in order to implement a replace or set-if-unset behavior. Fair enough. Nobody said the implementation is pretty. If we want to have list syntax, we need to introduce first class support for it. Here's a simple example of how to do this. If it is meant as a prototype only, and the final command-line syntax would be with repeated keys, that's okay. I think that Eduardo/Markus/I are focusing on the user interface, you're focusing in the implementation. In the meanwhile, however, it seems to me that Eduardo can use QemuOptsVisitor---which can also hide the details and provide type safety. Paolo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
On Wed, Feb 27, 2013 at 09:42:50AM -0600, Anthony Liguori wrote:
Paolo Bonzini pbonz...@redhat.com writes:
Il 26/02/2013 20:35, Anthony Liguori ha scritto:
See also discussion on multi-valued keys in command line option
arguments and config files in v1 thread. Hopefully we can reach a
conclusion soon, and then we'll see whether this patch is what we
want.
Yeah, let's drop this patch by now. I am starting to be convinced that
cpus=A,cpus=B,cpus=C is the best approach. It is not pretty, but at
least it uses generic parser code instead of yet another ad-hoc
parser.
No, we cannot rely on this behavior. We had to do it to support
backwards compat with netdev but it should not be used anywhere else.
We chose a config format (git's) because it was a good match for
QemuOpts, and multiple-valued operations are well supported by that
config format; see git-config(1). There is no reason to consider it a
backwards-compatibility hack.
There's such thing as list support in QemuOpts.
[skipping stuff about internal implementation details that don't matter]
If we want to have list syntax, we need to introduce first class support
for it.
Absolutely. But how the syntax for it should look like?
Here's a simple example of how to do this. Obviously we would
want to introduce some better error checking so we can catch if someone
tries to access a list with a non-list accessor. We also would need
list accessor functions.
But it's really not hard at all.
Of course. Once we decide how the syntax will look like, implementing
it should be very easy. But as far as I can see, we were trying to
discuss what's the appropriate syntax, here.
I still don't see why option=A:B:C with no possibility of having list
items containing : (like your proposal below) is a better generic
syntax for lists than option=A,option=B,option=C.
From 4ee7ed36d597f0defd78baac7480ecb29e11e1c1 Mon Sep 17 00:00:00 2001
From: Anthony Liguori aligu...@us.ibm.com
Date: Wed, 27 Feb 2013 09:32:09 -0600
Subject: [PATCH] qemu-opts: support lists
Signed-off-by: Anthony Liguori aligu...@us.ibm.com
---
include/qemu/option.h | 2 ++
util/qemu-option.c| 12
2 files changed, 14 insertions(+)
diff --git a/include/qemu/option.h b/include/qemu/option.h
index ba197cd..e4808c3 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -41,6 +41,7 @@ enum QEMUOptionParType {
typedef struct QEMUOptionParameter {
const char *name;
enum QEMUOptionParType type;
+bool list;
union {
uint64_t n;
char* s;
@@ -95,6 +96,7 @@ enum QemuOptType {
typedef struct QemuOptDesc {
const char *name;
enum QemuOptType type;
+bool list;
const char *help;
} QemuOptDesc;
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 5a1d03c..6b1ae6e 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -636,6 +636,18 @@ static void opt_set(QemuOpts *opts, const char *name,
const char *value,
return;
}
+if (desc-list strchr(value, ':')) {
+gchar **tokens = g_strsplit(value, :, 0);
+int i;
+for (i = 0; tokens[i]; i++) {
+opt_set(opts, name, tokens[i], prepend, errp);
+if (error_is_set(errp)) {
+return;
+}
+}
+g_strfreev(tokens);
+}
+
opt = g_malloc0(sizeof(*opt));
opt-name = g_strdup(name);
opt-opts = opts;
--
1.8.0
Regards,
Anthony Liguori
Paolo
--
Eduardo
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
On Wed, Feb 27, 2013 at 04:57:15PM +0100, Paolo Bonzini wrote: Il 27/02/2013 16:42, Anthony Liguori ha scritto: There's such thing as list support in QemuOpts. The only way QemuOptsVisitor was able to implement it was to expose QemuOpts publicly via options_int.h and rely on a implementation detail. There are fixed types supported by QemuOpts. It just so happens that whenever qemu_opt_set() is called, instead of replacing the last instance, the value is either prepended or appended in order to implement a replace or set-if-unset behavior. Fair enough. Nobody said the implementation is pretty. If we want to have list syntax, we need to introduce first class support for it. Here's a simple example of how to do this. If it is meant as a prototype only, and the final command-line syntax would be with repeated keys, that's okay. I think that Eduardo/Markus/I are focusing on the user interface, you're focusing in the implementation. In the meanwhile, however, it seems to me that Eduardo can use QemuOptsVisitor---which can also hide the details and provide type safety. Whatever I use to implement it, I still need to know how the command-line syntax will look like, because we need to tell libvirt developers how they should write the QEMU command-line. -- Eduardo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
Il 27/02/2013 17:19, Eduardo Habkost ha scritto: If it is meant as a prototype only, and the final command-line syntax would be with repeated keys, that's okay. I think that Eduardo/Markus/I are focusing on the user interface, you're focusing in the implementation. In the meanwhile, however, it seems to me that Eduardo can use QemuOptsVisitor---which can also hide the details and provide type safety. Whatever I use to implement it, I still need to know how the command-line syntax will look like, because we need to tell libvirt developers how they should write the QEMU command-line. I don't think any syntax makes sense except cpus=A,cpus=B. How we implement it is another story. Paolo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
On Wed, Feb 27, 2013 at 05:58:39PM +0100, Paolo Bonzini wrote: Il 27/02/2013 17:19, Eduardo Habkost ha scritto: If it is meant as a prototype only, and the final command-line syntax would be with repeated keys, that's okay. I think that Eduardo/Markus/I are focusing on the user interface, you're focusing in the implementation. In the meanwhile, however, it seems to me that Eduardo can use QemuOptsVisitor---which can also hide the details and provide type safety. Whatever I use to implement it, I still need to know how the command-line syntax will look like, because we need to tell libvirt developers how they should write the QEMU command-line. I don't think any syntax makes sense except cpus=A,cpus=B. How we implement it is another story. I agree completely, and I still don't know why Anthony doesn't like it. -- Eduardo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
Il 26/02/2013 20:35, Anthony Liguori ha scritto: See also discussion on multi-valued keys in command line option arguments and config files in v1 thread. Hopefully we can reach a conclusion soon, and then we'll see whether this patch is what we want. Yeah, let's drop this patch by now. I am starting to be convinced that cpus=A,cpus=B,cpus=C is the best approach. It is not pretty, but at least it uses generic parser code instead of yet another ad-hoc parser. No, we cannot rely on this behavior. We had to do it to support backwards compat with netdev but it should not be used anywhere else. We chose a config format (git's) because it was a good match for QemuOpts, and multiple-valued operations are well supported by that config format; see git-config(1). There is no reason to consider it a backwards-compatibility hack. Paolo -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option
This allows : to be used a separator between each CPU range, so the
command-line may look like:
-numa node,cpus=A-B:C-D
Note that the following format, currently used by libvirt:
-numa nodes,cpus=A-B,C-D
will _not_ work, as , is the option separator for the command-line
option parser, and it would require changing the -numa option parsing
code to handle cpus as a special case.
Signed-off-by: Eduardo Habkost ehabk...@redhat.com
---
Changes v2:
- Use : as separator
- Document the new format
---
qemu-options.hx | 10 --
vl.c| 14 +-
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/qemu-options.hx b/qemu-options.hx
index 4bc9c85..b135044 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -95,12 +95,18 @@ specifies the maximum number of hotpluggable CPUs.
ETEXI
DEF(numa, HAS_ARG, QEMU_OPTION_numa,
--numa node[,mem=size][,cpus=cpu[-cpu]][,nodeid=node]\n, QEMU_ARCH_ALL)
+-numa node[,mem=size][,cpus=cpu[-cpu][:...]][,nodeid=node]\n,
QEMU_ARCH_ALL)
STEXI
@item -numa @var{opts}
@findex -numa
Simulate a multi node NUMA system. If mem and cpus are omitted, resources
-are split equally.
+are split equally. Multiple CPU ranges in the cpus option may be separated
+by colons. e.g.:
+
+@example
+-numa node,mem=1024,cpus=0-1:4-5:8-9
+-numa node,mem=1024,cpus=2-3:6-7:10-11
+@end example
ETEXI
DEF(add-fd, HAS_ARG, QEMU_OPTION_add_fd,
diff --git a/vl.c b/vl.c
index 955d2ff..fe632db 100644
--- a/vl.c
+++ b/vl.c
@@ -1244,7 +1244,7 @@ char *get_boot_devices_list(size_t *size)
return list;
}
-static void numa_node_parse_cpus(int nodenr, const char *cpus)
+static void numa_node_parse_cpu_range(int nodenr, const char *cpus)
{
char *endptr;
unsigned long long value, endvalue;
@@ -1288,6 +1288,18 @@ error:
exit(1);
}
+static void numa_node_parse_cpus(int nodenr, const char *option)
+{
+char **parts;
+int i;
+
+parts = g_strsplit(option, :, 0);
+for (i = 0; parts[i]; i++) {
+numa_node_parse_cpu_range(nodenr, parts[i]);
+}
+g_strfreev(parts);
+}
+
static void numa_add(const char *optarg)
{
char option[128];
--
1.8.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
