[PATCH] libxc: free thread specific hypercall buffer on xc_interface_close

[PATCH] libxc: free thread specific hypercall buffer on xc_interface_close

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1282143629 -3600
# Node ID b1f4b4be1f94c0007794f86abd019f5c2629c59b
# Parent  ddbd38da07397dca4760fb687551c3f6f9134700
libxc: free thread specific hypercall buffer on xc_interface_close

The per-thread hypercall buffer is usually cleaned up on pthread_exit
by the destructor passed to pthread_key_create. However if the calling
application is not threaded then the destructor is never called.

This frees the data for the current thread only but that is OK since
any other threads will be cleaned up by the destructor.

Changed since v1:
 * Ensure hcall_buf_pkey is initialised before use. Thanks to
   Christoph Egger for his help diagnosing this issue on NetBSD.
 * Remove redundant if (hcall_buf) from xc_clean_hcall_buf since
   _xc_clean_hcall_buf includes the same check.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r ddbd38da0739 -r b1f4b4be1f94 tools/libxc/xc_private.c
--- a/tools/libxc/xc_private.c	Wed Aug 18 13:14:57 2010 +0100
+++ b/tools/libxc/xc_private.c	Wed Aug 18 16:00:29 2010 +0100
@@ -57,6 +57,8 @@ xc_interface *xc_interface_open(xentooll
     return 0;
 }
 
+static void xc_clean_hcall_buf(void);
+
 int xc_interface_close(xc_interface *xch)
 {
     int rc = 0;
@@ -68,6 +70,9 @@ int xc_interface_close(xc_interface *xch
         rc = xc_interface_close_core(xch, xch->fd);
         if (rc) PERROR("Could not close hypervisor interface");
     }
+
+    xc_clean_hcall_buf();
+
     free(xch);
     return rc;
 }
@@ -180,6 +185,8 @@ int hcall_buf_prep(void **addr, size_t l
 int hcall_buf_prep(void **addr, size_t len) { return 0; }
 void hcall_buf_release(void **addr, size_t len) { }
 
+static void xc_clean_hcall_buf(void) { }
+
 #else /* !__sun__ */
 
 int lock_pages(void *addr, size_t len)
@@ -228,6 +235,13 @@ static void _xc_init_hcall_buf(void)
 static void _xc_init_hcall_buf(void)
 {
     pthread_key_create(&hcall_buf_pkey, _xc_clean_hcall_buf);
+}
+
+static void xc_clean_hcall_buf(void)
+{
+    pthread_once(&hcall_buf_pkey_once, _xc_init_hcall_buf);
+
+    _xc_clean_hcall_buf(pthread_getspecific(hcall_buf_pkey));
 }
 
 int hcall_buf_prep(void **addr, size_t len)

Re: [PATCH] libxc: free thread specific hypercall buffer on xc_interface_close

[Christoph Egger]

This patch prevents me from starting a guest until
the outstanding issue - namely why is hcall_buf_prep() never called -
is solved.

Christoph



On Wednesday 18 August 2010 17:01:07 Ian Campbell wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1282143629 -3600
> # Node ID b1f4b4be1f94c0007794f86abd019f5c2629c59b
> # Parent  ddbd38da07397dca4760fb687551c3f6f9134700
> libxc: free thread specific hypercall buffer on xc_interface_close
>
> The per-thread hypercall buffer is usually cleaned up on pthread_exit
> by the destructor passed to pthread_key_create. However if the calling
> application is not threaded then the destructor is never called.
>
> This frees the data for the current thread only but that is OK since
> any other threads will be cleaned up by the destructor.
>
> Changed since v1:
>  * Ensure hcall_buf_pkey is initialised before use. Thanks to
>    Christoph Egger for his help diagnosing this issue on NetBSD.
>  * Remove redundant if (hcall_buf) from xc_clean_hcall_buf since
>    _xc_clean_hcall_buf includes the same check.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>
> diff -r ddbd38da0739 -r b1f4b4be1f94 tools/libxc/xc_private.c
> --- a/tools/libxc/xc_private.c	Wed Aug 18 13:14:57 2010 +0100
> +++ b/tools/libxc/xc_private.c	Wed Aug 18 16:00:29 2010 +0100
> @@ -57,6 +57,8 @@ xc_interface *xc_interface_open(xentooll
>      return 0;
>  }
>
> +static void xc_clean_hcall_buf(void);
> +
>  int xc_interface_close(xc_interface *xch)
>  {
>      int rc = 0;
> @@ -68,6 +70,9 @@ int xc_interface_close(xc_interface *xch
>          rc = xc_interface_close_core(xch, xch->fd);
>          if (rc) PERROR("Could not close hypervisor interface");
>      }
> +
> +    xc_clean_hcall_buf();
> +
>      free(xch);
>      return rc;
>  }
> @@ -180,6 +185,8 @@ int hcall_buf_prep(void **addr, size_t l
>  int hcall_buf_prep(void **addr, size_t len) { return 0; }
>  void hcall_buf_release(void **addr, size_t len) { }
>
> +static void xc_clean_hcall_buf(void) { }
> +
>  #else /* !__sun__ */
>
>  int lock_pages(void *addr, size_t len)
> @@ -228,6 +235,13 @@ static void _xc_init_hcall_buf(void)
>  static void _xc_init_hcall_buf(void)
>  {
>      pthread_key_create(&hcall_buf_pkey, _xc_clean_hcall_buf);
> +}
> +
> +static void xc_clean_hcall_buf(void)
> +{
> +    pthread_once(&hcall_buf_pkey_once, _xc_init_hcall_buf);
> +
> +    _xc_clean_hcall_buf(pthread_getspecific(hcall_buf_pkey));
>  }
>
>  int hcall_buf_prep(void **addr, size_t len)
>
> _______________________________________________
> 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] libxc: free thread specific hypercall buffer on xc_interface_close

[Ian Campbell]

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

On Wed, 2010-08-18 at 17:04 +0100, Christoph Egger wrote:
> This patch prevents me from starting a guest until
> the outstanding issue - namely why is hcall_buf_prep() never called -
> is solved.

You wouldn't expect hcall_buf_prep to be called if you opened the xc
interface and then closed it without doing a hypercall which required
any locked down memory.

This updated version of the patch was intended to handle exactly this
case since it appears that under NetBSD it can cause issues.

Are you saying that you are seeing hypercalls which you expect to need
memory locking down but for which that has not happened?

The callers of hcall_buf_prep are pretty explicit and there aren't that
many of them.

With the attached patch to libxc I get the following output when
starting xend, so you see it is quite normal for the xc interface to be
called when hcall_buf_prep has never been called and we now handle that
case correctly.

        xc_interface_open
        xc_interface_open
        xc_interface_open
        xc_interface_close
        xc_clean_hcall_buf
        _xc_init_hcall_buf
        _xc_clean_hcall_buf
        xc_interface_open
        xc_interface_open
        xc_interface_open
        xc_interface_open
        xc_interface_open
        xc_interface_open
        xc_interface_open
        xc_interface_close
        xc_clean_hcall_buf
        _xc_clean_hcall_buf
        xc_interface_close
        xc_clean_hcall_buf
        _xc_clean_hcall_buf
        xc_interface_close
        xc_clean_hcall_buf
        _xc_clean_hcall_buf
        xc_interface_close
        xc_clean_hcall_buf
        _xc_clean_hcall_buf
        xc_interface_close
        xc_clean_hcall_buf
        _xc_clean_hcall_buf
        xc_interface_close
        xc_clean_hcall_buf
        _xc_clean_hcall_buf
        xc_interface_close
        xc_clean_hcall_buf
        _xc_clean_hcall_buf
        xc_interface_close
        xc_clean_hcall_buf
        _xc_clean_hcall_buf
        xc_interface_close
        xc_clean_hcall_buf
        _xc_clean_hcall_buf

When starting a PV domain I get

        xc_interface_open
        xc_interface_close
        xc_clean_hcall_buf
        _xc_init_hcall_buf
        _xc_clean_hcall_buf
        Using config file "/etc/xen/debian-x86_32p-1".
        xc_interface_open
        hcall_buf_prep(0xbfb56ca8,144)
        _xc_init_hcall_buf
        hcall_buf_prep hcall_buf allocated at 0x804b0d8
        hcall_buf_prep hcall_buf->buf allocated at 0x804c000
        hcall_buf_prep using preallocated buffer at 0x804c000 for 0xbfb56ca8
        hcall_buf_release(0x804c000,144)
        hcall_buf_release releasing preallocated buffer at 0x804c000 for 0xbfb56ca8
        xc_interface_close
        xc_clean_hcall_buf
        _xc_clean_hcall_buf
        _xc_clean_hcall_buf unlock and free hcall_buf->buf 0x804c000
        _xc_clean_hcall_buf free hcall_buf 0x804b0d8

It is also safe to call _xc_clean_hcall_buf and then later call
hcall_buf_prep (and therefore allocate a buffer).

Ian.


[-- Attachment #2: foo --]
[-- Type: text/x-patch, Size: 3483 bytes --]

diff -r 02b341ca3612 tools/libxc/xc_private.c
--- a/tools/libxc/xc_private.c	Wed Aug 18 16:36:25 2010 +0100
+++ b/tools/libxc/xc_private.c	Wed Aug 18 17:22:34 2010 +0100
@@ -49,6 +49,7 @@ xc_interface *xc_interface_open(xentooll
             goto err;
     }
 
+    fprintf(stderr, "%s %p\n", __func__, xch);
     return xch;
 
  err:
@@ -62,6 +63,8 @@ int xc_interface_close(xc_interface *xch
 int xc_interface_close(xc_interface *xch)
 {
     int rc = 0;
+
+    fprintf(stderr, "%s xch:%p\n", __func__, xch);
 
     xtl_logger_destroy(xch->dombuild_logger_tofree);
     xtl_logger_destroy(xch->error_handler_tofree);
@@ -218,14 +221,17 @@ static void _xc_clean_hcall_buf(void *m)
 {
     struct hcall_buf *hcall_buf = m;
 
+    fprintf(stderr, "%s\n", __func__);
     if ( hcall_buf )
     {
         if ( hcall_buf->buf )
         {
+            fprintf(stderr, "%s unlock and free hcall_buf->buf %p\n", __func__, hcall_buf->buf);
             unlock_pages(hcall_buf->buf, PAGE_SIZE);
             free(hcall_buf->buf);
         }
 
+        fprintf(stderr, "%s free hcall_buf %p\n", __func__, hcall_buf);
         free(hcall_buf);
     }
 
@@ -234,11 +240,14 @@ static void _xc_clean_hcall_buf(void *m)
 
 static void _xc_init_hcall_buf(void)
 {
+    fprintf(stderr, "%s\n", __func__);
     pthread_key_create(&hcall_buf_pkey, _xc_clean_hcall_buf);
 }
 
 static void xc_clean_hcall_buf(void)
 {
+    fprintf(stderr, "%s\n", __func__);
+
     pthread_once(&hcall_buf_pkey_once, _xc_init_hcall_buf);
 
     _xc_clean_hcall_buf(pthread_getspecific(hcall_buf_pkey));
@@ -247,6 +256,8 @@ int hcall_buf_prep(void **addr, size_t l
 int hcall_buf_prep(void **addr, size_t len)
 {
     struct hcall_buf *hcall_buf;
+
+    fprintf(stderr, "%s(%p,%zd)\n", __func__, *addr, len);
 
     pthread_once(&hcall_buf_pkey_once, _xc_init_hcall_buf);
 
@@ -257,6 +268,7 @@ int hcall_buf_prep(void **addr, size_t l
         if ( !hcall_buf )
             goto out;
         pthread_setspecific(hcall_buf_pkey, hcall_buf);
+        fprintf(stderr, "%s hcall_buf allocated at %p\n", __func__, hcall_buf);
     }
 
     if ( !hcall_buf->buf )
@@ -268,10 +280,12 @@ int hcall_buf_prep(void **addr, size_t l
             hcall_buf->buf = NULL;
             goto out;
         }
+        fprintf(stderr, "%s hcall_buf->buf allocated at %p\n", __func__, hcall_buf->buf);
     }
 
     if ( (len < PAGE_SIZE) && !hcall_buf->oldbuf )
     {
+        fprintf(stderr, "%s using preallocated buffer at %p for %p\n", __func__, hcall_buf->buf, *addr);
         memcpy(hcall_buf->buf, *addr, len);
         hcall_buf->oldbuf = *addr;
         *addr = hcall_buf->buf;
@@ -279,6 +293,7 @@ int hcall_buf_prep(void **addr, size_t l
     }
 
  out:
+    fprintf(stderr, "%s locking buffer at %p %zd\n", __func__, *addr, len);
     return lock_pages(*addr, len);
 }
 
@@ -286,14 +301,18 @@ void hcall_buf_release(void **addr, size
 {
     struct hcall_buf *hcall_buf = pthread_getspecific(hcall_buf_pkey);
 
+    fprintf(stderr, "%s(%p,%zd)\n", __func__, *addr, len);
+
     if ( hcall_buf && (hcall_buf->buf == *addr) )
     {
+        fprintf(stderr, "%s releasing preallocated buffer at %p for %p\n", __func__, hcall_buf->buf, hcall_buf->oldbuf);
         memcpy(hcall_buf->oldbuf, *addr, len);
         *addr = hcall_buf->oldbuf;
         hcall_buf->oldbuf = NULL;
     }
     else
     {
+        fprintf(stderr, "%s unlocking buffer at %p %zd\n", __func__, *addr, len);
         unlock_pages(*addr, len);
     }
 }

[-- 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] libxc: free thread specific hypercall buffer on xc_interface_close

[Christoph Egger]

On Wednesday 18 August 2010 18:26:28 Ian Campbell wrote:
> On Wed, 2010-08-18 at 17:04 +0100, Christoph Egger wrote:
> > This patch prevents me from starting a guest until
> > the outstanding issue - namely why is hcall_buf_prep() never called -
> > is solved.
>
> You wouldn't expect hcall_buf_prep to be called if you opened the xc
> interface and then closed it without doing a hypercall which required
> any locked down memory.
>
> This updated version of the patch was intended to handle exactly this
> case

Yes, it works. The issue was in the debug code itself. After
removing the trace code I am able to start guests again.

Thanks for your help.

> since it appears that under NetBSD it can cause issues. 

I am wondering why this issue didn't appear on Linux.
Does Linux have some kind of auto-initialization?

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] libxc: free thread specific hypercall buffer on xc_interface_close

[Ian Campbell]

On Thu, 2010-08-19 at 13:39 +0100, Christoph Egger wrote:
> On Wednesday 18 August 2010 18:26:28 Ian Campbell wrote:
> > On Wed, 2010-08-18 at 17:04 +0100, Christoph Egger wrote:
> > > This patch prevents me from starting a guest until
> > > the outstanding issue - namely why is hcall_buf_prep() never called -
> > > is solved.
> >
> > You wouldn't expect hcall_buf_prep to be called if you opened the xc
> > interface and then closed it without doing a hypercall which required
> > any locked down memory.
> >
> > This updated version of the patch was intended to handle exactly this
> > case
> 
> Yes, it works. The issue was in the debug code itself. After
> removing the trace code I am able to start guests again.
> 
> Thanks for your help.

Thanks for confirming.

> > since it appears that under NetBSD it can cause issues. 
> 
> I am wondering why this issue didn't appear on Linux.
> Does Linux have some kind of auto-initialization?

I think it's just good/bad luck one way or the other if an uninitialised
pthread_key_t looks valid to pthread_getspecific or not. Ultimately
pthread_key_t is likely just an integer so there is always a chance that
an uninitialised pthread_key_t looks like an existing valid key.

Ian.