[PATCH] libxl: avoid bringing up vcpus already online

[PATCH] libxl: avoid bringing up vcpus already online

[Chao Peng]

Avoid sending duplicated qmp commands and eliminate the confusing error
messages like "Unable to add CPU: 0, it already exists".

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
 tools/libxl/libxl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index f7961f6..d040e5c 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5450,7 +5450,7 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
         LOGE(ERROR, "getting domain info list");
         return ERROR_FAIL;
     }
-    for (i = 0; i <= info.vcpu_max_id; i++) {
+    for (i = info.vcpu_online; i <= info.vcpu_max_id; i++) {
         if (libxl_bitmap_test(cpumap, i)) {
             /* Return value is ignore because it does not tell anything useful
              * on the completion of the command.
-- 
1.7.9.5

Re: [PATCH] libxl: avoid bringing up vcpus already online

[Wei Liu]

On Mon, Nov 17, 2014 at 05:28:58PM +0800, Chao Peng wrote:
> Avoid sending duplicated qmp commands and eliminate the confusing error
> messages like "Unable to add CPU: 0, it already exists".
> 
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> ---
>  tools/libxl/libxl.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index f7961f6..d040e5c 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5450,7 +5450,7 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
>          LOGE(ERROR, "getting domain info list");
>          return ERROR_FAIL;
>      }
> -    for (i = 0; i <= info.vcpu_max_id; i++) {
> +    for (i = info.vcpu_online; i <= info.vcpu_max_id; i++) {

I don't think this is right. You're making an assumption that vcpu 0 to
vcpu "info.vcpu_online" are online, which might not be true in hotplug /
hot-unplug scenario.

Wei.

>          if (libxl_bitmap_test(cpumap, i)) {
>              /* Return value is ignore because it does not tell anything useful
>               * on the completion of the command.
> -- 
> 1.7.9.5

Re: [PATCH] libxl: avoid bringing up vcpus already online

[Ian Campbell]

On Mon, 2014-11-17 at 09:41 +0000, Wei Liu wrote:
> On Mon, Nov 17, 2014 at 05:28:58PM +0800, Chao Peng wrote:
> > Avoid sending duplicated qmp commands and eliminate the confusing error
> > messages like "Unable to add CPU: 0, it already exists".
> > 
> > Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> > ---
> >  tools/libxl/libxl.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index f7961f6..d040e5c 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -5450,7 +5450,7 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
> >          LOGE(ERROR, "getting domain info list");
> >          return ERROR_FAIL;
> >      }
> > -    for (i = 0; i <= info.vcpu_max_id; i++) {
> > +    for (i = info.vcpu_online; i <= info.vcpu_max_id; i++) {
> 
> I don't think this is right. You're making an assumption that vcpu 0 to
> vcpu "info.vcpu_online" are online, which might not be true in hotplug /
> hot-unplug scenario.

Indeed. Adding Anthony for his input.

> 
> Wei.
> 
> >          if (libxl_bitmap_test(cpumap, i)) {
> >              /* Return value is ignore because it does not tell anything useful
> >               * on the completion of the command.
> > -- 
> > 1.7.9.5

Re: [PATCH] libxl: avoid bringing up vcpus already online

[Chao Peng]

On Mon, Nov 17, 2014 at 09:41:17AM +0000, Wei Liu wrote:
> On Mon, Nov 17, 2014 at 05:28:58PM +0800, Chao Peng wrote:
> > Avoid sending duplicated qmp commands and eliminate the confusing error
> > messages like "Unable to add CPU: 0, it already exists".
> > 
> > Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> > ---
> >  tools/libxl/libxl.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index f7961f6..d040e5c 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -5450,7 +5450,7 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
> >          LOGE(ERROR, "getting domain info list");
> >          return ERROR_FAIL;
> >      }
> > -    for (i = 0; i <= info.vcpu_max_id; i++) {
> > +    for (i = info.vcpu_online; i <= info.vcpu_max_id; i++) {
> 
> I don't think this is right. You're making an assumption that vcpu 0 to
> vcpu "info.vcpu_online" are online, which might not be true in hotplug /
> hot-unplug scenario.
> 
You are right. Though the assumption is true right now as QEMU has not
implemented cpu hot-unplug.

The problem is: There is no way to obtain the vcpu online status in xl
as cpu hot-plug/hot-unplug for HVM are all done in QEMU. I guess it's
hard to fix it now. While it's also a minor issue :-)

Chao
> 
> >          if (libxl_bitmap_test(cpumap, i)) {
> >              /* Return value is ignore because it does not tell anything useful
> >               * on the completion of the command.
> > -- 
> > 1.7.9.5
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] libxl: avoid bringing up vcpus already online

[Anthony PERARD]

On Mon, Nov 17, 2014 at 10:29:19AM +0000, Ian Campbell wrote:
> On Mon, 2014-11-17 at 09:41 +0000, Wei Liu wrote:
> > On Mon, Nov 17, 2014 at 05:28:58PM +0800, Chao Peng wrote:
> > > Avoid sending duplicated qmp commands and eliminate the confusing error
> > > messages like "Unable to add CPU: 0, it already exists".
> > > 
> > > Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> > > ---
> > >  tools/libxl/libxl.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > index f7961f6..d040e5c 100644
> > > --- a/tools/libxl/libxl.c
> > > +++ b/tools/libxl/libxl.c
> > > @@ -5450,7 +5450,7 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
> > >          LOGE(ERROR, "getting domain info list");
> > >          return ERROR_FAIL;
> > >      }
> > > -    for (i = 0; i <= info.vcpu_max_id; i++) {
> > > +    for (i = info.vcpu_online; i <= info.vcpu_max_id; i++) {
> > 
> > I don't think this is right. You're making an assumption that vcpu 0 to
> > vcpu "info.vcpu_online" are online, which might not be true in hotplug /
> > hot-unplug scenario.
> 
> Indeed. Adding Anthony for his input.

It's probably not a good "fix" to avoid the error message. As Wei said,
vcpu 0 to vcpu ${info.vcpu_online} might not all be online.

At the time I've written this function, there were no way to query QEMU
to know which CPU are online. So the only way was to send the QMP
command anyway and get an error back. Unfortunately, the error that QEMU
send back is not suppose to be parsed (as it's only for human to read
and might change in the futur) and libxl_qmp is not ready to handle this
kind of error in a better way, so it just print them.

-- 
Anthony PERARD