[PATCH 7 of 8] xl: free memory before building a domain

[PATCH 7 of 8] xl: free memory before building a domain

[Stefano Stabellini]

xl: free memory before building a domain

Free the needed amount of memory before proceeding with the domain
build.
Use a filelock to prevent other xl instances from conflicting during
this operation.

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

diff -r b80a1688b5c9 tools/examples/xl.conf
--- a/tools/examples/xl.conf	Thu Aug 26 18:03:20 2010 +0100
+++ b/tools/examples/xl.conf	Thu Aug 26 18:26:04 2010 +0100
@@ -3,3 +3,6 @@
 # automatically balloon down dom0 when xen doesn't have enough free
 # memory to create a domain
 autoballon=1
+
+# full path of the lockfile used by xl during domain creation
+lockfile="/var/lock/xl"
diff -r b80a1688b5c9 tools/libxl/xl.c
--- a/tools/libxl/xl.c	Thu Aug 26 18:03:20 2010 +0100
+++ b/tools/libxl/xl.c	Thu Aug 26 18:26:04 2010 +0100
@@ -34,6 +34,7 @@
 
 xentoollog_logger_stdiostream *logger;
 int autoballoon = 1;
+char *lockfile = "/var/lock/xl";
 
 static xentoollog_level minmsglevel = XTL_PROGRESS;
 
@@ -44,6 +45,7 @@ static void parse_global_config(const ch
     long l;
     XLU_Config *config;
     int e;
+    const char *buf;
 
     config = xlu_cfg_init(stderr, configfile);
     if (!config) {
@@ -60,6 +62,9 @@ static void parse_global_config(const ch
     if (!xlu_cfg_get_long (config, "autoballoon", &l))
         autoballoon = l;
 
+    if (!xlu_cfg_get_string (config, "lockfile", &buf))
+        lockfile = strdup(buf);
+
     xlu_cfg_destroy(config);
 }
  
diff -r b80a1688b5c9 tools/libxl/xl.h
--- a/tools/libxl/xl.h	Thu Aug 26 18:03:20 2010 +0100
+++ b/tools/libxl/xl.h	Thu Aug 26 18:26:04 2010 +0100
@@ -92,5 +92,6 @@ extern xentoollog_logger_stdiostream *lo
 
 /* global options */
 extern int autoballoon;
+extern char *lockfile;
 
 #endif /* XL_H */
diff -r b80a1688b5c9 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Thu Aug 26 18:03:20 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Thu Aug 26 18:26:04 2010 +0100
@@ -68,6 +68,7 @@ libxl_ctx ctx;
 /* when we operate on a domain, it is this one: */
 static uint32_t domid;
 static const char *common_domname;
+static int fd_lock = -1;
 
 
 static const char savefileheader_magic[32]=
@@ -234,6 +235,65 @@ static void find_domain(const char *p)
     common_domname = was_name ? p : libxl_domid_to_name(&ctx, domid);
 }
 
+static int acquire_lock(void)
+{
+    int rc;
+    struct flock fl;
+
+    /* lock already acquired */
+    if (fd_lock >= 0)
+        return ERROR_INVAL;
+
+    fl.l_type = F_WRLCK;
+    fl.l_whence = SEEK_SET;
+    fl.l_start = 0;
+    fl.l_len = 0;
+    fd_lock = open(lockfile, O_WRONLY|O_CREAT, S_IWUSR);
+    if (fd_lock < 0) {
+        fprintf(stderr, "cannot open the lockfile %s errno=%d\n", lockfile, errno);
+        return ERROR_FAIL;
+    }
+get_lock:
+    rc = fcntl(fd_lock, F_SETLKW, &fl);
+    if (rc < 0 && errno == EINTR)
+        goto get_lock;
+    if (rc < 0) {
+        fprintf(stderr, "cannot acquire lock %s errno=%d\n", lockfile, errno);
+        rc = ERROR_FAIL;
+    } else
+        rc = 0;
+    return rc;
+}
+
+static int release_lock(void)
+{
+    int rc;
+    struct flock fl;
+
+    /* lock not acquired */
+    if (fd_lock < 0)
+        return ERROR_INVAL;
+
+release_lock:
+    fl.l_type = F_UNLCK;
+    fl.l_whence = SEEK_SET;
+    fl.l_start = 0;
+    fl.l_len = 0;
+
+    rc = fcntl(fd_lock, F_SETLKW, &fl);
+    if (rc < 0 && errno == EINTR)
+        goto release_lock;
+    if (rc < 0) {
+        fprintf(stderr, "cannot release lock %s, errno=%d\n", lockfile, errno);
+        rc = ERROR_FAIL;
+    } else
+        rc = 0;
+    close(fd_lock);
+    fd_lock = -1;
+
+    return rc;
+}
+
 #define LOG(_f, _a...)   dolog(__FILE__, __LINE__, __func__, _f "\n", ##_a)
 
 static void dolog(const char *file, int line, const char *func, char *fmt, ...)
@@ -1209,6 +1269,48 @@ struct domain_create {
     char **migration_domname_r;
 };
 
+static int freemem(libxl_domain_build_info *b_info, libxl_device_model_info *dm_info)
+{
+    int rc, retries = 3;
+    uint32_t need_memkb, free_memkb;
+
+    if (!autoballoon)
+        return 0;
+
+    rc = libxl_domain_need_memory(&ctx, b_info, dm_info, &need_memkb);
+    if (rc < 0)
+        return rc;
+
+    do {
+        rc = libxl_get_free_memory(&ctx, &free_memkb);
+        if (rc < 0)
+            return rc;
+
+        if (free_memkb >= need_memkb)
+            return 0;
+
+        rc = libxl_set_relative_memory_target(&ctx, 0, free_memkb - need_memkb, 0);
+        if (rc < 0)
+            return rc;
+
+        rc = libxl_wait_for_free_memory(&ctx, domid, need_memkb, 10);
+        if (!rc)
+            return 0;
+        else if (rc != ERROR_NOMEM)
+            return rc;
+
+        /* the memory target has been reached but the free memory is still
+         * not enough: loop over again */
+        rc = libxl_wait_for_memory_target(&ctx, 0, 1);
+        if (rc < 0)
+            return rc;
+
+        retries--;
+    } while (retries > 0);
+
+    return ERROR_NOMEM;
+}
+
 static int create_domain(struct domain_create *dom_info)
 {
     struct domain_config d_config;
@@ -1367,6 +1469,17 @@ static int create_domain(struct domain_c
 start:
     domid = 0;
 
+    rc = acquire_lock();
+    if (rc < 0)
+        goto error_out;
+
+    ret = freemem(&d_config.b_info, &dm_info);
+    if (ret < 0) {
+        fprintf(stderr, "failed to free memory for the domain\n");
+        ret = ERROR_FAIL;
+        goto error_out;
+    }
+
     ret = libxl_domain_make(&ctx, &d_config.c_info, &domid);
     if (ret) {
         fprintf(stderr, "cannot make domain: %d\n", ret);
@@ -1475,6 +1588,8 @@ start:
             goto error_out;
     }
 
+    release_lock();
+
     if (!paused)
         libxl_domain_unpause(&ctx, domid);
 
@@ -1612,6 +1727,7 @@ start:
     }
 
 error_out:
+    release_lock();
     if (domid)
         libxl_domain_destroy(&ctx, domid, 0);

Re: [PATCH 7 of 8] xl: free memory before building a domain

[Christoph Egger]

On Friday 27 August 2010 13:19:19 Stefano Stabellini wrote:
> xl: free memory before building a domain
>
> Free the needed amount of memory before proceeding with the domain
> build.
> Use a filelock to prevent other xl instances from conflicting during
> this operation.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> diff -r b80a1688b5c9 tools/examples/xl.conf
> --- a/tools/examples/xl.conf	Thu Aug 26 18:03:20 2010 +0100
> +++ b/tools/examples/xl.conf	Thu Aug 26 18:26:04 2010 +0100
> @@ -3,3 +3,6 @@
>  # automatically balloon down dom0 when xen doesn't have enough free
>  # memory to create a domain
>  autoballon=1
> +
> +# full path of the lockfile used by xl during domain creation
> +lockfile="/var/lock/xl"
> diff -r b80a1688b5c9 tools/libxl/xl.c
> --- a/tools/libxl/xl.c	Thu Aug 26 18:03:20 2010 +0100
> +++ b/tools/libxl/xl.c	Thu Aug 26 18:26:04 2010 +0100
> @@ -34,6 +34,7 @@
>
>  xentoollog_logger_stdiostream *logger;
>  int autoballoon = 1;
> +char *lockfile = "/var/lock/xl";

NetBSD doesn't have /var/lock. Is it ok to use /var/lib ?

Christoph


>
>  static xentoollog_level minmsglevel = XTL_PROGRESS;
>
> @@ -44,6 +45,7 @@ static void parse_global_config(const ch
>      long l;
>      XLU_Config *config;
>      int e;
> +    const char *buf;
>
>      config = xlu_cfg_init(stderr, configfile);
>      if (!config) {
> @@ -60,6 +62,9 @@ static void parse_global_config(const ch
>      if (!xlu_cfg_get_long (config, "autoballoon", &l))
>          autoballoon = l;
>
> +    if (!xlu_cfg_get_string (config, "lockfile", &buf))
> +        lockfile = strdup(buf);
> +
>      xlu_cfg_destroy(config);
>  }
>
> diff -r b80a1688b5c9 tools/libxl/xl.h
> --- a/tools/libxl/xl.h	Thu Aug 26 18:03:20 2010 +0100
> +++ b/tools/libxl/xl.h	Thu Aug 26 18:26:04 2010 +0100
> @@ -92,5 +92,6 @@ extern xentoollog_logger_stdiostream *lo
>
>  /* global options */
>  extern int autoballoon;
> +extern char *lockfile;
>
>  #endif /* XL_H */
> diff -r b80a1688b5c9 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Thu Aug 26 18:03:20 2010 +0100
> +++ b/tools/libxl/xl_cmdimpl.c	Thu Aug 26 18:26:04 2010 +0100
> @@ -68,6 +68,7 @@ libxl_ctx ctx;
>  /* when we operate on a domain, it is this one: */
>  static uint32_t domid;
>  static const char *common_domname;
> +static int fd_lock = -1;
>
>
>  static const char savefileheader_magic[32]=
> @@ -234,6 +235,65 @@ static void find_domain(const char *p)
>      common_domname = was_name ? p : libxl_domid_to_name(&ctx, domid);
>  }
>
> +static int acquire_lock(void)
> +{
> +    int rc;
> +    struct flock fl;
> +
> +    /* lock already acquired */
> +    if (fd_lock >= 0)
> +        return ERROR_INVAL;
> +
> +    fl.l_type = F_WRLCK;
> +    fl.l_whence = SEEK_SET;
> +    fl.l_start = 0;
> +    fl.l_len = 0;
> +    fd_lock = open(lockfile, O_WRONLY|O_CREAT, S_IWUSR);
> +    if (fd_lock < 0) {
> +        fprintf(stderr, "cannot open the lockfile %s errno=%d\n",
> lockfile, errno); +        return ERROR_FAIL;
> +    }
> +get_lock:
> +    rc = fcntl(fd_lock, F_SETLKW, &fl);
> +    if (rc < 0 && errno == EINTR)
> +        goto get_lock;
> +    if (rc < 0) {
> +        fprintf(stderr, "cannot acquire lock %s errno=%d\n", lockfile,
> errno); +        rc = ERROR_FAIL;
> +    } else
> +        rc = 0;
> +    return rc;
> +}
> +
> +static int release_lock(void)
> +{
> +    int rc;
> +    struct flock fl;
> +
> +    /* lock not acquired */
> +    if (fd_lock < 0)
> +        return ERROR_INVAL;
> +
> +release_lock:
> +    fl.l_type = F_UNLCK;
> +    fl.l_whence = SEEK_SET;
> +    fl.l_start = 0;
> +    fl.l_len = 0;
> +
> +    rc = fcntl(fd_lock, F_SETLKW, &fl);
> +    if (rc < 0 && errno == EINTR)
> +        goto release_lock;
> +    if (rc < 0) {
> +        fprintf(stderr, "cannot release lock %s, errno=%d\n", lockfile,
> errno); +        rc = ERROR_FAIL;
> +    } else
> +        rc = 0;
> +    close(fd_lock);
> +    fd_lock = -1;
> +
> +    return rc;
> +}
> +
>  #define LOG(_f, _a...)   dolog(__FILE__, __LINE__, __func__, _f "\n",
> ##_a)
>
>  static void dolog(const char *file, int line, const char *func, char *fmt,
> ...) @@ -1209,6 +1269,48 @@ struct domain_create {
>      char **migration_domname_r;
>  };
>
> +static int freemem(libxl_domain_build_info *b_info,
> libxl_device_model_info *dm_info) +{
> +    int rc, retries = 3;
> +    uint32_t need_memkb, free_memkb;
> +
> +    if (!autoballoon)
> +        return 0;
> +
> +    rc = libxl_domain_need_memory(&ctx, b_info, dm_info, &need_memkb);
> +    if (rc < 0)
> +        return rc;
> +
> +    do {
> +        rc = libxl_get_free_memory(&ctx, &free_memkb);
> +        if (rc < 0)
> +            return rc;
> +
> +        if (free_memkb >= need_memkb)
> +            return 0;
> +
> +        rc = libxl_set_relative_memory_target(&ctx, 0, free_memkb -
> need_memkb, 0); +        if (rc < 0)
> +            return rc;
> +
> +        rc = libxl_wait_for_free_memory(&ctx, domid, need_memkb, 10);
> +        if (!rc)
> +            return 0;
> +        else if (rc != ERROR_NOMEM)
> +            return rc;
> +
> +        /* the memory target has been reached but the free memory is still
> +         * not enough: loop over again */
> +        rc = libxl_wait_for_memory_target(&ctx, 0, 1);
> +        if (rc < 0)
> +            return rc;
> +
> +        retries--;
> +    } while (retries > 0);
> +
> +    return ERROR_NOMEM;
> +}
> +
>  static int create_domain(struct domain_create *dom_info)
>  {
>      struct domain_config d_config;
> @@ -1367,6 +1469,17 @@ static int create_domain(struct domain_c
>  start:
>      domid = 0;
>
> +    rc = acquire_lock();
> +    if (rc < 0)
> +        goto error_out;
> +
> +    ret = freemem(&d_config.b_info, &dm_info);
> +    if (ret < 0) {
> +        fprintf(stderr, "failed to free memory for the domain\n");
> +        ret = ERROR_FAIL;
> +        goto error_out;
> +    }
> +
>      ret = libxl_domain_make(&ctx, &d_config.c_info, &domid);
>      if (ret) {
>          fprintf(stderr, "cannot make domain: %d\n", ret);
> @@ -1475,6 +1588,8 @@ start:
>              goto error_out;
>      }
>
> +    release_lock();
> +
>      if (!paused)
>          libxl_domain_unpause(&ctx, domid);
>
> @@ -1612,6 +1727,7 @@ start:
>      }
>
>  error_out:
> +    release_lock();
>      if (domid)
>          libxl_domain_destroy(&ctx, domid, 0);
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel



-- 
---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 7 of 8] xl: free memory before building a domain

[Stefano Stabellini]

On Fri, 27 Aug 2010, Christoph Egger wrote:
> On Friday 27 August 2010 13:19:19 Stefano Stabellini wrote:
> > xl: free memory before building a domain
> >
> > Free the needed amount of memory before proceeding with the domain
> > build.
> > Use a filelock to prevent other xl instances from conflicting during
> > this operation.
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >
> > diff -r b80a1688b5c9 tools/examples/xl.conf
> > --- a/tools/examples/xl.conf	Thu Aug 26 18:03:20 2010 +0100
> > +++ b/tools/examples/xl.conf	Thu Aug 26 18:26:04 2010 +0100
> > @@ -3,3 +3,6 @@
> >  # automatically balloon down dom0 when xen doesn't have enough free
> >  # memory to create a domain
> >  autoballon=1
> > +
> > +# full path of the lockfile used by xl during domain creation
> > +lockfile="/var/lock/xl"
> > diff -r b80a1688b5c9 tools/libxl/xl.c
> > --- a/tools/libxl/xl.c	Thu Aug 26 18:03:20 2010 +0100
> > +++ b/tools/libxl/xl.c	Thu Aug 26 18:26:04 2010 +0100
> > @@ -34,6 +34,7 @@
> >
> >  xentoollog_logger_stdiostream *logger;
> >  int autoballoon = 1;
> > +char *lockfile = "/var/lock/xl";
> 
> NetBSD doesn't have /var/lock. Is it ok to use /var/lib ?
> 

you can set it in the global xl config file to /var/lib or anything you
want.

Re: [PATCH 7 of 8] xl: free memory before building a domain

[Ian Jackson]

Stefano Stabellini writes ("[Xen-devel] [PATCH 7 of 8] xl: free memory before building a domain"):
...
> +# full path of the lockfile used by xl during domain creation
> +lockfile="/var/lock/xl"

I think the default config file should contain commented-out copies of
the compiled-in defaults, not uncommented versions.

> +char *lockfile = "/var/lock/xl";

And the value should be from libx_paths.h et al surely ?

Ian.

Re: [PATCH 7 of 8] xl: free memory before building a domain

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 7 of 8] xl: free memory before building a domain"):
> you can set it in the global xl config file to /var/lib or anything you
> want.

The compiled-in default should be sensible for the build platform.  We
shouldn't be having people fiddle with config files to make the
software work.

Ian.

Re: [PATCH 7 of 8] xl: free memory before building a domain

[Stefano Stabellini]

On Tue, 31 Aug 2010, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 7 of 8] xl: free memory before building a domain"):
> > you can set it in the global xl config file to /var/lib or anything you
> > want.
> 
> The compiled-in default should be sensible for the build platform.  We
> shouldn't be having people fiddle with config files to make the
> software work.
 
Yes, I had already repented and sent an update with this fix (it
contains one more patch called "introduce XEN_LOCK_DIR").