xl create: endless loop

xl create: endless loop

[Christoph Egger]

Hi!

I cannot start a guest with 'xl create' due to an endless loop in libxl.c, 
function libxl__get_free_memory_slack():

There is this code snippet:

retry:
    free_mem_slack_s = libxl__xs_read(gc, XBT_NULL, free_mem_slack_path);
    if (!free_mem_slack_s) {
        rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
        if (rc < 0)
            return rc;
        goto retry;
    } else {


libxl__xs_read() returns 0 and libxl__fill_dom0_memory_info() also returns 0.
So there's a loop of retries.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH] xl create: endless loop

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 963 bytes --]

On Monday 18 October 2010 14:50:35 Christoph Egger wrote:
> Hi!
>
> I cannot start a guest with 'xl create' due to an endless loop in libxl.c,
> function libxl__get_free_memory_slack():
>
> There is this code snippet:
>
> retry:
>     free_mem_slack_s = libxl__xs_read(gc, XBT_NULL, free_mem_slack_path);
>     if (!free_mem_slack_s) {
>         rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
>         if (rc < 0)
>             return rc;
>         goto retry;
>     } else {
>
>
> libxl__xs_read() returns 0 and libxl__fill_dom0_memory_info() also returns
> 0. So there's a loop of retries.

Attached patch fixes the endless loop.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

[-- Attachment #2: xen_xl.diff --]
[-- Type: text/x-diff, Size: 454 bytes --]

diff -r 9d8d6b93114e tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Mon Oct 18 15:24:39 2010 +0200
+++ b/tools/libxl/libxl.c	Tue Oct 19 10:29:00 2010 +0200
@@ -2836,7 +2836,7 @@ retry:
     free_mem_slack_s = libxl__xs_read(gc, XBT_NULL, free_mem_slack_path);
     if (!free_mem_slack_s) {
         rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
-        if (rc < 0)
+        if (rc <= 0)
             return rc;
         goto retry;
     } else {

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] xl create: endless loop

[Stefano Stabellini]

On Tue, 19 Oct 2010, Christoph Egger wrote:
> On Monday 18 October 2010 14:50:35 Christoph Egger wrote:
> > Hi!
> >
> > I cannot start a guest with 'xl create' due to an endless loop in libxl.c,
> > function libxl__get_free_memory_slack():
> >
> > There is this code snippet:
> >
> > retry:
> >     free_mem_slack_s = libxl__xs_read(gc, XBT_NULL, free_mem_slack_path);
> >     if (!free_mem_slack_s) {
> >         rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
> >         if (rc < 0)
> >             return rc;
> >         goto retry;
> >     } else {
> >
> >
> > libxl__xs_read() returns 0 and libxl__fill_dom0_memory_info() also returns
> > 0. So there's a loop of retries.
> 
> Attached patch fixes the endless loop.
> 
> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

thanks for the patch but this cannot possibly be the right fix:


diff -r 9d8d6b93114e tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Mon Oct 18 15:24:39 2010 +0200
+++ b/tools/libxl/libxl.c	Tue Oct 19 10:29:00 2010 +0200
@@ -2836,7 +2836,7 @@ retry:
     free_mem_slack_s = libxl__xs_read(gc, XBT_NULL, free_mem_slack_path);
     if (!free_mem_slack_s) {
         rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
-        if (rc < 0)
+        if (rc <= 0)
             return rc;
         goto retry;
     } else {

the idea is that libxl__fill_dom0_memory_info should fill the missing
informations in xenstore so that we can go ahead and try to read them
again and the second time should be successful.
Libxl__fill_dom0_memory_info returns 0 on success, so it is correct to
goto retry in that case.

The bug must be in libxl__fill_dom0_memory_info that doesn't return
error when it should.
Does the appended patch works for you?

---

diff -r 00b92112b055 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Wed Oct 20 17:26:51 2010 +0100
+++ b/tools/libxl/libxl.c	Thu Oct 21 15:08:29 2010 +0100
@@ -2793,11 +2793,11 @@ retry_transaction:
 
     rc = libxl_domain_info(ctx, &info, 0);
     if (rc < 0)
-        return rc;
+        goto out;
 
     rc = libxl_get_physinfo(ctx, &physinfo);
     if (rc < 0)
-        return rc;
+        goto out;
 
     libxl__xs_write(gc, t, target_path, "%"PRIu32,
             (uint32_t) info.current_memkb);
@@ -2816,9 +2816,12 @@ retry_transaction:
     rc = 0;
 
 out:
-    if (!xs_transaction_end(ctx->xsh, t, 0))
+    if (!xs_transaction_end(ctx->xsh, t, 0)) {
         if (errno == EAGAIN)
             goto retry_transaction;
+        else
+            rc = ERROR_FAIL;
+    }
 
 
     return rc;

Re: [PATCH] xl create: endless loop

[Christoph Egger]

On Thursday 21 October 2010 16:11:55 Stefano Stabellini wrote:
> On Tue, 19 Oct 2010, Christoph Egger wrote:
> > On Monday 18 October 2010 14:50:35 Christoph Egger wrote:
> > > Hi!
> > >
> > > I cannot start a guest with 'xl create' due to an endless loop in
> > > libxl.c, function libxl__get_free_memory_slack():
> > >
> > > There is this code snippet:
> > >
> > > retry:
> > >     free_mem_slack_s = libxl__xs_read(gc, XBT_NULL,
> > > free_mem_slack_path); if (!free_mem_slack_s) {
> > >         rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
> > >         if (rc < 0)
> > >             return rc;
> > >         goto retry;
> > >     } else {
> > >
> > >
> > > libxl__xs_read() returns 0 and libxl__fill_dom0_memory_info() also
> > > returns 0. So there's a loop of retries.
> >
> > Attached patch fixes the endless loop.
> >
> > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
>
> thanks for the patch but this cannot possibly be the right fix:
>
>
> diff -r 9d8d6b93114e tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Mon Oct 18 15:24:39 2010 +0200
> +++ b/tools/libxl/libxl.c	Tue Oct 19 10:29:00 2010 +0200
> @@ -2836,7 +2836,7 @@ retry:
>      free_mem_slack_s = libxl__xs_read(gc, XBT_NULL, free_mem_slack_path);
>      if (!free_mem_slack_s) {
>          rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
> -        if (rc < 0)
> +        if (rc <= 0)
>              return rc;
>          goto retry;
>      } else {
>
> the idea is that libxl__fill_dom0_memory_info should fill the missing
> informations in xenstore so that we can go ahead and try to read them
> again and the second time should be successful.
> Libxl__fill_dom0_memory_info returns 0 on success, so it is correct to
> goto retry in that case.
>
> The bug must be in libxl__fill_dom0_memory_info that doesn't return
> error when it should.
> Does the appended patch works for you?
>
> ---
>
> diff -r 00b92112b055 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Wed Oct 20 17:26:51 2010 +0100
> +++ b/tools/libxl/libxl.c	Thu Oct 21 15:08:29 2010 +0100
> @@ -2793,11 +2793,11 @@ retry_transaction:
>
>      rc = libxl_domain_info(ctx, &info, 0);
>      if (rc < 0)
> -        return rc;
> +        goto out;
>
>      rc = libxl_get_physinfo(ctx, &physinfo);
>      if (rc < 0)
> -        return rc;
> +        goto out;
>
>      libxl__xs_write(gc, t, target_path, "%"PRIu32,
>              (uint32_t) info.current_memkb);
> @@ -2816,9 +2816,12 @@ retry_transaction:
>      rc = 0;
>
>  out:
> -    if (!xs_transaction_end(ctx->xsh, t, 0))
> +    if (!xs_transaction_end(ctx->xsh, t, 0)) {
>          if (errno == EAGAIN)
>              goto retry_transaction;
> +        else
> +            rc = ERROR_FAIL;
> +    }
>
>
>      return rc;


No, this patch has no effect for me.
In libxl__fill_dom0_memory_info(), the code path goes that way:

    t = xs_transaction_start(ctx->xsh);

    target = libxl__xs_read(gc, t, target_path);
    if (target) {    <-- target contains "5"
        *target_memkb = strtoul(target, &endptr, 10);
        if (*endptr != '\0') {     <-- *endptr contains '\0'
            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
                    "invalid memory target %s from %s\n", target, 
target_path);
            rc = ERROR_FAIL;
            goto out;
        }
        rc = 0;
        goto out;    <-- take this jump with rc being 0
    }


Christoph



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH] xl create: endless loop

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 4044 bytes --]

On Wednesday 03 November 2010 17:12:47 Christoph Egger wrote:
> On Thursday 21 October 2010 16:11:55 Stefano Stabellini wrote:
> > On Tue, 19 Oct 2010, Christoph Egger wrote:
> > > On Monday 18 October 2010 14:50:35 Christoph Egger wrote:
> > > > Hi!
> > > >
> > > > I cannot start a guest with 'xl create' due to an endless loop in
> > > > libxl.c, function libxl__get_free_memory_slack():
> > > >
> > > > There is this code snippet:
> > > >
> > > > retry:
> > > >     free_mem_slack_s = libxl__xs_read(gc, XBT_NULL,
> > > > free_mem_slack_path); if (!free_mem_slack_s) {
> > > >         rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
> > > >         if (rc < 0)
> > > >             return rc;
> > > >         goto retry;
> > > >     } else {
> > > >
> > > >
> > > > libxl__xs_read() returns 0 and libxl__fill_dom0_memory_info() also
> > > > returns 0. So there's a loop of retries.
> > >
> > > Attached patch fixes the endless loop.
> > >
> > > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
> >
> > thanks for the patch but this cannot possibly be the right fix:
> >
> >
> > diff -r 9d8d6b93114e tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c	Mon Oct 18 15:24:39 2010 +0200
> > +++ b/tools/libxl/libxl.c	Tue Oct 19 10:29:00 2010 +0200
> > @@ -2836,7 +2836,7 @@ retry:
> >      free_mem_slack_s = libxl__xs_read(gc, XBT_NULL,
> > free_mem_slack_path); if (!free_mem_slack_s) {
> >          rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
> > -        if (rc < 0)
> > +        if (rc <= 0)
> >              return rc;
> >          goto retry;
> >      } else {
> >
> > the idea is that libxl__fill_dom0_memory_info should fill the missing
> > informations in xenstore so that we can go ahead and try to read them
> > again and the second time should be successful.
> > Libxl__fill_dom0_memory_info returns 0 on success, so it is correct to
> > goto retry in that case.
> >
> > The bug must be in libxl__fill_dom0_memory_info that doesn't return
> > error when it should.
> > Does the appended patch works for you?
> >
> > ---
> >
> > diff -r 00b92112b055 tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c	Wed Oct 20 17:26:51 2010 +0100
> > +++ b/tools/libxl/libxl.c	Thu Oct 21 15:08:29 2010 +0100
> > @@ -2793,11 +2793,11 @@ retry_transaction:
> >
> >      rc = libxl_domain_info(ctx, &info, 0);
> >      if (rc < 0)
> > -        return rc;
> > +        goto out;
> >
> >      rc = libxl_get_physinfo(ctx, &physinfo);
> >      if (rc < 0)
> > -        return rc;
> > +        goto out;
> >
> >      libxl__xs_write(gc, t, target_path, "%"PRIu32,
> >              (uint32_t) info.current_memkb);
> > @@ -2816,9 +2816,12 @@ retry_transaction:
> >      rc = 0;
> >
> >  out:
> > -    if (!xs_transaction_end(ctx->xsh, t, 0))
> > +    if (!xs_transaction_end(ctx->xsh, t, 0)) {
> >          if (errno == EAGAIN)
> >              goto retry_transaction;
> > +        else
> > +            rc = ERROR_FAIL;
> > +    }
> >
> >
> >      return rc;
>
> No, this patch has no effect for me.
> In libxl__fill_dom0_memory_info(), the code path goes that way:
>
>     t = xs_transaction_start(ctx->xsh);
>
>     target = libxl__xs_read(gc, t, target_path);
>     if (target) {    <-- target contains "5"
>         *target_memkb = strtoul(target, &endptr, 10);
>         if (*endptr != '\0') {     <-- *endptr contains '\0'
>             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
>                     "invalid memory target %s from %s\n", target,
> target_path);
>             rc = ERROR_FAIL;
>             goto out;
>         }
>         rc = 0;
>         goto out;    <-- take this jump with rc being 0
>     }
>

A slightly modified version works.

Signed-off-by: Mark Langsdorf <Mark.Langsdorf@amd.com>
Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>




-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

[-- Attachment #2: xen_xl.diff --]
[-- Type: text/x-diff, Size: 909 bytes --]

diff -r 8f66a16efee0 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Tue Nov 02 16:08:56 2010 +0100
+++ b/tools/libxl/libxl.c	Thu Nov 04 16:30:27 2010 +0100
@@ -2787,17 +2787,15 @@ retry_transaction:
             rc = ERROR_FAIL;
             goto out;
         }
-        rc = 0;
-        goto out;
     }
 
     rc = libxl_domain_info(ctx, &info, 0);
     if (rc < 0)
-        return rc;
+        goto out;
 
     rc = libxl_get_physinfo(ctx, &physinfo);
     if (rc < 0)
-        return rc;
+        goto out;
 
     libxl__xs_write(gc, t, target_path, "%"PRIu32,
             (uint32_t) info.current_memkb);
@@ -2816,10 +2814,12 @@ retry_transaction:
     rc = 0;
 
 out:
-    if (!xs_transaction_end(ctx->xsh, t, 0))
+    if (!xs_transaction_end(ctx->xsh, t, 0)) {
         if (errno == EAGAIN)
             goto retry_transaction;
-
+        else
+            rc = ERROR_FAIL;
+    }
 
     return rc;
 }

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [PATCH] xl create: endless loop

[Zhang, Yang Z]

Did the patch apply? This issue still exist in xen-unstable(22364).

best regards
yang


> -----Original Message-----
> From: xen-devel-bounces@lists.xensource.com
> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Christoph Egger
> Sent: Thursday, November 04, 2010 11:40 PM
> To: xen-devel@lists.xensource.com
> Cc: Jackson; Stefano Stabellini
> Subject: Re: [Xen-devel][PATCH] xl create: endless loop
> 
> On Wednesday 03 November 2010 17:12:47 Christoph Egger wrote:
> > On Thursday 21 October 2010 16:11:55 Stefano Stabellini wrote:
> > > On Tue, 19 Oct 2010, Christoph Egger wrote:
> > > > On Monday 18 October 2010 14:50:35 Christoph Egger wrote:
> > > > > Hi!
> > > > >
> > > > > I cannot start a guest with 'xl create' due to an endless loop
> > > > > in libxl.c, function libxl__get_free_memory_slack():
> > > > >
> > > > > There is this code snippet:
> > > > >
> > > > > retry:
> > > > >     free_mem_slack_s = libxl__xs_read(gc, XBT_NULL,
> > > > > free_mem_slack_path); if (!free_mem_slack_s) {
> > > > >         rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
> > > > >         if (rc < 0)
> > > > >             return rc;
> > > > >         goto retry;
> > > > >     } else {
> > > > >
> > > > >
> > > > > libxl__xs_read() returns 0 and libxl__fill_dom0_memory_info()
> > > > > also returns 0. So there's a loop of retries.
> > > >
> > > > Attached patch fixes the endless loop.
> > > >
> > > > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
> > >
> > > thanks for the patch but this cannot possibly be the right fix:
> > >
> > >
> > > diff -r 9d8d6b93114e tools/libxl/libxl.c
> > > --- a/tools/libxl/libxl.c	Mon Oct 18 15:24:39 2010 +0200
> > > +++ b/tools/libxl/libxl.c	Tue Oct 19 10:29:00 2010 +0200
> > > @@ -2836,7 +2836,7 @@ retry:
> > >      free_mem_slack_s = libxl__xs_read(gc, XBT_NULL,
> > > free_mem_slack_path); if (!free_mem_slack_s) {
> > >          rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
> > > -        if (rc < 0)
> > > +        if (rc <= 0)
> > >              return rc;
> > >          goto retry;
> > >      } else {
> > >
> > > the idea is that libxl__fill_dom0_memory_info should fill the
> > > missing informations in xenstore so that we can go ahead and try to
> > > read them again and the second time should be successful.
> > > Libxl__fill_dom0_memory_info returns 0 on success, so it is correct
> > > to goto retry in that case.
> > >
> > > The bug must be in libxl__fill_dom0_memory_info that doesn't return
> > > error when it should.
> > > Does the appended patch works for you?
> > >
> > > ---
> > >
> > > diff -r 00b92112b055 tools/libxl/libxl.c
> > > --- a/tools/libxl/libxl.c	Wed Oct 20 17:26:51 2010 +0100
> > > +++ b/tools/libxl/libxl.c	Thu Oct 21 15:08:29 2010 +0100
> > > @@ -2793,11 +2793,11 @@ retry_transaction:
> > >
> > >      rc = libxl_domain_info(ctx, &info, 0);
> > >      if (rc < 0)
> > > -        return rc;
> > > +        goto out;
> > >
> > >      rc = libxl_get_physinfo(ctx, &physinfo);
> > >      if (rc < 0)
> > > -        return rc;
> > > +        goto out;
> > >
> > >      libxl__xs_write(gc, t, target_path, "%"PRIu32,
> > >              (uint32_t) info.current_memkb); @@ -2816,9 +2816,12
> @@
> > > retry_transaction:
> > >      rc = 0;
> > >
> > >  out:
> > > -    if (!xs_transaction_end(ctx->xsh, t, 0))
> > > +    if (!xs_transaction_end(ctx->xsh, t, 0)) {
> > >          if (errno == EAGAIN)
> > >              goto retry_transaction;
> > > +        else
> > > +            rc = ERROR_FAIL;
> > > +    }
> > >
> > >
> > >      return rc;
> >
> > No, this patch has no effect for me.
> > In libxl__fill_dom0_memory_info(), the code path goes that way:
> >
> >     t = xs_transaction_start(ctx->xsh);
> >
> >     target = libxl__xs_read(gc, t, target_path);
> >     if (target) {    <-- target contains "5"
> >         *target_memkb = strtoul(target, &endptr, 10);
> >         if (*endptr != '\0') {     <-- *endptr contains '\0'
> >             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> >                     "invalid memory target %s from %s\n", target,
> > target_path);
> >             rc = ERROR_FAIL;
> >             goto out;
> >         }
> >         rc = 0;
> >         goto out;    <-- take this jump with rc being 0
> >     }
> >
> 
> A slightly modified version works.
> 
> Signed-off-by: Mark Langsdorf <Mark.Langsdorf@amd.com>
> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
> 
> 
> 
> 
> --
> ---to satisfy European Law for business letters:
> Advanced Micro Devices GmbH
> Einsteinring 24, 85609 Dornach b. Muenchen
> Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
> Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht
> Muenchen, HRB Nr. 43632

Re: [PATCH] xl create: endless loop

[Christoph Egger]

On Monday 08 November 2010 09:18:19 Zhang, Yang Z wrote:
> Did the patch apply? This issue still exist in xen-unstable(22364).

Yes.

>
> best regards
> yang
>
> > -----Original Message-----
> > From: xen-devel-bounces@lists.xensource.com
> > [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Christoph
> > Egger Sent: Thursday, November 04, 2010 11:40 PM
> > To: xen-devel@lists.xensource.com
> > Cc: Jackson; Stefano Stabellini
> > Subject: Re: [Xen-devel][PATCH] xl create: endless loop
> >
> > On Wednesday 03 November 2010 17:12:47 Christoph Egger wrote:
> > > On Thursday 21 October 2010 16:11:55 Stefano Stabellini wrote:
> > > > On Tue, 19 Oct 2010, Christoph Egger wrote:
> > > > > On Monday 18 October 2010 14:50:35 Christoph Egger wrote:
> > > > > > Hi!
> > > > > >
> > > > > > I cannot start a guest with 'xl create' due to an endless loop
> > > > > > in libxl.c, function libxl__get_free_memory_slack():
> > > > > >
> > > > > > There is this code snippet:
> > > > > >
> > > > > > retry:
> > > > > >     free_mem_slack_s = libxl__xs_read(gc, XBT_NULL,
> > > > > > free_mem_slack_path); if (!free_mem_slack_s) {
> > > > > >         rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
> > > > > >         if (rc < 0)
> > > > > >             return rc;
> > > > > >         goto retry;
> > > > > >     } else {
> > > > > >
> > > > > >
> > > > > > libxl__xs_read() returns 0 and libxl__fill_dom0_memory_info()
> > > > > > also returns 0. So there's a loop of retries.
> > > > >
> > > > > Attached patch fixes the endless loop.
> > > > >
> > > > > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
> > > >
> > > > thanks for the patch but this cannot possibly be the right fix:
> > > >
> > > >
> > > > diff -r 9d8d6b93114e tools/libxl/libxl.c
> > > > --- a/tools/libxl/libxl.c	Mon Oct 18 15:24:39 2010 +0200
> > > > +++ b/tools/libxl/libxl.c	Tue Oct 19 10:29:00 2010 +0200
> > > > @@ -2836,7 +2836,7 @@ retry:
> > > >      free_mem_slack_s = libxl__xs_read(gc, XBT_NULL,
> > > > free_mem_slack_path); if (!free_mem_slack_s) {
> > > >          rc = libxl__fill_dom0_memory_info(gc, &target_memkb);
> > > > -        if (rc < 0)
> > > > +        if (rc <= 0)
> > > >              return rc;
> > > >          goto retry;
> > > >      } else {
> > > >
> > > > the idea is that libxl__fill_dom0_memory_info should fill the
> > > > missing informations in xenstore so that we can go ahead and try to
> > > > read them again and the second time should be successful.
> > > > Libxl__fill_dom0_memory_info returns 0 on success, so it is correct
> > > > to goto retry in that case.
> > > >
> > > > The bug must be in libxl__fill_dom0_memory_info that doesn't return
> > > > error when it should.
> > > > Does the appended patch works for you?
> > > >
> > > > ---
> > > >
> > > > diff -r 00b92112b055 tools/libxl/libxl.c
> > > > --- a/tools/libxl/libxl.c	Wed Oct 20 17:26:51 2010 +0100
> > > > +++ b/tools/libxl/libxl.c	Thu Oct 21 15:08:29 2010 +0100
> > > > @@ -2793,11 +2793,11 @@ retry_transaction:
> > > >
> > > >      rc = libxl_domain_info(ctx, &info, 0);
> > > >      if (rc < 0)
> > > > -        return rc;
> > > > +        goto out;
> > > >
> > > >      rc = libxl_get_physinfo(ctx, &physinfo);
> > > >      if (rc < 0)
> > > > -        return rc;
> > > > +        goto out;
> > > >
> > > >      libxl__xs_write(gc, t, target_path, "%"PRIu32,
> > > >              (uint32_t) info.current_memkb); @@ -2816,9 +2816,12
> >
> > @@
> >
> > > > retry_transaction:
> > > >      rc = 0;
> > > >
> > > >  out:
> > > > -    if (!xs_transaction_end(ctx->xsh, t, 0))
> > > > +    if (!xs_transaction_end(ctx->xsh, t, 0)) {
> > > >          if (errno == EAGAIN)
> > > >              goto retry_transaction;
> > > > +        else
> > > > +            rc = ERROR_FAIL;
> > > > +    }
> > > >
> > > >
> > > >      return rc;
> > >
> > > No, this patch has no effect for me.
> > > In libxl__fill_dom0_memory_info(), the code path goes that way:
> > >
> > >     t = xs_transaction_start(ctx->xsh);
> > >
> > >     target = libxl__xs_read(gc, t, target_path);
> > >     if (target) {    <-- target contains "5"
> > >         *target_memkb = strtoul(target, &endptr, 10);
> > >         if (*endptr != '\0') {     <-- *endptr contains '\0'
> > >             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> > >                     "invalid memory target %s from %s\n", target,
> > > target_path);
> > >             rc = ERROR_FAIL;
> > >             goto out;
> > >         }
> > >         rc = 0;
> > >         goto out;    <-- take this jump with rc being 0
> > >     }
> >
> > A slightly modified version works.
> >
> > Signed-off-by: Mark Langsdorf <Mark.Langsdorf@amd.com>
> > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH] xl create: endless loop

[Stefano Stabellini]

On Wed, 3 Nov 2010, Christoph Egger wrote:
> No, this patch has no effect for me.
> In libxl__fill_dom0_memory_info(), the code path goes that way:
> 
>     t = xs_transaction_start(ctx->xsh);
> 
>     target = libxl__xs_read(gc, t, target_path);
>     if (target) {    <-- target contains "5"
>         *target_memkb = strtoul(target, &endptr, 10);
>         if (*endptr != '\0') {     <-- *endptr contains '\0'
>             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
>                     "invalid memory target %s from %s\n", target, 
> target_path);
>             rc = ERROR_FAIL;
>             goto out;
>         }
>         rc = 0;
>         goto out;    <-- take this jump with rc being 0
>     }
> 
> 

The problem you are having is that somebody in your system is setting a
target for dom0 without setting freemem-slack. Are you still running
xend at boot?

Currently libxl__fill_dom0_memory_info assumes that both values are set
initially at the same time (by libxl__fill_dom0_memory_info).
This patch should fix the issue, I would appreciate if you could test
it.

---


libxl: do not assume target and freemem-slack are written at the same time

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

diff -r 7188d1e4b0e1 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Tue Nov 09 12:00:05 2010 +0000
+++ b/tools/libxl/libxl.c	Tue Nov 09 18:05:52 2010 +0000
@@ -2779,18 +2779,25 @@ static int libxl__fill_dom0_memory_info(
     int rc;
     libxl_dominfo info;
     libxl_physinfo physinfo;
-    char *target = NULL, *endptr = NULL;
+    char *target = NULL, *staticmax = NULL, *freememslack = NULL, *endptr = NULL;
     char *target_path = "/local/domain/0/memory/target";
     char *max_path = "/local/domain/0/memory/static-max";
     char *free_mem_slack_path = "/local/domain/0/memory/freemem-slack";
     xs_transaction_t t;
     libxl_ctx *ctx = libxl__gc_owner(gc);
-    uint32_t free_mem_slack = 0;
+    uint32_t free_mem_slack_kb = 0;
 
 retry_transaction:
     t = xs_transaction_start(ctx->xsh);
 
     target = libxl__xs_read(gc, t, target_path);
+    staticmax = libxl__xs_read(gc, t, target_path);
+    freememslack = libxl__xs_read(gc, t, target_path);
+    if (target && staticmax && freememslack) {
+        rc = 0;
+        goto out;
+    }
+
     if (target) {
         *target_memkb = strtoul(target, &endptr, 10);
         if (*endptr != '\0') {
@@ -2799,38 +2806,43 @@ retry_transaction:
             rc = ERROR_FAIL;
             goto out;
         }
-        rc = 0;
-        goto out;
     }
 
     rc = libxl_domain_info(ctx, &info, 0);
     if (rc < 0)
-        return rc;
+        goto out;
 
     rc = libxl_get_physinfo(ctx, &physinfo);
     if (rc < 0)
-        return rc;
-
-    libxl__xs_write(gc, t, target_path, "%"PRIu32,
-            (uint32_t) info.current_memkb);
-    libxl__xs_write(gc, t, max_path, "%"PRIu32,
-            (uint32_t) info.max_memkb);
-
-    free_mem_slack = (uint32_t) (PAGE_TO_MEMKB(physinfo.total_pages) -
-            info.current_memkb);
-    /* From empirical measurements the free_mem_slack shouldn't be more
-     * than 15% of the total memory present on the system. */
-    if (free_mem_slack > PAGE_TO_MEMKB(physinfo.total_pages) * 0.15)
-        free_mem_slack = PAGE_TO_MEMKB(physinfo.total_pages) * 0.15;
-    libxl__xs_write(gc, t, free_mem_slack_path, "%"PRIu32, free_mem_slack);
-
-    *target_memkb = (uint32_t) info.current_memkb;
+        goto out;
+
+    if (target == NULL) {
+        libxl__xs_write(gc, t, target_path, "%"PRIu32,
+                (uint32_t) info.current_memkb);
+        *target_memkb = (uint32_t) info.current_memkb;
+    }
+    if (staticmax == NULL)
+        libxl__xs_write(gc, t, max_path, "%"PRIu32,
+                (uint32_t) info.max_memkb);
+
+    if (freememslack == NULL) {
+        free_mem_slack_kb = (uint32_t) (PAGE_TO_MEMKB(physinfo.total_pages) -
+                info.current_memkb);
+        /* From empirical measurements the free_mem_slack shouldn't be more
+         * than 15% of the total memory present on the system. */
+        if (free_mem_slack_kb > PAGE_TO_MEMKB(physinfo.total_pages) * 0.15)
+            free_mem_slack_kb = PAGE_TO_MEMKB(physinfo.total_pages) * 0.15;
+        libxl__xs_write(gc, t, free_mem_slack_path, "%"PRIu32, free_mem_slack_kb);
+    }
     rc = 0;
 
 out:
-    if (!xs_transaction_end(ctx->xsh, t, 0))
+    if (!xs_transaction_end(ctx->xsh, t, 0)) {
         if (errno == EAGAIN)
             goto retry_transaction;
+        else
+            rc = ERROR_FAIL;
+    }
 
 
     return rc;

Re: [PATCH] xl create: endless loop

[Christoph Egger]

On Tuesday 09 November 2010 19:08:14 Stefano Stabellini wrote:
> On Wed, 3 Nov 2010, Christoph Egger wrote:
> > No, this patch has no effect for me.
> > In libxl__fill_dom0_memory_info(), the code path goes that way:
> >
> >     t = xs_transaction_start(ctx->xsh);
> >
> >     target = libxl__xs_read(gc, t, target_path);
> >     if (target) {    <-- target contains "5"
> >         *target_memkb = strtoul(target, &endptr, 10);
> >         if (*endptr != '\0') {     <-- *endptr contains '\0'
> >             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> >                     "invalid memory target %s from %s\n", target,
> > target_path);
> >             rc = ERROR_FAIL;
> >             goto out;
> >         }
> >         rc = 0;
> >         goto out;    <-- take this jump with rc being 0
> >     }
>
> The problem you are having is that somebody in your system is setting a
> target for dom0 without setting freemem-slack. Are you still running
> xend at boot?

Yes, I do.
Xen is booted with dom0_mem.
Dom0 has autoballooning disabled in the kernel.

> Currently libxl__fill_dom0_memory_info assumes that both values are set
> initially at the same time (by libxl__fill_dom0_memory_info).
> This patch should fix the issue, I would appreciate if you could test
> it.
>
> ---
>
>
> libxl: do not assume target and freemem-slack are written at the same time
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> diff -r 7188d1e4b0e1 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Tue Nov 09 12:00:05 2010 +0000
> +++ b/tools/libxl/libxl.c	Tue Nov 09 18:05:52 2010 +0000
> @@ -2779,18 +2779,25 @@ static int libxl__fill_dom0_memory_info(
>      int rc;
>      libxl_dominfo info;
>      libxl_physinfo physinfo;
> -    char *target = NULL, *endptr = NULL;
> +    char *target = NULL, *staticmax = NULL, *freememslack = NULL, *endptr
> = NULL; char *target_path = "/local/domain/0/memory/target";
>      char *max_path = "/local/domain/0/memory/static-max";
>      char *free_mem_slack_path = "/local/domain/0/memory/freemem-slack";
>      xs_transaction_t t;
>      libxl_ctx *ctx = libxl__gc_owner(gc);
> -    uint32_t free_mem_slack = 0;
> +    uint32_t free_mem_slack_kb = 0;
>
>  retry_transaction:
>      t = xs_transaction_start(ctx->xsh);
>
>      target = libxl__xs_read(gc, t, target_path);
> +    staticmax = libxl__xs_read(gc, t, target_path);
> +    freememslack = libxl__xs_read(gc, t, target_path);
> +    if (target && staticmax && freememslack) {
> +        rc = 0;
> +        goto out;
> +    }

*target, *staticmax and *freememslack contain the value "5".
So with this patch, rc always returns 0 from there.

Christoph

> +
>      if (target) {
>          *target_memkb = strtoul(target, &endptr, 10);
>          if (*endptr != '\0') {
> @@ -2799,38 +2806,43 @@ retry_transaction:
>              rc = ERROR_FAIL;
>              goto out;
>          }
> -        rc = 0;
> -        goto out;
>      }
>
>      rc = libxl_domain_info(ctx, &info, 0);
>      if (rc < 0)
> -        return rc;
> +        goto out;
>
>      rc = libxl_get_physinfo(ctx, &physinfo);
>      if (rc < 0)
> -        return rc;
> -
> -    libxl__xs_write(gc, t, target_path, "%"PRIu32,
> -            (uint32_t) info.current_memkb);
> -    libxl__xs_write(gc, t, max_path, "%"PRIu32,
> -            (uint32_t) info.max_memkb);
> -
> -    free_mem_slack = (uint32_t) (PAGE_TO_MEMKB(physinfo.total_pages) -
> -            info.current_memkb);
> -    /* From empirical measurements the free_mem_slack shouldn't be more
> -     * than 15% of the total memory present on the system. */
> -    if (free_mem_slack > PAGE_TO_MEMKB(physinfo.total_pages) * 0.15)
> -        free_mem_slack = PAGE_TO_MEMKB(physinfo.total_pages) * 0.15;
> -    libxl__xs_write(gc, t, free_mem_slack_path, "%"PRIu32,
> free_mem_slack); -
> -    *target_memkb = (uint32_t) info.current_memkb;
> +        goto out;
> +
> +    if (target == NULL) {
> +        libxl__xs_write(gc, t, target_path, "%"PRIu32,
> +                (uint32_t) info.current_memkb);
> +        *target_memkb = (uint32_t) info.current_memkb;
> +    }
> +    if (staticmax == NULL)
> +        libxl__xs_write(gc, t, max_path, "%"PRIu32,
> +                (uint32_t) info.max_memkb);
> +
> +    if (freememslack == NULL) {
> +        free_mem_slack_kb = (uint32_t)
> (PAGE_TO_MEMKB(physinfo.total_pages) - +               
> info.current_memkb);
> +        /* From empirical measurements the free_mem_slack shouldn't be
> more +         * than 15% of the total memory present on the system. */ +  
>      if (free_mem_slack_kb > PAGE_TO_MEMKB(physinfo.total_pages) * 0.15) + 
>           free_mem_slack_kb = PAGE_TO_MEMKB(physinfo.total_pages) * 0.15; +
>        libxl__xs_write(gc, t, free_mem_slack_path, "%"PRIu32,
> free_mem_slack_kb); +    }
>      rc = 0;
>
>  out:
> -    if (!xs_transaction_end(ctx->xsh, t, 0))
> +    if (!xs_transaction_end(ctx->xsh, t, 0)) {
>          if (errno == EAGAIN)
>              goto retry_transaction;
> +        else
> +            rc = ERROR_FAIL;
> +    }
>
>
>      return rc;



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH] xl create: endless loop

[Stefano Stabellini]

On Wed, 10 Nov 2010, Christoph Egger wrote:
> On Tuesday 09 November 2010 19:08:14 Stefano Stabellini wrote:
> > On Wed, 3 Nov 2010, Christoph Egger wrote:
> > > No, this patch has no effect for me.
> > > In libxl__fill_dom0_memory_info(), the code path goes that way:
> > >
> > >     t = xs_transaction_start(ctx->xsh);
> > >
> > >     target = libxl__xs_read(gc, t, target_path);
> > >     if (target) {    <-- target contains "5"
> > >         *target_memkb = strtoul(target, &endptr, 10);
> > >         if (*endptr != '\0') {     <-- *endptr contains '\0'
> > >             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> > >                     "invalid memory target %s from %s\n", target,
> > > target_path);
> > >             rc = ERROR_FAIL;
> > >             goto out;
> > >         }
> > >         rc = 0;
> > >         goto out;    <-- take this jump with rc being 0
> > >     }
> >
> > The problem you are having is that somebody in your system is setting a
> > target for dom0 without setting freemem-slack. Are you still running
> > xend at boot?
> 
> Yes, I do.

running xend alongside xl is not recommended, it could cause bugs,
especially if you don't disable autoballooning


> Xen is booted with dom0_mem.
> Dom0 has autoballooning disabled in the kernel.
> 

You still need to disable autoballooning in xl, setting autoballooning
to 0 in /etc/xen/xl.conf


> > Currently libxl__fill_dom0_memory_info assumes that both values are set
> > initially at the same time (by libxl__fill_dom0_memory_info).
> > This patch should fix the issue, I would appreciate if you could test
> > it.
> >
> > ---
> >
> >
> > libxl: do not assume target and freemem-slack are written at the same time
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >
> > diff -r 7188d1e4b0e1 tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c	Tue Nov 09 12:00:05 2010 +0000
> > +++ b/tools/libxl/libxl.c	Tue Nov 09 18:05:52 2010 +0000
> > @@ -2779,18 +2779,25 @@ static int libxl__fill_dom0_memory_info(
> >      int rc;
> >      libxl_dominfo info;
> >      libxl_physinfo physinfo;
> > -    char *target = NULL, *endptr = NULL;
> > +    char *target = NULL, *staticmax = NULL, *freememslack = NULL, *endptr
> > = NULL; char *target_path = "/local/domain/0/memory/target";
> >      char *max_path = "/local/domain/0/memory/static-max";
> >      char *free_mem_slack_path = "/local/domain/0/memory/freemem-slack";
> >      xs_transaction_t t;
> >      libxl_ctx *ctx = libxl__gc_owner(gc);
> > -    uint32_t free_mem_slack = 0;
> > +    uint32_t free_mem_slack_kb = 0;
> >
> >  retry_transaction:
> >      t = xs_transaction_start(ctx->xsh);
> >
> >      target = libxl__xs_read(gc, t, target_path);
> > +    staticmax = libxl__xs_read(gc, t, target_path);
> > +    freememslack = libxl__xs_read(gc, t, target_path);
> > +    if (target && staticmax && freememslack) {
> > +        rc = 0;
> > +        goto out;
> > +    }
> 
> *target, *staticmax and *freememslack contain the value "5".
> So with this patch, rc always returns 0 from there.
> 

that is correct: the values are there, so there is no need to write them
to xenstore again, and the caller should be able to read the target
correctly

RE: [PATCH] xl create: endless loop

[Zhang, Yang Z]

This issue still exist in my side with latest xen unstable. Did the fixed patch work for you?

best regards
yang

> -----Original Message-----
> From: xen-devel-bounces@lists.xensource.com
> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Stefano
> Stabellini
> Sent: Wednesday, November 10, 2010 7:53 PM
> To: Christoph Egger
> Cc: xen-devel@lists.xensource.com; Ian Jackson; Stefano Stabellini
> Subject: Re: [Xen-devel][PATCH] xl create: endless loop
> 
> On Wed, 10 Nov 2010, Christoph Egger wrote:
> > On Tuesday 09 November 2010 19:08:14 Stefano Stabellini wrote:
> > > On Wed, 3 Nov 2010, Christoph Egger wrote:
> > > > No, this patch has no effect for me.
> > > > In libxl__fill_dom0_memory_info(), the code path goes that way:
> > > >
> > > >     t = xs_transaction_start(ctx->xsh);
> > > >
> > > >     target = libxl__xs_read(gc, t, target_path);
> > > >     if (target) {    <-- target contains "5"
> > > >         *target_memkb = strtoul(target, &endptr, 10);
> > > >         if (*endptr != '\0') {     <-- *endptr contains '\0'
> > > >             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> > > >                     "invalid memory target %s from %s\n", target,
> > > > target_path);
> > > >             rc = ERROR_FAIL;
> > > >             goto out;
> > > >         }
> > > >         rc = 0;
> > > >         goto out;    <-- take this jump with rc being 0
> > > >     }
> > >
> > > The problem you are having is that somebody in your system is setting a
> > > target for dom0 without setting freemem-slack. Are you still running
> > > xend at boot?
> >
> > Yes, I do.
> 
> running xend alongside xl is not recommended, it could cause bugs,
> especially if you don't disable autoballooning
> 
> 
> > Xen is booted with dom0_mem.
> > Dom0 has autoballooning disabled in the kernel.
> >
> 
> You still need to disable autoballooning in xl, setting autoballooning
> to 0 in /etc/xen/xl.conf
> 
> 
> > > Currently libxl__fill_dom0_memory_info assumes that both values are set
> > > initially at the same time (by libxl__fill_dom0_memory_info).
> > > This patch should fix the issue, I would appreciate if you could test
> > > it.
> > >
> > > ---
> > >
> > >
> > > libxl: do not assume target and freemem-slack are written at the same time
> > >
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > >
> > > diff -r 7188d1e4b0e1 tools/libxl/libxl.c
> > > --- a/tools/libxl/libxl.c	Tue Nov 09 12:00:05 2010 +0000
> > > +++ b/tools/libxl/libxl.c	Tue Nov 09 18:05:52 2010 +0000
> > > @@ -2779,18 +2779,25 @@ static int libxl__fill_dom0_memory_info(
> > >      int rc;
> > >      libxl_dominfo info;
> > >      libxl_physinfo physinfo;
> > > -    char *target = NULL, *endptr = NULL;
> > > +    char *target = NULL, *staticmax = NULL, *freememslack = NULL,
> *endptr
> > > = NULL; char *target_path = "/local/domain/0/memory/target";
> > >      char *max_path = "/local/domain/0/memory/static-max";
> > >      char *free_mem_slack_path =
> "/local/domain/0/memory/freemem-slack";
> > >      xs_transaction_t t;
> > >      libxl_ctx *ctx = libxl__gc_owner(gc);
> > > -    uint32_t free_mem_slack = 0;
> > > +    uint32_t free_mem_slack_kb = 0;
> > >
> > >  retry_transaction:
> > >      t = xs_transaction_start(ctx->xsh);
> > >
> > >      target = libxl__xs_read(gc, t, target_path);
> > > +    staticmax = libxl__xs_read(gc, t, target_path);
> > > +    freememslack = libxl__xs_read(gc, t, target_path);
> > > +    if (target && staticmax && freememslack) {
> > > +        rc = 0;
> > > +        goto out;
> > > +    }
> >
> > *target, *staticmax and *freememslack contain the value "5".
> > So with this patch, rc always returns 0 from there.
> >
> 
> that is correct: the values are there, so there is no need to write them
> to xenstore again, and the caller should be able to read the target
> correctly
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

RE: [PATCH] xl create: endless loop

[Stefano Stabellini]

On Tue, 16 Nov 2010, Zhang, Yang Z wrote:
> This issue still exist in my side with latest xen unstable. Did the fixed patch work for you?
> 

Do you have autoballoon (in /etc/xen/xl.conf) set to 0 or 1?
Do you pass dom0_mem as Xen command line option?

If you pass dom0_mem to Xen, then autoballoon should be set to 0.
If that's not the case, how much memory do you have on your testbox?
How big is the VM you are trying to start?

RE: [PATCH] xl create: endless loop

[Zhang, Yang Z]

Yes I have the dom0_mem in xen command line. But I think the autoballoon is disable by default since there had an annotate before it, if it not I think we should add some comments like "autoballoon default is enable" as we see in the xmexample.hvm.

And the another question is why I should to disable the autoballoon manually in the xl.conf? why don't you to add a check in the xl to see whether the autoballon is enable or disable?
 
best regards
yang


> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Tuesday, November 16, 2010 10:07 PM
> To: Zhang, Yang Z
> Cc: Stefano Stabellini; Christoph Egger; xen-devel@lists.xensource.com; Ian
> Jackson
> Subject: RE: [Xen-devel][PATCH] xl create: endless loop
> 
> On Tue, 16 Nov 2010, Zhang, Yang Z wrote:
> > This issue still exist in my side with latest xen unstable. Did the fixed patch
> work for you?
> >
> 
> Do you have autoballoon (in /etc/xen/xl.conf) set to 0 or 1?
> Do you pass dom0_mem as Xen command line option?
> 
> If you pass dom0_mem to Xen, then autoballoon should be set to 0.
> If that's not the case, how much memory do you have on your testbox?
> How big is the VM you are trying to start?

RE: [PATCH] xl create: endless loop

[Stefano Stabellini]

On Tue, 16 Nov 2010, Zhang, Yang Z wrote:
> Yes I have the dom0_mem in xen command line. But I think the autoballoon is disable by default since there had an annotate before it, if it not I think we should add some comments like "autoballoon default is enable" as we see in the xmexample.hvm.
> 

Autoballoon is enabled by default, a comment would probably be good


> And the another question is why I should to disable the autoballoon manually in the xl.conf? why don't you to add a check in the xl to see whether the autoballon is enable or disable?
>  

Good question. The problem is: how should I write this check?
AFAIK there is no clear way to know if autoballooning should be enabled
or disabled, apart from parsing the xen command line options, but it
seems really ugly to me.

In any case, could you please try disabling autoballooning and let me
know if you still have this issue?

RE: [PATCH] xl create: endless loop

[Zhang, Yang Z]

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Tuesday, November 16, 2010 10:40 PM
> To: Zhang, Yang Z
> Cc: Stefano Stabellini; Christoph Egger; xen-devel@lists.xensource.com; Ian
> Jackson
> Subject: RE: [Xen-devel][PATCH] xl create: endless loop
> 
> On Tue, 16 Nov 2010, Zhang, Yang Z wrote:
> > Yes I have the dom0_mem in xen command line. But I think the autoballoon is
> disable by default since there had an annotate before it, if it not I think we
> should add some comments like "autoballoon default is enable" as we see in
> the xmexample.hvm.
> >
> 
> Autoballoon is enabled by default, a comment would probably be good
> 
> 
> > And the another question is why I should to disable the autoballoon manually
> in the xl.conf? why don't you to add a check in the xl to see whether the
> autoballon is enable or disable?
> >
> 
> Good question. The problem is: how should I write this check?
> AFAIK there is no clear way to know if autoballooning should be enabled
> or disabled, apart from parsing the xen command line options, but it
> seems really ugly to me.
> 
> In any case, could you please try disabling autoballooning and let me
> know if you still have this issue?
It works with disable the autoballoon. Thanks for you help.

And a question with qcow issue I reported in another mail. Since our automation test is based in qcow images, because copy raw images take a long times(we have 60+ case), so will anyone to take a look at this issue? it will block our automation test with xl.


best regards
yang

RE: [PATCH] xl create: endless loop

[Stefano Stabellini]

On Tue, 16 Nov 2010, Zhang, Yang Z wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: Tuesday, November 16, 2010 10:40 PM
> > To: Zhang, Yang Z
> > Cc: Stefano Stabellini; Christoph Egger; xen-devel@lists.xensource.com; Ian
> > Jackson
> > Subject: RE: [Xen-devel][PATCH] xl create: endless loop
> > 
> > On Tue, 16 Nov 2010, Zhang, Yang Z wrote:
> > > Yes I have the dom0_mem in xen command line. But I think the autoballoon is
> > disable by default since there had an annotate before it, if it not I think we
> > should add some comments like "autoballoon default is enable" as we see in
> > the xmexample.hvm.
> > >
> > 
> > Autoballoon is enabled by default, a comment would probably be good
> > 
> > 
> > > And the another question is why I should to disable the autoballoon manually
> > in the xl.conf? why don't you to add a check in the xl to see whether the
> > autoballon is enable or disable?
> > >
> > 
> > Good question. The problem is: how should I write this check?
> > AFAIK there is no clear way to know if autoballooning should be enabled
> > or disabled, apart from parsing the xen command line options, but it
> > seems really ugly to me.
> > 
> > In any case, could you please try disabling autoballooning and let me
> > know if you still have this issue?
> It works with disable the autoballoon. Thanks for you help.
> 
> And a question with qcow issue I reported in another mail. Since our automation test is based in qcow images, because copy raw images take a long times(we have 60+ case), so will anyone to take a look at this issue? it will block our automation test with xl.
> 
 
Daniel, do you have any ideas?

RE: [PATCH] xl create: endless loop

[Daniel Stodden]

On Tue, 2010-11-16 at 10:57 -0500, Stefano Stabellini wrote:
> On Tue, 16 Nov 2010, Zhang, Yang Z wrote:
> > > -----Original Message-----
> > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > > Sent: Tuesday, November 16, 2010 10:40 PM
> > > To: Zhang, Yang Z
> > > Cc: Stefano Stabellini; Christoph Egger; xen-devel@lists.xensource.com; Ian
> > > Jackson
> > > Subject: RE: [Xen-devel][PATCH] xl create: endless loop
> > > 
> > > On Tue, 16 Nov 2010, Zhang, Yang Z wrote:
> > > > Yes I have the dom0_mem in xen command line. But I think the autoballoon is
> > > disable by default since there had an annotate before it, if it not I think we
> > > should add some comments like "autoballoon default is enable" as we see in
> > > the xmexample.hvm.
> > > >
> > > 
> > > Autoballoon is enabled by default, a comment would probably be good
> > > 
> > > 
> > > > And the another question is why I should to disable the autoballoon manually
> > > in the xl.conf? why don't you to add a check in the xl to see whether the
> > > autoballon is enable or disable?
> > > >
> > > 
> > > Good question. The problem is: how should I write this check?
> > > AFAIK there is no clear way to know if autoballooning should be enabled
> > > or disabled, apart from parsing the xen command line options, but it
> > > seems really ugly to me.
> > > 
> > > In any case, could you please try disabling autoballooning and let me
> > > know if you still have this issue?
> > It works with disable the autoballoon. Thanks for you help.
> > 
> > And a question with qcow issue I reported in another mail. Since our automation test is based in qcow images, because copy raw images take a long times(we have 60+ case), so will anyone to take a look at this issue? it will block our automation test with xl.
> > 
>  
> Daniel, do you have any ideas?

Not the slightest. Just proof that: Are we talking about blktap qcow or
qemu? Still trying to find that mail.

Daniel

Re: [PATCH] xl create: endless loop

[Christoph Egger]

On Wednesday 17 November 2010 03:50:51 Daniel Stodden wrote:
> On Tue, 2010-11-16 at 10:57 -0500, Stefano Stabellini wrote:
> > On Tue, 16 Nov 2010, Zhang, Yang Z wrote:
> > > > -----Original Message-----
> > > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > > > Sent: Tuesday, November 16, 2010 10:40 PM
> > > > To: Zhang, Yang Z
> > > > Cc: Stefano Stabellini; Christoph Egger;
> > > > xen-devel@lists.xensource.com; Ian Jackson
> > > > Subject: RE: [Xen-devel][PATCH] xl create: endless loop
> > > >
> > > > On Tue, 16 Nov 2010, Zhang, Yang Z wrote:
> > > > > Yes I have the dom0_mem in xen command line. But I think the
> > > > > autoballoon is
> > > >
> > > > disable by default since there had an annotate before it, if it not I
> > > > think we should add some comments like "autoballoon default is
> > > > enable" as we see in the xmexample.hvm.
> > > >
> > > >
> > > > Autoballoon is enabled by default, a comment would probably be good
> > > >
> > > > > And the another question is why I should to disable the autoballoon
> > > > > manually
> > > >
> > > > in the xl.conf? why don't you to add a check in the xl to see whether
> > > > the autoballon is enable or disable?
> > > >
> > > >
> > > > Good question. The problem is: how should I write this check?
> > > > AFAIK there is no clear way to know if autoballooning should be
> > > > enabled or disabled, apart from parsing the xen command line options,
> > > > but it seems really ugly to me.
> > > >
> > > > In any case, could you please try disabling autoballooning and let me
> > > > know if you still have this issue?
> > >
> > > It works with disable the autoballoon. Thanks for you help.
> > >
> > > And a question with qcow issue I reported in another mail. Since our
> > > automation test is based in qcow images, because copy raw images take a
> > > long times(we have 60+ case), so will anyone to take a look at this
> > > issue? it will block our automation test with xl.
> >
> > Daniel, do you have any ideas?
>
> Not the slightest. Just proof that: Are we talking about blktap qcow or
> qemu? Still trying to find that mail.

xl links against blktap2, qemu links against blktap. I think, this may cause
some random behaviour.
qemu should move to blktap2 and drop blktap from the tools then.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH] xl create: endless loop

[Stefano Stabellini]

On Thu, 18 Nov 2010, Christoph Egger wrote:
> > Not the slightest. Just proof that: Are we talking about blktap qcow or
> > qemu? Still trying to find that mail.
> 
> xl links against blktap2, qemu links against blktap. I think, this may cause
> some random behaviour.
> qemu should move to blktap2 and drop blktap from the tools then.
> 

Is blktap support in qemu even used anywhere?
Maybe with xend and the XCP kernel because xl and new pvops kernels
don't even work with blktap.

I think we should probably remove it.

Re: [PATCH] xl create: endless loop

[Christoph Egger]

On Wednesday 10 November 2010 12:52:42 Stefano Stabellini wrote:
> On Wed, 10 Nov 2010, Christoph Egger wrote:
> > On Tuesday 09 November 2010 19:08:14 Stefano Stabellini wrote:
> > > On Wed, 3 Nov 2010, Christoph Egger wrote:
> > > > No, this patch has no effect for me.
> > > > In libxl__fill_dom0_memory_info(), the code path goes that way:
> > > >
> > > >     t = xs_transaction_start(ctx->xsh);
> > > >
> > > >     target = libxl__xs_read(gc, t, target_path);
> > > >     if (target) {    <-- target contains "5"
> > > >         *target_memkb = strtoul(target, &endptr, 10);
> > > >         if (*endptr != '\0') {     <-- *endptr contains '\0'
> > > >             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> > > >                     "invalid memory target %s from %s\n", target,
> > > > target_path);
> > > >             rc = ERROR_FAIL;
> > > >             goto out;
> > > >         }
> > > >         rc = 0;
> > > >         goto out;    <-- take this jump with rc being 0
> > > >     }
> > >
> > > The problem you are having is that somebody in your system is setting a
> > > target for dom0 without setting freemem-slack. Are you still running
> > > xend at boot?
> >
> > Yes, I do.
>
> running xend alongside xl is not recommended, it could cause bugs,
> especially if you don't disable autoballooning
>
>
> > > Currently libxl__fill_dom0_memory_info assumes that both values are set
> > > initially at the same time (by libxl__fill_dom0_memory_info).
> > > This patch should fix the issue, I would appreciate if you could test
> > > it.
> > >
> > > ---
> > >
> > >
> > > libxl: do not assume target and freemem-slack are written at the same
> > > time
> > >
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > >
> > > diff -r 7188d1e4b0e1 tools/libxl/libxl.c
> > > --- a/tools/libxl/libxl.c	Tue Nov 09 12:00:05 2010 +0000
> > > +++ b/tools/libxl/libxl.c	Tue Nov 09 18:05:52 2010 +0000
> > > @@ -2779,18 +2779,25 @@ static int libxl__fill_dom0_memory_info(
> > >      int rc;
> > >      libxl_dominfo info;
> > >      libxl_physinfo physinfo;
> > > -    char *target = NULL, *endptr = NULL;
> > > +    char *target = NULL, *staticmax = NULL, *freememslack = NULL,
> > > *endptr = NULL; char *target_path = "/local/domain/0/memory/target";
> > >      char *max_path = "/local/domain/0/memory/static-max";
> > >      char *free_mem_slack_path =
> > > "/local/domain/0/memory/freemem-slack"; xs_transaction_t t;
> > >      libxl_ctx *ctx = libxl__gc_owner(gc);
> > > -    uint32_t free_mem_slack = 0;
> > > +    uint32_t free_mem_slack_kb = 0;
> > >
> > >  retry_transaction:
> > >      t = xs_transaction_start(ctx->xsh);
> > >
> > >      target = libxl__xs_read(gc, t, target_path);
> > > +    staticmax = libxl__xs_read(gc, t, target_path);
> > > +    freememslack = libxl__xs_read(gc, t, target_path);
> > > +    if (target && staticmax && freememslack) {
> > > +        rc = 0;
> > > +        goto out;
> > > +    }
> >
> > *target, *staticmax and *freememslack contain the value "5".
> > So with this patch, rc always returns 0 from there.
>
> that is correct: the values are there, so there is no need to write them
> to xenstore again, and the caller should be able to read the target
> correctly

> > Xen is booted with dom0_mem.
> > Dom0 has autoballooning disabled in the kernel.
>
> You still need to disable autoballooning in xl, setting autoballooning
> to 0 in /etc/xen/xl.conf

Yes, that makes the patch work for me. I have success in booting the
guest the first time with 'xl create'.

Please commit the patch.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH] xl create: endless loop [and 1 more messages]

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel][PATCH] xl create: endless loop"):
> libxl: do not assume target and freemem-slack are written at the same time

I have applied this patch, thanks.

Ian.