[Spice-devel] [PATCH 13/26] server/red_channel: add red_channel_all_blocked

[Alon Levy]

On Tue, Feb 15, 2011 at 02:30:48AM +0100, Marc-André Lureau wrote:
> Are you going in the direction of IO queued per stream? (message
> buffer pointer per client "peer") Or do you want to maintain some
Yes, I refer to this as multiple pipe's below. (since we already have
the idea of a pipe per channel, it becomes a pipe per channel per client).

> message/buffer synchronizations between clients? (if one client socket
> is EAGAIN, we don't push on any other client?) Please be a bit more
This I think would be a mistake - easier to implement and get correct, but
bad performance, and not an inherent limitation.

> verbose on your plan :)

I never stated it - initially because I didn't have one. The final solution
I came to (which of course is not in any of these patches, it comes later) is
to allow slow clients to not block fast clients. I haven't actually implemented
this completely, but I did implement a required condition, which is to have
separate pipes for each client so that each message gets sent multiple times
(once per client), and any reference counting to the underlying structures
(i.e. drawing operation) are kept track of.

The part I didn't complete is the "what to do when there is actual different bw,
not just a hiccup", so right now I fail because I allocate all the drawing operations
(because one client's pipe grows unboundedly). This can be solved by:

 1. rendering - same thing we do when a new client connects. Basically you turn
 all pipes to zero, but it means the faster client is also affected (not sure
 if this is a problem).

 2. rendering per pipe - haven't thought this through, but basically releasing
 the original operations and allocating a single new operation (thereby keeping
 the slow client's pipe bounded).

> 
> On Fri, Feb 11, 2011 at 6:48 PM, Alon Levy <alevy at redhat.com> wrote:
> > ---
> >  server/red_channel.c |   10 ++++++++++
> >  server/red_channel.h |    6 ++++++
> >  server/red_worker.c  |   10 +++++-----
> >  3 files changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/server/red_channel.c b/server/red_channel.c
> > index fc25ebe..5830f4d 100644
> > --- a/server/red_channel.c
> > +++ b/server/red_channel.c
> > @@ -712,3 +712,13 @@ int red_channel_get_first_socket(RedChannel *channel)
> >     return channel->peer->socket;
> >  }
> >
> > +int red_channel_all_blocked(RedChannel *channel)
> > +{
> > +    return channel->send_data.blocked;
> > +}
> > +
> > +int red_channel_any_blocked(RedChannel *channel)
> > +{
> > +    return channel->send_data.blocked;
> > +}
> > +
> > diff --git a/server/red_channel.h b/server/red_channel.h
> > index 2c2c231..89893d4 100644
> > --- a/server/red_channel.h
> > +++ b/server/red_channel.h
> > @@ -253,6 +253,12 @@ void red_channel_set_shut(RedChannel *channel);
> >
> >  int red_channel_get_first_socket(RedChannel *channel);
> >
> > +/* return TRUE if all of the connected clients to this channel are blocked */
> > +int red_channel_all_blocked(RedChannel *channel);
> > +
> > +/* return TRUE if any of the connected clients to this channel are blocked */
> > +int red_channel_any_blocked(RedChannel *channel);
> > +
> >  // TODO: unstaticed for display/cursor channels. they do some specific pushes not through
> >  // adding elements or on events. but not sure if this is actually required (only result
> >  // should be that they ""try"" a little harder, but if the event system is correct it
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 2a57060..58d6a38 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -4289,7 +4289,7 @@ static int red_process_commands(RedWorker *worker, uint32_t max_pipe_size, int *
> >             red_error("bad command type");
> >         }
> >         n++;
> > -        if ((worker->display_channel && worker->display_channel->common.base.send_data.blocked) ||
> > +        if ((worker->display_channel && red_channel_all_blocked(&worker->display_channel->common.base)) ||
> >             red_now() - start > 10 * 1000 * 1000) {
> >             worker->epoll_timeout = 0;
> >             return n;
> > @@ -9134,7 +9134,7 @@ static void handle_channel_events(EventListener *in_listener, uint32_t events)
> >         red_channel_receive(channel);
> >     }
> >
> > -    if (channel->send_data.blocked) {
> > +    if (red_channel_any_blocked(channel)) {
> >         red_channel_push(channel);
> >     }
> >  }
> > @@ -9411,7 +9411,7 @@ static void red_wait_outgoing_item(RedChannel *channel)
> >     uint64_t end_time;
> >     int blocked;
> >
> > -    if (!channel || !channel->send_data.blocked) {
> > +    if (!channel || !red_channel_all_blocked(channel)) {
> >         return;
> >     }
> >     red_ref_channel(channel);
> > @@ -9423,7 +9423,7 @@ static void red_wait_outgoing_item(RedChannel *channel)
> >         usleep(DETACH_SLEEP_DURATION);
> >         red_channel_receive(channel);
> >         red_channel_send(channel);
> > -    } while ((blocked = channel->send_data.blocked) && red_now() < end_time);
> > +    } while ((blocked = red_channel_all_blocked(channel)) && red_now() < end_time);
> >
> >     if (blocked) {
> >         red_printf("timeout");
> > @@ -9449,7 +9449,7 @@ static void red_wait_pipe_item_sent(RedChannel *channel, PipeItem *item)
> >
> >     end_time = red_now() + CHANNEL_PUSH_TIMEOUT;
> >
> > -    if (channel->send_data.blocked) {
> > +    if (red_channel_all_blocked(channel)) {
> >         red_channel_receive(channel);
> >         red_channel_send(channel);
> >     }
> > --
> > 1.7.4
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> 
> 
> 
> -- 
> Marc-André Lureau