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 <jgr...@suse.com>
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
>
>
