[Spice-devel] [RFC v4 00/62] server: multiple client support
Alon Levy
alevy at redhat.com
Mon May 2 23:29:52 PDT 2011
On Tue, May 03, 2011 at 01:51:46AM +0200, Marc-André Lureau wrote:
> 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
As I mentioned in the RFC this is expected and known, and a result of
two things:
1. we use more drawables on average - twice for two clients
2. we can use an unlimited number of drawables - because while we still
limit the minimal length of a pipe we don't limit the maximal.
And the solution still to be written is to release all the operations in
a single pipe (the slowest / longest one) and replace them by the minimal
equivalent operations. More specifically do it once for cursor and once for
display, for the display it would need to be one operation per surface touched.
This isn't guranteed to reduce the bandwidth, but it is guranteed to reduce the
amount of drawables referenced (from pipe_length to number_of_surfaces_touched).
Actually the assert you are pointing to is in the copy_surfaces, so it is
probably just fixed by increasing the maximal number of drawables. That number
should probably be not pre-allocated to avoid needless memory consumption for a
server with a single client for it's lifetime.
>
> 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.
The problem with reffing is that if I have two pipes and I want to remove
an item from one of them, order not known, I need a separate item to remove.
The PipeItem only has a single RingItem embedded in it. Refference counting doesn't
help this.
For the stuff that is actually referenced from the PipeItem I can do reference
counting, and I think I did in several places, but it becomes iffy - you will have
n allocations if the PipeItemDerivative has all the data, and n+1 where the last
is reference counted if you split the data into a separate struct. I do want to have
the PipeItems preallocated, possibly have a generic PipeItemRef and then preallocation
would be simple (this would be implemented at the red_channel.c/red_channel.h level)?
> 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 Channel/RedChannel split is annoying, I was trying to do minimal renames,
but considering the amount of churn there already is in this patch we can
also join Channel and RedChannel, making them allocated on the start and not
as a result of a client connection, and renamed to ChannelFactory, and rename
RedChannelClient to Channel.
>
> - 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?
Sounds resounable. I'll push there. btw, this is v4 because I had a few iterations
privately, I hope you don't mind I continue that numbering.
>
> 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