[Spice-devel] [PATCH 03/11] worker: change CursorItem memory allocation
Jonathon Jongsma
jjongsma at redhat.com
Wed Oct 28 13:36:41 PDT 2015
On Wed, 2015-10-28 at 15:23 -0500, Jonathon Jongsma wrote:
> On Wed, 2015-10-28 at 09:01 -0400, Frediano Ziglio wrote:
> > >
> > > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > >
> > > Do not use static allocate space but handle dynamically
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau at gmail.com>
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > > server/cursor-channel.h | 9 --------
> > > server/red_worker.c | 56
> > > ++++++++++---------------------------------------
> > > 2 files changed, 11 insertions(+), 54 deletions(-)
> > >
> > > diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> > > index 48b2c71..4654bcb 100644
> > > --- a/server/cursor-channel.h
> > > +++ b/server/cursor-channel.h
> > > @@ -64,15 +64,6 @@ typedef struct CursorChannel {
> > > #endif
> > > } CursorChannel;
> > >
> > > -typedef struct _CursorItem _CursorItem;
> > > -
> > > -struct _CursorItem {
> > > - union {
> > > - CursorItem cursor_item;
> > > - _CursorItem *next;
> > > - } u;
> > > -};
> > > -
> > > G_STATIC_ASSERT(sizeof(CursorItem) <=
> > > QXL_CURSUR_DEVICE_DATA_SIZE);
> > >
> > >
> > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > index a55e57a..631ea11 100644
> > > --- a/server/red_worker.c
> > > +++ b/server/red_worker.c
> > > @@ -646,9 +646,6 @@ typedef struct RedWorker {
> > > _Drawable drawables[NUM_DRAWABLES];
> > > _Drawable *free_drawables;
> > >
> > > - _CursorItem cursor_items[NUM_CURSORS];
> > > - _CursorItem *free_cursor_items;
> > > -
> > > RedMemSlotInfo mem_slots;
> > >
> > > ImageCache image_cache;
> > > @@ -4234,8 +4231,6 @@ static void red_update_area(RedWorker
> > > *worker, const
> > > SpiceRect *area, int surfac
> > > validate_area(worker, area, surface_id);
> > > }
> > >
> > > -static inline void free_cursor_item(RedWorker *worker,
> > > CursorItem
> > > *item);
> > > -
> > > static void red_release_cursor(RedWorker *worker, CursorItem
> > > *cursor)
> > > {
> > > if (!--cursor->refs) {
> > > @@ -4246,9 +4241,10 @@ static void red_release_cursor(RedWorker
> > > *worker,
> > > CursorItem *cursor)
> > > release_info_ext.group_id = cursor->group_id;
> > > release_info_ext.info = cursor_cmd->release_info;
> > > worker->qxl->st->qif->release_resource(worker->qxl,
> > > release_info_ext);
> > > - free_cursor_item(worker, cursor);
> > > red_put_cursor_cmd(cursor_cmd);
> > > free(cursor_cmd);
> > > +
> > > + g_slice_free(CursorItem, cursor);
> > > }
> > > }
> > >
> > > @@ -4261,52 +4257,23 @@ static void red_set_cursor(RedWorker
> > > *worker,
> > > CursorItem *cursor)
> > > worker->cursor = cursor;
> > > }
> > >
> > > -#ifdef DEBUG_CURSORS
> > > -static int _cursor_count = 0;
> > > -#endif
> > > -
> > > -static inline CursorItem *alloc_cursor_item(RedWorker *worker)
> > > -{
> > > - CursorItem *cursor;
> > > -
> > > - if (!worker->free_cursor_items) {
> > > - return NULL;
> > > - }
> > > -#ifdef DEBUG_CURSORS
> > > - --_cursor_count;
> > > -#endif
> > > - cursor = &worker->free_cursor_items->u.cursor_item;
> > > - worker->free_cursor_items = worker->free_cursor_items
> > > ->u.next;
> > > - return cursor;
> > > -}
> > > -
> > > -static inline void free_cursor_item(RedWorker *worker,
> > > CursorItem
> > > *item)
> > > +static inline CursorItem *alloc_cursor_item(void)
> > > {
> > > - ((_CursorItem *)item)->u.next = worker->free_cursor_items;
> > > - worker->free_cursor_items = (_CursorItem *)item;
> > > -#ifdef DEBUG_CURSORS
> > > - ++_cursor_count;
> > > - spice_assert(_cursor_count <= NUM_CURSORS);
> > > -#endif
> > > -}
> > > + CursorItem *cursor_item;
> > >
> > > -static void cursor_items_init(RedWorker *worker)
> > > -{
> > > - int i;
> > > + cursor_item = g_slice_new0(CursorItem);
> > > + cursor_item->refs = 1;
> > >
> > > - worker->free_cursor_items = NULL;
> > > - for (i = 0; i < NUM_CURSORS; i++) {
> > > - free_cursor_item(worker, &worker
> > > ->cursor_items[i].u.cursor_item);
> > > - }
> > > + return cursor_item;
> > > }
> > >
> > > -static CursorItem *get_cursor_item(RedWorker *worker,
> > > RedCursorCmd
> > > *cmd,
> > > uint32_t group_id)
> > > +static CursorItem *get_cursor_item(RedCursorCmd *cmd, uint32_t
> > > group_id)
> > > {
> > > CursorItem *cursor_item;
> > >
> > > - spice_warn_if(!(cursor_item = alloc_cursor_item(worker)));
> > > + spice_return_val_if_fail(cmd != NULL, NULL);
> > > + spice_warn_if(!(cursor_item = alloc_cursor_item()));
> > >
> >
> > Next instruction are going to core if the pointer is ULL, perhaps
> > would
> > be better a spice_error or spice_critical.
>
>
> It seems to me that we should instead replace spice_warn_if() with
> spice_return_val_if_fail(cursor_item, NULL). get_cursor_item() is
> only
> used from qxl_process_cursor(), so we would only need to change that
> function to handle a NULL return from this function. Then we will
> have
> a useful warning message on the debug output and won't abort the qemu
> process when this error occurs. But this is a pre-existing issue that
> can be handled in a separate patch.
Oh, nevermind. In fact we can just remove this warning completely,
because alloc_cursor_item() now allocates memory dynamically using
g_slice_new (which aborts on failure), instead of looking for an empty
slot in a statically allocated pool. So cursor_item can never actually
be NULL here.
>
>
> >
> > > - cursor_item->refs = 1;
> > > cursor_item->group_id = group_id;
> > > cursor_item->red_cursor = cmd;
> > >
> > > @@ -4350,7 +4317,7 @@ static void qxl_process_cursor(RedWorker
> > > *worker,
> > > RedCursorCmd *cursor_cmd, uint
> > > CursorItem *cursor_item;
> > > int cursor_show = FALSE;
> > >
> > > - cursor_item = get_cursor_item(worker, cursor_cmd, group_id);
> > > + cursor_item = get_cursor_item(cursor_cmd, group_id);
> > >
> > > switch (cursor_cmd->type) {
> > > case QXL_CURSOR_SET:
> > > @@ -11629,7 +11596,6 @@ RedWorker* red_worker_new(QXLInstance
> > > *qxl,
> > > RedDispatcher *red_dispatcher)
> > > image_cache_init(&worker->image_cache);
> > > image_surface_init(worker);
> > > drawables_init(worker);
> > > - cursor_items_init(worker);
> > > red_init_streams(worker);
> > > stat_init(&worker->add_stat, add_stat_name);
> > > stat_init(&worker->exclude_stat, exclude_stat_name);
> > > --
> > > 2.4.3
> > >
> > >
> >
> > Frediano
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list