[Spice-devel] [PATCH spice-server] Release cursor as soon as possible

[Frediano Ziglio]

> 
> On Tue, Feb 28, 2017 at 03:20:09PM +0000, Frediano Ziglio wrote:
> > Cursor resources (basically the shape of it) was retained till
> > it was used however it was copied so there were no reason to not release
> > this resource.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/cursor-channel.c         | 14 ++++----------
> >  server/cursor-channel.h         |  4 ++--
> >  server/red-parse-qxl.c          | 10 +++++++---
> >  server/red-parse-qxl.h          |  4 ++--
> >  server/red-worker.c             |  5 +++--
> >  server/tests/test-qxl-parsing.c | 26 +++++++++++++++++++++++---
> >  6 files changed, 41 insertions(+), 22 deletions(-)
> > 
> > diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> > index 89cb120..5ed36df 100644
> > --- a/server/red-parse-qxl.c
> > +++ b/server/red-parse-qxl.c
> > @@ -27,6 +27,7 @@
> >  #include "red-common.h"
> >  #include "memslot.h"
> >  #include "red-parse-qxl.h"
> > +#include "red-qxl.h"
> >  
> >  /* Max size in bytes for any data field used in a QXL command.
> >   * This will for example be useful to prevent the guest from saturating
> >   the
> > @@ -1470,8 +1471,10 @@ static void red_put_cursor(SpiceCursor *red)
> >  }
> >  
> >  int red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
> > -                       RedCursorCmd *red, QXLPHYSICAL addr)
> > +                       RedCursorCmd *red, QXLPHYSICAL addr,
> > +                       QXLInstance *qxl_instance)
> 
> Not convinced about this patch, mainly I think because of the added
> QXLInstance dependency which gets added to red-parse-qxl.h,
> red_get_cursor_cmd() becoming odd compared to the other similar
> functions, ...
> 
> I think it would work to have the red_qxl_release_resource() call in
> red_process_cursor_cmd()?
> 

This would be just a step before this...
Memory management and resources are two sides of the coin.
red-parse-qxl.c already deals with release_info information so
adding a free is not really a big jump.
Actually already some structures in red-parse-qxl.h has a QXLInstance
which is initialized by red-worker.c. Maybe this is the issue, some
structures are basically initialized half in red-parse-qxl.c and
half in red-worker.c.

I think I have 3/4 branches trying in different way to do
some sort of this but I think this is the best even considering
guest resources. Resources are not used, just retained more
(the cursor is copied in host allocated memory in any way).

> >  {
> > +    QXLReleaseInfoExt release_info_ext;
> >      QXLCursorCmd *qxl;
> >      int error;
> >  
> > @@ -1479,8 +1482,8 @@ int red_get_cursor_cmd(RedMemSlotInfo *slots, int
> > group_id,
> >      if (error) {
> >          return error;
> >      }
> > -    red->release_info_ext.info      = &qxl->release_info;
> > -    red->release_info_ext.group_id  = group_id;
> > +    release_info_ext.info      = &qxl->release_info;
> > +    release_info_ext.group_id  = group_id;
> >  
> >      red->type = qxl->type;
> >      switch (red->type) {
> > @@ -1497,6 +1500,7 @@ int red_get_cursor_cmd(RedMemSlotInfo *slots, int
> > group_id,
> >          red->u.trail.frequency = qxl->u.trail.frequency;
> >          break;
> >      }
> > +    red_qxl_release_resource(qxl_instance, release_info_ext);
> >      return error;
> >  }
> >  
> > diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
> > index 86a2d93..9aa1077 100644
> > --- a/server/red-parse-qxl.h
> > +++ b/server/red-parse-qxl.h
> > @@ -99,7 +99,6 @@ typedef struct RedSurfaceCmd {
> >  } RedSurfaceCmd;
> >  
> >  typedef struct RedCursorCmd {
> > -    QXLReleaseInfoExt release_info_ext;
> >      uint8_t type;
> >      union {
> >          struct {
> > @@ -138,7 +137,8 @@ int red_get_surface_cmd(RedMemSlotInfo *slots, int
> > group_id,
> >  void red_put_surface_cmd(RedSurfaceCmd *red);
> >  
> >  int red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,
> > -                       RedCursorCmd *red, QXLPHYSICAL addr);
> > +                       RedCursorCmd *red, QXLPHYSICAL addr,
> > +                       QXLInstance *qxl);
> >  void red_put_cursor_cmd(RedCursorCmd *red);
> >  
> >  #endif
> > diff --git a/server/red-worker.c b/server/red-worker.c
> > index e5adbaa..20b15bc 100644
> > --- a/server/red-worker.c
> > +++ b/server/red-worker.c
> > @@ -107,7 +107,8 @@ static gboolean red_process_cursor_cmd(RedWorker
> > *worker, const QXLCommandExt *e
> >      RedCursorCmd *cursor_cmd;
> >  
> >      cursor_cmd = spice_new0(RedCursorCmd, 1);
> > -    if (red_get_cursor_cmd(&worker->mem_slots, ext->group_id, cursor_cmd,
> > ext->cmd.data)) {
> > +    if (red_get_cursor_cmd(&worker->mem_slots, ext->group_id, cursor_cmd,
> > ext->cmd.data,
> > +                           worker->qxl)) {
> >          free(cursor_cmd);
> >          return FALSE;
> >      }
> > @@ -1364,7 +1365,7 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> >  
> >      worker->event_timeout = INF_EVENT_WAIT;
> >  
> > -    worker->cursor_channel = cursor_channel_new(reds, qxl,
> > +    worker->cursor_channel = cursor_channel_new(reds, qxl->id,
> >                                                  &worker->core);
> 
> This change is not strictly related to what you are doing in this
> commit, and in my opinion this belongs more in a follow-up patch moving
> CommonGraphicsChannel::qxl to a DisplayChannel::qxl property as after
> this change, it will always be NULL.
> 
> Christophe
> 

Yes, I can split. And also start follow ups.

Frediano