subject:"\[libvirt\] \[PATCH\] \[PATCH\] vcpupin\: add clear feature"

Re: [libvirt] [PATCH] [PATCH] vcpupin: add clear feature

2018-01-04 Thread wang.yi59

Thanks for your reply, and I will send a new version of this
patch.

>On Thu, Jan 04, 2018 at 05:46:32 -0500, Yi Wang wrote:
>> We can't clear vcpupin settings of XML once we did vcpupin
>> command, this is not convenient under some condition such
>> as migration.
>>
>> This patch introduces clear feature, which can clear vcpuin
>> setting of XML.
>>
>> Signed-off-by: Yi Wang 
>> Signed-off-by: Xi Xu 
>> ---
>> include/libvirt/libvirt-domain.h | 1 +
>> src/qemu/qemu_driver.c | 24 +++-
>> tools/virsh-domain.c | 5 -
>> tools/virsh.pod | 1 +
>> 4 files changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/libvirt/libvirt-domain.h 
>> b/include/libvirt/libvirt-domain.h
>> index 4048acf..7b171df 100644
>> --- a/include/libvirt/libvirt-domain.h
>> +++ b/include/libvirt/libvirt-domain.h
>> @@ -1837,6 +1837,7 @@ typedef enum {
>> VIR_DOMAIN_VCPU_MAXIMUM = (1 << 2), /* Max rather than current count */
>> VIR_DOMAIN_VCPU_GUEST = (1 << 3), /* Modify state of the cpu in the guest */
>> VIR_DOMAIN_VCPU_HOTPLUGGABLE = (1 << 4), /* Make vcpus added 
>> hot(un)pluggable */
>> + VIR_DOMAIN_VCPU_CLEAR = (1 << 5), /* Clear vcpus pin info */
>
>These are flags for a wrong API, you'll need to introduce new set of
>flags for the pinning API.
>
>> } virDomainVcpuFlags;
>>
>> int virDomainSetVcpus (virDomainPtr domain,
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 97b194b..9e8759f 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -5001,7 +5001,7 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
>> int vcpu,
>> virQEMUDriverPtr driver,
>> virQEMUDriverConfigPtr cfg,
>> - virBitmapPtr cpumap)
>> + virBitmapPtr cpumap, bool clear)
>
>Please adhere to the coding standards.
>
>> {
>> virBitmapPtr tmpmap = NULL;
>> virDomainVcpuDefPtr vcpuinfo;
>> @@ -5049,7 +5049,12 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
>> }
>>
>> virBitmapFree(vcpuinfo->cpumask);
>> - vcpuinfo->cpumask = tmpmap;
>> + if (clear) {
>> + virBitmapFree(tmpmap);
>> + vcpuinfo->cpumask = NULL;
>> + } else {
>> + vcpuinfo->cpumask = tmpmap;
>> + }
>
>This will not work as expected. The API still takes the pinning map into
>account when setting the pinning, so setting 'clear' will only remove
>the information from the XML but will still apply 'cpumap' as the
>desired pinning.
>
>The API should not rely on the fact that the user passes in correct
>cpumap in this case. You'll need to add logic which will set the pinning
>to the value as if it was omitted in the XML.
>
>> tmpmap = NULL;
>>
>> if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0)
>> @@ -5093,9 +5098,11 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
>> virBitmapPtr pcpumap = NULL;
>> virDomainVcpuDefPtr vcpuinfo = NULL;
>> virQEMUDriverConfigPtr cfg = NULL;
>> + bool clear = false;
>>
>> virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
>> - VIR_DOMAIN_AFFECT_CONFIG, -1);
>> + VIR_DOMAIN_AFFECT_CONFIG |
>> + VIR_DOMAIN_VCPU_CLEAR, -1);
>>
>> cfg = virQEMUDriverGetConfig(driver);
>>
>
>[...]
>
>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>> index 93cb020..4bad9e7 100644
>> --- a/tools/virsh-domain.c
>> +++ b/tools/virsh-domain.c
>> @@ -6860,7 +6860,7 @@ virshParseCPUList(vshControl *ctl, int *cpumaplen,
>> unsigned char *cpumap = NULL;
>> virBitmapPtr map = NULL;
>>
>> - if (cpulist[0] == 'r') {
>> + if (cpulist[0] == 'r' || STREQ("clear", cpulist)) {
>> if (!(map = virBitmapNew(maxcpu)))
>> return NULL;
>> virBitmapSetAll(map);
>> @@ -6938,6 +6938,9 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
>> goto cleanup;
>> }
>>
>> + if (STREQ(cpulist, "clear"))
>> + flags |= VIR_DOMAIN_VCPU_CLEAR;
>> +
>> /* Pin mode: pinning specified vcpu to specified physical cpus*/
>> if (!(cpumap = virshParseCPUList(ctl, , cpulist, maxcpu)))
>> goto cleanup;
>> diff --git a/tools/virsh.pod b/tools/virsh.pod
>> index 69cc423..caaa230 100644
>> --- a/tools/virsh.pod
>> +++ b/tools/virsh.pod
>> @@ -2857,6 +2857,7 @@ I is a list of physical CPU numbers. Its 
>> syntax is a comma
>> separated list and a special markup using '-' and '^' (ex. '0-4', '0-3,^2') 
>> can
>> also be allowed. The '-' denotes the range and the '^' denotes exclusive.
>> For pinning the I to all physical cpus specify 'r' as a I.
>> +For clearing pinning info, specify 'clear' as a I.
>
>The other special value is 'r' so I think this should also be a single
>letter option. Or better a switch by itself.


---
Best wishes
Yi Wang--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] [PATCH] vcpupin: add clear feature

2018-01-04 Thread Peter Krempa

On Thu, Jan 04, 2018 at 05:46:32 -0500, Yi Wang wrote:
> We can't clear vcpupin settings of XML once we did vcpupin
> command, this is not convenient under some condition such
> as migration.
> 
> This patch introduces clear feature, which can clear vcpuin
> setting of XML.
> 
> Signed-off-by: Yi Wang 
> Signed-off-by: Xi Xu 
> ---
>  include/libvirt/libvirt-domain.h |  1 +
>  src/qemu/qemu_driver.c   | 24 +++-
>  tools/virsh-domain.c |  5 -
>  tools/virsh.pod  |  1 +
>  4 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-domain.h 
> b/include/libvirt/libvirt-domain.h
> index 4048acf..7b171df 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -1837,6 +1837,7 @@ typedef enum {
>  VIR_DOMAIN_VCPU_MAXIMUM = (1 << 2), /* Max rather than current count */
>  VIR_DOMAIN_VCPU_GUEST   = (1 << 3), /* Modify state of the cpu in the 
> guest */
>  VIR_DOMAIN_VCPU_HOTPLUGGABLE = (1 << 4), /* Make vcpus added 
> hot(un)pluggable */
> +VIR_DOMAIN_VCPU_CLEAR = (1 << 5), /* Clear vcpus pin info */

These are flags for a wrong API, you'll need to introduce new set of
flags for the pinning API.

>  } virDomainVcpuFlags;
>  
>  int virDomainSetVcpus   (virDomainPtr domain,
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 97b194b..9e8759f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5001,7 +5001,7 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
>int vcpu,
>virQEMUDriverPtr driver,
>virQEMUDriverConfigPtr cfg,
> -  virBitmapPtr cpumap)
> +  virBitmapPtr cpumap, bool clear)

Please adhere to the coding standards.

>  {
>  virBitmapPtr tmpmap = NULL;
>  virDomainVcpuDefPtr vcpuinfo;
> @@ -5049,7 +5049,12 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
>  }
>  
>  virBitmapFree(vcpuinfo->cpumask);
> -vcpuinfo->cpumask = tmpmap;
> +if (clear) {
> +virBitmapFree(tmpmap);
> +vcpuinfo->cpumask = NULL;
> +} else {
> +vcpuinfo->cpumask = tmpmap;
> +}

This will not work as expected. The API still takes the pinning map into
account when setting the pinning, so setting 'clear' will only remove
the information from the XML but will still apply 'cpumap' as the
desired pinning.

The API should not rely on the fact that the user passes in correct
cpumap in this case. You'll need to add logic which will set the pinning
to the value as if it was omitted in the XML.

>  tmpmap = NULL;
>  
>  if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) 
> < 0)
> @@ -5093,9 +5098,11 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
>  virBitmapPtr pcpumap = NULL;
>  virDomainVcpuDefPtr vcpuinfo = NULL;
>  virQEMUDriverConfigPtr cfg = NULL;
> +bool clear = false;
>  
>  virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> -  VIR_DOMAIN_AFFECT_CONFIG, -1);
> +  VIR_DOMAIN_AFFECT_CONFIG |
> +  VIR_DOMAIN_VCPU_CLEAR, -1);
>  
>  cfg = virQEMUDriverGetConfig(driver);
>  

[...]

> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 93cb020..4bad9e7 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -6860,7 +6860,7 @@ virshParseCPUList(vshControl *ctl, int *cpumaplen,
>  unsigned char *cpumap = NULL;
>  virBitmapPtr map = NULL;
>  
> -if (cpulist[0] == 'r') {
> +if (cpulist[0] == 'r' || STREQ("clear", cpulist)) {
>  if (!(map = virBitmapNew(maxcpu)))
>  return NULL;
>  virBitmapSetAll(map);
> @@ -6938,6 +6938,9 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
>  goto cleanup;
>  }
>  
> +if (STREQ(cpulist, "clear"))
> +flags |= VIR_DOMAIN_VCPU_CLEAR;
> +
>  /* Pin mode: pinning specified vcpu to specified physical cpus*/
>  if (!(cpumap = virshParseCPUList(ctl, , cpulist, maxcpu)))
>  goto cleanup;
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 69cc423..caaa230 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -2857,6 +2857,7 @@ I is a list of physical CPU numbers. Its 
> syntax is a comma
>  separated list and a special markup using '-' and '^' (ex. '0-4', '0-3,^2') 
> can
>  also be allowed. The '-' denotes the range and the '^' denotes exclusive.
>  For pinning the I to all physical cpus specify 'r' as a I.
> +For clearing pinning info, specify 'clear' as a I.

The other special value is 'r' so I think this should also be a single
letter option. Or better a switch by itself.


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

[libvirt] [PATCH] [PATCH] vcpupin: add clear feature

2018-01-04 Thread Yi Wang

We can't clear vcpupin settings of XML once we did vcpupin
command, this is not convenient under some condition such
as migration.

This patch introduces clear feature, which can clear vcpuin
setting of XML.

Signed-off-by: Yi Wang 
Signed-off-by: Xi Xu 
---
 include/libvirt/libvirt-domain.h |  1 +
 src/qemu/qemu_driver.c   | 24 +++-
 tools/virsh-domain.c |  5 -
 tools/virsh.pod  |  1 +
 4 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 4048acf..7b171df 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1837,6 +1837,7 @@ typedef enum {
 VIR_DOMAIN_VCPU_MAXIMUM = (1 << 2), /* Max rather than current count */
 VIR_DOMAIN_VCPU_GUEST   = (1 << 3), /* Modify state of the cpu in the 
guest */
 VIR_DOMAIN_VCPU_HOTPLUGGABLE = (1 << 4), /* Make vcpus added 
hot(un)pluggable */
+VIR_DOMAIN_VCPU_CLEAR = (1 << 5), /* Clear vcpus pin info */
 } virDomainVcpuFlags;
 
 int virDomainSetVcpus   (virDomainPtr domain,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 97b194b..9e8759f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5001,7 +5001,7 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
   int vcpu,
   virQEMUDriverPtr driver,
   virQEMUDriverConfigPtr cfg,
-  virBitmapPtr cpumap)
+  virBitmapPtr cpumap, bool clear)
 {
 virBitmapPtr tmpmap = NULL;
 virDomainVcpuDefPtr vcpuinfo;
@@ -5049,7 +5049,12 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
 }
 
 virBitmapFree(vcpuinfo->cpumask);
-vcpuinfo->cpumask = tmpmap;
+if (clear) {
+virBitmapFree(tmpmap);
+vcpuinfo->cpumask = NULL;
+} else {
+vcpuinfo->cpumask = tmpmap;
+}
 tmpmap = NULL;
 
 if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 
0)
@@ -5093,9 +5098,11 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 virBitmapPtr pcpumap = NULL;
 virDomainVcpuDefPtr vcpuinfo = NULL;
 virQEMUDriverConfigPtr cfg = NULL;
+bool clear = false;
 
 virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
-  VIR_DOMAIN_AFFECT_CONFIG, -1);
+  VIR_DOMAIN_AFFECT_CONFIG |
+  VIR_DOMAIN_VCPU_CLEAR, -1);
 
 cfg = virQEMUDriverGetConfig(driver);
 
@@ -5111,6 +5118,8 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 if (virDomainObjGetDefs(vm, flags, , ) < 0)
 goto endjob;
 
+clear = !!(flags & VIR_DOMAIN_VCPU_CLEAR);
+
 if ((def && def->virtType == VIR_DOMAIN_VIRT_QEMU) ||
 (persistentDef && persistentDef->virtType == VIR_DOMAIN_VIRT_QEMU)) {
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
@@ -5136,12 +5145,17 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 }
 
 if (def &&
-qemuDomainPinVcpuLive(vm, def, vcpu, driver, cfg, pcpumap) < 0)
+qemuDomainPinVcpuLive(vm, def, vcpu, driver, cfg, pcpumap, clear) < 0)
 goto endjob;
 
 if (persistentDef) {
 virBitmapFree(vcpuinfo->cpumask);
-vcpuinfo->cpumask = pcpumap;
+if (clear) {
+virBitmapFree(pcpumap);
+vcpuinfo->cpumask = NULL;
+} else {
+vcpuinfo->cpumask = pcpumap;
+}
 pcpumap = NULL;
 
 ret = virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef);
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 93cb020..4bad9e7 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6860,7 +6860,7 @@ virshParseCPUList(vshControl *ctl, int *cpumaplen,
 unsigned char *cpumap = NULL;
 virBitmapPtr map = NULL;
 
-if (cpulist[0] == 'r') {
+if (cpulist[0] == 'r' || STREQ("clear", cpulist)) {
 if (!(map = virBitmapNew(maxcpu)))
 return NULL;
 virBitmapSetAll(map);
@@ -6938,6 +6938,9 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
 goto cleanup;
 }
 
+if (STREQ(cpulist, "clear"))
+flags |= VIR_DOMAIN_VCPU_CLEAR;
+
 /* Pin mode: pinning specified vcpu to specified physical cpus*/
 if (!(cpumap = virshParseCPUList(ctl, , cpulist, maxcpu)))
 goto cleanup;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 69cc423..caaa230 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -2857,6 +2857,7 @@ I is a list of physical CPU numbers. Its syntax 
is a comma
 separated list and a special markup using '-' and '^' (ex. '0-4', '0-3,^2') can
 also be allowed. The '-' denotes the range and the '^' denotes exclusive.
 For pinning the I to all physical cpus specify 'r' as a I.
+For clearing pinning info, specify 'clear' as a I.
 If I<--live> is specified, affect a running guest.
 If I<--config> is specified, affect the next boot of a persistent guest.
 If I<--current> is specified,

Re: [libvirt] [PATCH] [PATCH] vcpupin: add clear feature

Re: [libvirt] [PATCH] [PATCH] vcpupin: add clear feature

[libvirt] [PATCH] [PATCH] vcpupin: add clear feature

3 matches

Site Navigation

Mail list logo

Footer information