[Spice-devel] [PATCH 00/18] red_channel introducing refactoring, part 2, v1

[Alon Levy]

On Tue, Feb 08, 2011 at 07:47:23PM +0100, Marc-André Lureau wrote:
> Hi Alon, Gerd,
> 
> While reviewing Alon patches, I started to get annoyed by a crash I
> didn't see before, although it seems to be an old bug, and comes from
> qemu perhaps?
> 
> I am investing that crash before continuing the review, as it gets
> worst when I applied the first 3/4 patches (double free..).
> 
> The crash happens under valgrind, but not under gdb, which could mean
> it is some kind of race.
> 
> Using either spice.v30 or spice.kvm.v28 branch,  and spice
> 0.6.3-173-g8c17f83 (current master)
> 
> valgrind --db-attach=yes --smc-check=all --undef-value-errors=no
> /usr/local/bin/qemu-system-x86_64 --enable-kvm -net nic -net user -m
> 512 -localtime  -device AC97 -spice port=5930,disable-ticketing -vga
> qxl
> 
> start a client, and hit C-c to interrupt it, you get this:
> 
> reds_disconnect:
> red_dispatcher_shutdown_cursor_peer:
> red_dispatcher_shutdown_peer:
> handle_dev_input: cursor disconnect
> ==4797== Invalid read of size 4
> ==4797==    at 0x50ABA24: red_channel_event (red_channel.c:433)
> ==4797==    by 0x4D41CC: watch_read (spice-core.c:93)
> ==4797==    by 0x59532E: main_loop_wait (vl.c:1349)
> ==4797==    by 0x595551: main_loop (vl.c:1406)
> ==4797==    by 0x599620: main (vl.c:3120)
> ==4797==  Address 0x37205f88 is 104 bytes inside a block of size 28,608 free'd
> ==4797==    at 0x4A05187: free (vg_replace_malloc.c:325)
> ==4797==    by 0x50AB858: red_channel_destroy (red_channel.c:377)
> ==4797==    by 0x508C333: main_disconnect (main_channel.c:140)
> ==4797==    by 0x508D98B: main_channel_shutdown (main_channel.c:835)
> ==4797==    by 0x5086264: reds_disconnect (reds.c:644)
> ==4797==    by 0x508D6E6: main_channel_on_error (main_channel.c:759)
> ==4797==    by 0x50AB27D: red_channel_peer_on_incoming_error (red_channel.c:205)
> ==4797==    by 0x50AACFB: red_peer_handle_incoming (red_channel.c:85)
> ==4797==    by 0x50AAFF6: red_channel_receive (red_channel.c:152)
> ==4797==    by 0x50ABA15: red_channel_event (red_channel.c:429)
> ==4797==    by 0x4D41CC: watch_read (spice-core.c:93)
> ==4797==    by 0x59532E: main_loop_wait (vl.c:1349)
> ==4797==
> ==4797==
> 
> 
> If you try to reproduce the bug with gdb, by breaking first on
> red_channel_destroy and then on red_channel_event, you can see that
> red_channel_event is not called.

Do you mean you couldn't get a stacktrace from gdb showing the same?

Anyway, (sidetracking just to make sure you know the architecture), we actually
have two main loops in spice-server, or rather use two main loops:
 * all channels except cursor and display piggyback on the qemu main loop, consequently
  they run in the main thread (not the kvm cpu thread, nor the worker thread).
 * cursor and display channels both have their own main loop (epoll based just to make
 it more interesting), where they handle events from their channel sockets and from a pipe
 beeing fed by the red_dispatcher, which is called from the vcpu thread on io exits (actually,
 right now it can also be fed from the main thread, which might be related to this bug).

It looks like the main channel disconnect, so it should be related to all this exposition
material. So just looks like we are not removing the watch event fast enough? or maybe
like you say qemu is calling an event we already requested removal of (we use core->watch_remove
I think, core being a SpiceCoreInterface).

Alon

> 
> Help appreciated!
> 
> regards
> 
> On Mon, Feb 7, 2011 at 7:19 PM, Alon Levy <alevy at redhat.com> wrote:
> > This is a part of a much larger series so I don't have a good concise
> > description. Ultimately this all leads to a reuse of RedChannel across
> > most of the channels (sound remains an odd out because of not using a
> > pipe abstraction, but a bitmap of outstanding packets, you can have 0
> > or one of each type waiting to be sent, this makes sense for audio.
> > Probably for video too actually).
> >
> > Anyway this patchset touches almost only red_worker (and it's helper
> > cache files, split out because they are used for a number of different
> > caches via macros before inclusion, that usual trick). It is mostly about
> > renaming, making a CommonChannel take the place of the internal RedChannel
> > to allow future use of the external RedChannel defined in red_channel.h
> >
> > Alon Levy (18):
> >  server/red_worker: change hold_item sig, drop the void*
> >  server/red_worker: use ack_data struct
> >  server/red_worker: introduce CommonChannel
> >  server/red_worker: add free cb to EventHandler
> >  server/red_worker: shorten some lines using alias variables
> >  server/red_worker: use red_channel pipe add versions
> >  server/red_worker: extract common_release_pipe_item from
> >    red_pipe_clear
> >  server/red_worker: split display_channel_send_item
> >  server/red_worker: add red_channel_init_send_data
> >  server/red_worker: use red_channel begin_send_message
> >  server/red_worker: small cleanup with worker alias
> >  server/red_worker: split cursor_channel_send_item
> >  server/red_worker: s/disconnect_channel_proc/channel_disconnect_proc/
> >  server/red_worker: s/hold_pipe_item_proc/channel_hold_pipe_item/
> >  server/red_worker: s/handle_message_proc/handle_parsed_proc/
> >  server/red_worker: introduce an outgoing struct around
> >    out_bytes_counter
> >  server/red_worker:
> >    s/release_item_proc/channel_release_pipe_item_proc/
> >  server/red_worker: match channel_release_pipe_item_proc to
> >    red_channel
> >
> >  server/red_client_cache.h        |    2 +-
> >  server/red_client_shared_cache.h |   20 +-
> >  server/red_worker.c              | 1018 ++++++++++++++++++++------------------
> >  3 files changed, 553 insertions(+), 487 deletions(-)
> >
> > --
> > 1.7.4
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> 
> 
> 
> -- 
> Marc-André Lureau