[Spice-devel] [PATCH 05/18] Store 'renderers' as GArray in RedsState
Jonathon Jongsma
jjongsma at redhat.com
Wed Feb 10 21:30:57 CET 2016
On Wed, 2016-02-10 at 06:17 -0500, Frediano Ziglio wrote:
> >
> > On Mon, 2016-02-01 at 08:47 -0500, Frediano Ziglio wrote:
> > > >
> > > > From: Jonathon Jongsma <jjongsma at redhat.com>
> > > >
> > > > Acked-by: Frediano Ziglio <fziglio at redhat.com>
> > > > ---
> > > > server/display-channel.c | 13 +++++++------
> > > > server/display-channel.h | 3 +--
> > > > server/reds-private.h | 1 +
> > > > server/reds.c | 20 ++++++++++++--------
> > > > server/reds.h | 5 ++---
> > > > 5 files changed, 23 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/server/display-channel.c b/server/display-channel.c
> > > > index f0d133a..2282847 100644
> > > > --- a/server/display-channel.c
> > > > +++ b/server/display-channel.c
> > > > @@ -1911,10 +1911,11 @@ void
> > > > display_channel_create_surface(DisplayChannel
> > > > *display, uint32_t surface_id
> > > >
> > > > if (display->renderer == RED_RENDERER_INVALID) {
> > > > int i;
> > > > - for (i = 0; i < display->num_renderers; i++) {
> > > > - surface->context.canvas =
> > > > create_canvas_for_surface(display,
> > > > surface, display->renderers[i]);
> > > > + for (i = 0; i < display->renderers->len; i++) {
> > > > + uint32_t renderer = g_array_index(display->renderers,
> > > > uint32_t,
> > > > i);
> > > > + surface->context.canvas =
> > > > create_canvas_for_surface(display,
> > > > surface, renderer);
> > > > if (surface->context.canvas) {
> > > > - display->renderer = display->renderers[i];
> > > > + display->renderer = renderer;
> > > > break;
> > > > }
> > > > }
> > > > @@ -2030,8 +2031,9 @@ DisplayChannel* display_channel_new(RedWorker
> > > > *worker,
> > > > int migrate, int stream_v
> > > > static SpiceImageSurfacesOps image_surfaces_ops = {
> > > > image_surfaces_get,
> > > > };
> > > > + GArray *renderers = reds_get_renderers(reds);
> > > >
> > > > - spice_return_val_if_fail(num_renderers > 0, NULL);
> > > > + spice_return_val_if_fail(renderers->len > 0, NULL);
> > > >
> > > > spice_info("create display channel");
> > > > display = (DisplayChannel *)red_worker_new_channel(
> > > > @@ -2063,8 +2065,7 @@ DisplayChannel* display_channel_new(RedWorker
> > > > *worker,
> > > > int migrate, int stream_v
> > > > stat_compress_init(&display->lz4_stat, "lz4", stat_clock);
> > > >
> > > > display->n_surfaces = n_surfaces;
> > > > - display->num_renderers = num_renderers;
> > > > - memcpy(display->renderers, renderers, sizeof(display->renderers));
> > > > + display->renderers = g_array_ref(renderers);
> > > > display->renderer = RED_RENDERER_INVALID;
> > > >
> > > > ring_init(&display->current_list);
> > >
> > > Actually this is changing the way code behave and in theory introduce a
> > > race
> > > condition.
> > > In the previous code array was copied to channel, in the new code array is
> > > referenced
> > > in the channel. Potentially you could insert new rendered in the array
> > > after
> > > array is
> > > referenced and before is used.
> >
> > True, it is potentially a slight change in behavior. On the other hand, does
> > it
> > make any sense to add new renderers after the server is already running? I
> > am
> > almost tempted to add something like this to make it clear that this isn't
> > really supported.
> >
>
> Yes, this was one of my point. We copy the array to use later but actually we
> suppose the array never changes so what's the point of having a copy.
> We are touching the code so why don't change the approach.
> Instead of accessing the array directly ask the server to provide a rendered.
> So on first call the reds.c code will detect the renderer and give it back to
> caller
> while caching the result that will be used on later calls.
> Does it make sense?
It sounds reasonable to me
>
> > diff --git a/server/reds.c b/server/reds.c
> > index 35442b0..f9251b9 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -3779,6 +3779,9 @@ SPICE_GNUC_VISIBLE int
> > spice_server_is_server_mouse(SpiceServer *s)
> > SPICE_GNUC_VISIBLE int spice_server_add_renderer(SpiceServer *s, const char
> > *name)
> > {
> > spice_assert(reds == s);
> > + /* warn if we try to add new renderers after the spice server is
> > already
> > + * initialized */
> > + if (s->main_channel != NULL)
> > + g_warning("Please add renderers before initializing the spice
> > server");
> > +
> > if (!red_add_renderer(name)) {
> > return -1;
> > }
> >
> > or even using g_return_if_fail() instead of just issuing a warning. Looking
> > at
> > code that uses spice server, neither qemu nor xspice actually call
> > spice_server_add_renderer(). So it doesn't appear that any changes would be
> > required.
> >
> >
> > > In practice this should not happen. Actually in practice
> > > there is no reason why first channel should use a renderer and others
> > > other
> > > renderers.
> > > Considering that you cannot remove renderers from the list, that first
> > > working
> > > renderer
> > > are chosen and first channel succeeded this mean you will always use same
> > > renderer for
> > > all channels on same server. At this point rendered could be cached first
> > > time
> > > is tested
> > > (perhaps instead of copying the array we should just finding the renderer
> > > working).
> >
> > Not sure I understand what you're suggesting here.
> >
> > > Also in our environment seems a bit overkilling these dynamic allocated
> > > arrays.
> >
> > Since this is a function that is only executed a couple of times during the
> > lifetime of the spice server, I don't see any reason to avoid it. And I
> > personally think it makes the code simpler to use these slightly higher
> > -level
> > data types.
> >
>
> My point was (well one of the point) that copying the array was useless.
>
> > > Actually RED_RENDERER_LAST is two as 0 is used for invalid and 1 for the
> > > only
> > > real
> > > renderer available. I think event uint32_t is overkilling, uint8_t is
> > > enough.
> > >
> > > > diff --git a/server/display-channel.h b/server/display-channel.h
> > > > index bf29cd3..34fb57f 100644
> > > > --- a/server/display-channel.h
> > > > +++ b/server/display-channel.h
> > > > @@ -168,8 +168,7 @@ struct DisplayChannel {
> > > >
> > > > MonitorsConfig *monitors_config;
> > > >
> > > > - uint32_t num_renderers;
> > > > - uint32_t renderers[RED_RENDERER_LAST];
> > > > + GArray *renderers;
> > > > uint32_t renderer;
> > > > int enable_jpeg;
> > > > int enable_zlib_glz_wrap;
> > > > diff --git a/server/reds-private.h b/server/reds-private.h
> > > > index 285f226..60c38aa 100644
> > > > --- a/server/reds-private.h
> > > > +++ b/server/reds-private.h
> > > > @@ -196,6 +196,7 @@ struct RedsState {
> > > > int default_channel_security;
> > > > ChannelSecurityOptions *channels_security;
> > > > const char *default_renderer;
> > > > + GArray *renderers;
> > > >
> > > > int spice_port;
> > > > int spice_secure_port;
> > > > diff --git a/server/reds.c b/server/reds.c
> > > > index feae0cc..0c94d6e 100644
> > > > --- a/server/reds.c
> > > > +++ b/server/reds.c
> > > > @@ -3440,6 +3440,7 @@ SPICE_GNUC_VISIBLE SpiceServer
> > > > *spice_server_new(void)
> > > > reds->default_channel_security =
> > > > SPICE_CHANNEL_SECURITY_NONE | SPICE_CHANNEL_SECURITY_SSL;
> > > > reds->default_renderer = "sw";
> > > > + reds->renderers = g_array_sized_new(FALSE, TRUE, sizeof(uint32_t),
> > > > RED_RENDERER_LAST);
> > > > reds->spice_port = -1;
> > > > reds->spice_secure_port = -1;
> > > > return reds;
> > > > @@ -3455,9 +3456,6 @@ static RendererInfo renderers_info[] = {
> > > > {RED_RENDERER_INVALID, NULL},
> > > > };
> > > >
> > > > -uint32_t renderers[RED_RENDERER_LAST];
> > > > -uint32_t num_renderers = 0;
> > > > -
> > > > static RendererInfo *find_renderer(const char *name)
> > > > {
> > > > RendererInfo *inf = renderers_info;
> > > > @@ -3470,14 +3468,14 @@ static RendererInfo *find_renderer(const char
> > > > *name)
> > > > return NULL;
> > > > }
> > > >
> > > > -static int red_add_renderer(const char *name)
> > > > +static int reds_add_renderer(RedsState *reds, const char *name)
> > > > {
> > > > RendererInfo *inf;
> > > >
> > > > - if (num_renderers == RED_RENDERER_LAST || !(inf =
> > > > find_renderer(name)))
> > > > {
> > > > + if (reds->renderers->len == RED_RENDERER_LAST || !(inf =
> > > > find_renderer(name))) {
> > >
> > > Here the check was done to avoid overflow, can be removed or we could
> > > check that there are no repetitions on the array.
> > >
> > > > return FALSE;
> > > > }
> > > > - renderers[num_renderers++] = inf->id;
> > > > + g_array_append_val(reds->renderers, inf->id);
> > > > return TRUE;
> > > > }
> > > >
> > > > @@ -3488,7 +3486,7 @@ SPICE_GNUC_VISIBLE int
> > > > spice_server_init(SpiceServer
> > > > *s, SpiceCoreInterface *cor
> > > > spice_assert(reds == s);
> > > > ret = do_spice_init(s, core);
> > > > if (s->default_renderer) {
> > > > - red_add_renderer(s->default_renderer);
> > > > + reds_add_renderer(s, s->default_renderer);
> > > > }
> > > > return ret;
> > > > }
> > > > @@ -3496,6 +3494,7 @@ SPICE_GNUC_VISIBLE int
> > > > spice_server_init(SpiceServer
> > > > *s, SpiceCoreInterface *cor
> > > > SPICE_GNUC_VISIBLE void spice_server_destroy(SpiceServer *s)
> > > > {
> > > > spice_assert(reds == s);
> > > > + g_array_unref(s->renderers);
> > > > reds_exit();
> > > > }
> > > >
> > > > @@ -3779,7 +3778,7 @@ SPICE_GNUC_VISIBLE int
> > > > spice_server_is_server_mouse(SpiceServer *s)
> > > > SPICE_GNUC_VISIBLE int spice_server_add_renderer(SpiceServer *s, const
> > > > char
> > > > *name)
> > > > {
> > > > spice_assert(reds == s);
> > > > - if (!red_add_renderer(name)) {
> > > > + if (!reds_add_renderer(s, name)) {
> > > > return -1;
> > > > }
> > > > s->default_renderer = NULL;
> > > > @@ -4027,3 +4026,8 @@ SPICE_GNUC_VISIBLE void
> > > > spice_server_set_keepalive_timeout(SpiceServer *s, int t
> > > > reds->keepalive_timeout = timeout;
> > > > spice_debug("keepalive timeout=%d", timeout);
> > > > }
> > > > +
> > > > +GArray* reds_get_renderers(RedsState *reds)
> > > > +{
> > > > + return reds->renderers;
> > > > +}
> > >
>
> So I would replace this with a
>
> uint32_t reds_get_renderer(RedsState *reds, whatever needed to get it)
> {
> if (reds->renderer == INVALID) {
> reds->renderer = compute_renderer(params);
> }
> return reds->renderer;
> }
>
> > > Why not incrementing reference here?
> > >
> > > > diff --git a/server/reds.h b/server/reds.h
> > > > index e398607..4034199 100644
> > > > --- a/server/reds.h
> > > > +++ b/server/reds.h
> > > > @@ -63,6 +63,8 @@ int reds_get_agent_mouse(void); // used by
> > > > inputs_channel
> > > > int reds_has_vdagent(void); // used by inputs channel
> > > > void reds_handle_agent_mouse_event(RedsState *reds, const
> > > > VDAgentMouseState
> > > > *mouse_state); // used by inputs_channel
> > > >
> > > > +GArray* reds_get_renderers(RedsState *reds);
> > > > +
> > > > enum {
> > > > RED_RENDERER_INVALID,
> > > > RED_RENDERER_SW,
> > > > @@ -70,9 +72,6 @@ enum {
> > > > RED_RENDERER_LAST
> > > > };
> > > >
> > > > -extern uint32_t renderers[RED_RENDERER_LAST];
> > > > -extern uint32_t num_renderers;
> > > > -
> > > > extern struct SpiceCoreInterfaceInternal *core;
> > > > extern uint32_t streaming_video;
> > > > extern SpiceImageCompression image_compression;
> > > > --
> > > > 2.4.3
> > > >
> > >
>
> Frediano
More information about the Spice-devel
mailing list