Regression with commit 095497ffc66b7f031

Commit Message

Peter Lieven July 15, 2016, 9:03 a.m. UTC

Am 15.07.2016 um 10:47 schrieb Juergen Gross:
> On 15/07/16 09:39, Peter Lieven wrote:
>> Am 15.07.2016 um 08:32 schrieb Juergen Gross:
>>> Commit 095497ffc66b7f031ff2a17f1e50f5cb105ce588 ("vnc-enc-tight: use
>>> thread local storage for palette") introduced a regression with Xen:
>>> Since this commit qemu used as a device model is no longer capable
>>> to register Xenstore watches (that's the effect visible to a user).
>>> Reverting this commit makes qemu behave well again. I have no idea
>>> why that commit would have this effect with Xen, may be some memory
>>> is clobbered?
>> I personally have no idea, maybe @Paolo has?
>>
>> Maybe the corruption happens somewhere else and is just visible
>> due to this change.
>>
>> Do you see sth when you ran qemu/xen in valgrind?
> Nothing scaring and no real difference between working and not working
> variant.
>
> Meanwhile I've been digging a little bit deeper and found the reason:
> libxenstore is setting up a reader thread which is waiting for the
> watch to fire. With above commit the stack size of that thread (16kB)
> is too small. Setting it to 32kB made qemu work again.
>
> So I'd recommend to have just a thread local palette pointer and
> allocate the palette when needed and don't free it after using it but
> keep it for reuse. Do you want to write that patch or should I do it?

As you like. But as I have introduced this regression, maybe I should
fix it ;-)

Actually I do not understand what libxenstore confuses about 16 and 32kB,
but I have no knowledge about XEN. However, let me know if this here works for you:


Peter

Comments

Jürgen Groß July 15, 2016, 9:23 a.m. UTC | #1

On 15/07/16 11:03, Peter Lieven wrote:
> Am 15.07.2016 um 10:47 schrieb Juergen Gross:
>> On 15/07/16 09:39, Peter Lieven wrote:
>>> Am 15.07.2016 um 08:32 schrieb Juergen Gross:
>>>> Commit 095497ffc66b7f031ff2a17f1e50f5cb105ce588 ("vnc-enc-tight: use
>>>> thread local storage for palette") introduced a regression with Xen:
>>>> Since this commit qemu used as a device model is no longer capable
>>>> to register Xenstore watches (that's the effect visible to a user).
>>>> Reverting this commit makes qemu behave well again. I have no idea
>>>> why that commit would have this effect with Xen, may be some memory
>>>> is clobbered?
>>> I personally have no idea, maybe @Paolo has?
>>>
>>> Maybe the corruption happens somewhere else and is just visible
>>> due to this change.
>>>
>>> Do you see sth when you ran qemu/xen in valgrind?
>> Nothing scaring and no real difference between working and not working
>> variant.
>>
>> Meanwhile I've been digging a little bit deeper and found the reason:
>> libxenstore is setting up a reader thread which is waiting for the
>> watch to fire. With above commit the stack size of that thread (16kB)
>> is too small. Setting it to 32kB made qemu work again.
>>
>> So I'd recommend to have just a thread local palette pointer and
>> allocate the palette when needed and don't free it after using it but
>> keep it for reuse. Do you want to write that patch or should I do it?
> 
> As you like. But as I have introduced this regression, maybe I should
> fix it ;-)

Sure.

> Actually I do not understand what libxenstore confuses about 16 and 32kB,
> but I have no knowledge about XEN. However, let me know if this here works for you:

Thanks, is working again. You can add

Tested-by: Juergen Gross <[e-mail]>

when submitting it.

The thread local static variable you added is occupying stack space in
each thread started by qemu. As it is rather large (about 6kB on a 64
bit system) the stack of the thread created by libxenstore is exhausted
resulting in a failing library call.


Juergen

> 
> diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
> index b8581dd..5731cf6 100644
> --- a/ui/vnc-enc-tight.c
> +++ b/ui/vnc-enc-tight.c
> @@ -1457,11 +1457,18 @@ static int send_sub_rect_jpeg(VncState *vs, int x, int y, int w, int h,
>  }
>  #endif
>  
> -static __thread VncPalette color_count_palette;
> +static __thread VncPalette *color_count_palette = NULL;
> +static __thread Notifier vnc_tight_cleanup_notifier;
> +
> +static void vnc_tight_cleanup(Notifier *n, void *value)
> +{
> +    printf("thread %d: free tight palette %p\n", qemu_get_thread_id(), color_count_palette);
> +    g_free(color_count_palette);
> +    color_count_palette = NULL;
> +}
>  
>  static int send_sub_rect(VncState *vs, int x, int y, int w, int h)
>  {
> -    VncPalette *palette = &color_count_palette;
>      uint32_t bg = 0, fg = 0;
>      int colors;
>      int ret = 0;
> @@ -1470,6 +1477,13 @@ static int send_sub_rect(VncState *vs, int x, int y, int w, int h)
>      bool allow_jpeg = true;
>  #endif
>  
> +    if (!color_count_palette) {
> +        color_count_palette = g_malloc(sizeof(VncPalette));
> +        vnc_tight_cleanup_notifier.notify = vnc_tight_cleanup;
> +        qemu_thread_atexit_add(&vnc_tight_cleanup_notifier);
> +        printf("thread %d: alloc tight palette %p\n", qemu_get_thread_id(), color_count_palette);
> +    }
> +
>      vnc_framebuffer_update(vs, x, y, w, h, vs->tight.type);
>  
>      vnc_tight_start(vs);
> @@ -1490,17 +1504,17 @@ static int send_sub_rect(VncState *vs, int x, int y, int w, int h)
>      }
>  #endif
>  
> -    colors = tight_fill_palette(vs, x, y, w * h, &bg, &fg, palette);
> +    colors = tight_fill_palette(vs, x, y, w * h, &bg, &fg, color_count_palette);
>  
>  #ifdef CONFIG_VNC_JPEG
>      if (allow_jpeg && vs->tight.quality != (uint8_t)-1) {
> -        ret = send_sub_rect_jpeg(vs, x, y, w, h, bg, fg, colors, palette,
> +        ret = send_sub_rect_jpeg(vs, x, y, w, h, bg, fg, colors, color_count_palette,
>                                   force_jpeg);
>      } else {
> -        ret = send_sub_rect_nojpeg(vs, x, y, w, h, bg, fg, colors, palette);
> +        ret = send_sub_rect_nojpeg(vs, x, y, w, h, bg, fg, colors, color_count_palette);
>      }
>  #else
> -    ret = send_sub_rect_nojpeg(vs, x, y, w, h, bg, fg, colors, palette);
> +    ret = send_sub_rect_nojpeg(vs, x, y, w, h, bg, fg, colors, color_count_palette);
>  #endif
>  
>      return ret;
> 
> Peter
> 
>

Patch

diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
index b8581dd..5731cf6 100644
--- a/ui/vnc-enc-tight.c
+++ b/ui/vnc-enc-tight.c
@@ -1457,11 +1457,18 @@  static int send_sub_rect_jpeg(VncState *vs, int x, int y, int w, int h,
 }
 #endif
 
-static __thread VncPalette color_count_palette;
+static __thread VncPalette *color_count_palette = NULL;
+static __thread Notifier vnc_tight_cleanup_notifier;
+
+static void vnc_tight_cleanup(Notifier *n, void *value)
+{
+    printf("thread %d: free tight palette %p\n", qemu_get_thread_id(), color_count_palette);
+    g_free(color_count_palette);
+    color_count_palette = NULL;
+}
 
 static int send_sub_rect(VncState *vs, int x, int y, int w, int h)
 {
-    VncPalette *palette = &color_count_palette;
     uint32_t bg = 0, fg = 0;
     int colors;
     int ret = 0;
@@ -1470,6 +1477,13 @@  static int send_sub_rect(VncState *vs, int x, int y, int w, int h)
     bool allow_jpeg = true;
 #endif
 
+    if (!color_count_palette) {
+        color_count_palette = g_malloc(sizeof(VncPalette));
+        vnc_tight_cleanup_notifier.notify = vnc_tight_cleanup;
+        qemu_thread_atexit_add(&vnc_tight_cleanup_notifier);
+        printf("thread %d: alloc tight palette %p\n", qemu_get_thread_id(), color_count_palette);
+    }
+
     vnc_framebuffer_update(vs, x, y, w, h, vs->tight.type);
 
     vnc_tight_start(vs);
@@ -1490,17 +1504,17 @@  static int send_sub_rect(VncState *vs, int x, int y, int w, int h)
     }
 #endif
 
-    colors = tight_fill_palette(vs, x, y, w * h, &bg, &fg, palette);
+    colors = tight_fill_palette(vs, x, y, w * h, &bg, &fg, color_count_palette);
 
 #ifdef CONFIG_VNC_JPEG
     if (allow_jpeg && vs->tight.quality != (uint8_t)-1) {
-        ret = send_sub_rect_jpeg(vs, x, y, w, h, bg, fg, colors, palette,
+        ret = send_sub_rect_jpeg(vs, x, y, w, h, bg, fg, colors, color_count_palette,
                                  force_jpeg);
     } else {
-        ret = send_sub_rect_nojpeg(vs, x, y, w, h, bg, fg, colors, palette);
+        ret = send_sub_rect_nojpeg(vs, x, y, w, h, bg, fg, colors, color_count_palette);
     }
 #else
-    ret = send_sub_rect_nojpeg(vs, x, y, w, h, bg, fg, colors, palette);
+    ret = send_sub_rect_nojpeg(vs, x, y, w, h, bg, fg, colors, color_count_palette);
 #endif
 
     return ret;