subject:"\[Spice\-devel\] \[spice\-server\] cursor\: Delay release of QXL guest cursor resources"

Re: [Spice-devel] [spice-server] cursor: Delay release of QXL guest cursor resources

2018-04-10 Thread Christophe Fergeau

Hey,

On Tue, Apr 10, 2018 at 08:16:32AM -0400, Frediano Ziglio wrote:
> > > 
> > > Ping? I would like to move forward with having this fix temporarily in
> > > spice-server even if in the long run, we'll be fixing QEMU too.
> > > This would ease the upgrade path, as having this patch means we don't
> > > tie upgrades to spice-server 0.14 with QEMU upgrades, it does not matter
> > > if you upgrade both at once or not, spice-server will have the same
> > > behaviour as in the 0.12 branch.
> > > 
> > > Christophe
> > > 
> > 
> > When do you plan to remove this patch from spice-server?
> > 
> > > On Thu, Apr 05, 2018 at 10:36:27AM +0200, Christophe Fergeau wrote:
> > > > There's an implicit API/ABI contract between QEMU and SPICE that SPICE
> > > > will keep the guest QXL resources alive as long as QEMU can hold a
> > > > pointer to them. This implicit contract was broken in 1c6e7cf7 "Release
> > > > cursor as soon as possible", causing crashes at migration time.
> > > > While the proper fix would be in QEMU so that spice-server does not need
> > > > to have that kind of knowledge regarding QEMU internal implementation,
> > > > this commit reverts to the pre-1c6e7cf7 behaviour to avoid a regression
> > > > while QEMU is being fixed.
> > > > 
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1540919
> > > > 
> > > > Signed-off-by: Christophe Fergeau 
> > 
> > Would not be better to add a qxl field in RedCursorCmd and free the resource
> > in red_put_cursor_cmd?
> > This patch looks pretty invasive.
> > 
> 
> Like:

Yes, definitely a very good suggestion, I'll try that, thanks !

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server] cursor: Delay release of QXL guest cursor resources

2018-04-10 Thread Frediano Ziglio

> > 
> > Ping? I would like to move forward with having this fix temporarily in
> > spice-server even if in the long run, we'll be fixing QEMU too.
> > This would ease the upgrade path, as having this patch means we don't
> > tie upgrades to spice-server 0.14 with QEMU upgrades, it does not matter
> > if you upgrade both at once or not, spice-server will have the same
> > behaviour as in the 0.12 branch.
> > 
> > Christophe
> > 
> 
> When do you plan to remove this patch from spice-server?
> 
> > On Thu, Apr 05, 2018 at 10:36:27AM +0200, Christophe Fergeau wrote:
> > > There's an implicit API/ABI contract between QEMU and SPICE that SPICE
> > > will keep the guest QXL resources alive as long as QEMU can hold a
> > > pointer to them. This implicit contract was broken in 1c6e7cf7 "Release
> > > cursor as soon as possible", causing crashes at migration time.
> > > While the proper fix would be in QEMU so that spice-server does not need
> > > to have that kind of knowledge regarding QEMU internal implementation,
> > > this commit reverts to the pre-1c6e7cf7 behaviour to avoid a regression
> > > while QEMU is being fixed.
> > > 
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1540919
> > > 
> > > Signed-off-by: Christophe Fergeau 
> 
> Would not be better to add a qxl field in RedCursorCmd and free the resource
> in red_put_cursor_cmd?
> This patch looks pretty invasive.
> 

Like:


diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 69748698..f421a35b 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -26,6 +26,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
@@ -1495,6 +1496,9 @@ void red_put_cursor_cmd(RedCursorCmd *red)
 {
 switch (red->type) {
 case QXL_CURSOR_SET:
+if (red->qxl) {
+red_qxl_release_resource(red->qxl, red->release_info_ext);
+}
 red_put_cursor(>u.set.shape);
 break;
 }
diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
index ce7d8b05..a33f36ad 100644
--- a/server/red-parse-qxl.h
+++ b/server/red-parse-qxl.h
@@ -99,6 +99,7 @@ typedef struct RedSurfaceCmd {
 } RedSurfaceCmd;

 typedef struct RedCursorCmd {
+QXLInstance *qxl;
 QXLReleaseInfoExt release_info_ext;
 uint8_t type;
 union {
diff --git a/server/red-worker.c b/server/red-worker.c
index 387f500e..eb927f3e 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -118,7 +118,7 @@ static gboolean red_process_cursor_cmd(RedWorker *worker, 
const QXLCommandExt *e
 g_free(cursor_cmd);
 return FALSE;
 }
-red_qxl_release_resource(worker->qxl, cursor_cmd->release_info_ext);
+cursor_cmd->qxl = worker->qxl;
 cursor_channel_process_cmd(worker->cursor_channel, cursor_cmd);
 return TRUE;
 }



Frediano

> > > ---
> > >  server/cursor-channel.c| 16 +---
> > >  server/cursor-channel.h|  5 -
> > >  server/red-stream-device.c |  4 ++--
> > >  server/red-worker.c| 10 --
> > >  4 files changed, 27 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > > index 522261e3f..a7bee9815 100644
> > > --- a/server/cursor-channel.c
> > > +++ b/server/cursor-channel.c
> > > @@ -31,6 +31,8 @@
> > >  typedef struct RedCursorPipeItem {
> > >  RedPipeItem base;
> > >  RedCursorCmd *red_cursor;
> > > +ReleaseCommandCb release_command_cb;
> > > +gpointer user_data;
> > >  } RedCursorPipeItem;
> > >  
> > >  struct CursorChannel
> > > @@ -54,7 +56,9 @@ G_DEFINE_TYPE(CursorChannel, cursor_channel,
> > > TYPE_COMMON_GRAPHICS_CHANNEL)
> > >  
> > >  static void cursor_pipe_item_free(RedPipeItem *pipe_item);
> > >  
> > > -static RedCursorPipeItem *cursor_pipe_item_new(RedCursorCmd *cmd)
> > > +static RedCursorPipeItem *cursor_pipe_item_new(RedCursorCmd *cmd,
> > > +   ReleaseCommandCb
> > > release_command_cb,
> > > +   gpointer user_data)
> > >  {
> > >  RedCursorPipeItem *item = g_new0(RedCursorPipeItem, 1);
> > >  
> > > @@ -63,6 +67,8 @@ static RedCursorPipeItem
> > > *cursor_pipe_item_new(RedCursorCmd *cmd)
> > >  red_pipe_item_init_full(>base, RED_PIPE_ITEM_TYPE_CURSOR,
> > >  cursor_pipe_item_free);
> > >  item->red_cursor = cmd;
> > > +item->release_command_cb = release_command_cb;
> > > +item->user_data = user_data;
> > >  
> > >  return item;
> > >  }
> > > @@ -73,6 +79,9 @@ static void cursor_pipe_item_free(RedPipeItem *base)
> > >  RedCursorPipeItem *pipe_item = SPICE_UPCAST(RedCursorPipeItem,
> > >  base);
> > >  
> > >  cursor_cmd = pipe_item->red_cursor;
> > > +if (pipe_item->release_command_cb) {
> > > +

Re: [Spice-devel] [spice-server] cursor: Delay release of QXL guest cursor resources

2018-04-10 Thread Christophe Fergeau

Ping? I would like to move forward with having this fix temporarily in
spice-server even if in the long run, we'll be fixing QEMU too.
This would ease the upgrade path, as having this patch means we don't
tie upgrades to spice-server 0.14 with QEMU upgrades, it does not matter
if you upgrade both at once or not, spice-server will have the same
behaviour as in the 0.12 branch.

Christophe

On Thu, Apr 05, 2018 at 10:36:27AM +0200, Christophe Fergeau wrote:
> There's an implicit API/ABI contract between QEMU and SPICE that SPICE
> will keep the guest QXL resources alive as long as QEMU can hold a
> pointer to them. This implicit contract was broken in 1c6e7cf7 "Release
> cursor as soon as possible", causing crashes at migration time.
> While the proper fix would be in QEMU so that spice-server does not need
> to have that kind of knowledge regarding QEMU internal implementation,
> this commit reverts to the pre-1c6e7cf7 behaviour to avoid a regression
> while QEMU is being fixed.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1540919
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  server/cursor-channel.c| 16 +---
>  server/cursor-channel.h|  5 -
>  server/red-stream-device.c |  4 ++--
>  server/red-worker.c| 10 --
>  4 files changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 522261e3f..a7bee9815 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -31,6 +31,8 @@
>  typedef struct RedCursorPipeItem {
>  RedPipeItem base;
>  RedCursorCmd *red_cursor;
> +ReleaseCommandCb release_command_cb;
> +gpointer user_data;
>  } RedCursorPipeItem;
>  
>  struct CursorChannel
> @@ -54,7 +56,9 @@ G_DEFINE_TYPE(CursorChannel, cursor_channel, 
> TYPE_COMMON_GRAPHICS_CHANNEL)
>  
>  static void cursor_pipe_item_free(RedPipeItem *pipe_item);
>  
> -static RedCursorPipeItem *cursor_pipe_item_new(RedCursorCmd *cmd)
> +static RedCursorPipeItem *cursor_pipe_item_new(RedCursorCmd *cmd,
> +   ReleaseCommandCb 
> release_command_cb,
> +   gpointer user_data)
>  {
>  RedCursorPipeItem *item = g_new0(RedCursorPipeItem, 1);
>  
> @@ -63,6 +67,8 @@ static RedCursorPipeItem *cursor_pipe_item_new(RedCursorCmd 
> *cmd)
>  red_pipe_item_init_full(>base, RED_PIPE_ITEM_TYPE_CURSOR,
>  cursor_pipe_item_free);
>  item->red_cursor = cmd;
> +item->release_command_cb = release_command_cb;
> +item->user_data = user_data;
>  
>  return item;
>  }
> @@ -73,6 +79,9 @@ static void cursor_pipe_item_free(RedPipeItem *base)
>  RedCursorPipeItem *pipe_item = SPICE_UPCAST(RedCursorPipeItem, base);
>  
>  cursor_cmd = pipe_item->red_cursor;
> +if (pipe_item->release_command_cb) {
> +pipe_item->release_command_cb(cursor_cmd, pipe_item->user_data);
> +}
>  red_put_cursor_cmd(cursor_cmd);
>  g_free(cursor_cmd);
>  
> @@ -246,7 +255,8 @@ CursorChannel* cursor_channel_new(RedsState *server, int 
> id,
>  NULL);
>  }
>  
> -void cursor_channel_process_cmd(CursorChannel *cursor, RedCursorCmd 
> *cursor_cmd)
> +void cursor_channel_process_cmd(CursorChannel *cursor, RedCursorCmd 
> *cursor_cmd,
> +ReleaseCommandCb release_command_cb, 
> gpointer user_data)
>  {
>  RedCursorPipeItem *cursor_pipe_item;
>  bool cursor_show = false;
> @@ -254,7 +264,7 @@ void cursor_channel_process_cmd(CursorChannel *cursor, 
> RedCursorCmd *cursor_cmd)
>  spice_return_if_fail(cursor);
>  spice_return_if_fail(cursor_cmd);
>  
> -cursor_pipe_item = cursor_pipe_item_new(cursor_cmd);
> +cursor_pipe_item = cursor_pipe_item_new(cursor_cmd, release_command_cb, 
> user_data);
>  
>  switch (cursor_cmd->type) {
>  case QXL_CURSOR_SET:
> diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> index 603c2c0ac..e41e52438 100644
> --- a/server/cursor-channel.h
> +++ b/server/cursor-channel.h
> @@ -44,6 +44,8 @@ typedef struct CursorChannelClass CursorChannelClass;
>  
>  GType cursor_channel_get_type(void) G_GNUC_CONST;
>  
> +typedef void (*ReleaseCommandCb)(RedCursorCmd *command, gpointer user_data);
> +
>  /**
>   * Create CursorChannel.
>   * Since CursorChannel is intended to be run in a separate thread,
> @@ -61,7 +63,8 @@ CursorChannel* cursor_channel_new(RedsState *server, int id,
>  
>  void cursor_channel_reset   (CursorChannel *cursor);
>  void cursor_channel_do_init (CursorChannel *cursor);
> -void cursor_channel_process_cmd (CursorChannel *cursor, 
> RedCursorCmd *cursor_cmd);
> +void cursor_channel_process_cmd(CursorChannel *cursor, 
> RedCursorCmd *cursor_cmd,
> +ReleaseCommandCb 
> release_command_cb, gpointer user_data);
>  void

[Spice-devel] [spice-server] cursor: Delay release of QXL guest cursor resources

2018-04-05 Thread Christophe Fergeau

There's an implicit API/ABI contract between QEMU and SPICE that SPICE
will keep the guest QXL resources alive as long as QEMU can hold a
pointer to them. This implicit contract was broken in 1c6e7cf7 "Release
cursor as soon as possible", causing crashes at migration time.
While the proper fix would be in QEMU so that spice-server does not need
to have that kind of knowledge regarding QEMU internal implementation,
this commit reverts to the pre-1c6e7cf7 behaviour to avoid a regression
while QEMU is being fixed.

https://bugzilla.redhat.com/show_bug.cgi?id=1540919

Signed-off-by: Christophe Fergeau 
---
 server/cursor-channel.c| 16 +---
 server/cursor-channel.h|  5 -
 server/red-stream-device.c |  4 ++--
 server/red-worker.c| 10 --
 4 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index 522261e3f..a7bee9815 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -31,6 +31,8 @@
 typedef struct RedCursorPipeItem {
 RedPipeItem base;
 RedCursorCmd *red_cursor;
+ReleaseCommandCb release_command_cb;
+gpointer user_data;
 } RedCursorPipeItem;
 
 struct CursorChannel
@@ -54,7 +56,9 @@ G_DEFINE_TYPE(CursorChannel, cursor_channel, 
TYPE_COMMON_GRAPHICS_CHANNEL)
 
 static void cursor_pipe_item_free(RedPipeItem *pipe_item);
 
-static RedCursorPipeItem *cursor_pipe_item_new(RedCursorCmd *cmd)
+static RedCursorPipeItem *cursor_pipe_item_new(RedCursorCmd *cmd,
+   ReleaseCommandCb 
release_command_cb,
+   gpointer user_data)
 {
 RedCursorPipeItem *item = g_new0(RedCursorPipeItem, 1);
 
@@ -63,6 +67,8 @@ static RedCursorPipeItem *cursor_pipe_item_new(RedCursorCmd 
*cmd)
 red_pipe_item_init_full(>base, RED_PIPE_ITEM_TYPE_CURSOR,
 cursor_pipe_item_free);
 item->red_cursor = cmd;
+item->release_command_cb = release_command_cb;
+item->user_data = user_data;
 
 return item;
 }
@@ -73,6 +79,9 @@ static void cursor_pipe_item_free(RedPipeItem *base)
 RedCursorPipeItem *pipe_item = SPICE_UPCAST(RedCursorPipeItem, base);
 
 cursor_cmd = pipe_item->red_cursor;
+if (pipe_item->release_command_cb) {
+pipe_item->release_command_cb(cursor_cmd, pipe_item->user_data);
+}
 red_put_cursor_cmd(cursor_cmd);
 g_free(cursor_cmd);
 
@@ -246,7 +255,8 @@ CursorChannel* cursor_channel_new(RedsState *server, int id,
 NULL);
 }
 
-void cursor_channel_process_cmd(CursorChannel *cursor, RedCursorCmd 
*cursor_cmd)
+void cursor_channel_process_cmd(CursorChannel *cursor, RedCursorCmd 
*cursor_cmd,
+ReleaseCommandCb release_command_cb, gpointer 
user_data)
 {
 RedCursorPipeItem *cursor_pipe_item;
 bool cursor_show = false;
@@ -254,7 +264,7 @@ void cursor_channel_process_cmd(CursorChannel *cursor, 
RedCursorCmd *cursor_cmd)
 spice_return_if_fail(cursor);
 spice_return_if_fail(cursor_cmd);
 
-cursor_pipe_item = cursor_pipe_item_new(cursor_cmd);
+cursor_pipe_item = cursor_pipe_item_new(cursor_cmd, release_command_cb, 
user_data);
 
 switch (cursor_cmd->type) {
 case QXL_CURSOR_SET:
diff --git a/server/cursor-channel.h b/server/cursor-channel.h
index 603c2c0ac..e41e52438 100644
--- a/server/cursor-channel.h
+++ b/server/cursor-channel.h
@@ -44,6 +44,8 @@ typedef struct CursorChannelClass CursorChannelClass;
 
 GType cursor_channel_get_type(void) G_GNUC_CONST;
 
+typedef void (*ReleaseCommandCb)(RedCursorCmd *command, gpointer user_data);
+
 /**
  * Create CursorChannel.
  * Since CursorChannel is intended to be run in a separate thread,
@@ -61,7 +63,8 @@ CursorChannel* cursor_channel_new(RedsState *server, int id,
 
 void cursor_channel_reset   (CursorChannel *cursor);
 void cursor_channel_do_init (CursorChannel *cursor);
-void cursor_channel_process_cmd (CursorChannel *cursor, 
RedCursorCmd *cursor_cmd);
+void cursor_channel_process_cmd(CursorChannel *cursor, 
RedCursorCmd *cursor_cmd,
+ReleaseCommandCb 
release_command_cb, gpointer user_data);
 void cursor_channel_set_mouse_mode(CursorChannel *cursor, 
uint32_t mode);
 
 /**
diff --git a/server/red-stream-device.c b/server/red-stream-device.c
index e151de367..36f8b4b61 100644
--- a/server/red-stream-device.c
+++ b/server/red-stream-device.c
@@ -386,7 +386,7 @@ handle_msg_cursor_set(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 if (!cmd) {
 return handle_msg_invalid(dev, sin, NULL);
 }
-cursor_channel_process_cmd(dev->cursor_channel, cmd);
+cursor_channel_process_cmd(dev->cursor_channel, cmd, NULL, NULL);
 
 return true;
 }
@@ -413,7 +413,7 @@ handle_msg_cursor_move(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)

Re: [Spice-devel] [spice-server] cursor: Delay release of QXL guest cursor resources

Re: [Spice-devel] [spice-server] cursor: Delay release of QXL guest cursor resources

Re: [Spice-devel] [spice-server] cursor: Delay release of QXL guest cursor resources

[Spice-devel] [spice-server] cursor: Delay release of QXL guest cursor resources

4 matches

Site Navigation

Mail list logo

Footer information