Re: [libvirt] [PATCH 05/17] Support hypervisorpin xml parse.

2012-08-07 Thread Hu Tao 
On Mon, Aug 06, 2012 at 04:51:49PM -0600, Eric Blake wrote:
 On 08/03/2012 12:36 AM, Hu Tao wrote:
  From: Tang Chen tangc...@cn.fujitsu.com
  
  This patch adds a new xml element hypervisorpin cpuset='1',
 
 I would mention that this is a sibling to the existing vcpupin element
 under the cputune.
 
  and also the parser functions, docs, and tests.
  hypervisorpin means pinning hypervisor threads, and cpuset = '1'
  means pinning all hypervisor threads to cpu 1.
  
  Signed-off-by: Tang Chen tangc...@cn.fujitsu.com
  Signed-off-by: Hu Tao hu...@cn.fujitsu.com
  ---
   docs/schemas/domaincommon.rng   |7 ++
 
 Missing documentation.  I can't apply this as-is unless we also update
 the elementsCPUTuning section of docs/formatdomain.html.in.  That won't
 stop me from reviewing the rest of the patch, though.
 
   src/conf/domain_conf.c  |   97 
  ++-
   src/conf/domain_conf.h  |1 +
   tests/qemuxml2argvdata/qemuxml2argv-cputune.xml |1 +
   4 files changed, 103 insertions(+), 3 deletions(-)
  
 
  +++ b/src/conf/domain_conf.c
  @@ -7828,6 +7828,51 @@ error:
   goto cleanup;
   }
   
  +/* Parse the XML definition for hypervisorpin */
  +static virDomainVcpuPinDefPtr
  +virDomainHypervisorPinDefParseXML(const xmlNodePtr node)
  +{
 ...
  +}
 
 This is a lot of code duplication.  It might be worth refactoring things
 to write a helper function that parses '@cpuset', and which can be
 shared by both the existing virDomainVcpuPinDefParseXML and your new
 function.
 
  @@ -9280,7 +9353,7 @@ no_memory:
   virReportOOMError();
   /* fallthrough */
   
  - error:
  +error:
   VIR_FREE(tmp);
   VIR_FREE(nodes);
   virBitmapFree(bootMap);
 
 On its own, this whitespace cleanup has no bearing on the patch; it's
 generally wise to limit cleanups to portions already affected by the patch.
 
 But in general, the patch looked reasonable.

Thank you, I'll improve this patch in v2.

-- 
Thanks,
Hu Tao

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 05/17] Support hypervisorpin xml parse.

2012-08-06 Thread Eric Blake 
On 08/03/2012 12:36 AM, Hu Tao wrote:
 From: Tang Chen tangc...@cn.fujitsu.com
 
 This patch adds a new xml element hypervisorpin cpuset='1',

I would mention that this is a sibling to the existing vcpupin element
under the cputune.

 and also the parser functions, docs, and tests.
 hypervisorpin means pinning hypervisor threads, and cpuset = '1'
 means pinning all hypervisor threads to cpu 1.
 
 Signed-off-by: Tang Chen tangc...@cn.fujitsu.com
 Signed-off-by: Hu Tao hu...@cn.fujitsu.com
 ---
  docs/schemas/domaincommon.rng   |7 ++

Missing documentation.  I can't apply this as-is unless we also update
the elementsCPUTuning section of docs/formatdomain.html.in.  That won't
stop me from reviewing the rest of the patch, though.

  src/conf/domain_conf.c  |   97 
 ++-
  src/conf/domain_conf.h  |1 +
  tests/qemuxml2argvdata/qemuxml2argv-cputune.xml |1 +
  4 files changed, 103 insertions(+), 3 deletions(-)
 

 +++ b/src/conf/domain_conf.c
 @@ -7828,6 +7828,51 @@ error:
  goto cleanup;
  }
  
 +/* Parse the XML definition for hypervisorpin */
 +static virDomainVcpuPinDefPtr
 +virDomainHypervisorPinDefParseXML(const xmlNodePtr node)
 +{
...
 +}

This is a lot of code duplication.  It might be worth refactoring things
to write a helper function that parses '@cpuset', and which can be
shared by both the existing virDomainVcpuPinDefParseXML and your new
function.

 @@ -9280,7 +9353,7 @@ no_memory:
  virReportOOMError();
  /* fallthrough */
  
 - error:
 +error:
  VIR_FREE(tmp);
  VIR_FREE(nodes);
  virBitmapFree(bootMap);

On its own, this whitespace cleanup has no bearing on the patch; it's
generally wise to limit cleanups to portions already affected by the patch.

But in general, the patch looked reasonable.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 05/17] Support hypervisorpin xml parse.

2012-08-03 Thread Hu Tao 
From: Tang Chen tangc...@cn.fujitsu.com

This patch adds a new xml element hypervisorpin cpuset='1',
and also the parser functions, docs, and tests.
hypervisorpin means pinning hypervisor threads, and cpuset = '1'
means pinning all hypervisor threads to cpu 1.

Signed-off-by: Tang Chen tangc...@cn.fujitsu.com
Signed-off-by: Hu Tao hu...@cn.fujitsu.com
---
 docs/schemas/domaincommon.rng   |7 ++
 src/conf/domain_conf.c  |   97 ++-
 src/conf/domain_conf.h  |1 +
 tests/qemuxml2argvdata/qemuxml2argv-cputune.xml |1 +
 4 files changed, 103 insertions(+), 3 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index c85d763..c0b83b3 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -576,6 +576,13 @@
   /attribute
 /element
   /zeroOrMore
+  optional
+element name=hypervisorpin
+  attribute name=cpuset
+ref name=cpuset/
+  /attribute
+/element
+  /optional
 /element
   /optional
 
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 43b3f80..08a662e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7828,6 +7828,51 @@ error:
 goto cleanup;
 }
 
+/* Parse the XML definition for hypervisorpin */
+static virDomainVcpuPinDefPtr
+virDomainHypervisorPinDefParseXML(const xmlNodePtr node)
+{
+virDomainVcpuPinDefPtr def = NULL;
+char *tmp = NULL;
+
+if (VIR_ALLOC(def)  0) {
+virReportOOMError();
+return NULL;
+}
+
+def-vcpuid = -1;
+
+tmp = virXMLPropString(node, cpuset);
+
+if (tmp) {
+char *set = tmp;
+int cpumasklen = VIR_DOMAIN_CPUMASK_LEN;
+
+if (VIR_ALLOC_N(def-cpumask, cpumasklen)  0) {
+virReportOOMError();
+goto error;
+}
+
+if (virDomainCpuSetParse(set, 0, def-cpumask,
+ cpumasklen)  0)
+goto error;
+
+VIR_FREE(tmp);
+} else {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   %s, _(missing cpuset for hypervisor pin));
+goto error;
+}
+
+cleanup:
+return def;
+
+error:
+VIR_FREE(tmp);
+VIR_FREE(def);
+goto cleanup;
+}
+
 static int virDomainDefMaybeAddController(virDomainDefPtr def,
   int type,
   int idx)
@@ -8223,6 +8268,34 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr 
caps,
 }
 VIR_FREE(nodes);
 
+if ((n = virXPathNodeSet(./cputune/hypervisorpin, ctxt, nodes))  0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+   _(cannot extract hypervisorpin nodes));
+goto error;
+}
+
+if (n  1) {
+virReportError(VIR_ERR_XML_ERROR, %s,
+   _(only one hypervisorpin is supported));
+VIR_FREE(nodes);
+goto error;
+}
+
+if (n  VIR_ALLOC(def-cputune.hypervisorpin)  0) {
+goto no_memory;
+}
+
+if (n) {
+virDomainVcpuPinDefPtr hypervisorpin = NULL;
+hypervisorpin = virDomainHypervisorPinDefParseXML(nodes[0]);
+
+if (!hypervisorpin)
+goto error;
+
+def-cputune.hypervisorpin = hypervisorpin;
+}
+VIR_FREE(nodes);
+
 /* Extract numatune if exists. */
 if ((n = virXPathNodeSet(./numatune, ctxt, nodes))  0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -9280,7 +9353,7 @@ no_memory:
 virReportOOMError();
 /* fallthrough */
 
- error:
+error:
 VIR_FREE(tmp);
 VIR_FREE(nodes);
 virBitmapFree(bootMap);
@@ -12848,7 +12921,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
 virBufferAsprintf(buf, %u/vcpu\n, def-maxvcpus);
 
 if (def-cputune.shares || def-cputune.vcpupin ||
-def-cputune.period || def-cputune.quota)
+def-cputune.period || def-cputune.quota ||
+def-cputune.hypervisorpin)
 virBufferAddLit(buf,   cputune\n);
 
 if (def-cputune.shares)
@@ -12880,8 +12954,25 @@ virDomainDefFormatInternal(virDomainDefPtr def,
 }
 }
 
+if (def-cputune.hypervisorpin) {
+virBufferAsprintf(buf, hypervisorpin );
+
+char *cpumask = NULL;
+cpumask = virDomainCpuSetFormat(def-cputune.hypervisorpin-cpumask,
+VIR_DOMAIN_CPUMASK_LEN);
+if (cpumask == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   %s, _(failed to format cpuset for hypervisor));
+goto cleanup;
+}
+
+virBufferAsprintf(buf, cpuset='%s'/\n, cpumask);
+VIR_FREE(cpumask);
+}
+
 if (def-cputune.shares || def-cputune.vcpupin ||
-def-cputune.period || def-cputune.quota)
+def-cputune.period || def-cputune.quota ||
+