From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60539) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNzLL-0006Cf-9U for qemu-devel@nongnu.org; Fri, 15 Jul 2016 05:23:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bNzLG-0000p7-6x for qemu-devel@nongnu.org; Fri, 15 Jul 2016 05:23:35 -0400 Received: from mx2.suse.de ([195.135.220.15]:60792) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNzLF-0000p1-RE for qemu-devel@nongnu.org; Fri, 15 Jul 2016 05:23:30 -0400 References: <57888385.1080403@suse.com> <57889319.3030609@kamp.de> <5788A308.5010909@suse.com> <5788A6D4.5070804@kamp.de> From: Juergen Gross Message-ID: <5788AB8E.10205@suse.com> Date: Fri, 15 Jul 2016 11:23:26 +0200 MIME-Version: 1.0 In-Reply-To: <5788A6D4.5070804@kamp.de> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] Regression with commit 095497ffc66b7f031 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven , xen-devel , "qemu-devel@nongnu.org" Cc: Gerd Hoffmann , Paolo Bonzini 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 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 > >