[Spice-devel] [RFC v4 00/62] server: multiple client support
Marc-André Lureau
marcandre.lureau at gmail.com
Mon May 2 16:51:46 PDT 2011
Hi Alon,
After a couple of days getting a bit more familiar with the server
code, and reading each patches individually, I have made several
observations and commented some of the patches (see patch reply). I
still don't consider myself familiar enough with the server code to be
really helpful. I don't understand all the implications, most notably
with the display cache/surface/tree. I wish someone (more experienced)
will also do a review of a future iteration.
In general, most of the patches made sense to me. They could be tested
fine with a single client (of course, limited manual testing, sometime
with migration). However, after all the patch applied, and actually
testing with multiple clients, I run into various assert()/crash
easily (testing only with a XP guest). I didn't spent time to debug,
but I can try to look into it later. Here is one frequent assert:
red_dispatcher_set_cursor_peer:
free_one_drawable: ASSERT ring_item failed
Program received signal SIGABRT, Aborted.
[Switching to Thread 0x7ffff505b700 (LWP 31125)]
0x0000003c7ba330c5 in raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
64 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
(gdb) bt
#0 0x0000003c7ba330c5 in raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1 0x0000003c7ba34a76 in abort () at abort.c:92
#2 0x00007ffff78c79b9 in free_one_drawable (worker=0x7ffff4eae290,
surfaces=0x7fffc10ce8d0, force_glz_free=<value optimized out>) at
red_worker.c:3425
#3 0x00007ffff78c8925 in get_drawable (worker=0x7ffff4eae290,
surfaces=0x7fffc10ce8d0, drawable=0x7fffc058b390, group_id=1) at
red_worker.c:3443
#4 red_process_drawable_surfaces (worker=0x7ffff4eae290,
surfaces=0x7fffc10ce8d0, drawable=0x7fffc058b390, group_id=1) at
red_worker.c:3557
#5 0x00007ffff78cf788 in copy_surfaces (listener=0x7ffff4eae290,
events=<value optimized out>) at red_worker.c:8714
#6 handle_new_display_channel (listener=0x7ffff4eae290, events=<value
optimized out>) at red_worker.c:9963
#7 handle_dev_input (listener=0x7ffff4eae290, events=<value optimized
out>) at red_worker.c:10576
#8 0x00007ffff78d1164 in red_worker_main (arg=<value optimized out>)
at red_worker.c:10902
#9 0x0000003c7c206ccb in start_thread (arg=0x7ffff505b700) at
pthread_create.c:301
#10 0x0000003c7bae0c2d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:115
The comments I made for each patch are rather minor: I didn't catch
real bug probably. However some things that worried me:
- the added complexity to the code (it was already difficult to read)
- the use of several (same) PipeItem instances for each client pipe.
Where we had 1 allocation, it now makes n+1. I would ref instead.
Also, the code is more complicated because of this change (more
structure, more references, more functions etc..).
- the relations:
Channel (singleton, no logic)
-> RedChannel (pseudo-on demand singleton, channel logic)
-> RedChannelClient (hold the pipe, per-client logic)
where I would imagine simply instead:
ChannelFactory (channel singleton, channel logic and reference pipe)
-> Channel (per-client pipe and per-client logic)
- the lack of a simple coherent object/refcount system is killing us..
Since it's an RFC series, I think we have already enough to discuss
and to fix for a first iteration.
I am also wondering if we shouldn't have a separate MC branch in git,
to work collaboratively and incrementally (by submitting new patches
in the ML) instead of you only, having to rebase regularly etc... Have
you thought about it?
regards
On Tue, Apr 26, 2011 at 12:54 PM, Alon Levy <alevy at redhat.com> wrote:
> This is a resend of the start patches (server: introduce RedChannelClient) plus
> the rest of the series to support multiple connections to spice. It is missing
> some features, so it is more of an RFC right now, but I hope to do a few iterations
> completing the missing pieces to get it to PATCH status.
>
> The current series allows connection from multiple clients with display, cursor
> and inputs (and of course main) channels. It doesn't require any client changes.
>
> The approach taken is (there are mentiones of specific pieces in related patches):
> give a pipe to each client
> for display channel:
> give a copy of each surface to each client
> give a separate tree to each client
> give separate caches to each client
>
> This approach allows each client to have a separate server side rendering state,
> which will result from different throughput to the client.
>
> There is however one missing piece that I decided to finish after sending this revision
> of this patchset, namely preventing an unbounded pipe by squashing, i.e. replacing
> multiple outstanding operations with fewer operations. Specifically I intend to implement
> the following simple conversions:
> * outstanding cursor items: replace with a single cursor set / cursor pos.
> * outstanding drawing items (per surface):
> * replace with a single surface image (like we do on new client connection)
>
> The patch to do this is still being written.
>
> The definitely missing pieces:
> Don't turn on by default - requires a separate patch to qemu, and some api
> (spice_set_multi_client bool)
> Solving the unbounded pipe / resource allocation problem referred to above.
> Test migration
>
> The not planned for this series pieces:
> sound and record channel support - they are different enough (perhaps not a lot
> of work but can be done in a separate series anyway)
> smartcard channel support
> tunnel channel support
>
> Alon Levy (62):
> server/red_channel: renames to use _proc postfix consistently
> server/red_worker: drop red_pipe_add_tail, use
> red_channel_pipe_add_tail
> server/red_channel (all): introduce RedChannelClient
> server/red_worker: introduce {display,cursor}_connected
> server: move pipe from RedChannel to RedChannelClient
> server/main_channel.c: set NET_TEST_STAGE_INVALID=0 explicitly
> server/main_channel: use MainChannel in sig
> server/red_channel: workaround for fast client disconnect bug (TODO -
> real fix)
> server/red_client: clear pipe on disconnect
> server/red_worker: release PIPE_ITEM_TYPE_VERB in display channel
> server/red_channel: merge into red_channel_client_release_item
> introduction - bug
> server/main_channel+reds: make main_channel_init return
> MainChannelClient
> server: Add RedClient
> server/main_channel: move latency and bitrate to channel client
> server/main_channel: move ping here from reds.
> server/main_channel: move link_id from reds
> server/red_channel: ignore error if already shutdown
> server/red_channel: introduce pipes functions
> server/red_channel: add RedChannel.clients_num
> server/main_channel: support multiple clients
> server/inputs_channel: support multiple clients
> server/red_tunnel_worker: trivial multi client support
> server/smartcard: support multiple clients
> server/red_worker: split Surfaces from RedWorker
> server/red_worker: make stat_now static
> server/red_worker: fix typo (lats_send_time)
> server/red_worker: move streams from RedWorker to Surfaces
> server/red_channel: add two helper functions
> server/red_worker: split display and cursor channels
> server/red_worker: don't send redundant create surface to client
> server/red_worker: fix red_pipe_add_drawable_to_tail
> server/red_worker: fix red_pipe_get_tail
> server/red_worker: fix red_pipe_remove_drawable
> server/red_worker: remove more direct access to RedChannelClient.rcc
> server/red_worker: more removal of direct rcc access
> server/red_channel: add pipe_size helpers
> server/red_worker: use pipe_size helpers
> server/red_channel: introduce client ring in RedChannel
> server/red_worker: introduce RedSurfaceReleaseInfo
> server/red_worker: add ref counting to RedDrawable
> server/red_worker: start using SURFACES_FOREACH
> server/red_worker: tiny cleanups
> server/red_worker: red_create_surface - check for dcc before sending
> messages
> server/red_worker: whitespace fixes
> server/red_worker: copy and free new surfaces after first client
> server/red_worker: handle_dev_update: for all clients (checkme)
> server/red_worker: handle_dev_destroy_surface_wait: all clients
> surfaces
> server/red_worker: handle_dev_destroy_surfaces: clear all surfaces
> server/red_worker: handle_dev_destroy_primary_surface: clear all
> primary copies
> server/red_worker: handle_dev_input RED_WORKER_MESSAGE_STOP: all
> clients
> server/red_worker: get_streams_timeout: go over all clients
> server/red_worker: red_worker_main: call red_handle_streams_timeout
> for all clients
> server/red_worker: move free from display_channel_send_item to
> _release_item
> server/red_worker: split cursor pipe item from cursor item
> server/red_worker: remove forced disconnect on connect
> server/red_worker: add cursor_channel_client_disconnect
> server/reds: add RedsState.allow_multiple_clients (temp - add
> accessors too)
> server/reds: make reds_fill_channels return no sound channels for
> client > 1
> server/red_worker: red_current_add_equal: add dcc null checks
> server/red_worker: on_new_display_channel_client: push ack, cleanup
> server/red_worker: give each client a different id for caches
> server/red_worker: DEBUG_CURSORS
>
> server/inputs_channel.c | 147 +-
> server/main_channel.c | 641 ++++---
> server/main_channel.h | 47 +-
> server/red_channel.c | 1024 ++++++++---
> server/red_channel.h | 194 ++-
> server/red_client_cache.h | 58 +-
> server/red_client_shared_cache.h | 47 +-
> server/red_dispatcher.c | 36 +-
> server/red_dispatcher.h | 7 +-
> server/red_parse_qxl.h | 1 +
> server/red_tunnel_worker.c | 403 +++--
> server/red_worker.c | 3819 +++++++++++++++++++++++---------------
> server/red_worker.h | 2 +
> server/reds.c | 175 +-
> server/reds.h | 12 +-
> server/smartcard.c | 196 +-
> server/snd_worker.c | 24 +-
> 17 files changed, 4298 insertions(+), 2535 deletions(-)
>
> --
> 1.7.4.4
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
--
Marc-André Lureau
More information about the Spice-devel
mailing list