Re: [Spice-devel] [PATCH 3/3] server/red_worker: fix possible leak of self_bitmap

2012-05-15 Thread Yonit Halperin 

Hi,

Instead of this patch series and the previous self_bitmap patch, I 
think we should do the following:
Both GLZDrawable and Drawable share references to RedDrawable and 
self_bitmap, and self_bitmap life time is equal to RedDrawable's one.
So we need to have a new type which warps RedDrawable and self_bitmap, 
and move the ref count from RedDrawable to this new type.
Then, Drawable and GlzDrawable will just hold a reference to 
RedDrawableWrapper, instead of two references to RedDrawable and 
self_bitmap, and they will just decrease the reference to 
RedDrawableWrapper when they are released.


Thanks,
Yonit.
On 05/14/2012 03:32 PM, Alon Levy wrote:

After the previous patch moving self_bitmap freeing inside red_drawable
ref count, we have a possible self_bitmap leak:

red_process_commands
  red_get_drawable | red_drawable #1, red_drawable-self_bitmap == 1
   red_process_drawable | rd #2, d #1, d-self_bitmap != NULL
release_drawable | rd #1, d# = 0, try to release self_bitmap, but
 blocked by rd #1
   put_red_drawable | rd #0

This patch moves the call to release_drawable after the call to
put_red_drawable, thereby fixing the above situation.
---
  server/red_worker.c |   12 +++-
  1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index e1c86fa..1adf4c9 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -3867,8 +3867,8 @@ static inline void 
red_inc_surfaces_drawable_dependencies(RedWorker *worker, Dra
  }
  }

-static inline void red_process_drawable(RedWorker *worker, RedDrawable 
*drawable,
-uint32_t group_id)
+static inline Drawable *red_process_drawable(RedWorker *worker, RedDrawable 
*drawable,
+ uint32_t group_id)
  {
  int surface_id;
  Drawable *item = get_drawable(worker, drawable-effect, drawable, 
group_id);
@@ -3931,7 +3931,7 @@ static inline void red_process_drawable(RedWorker 
*worker, RedDrawable *drawable
  #endif
  }
  cleanup:
-release_drawable(worker, item);
+return item;
  }

  static inline void red_create_surface(RedWorker *worker, uint32_t 
surface_id,uint32_t width,
@@ -4844,14 +4844,16 @@ static int red_process_commands(RedWorker *worker, 
uint32_t max_pipe_size, int *
  switch (ext_cmd.cmd.type) {
  case QXL_CMD_DRAW: {
  RedDrawable *red_drawable = red_drawable_new(); // returns with 1 
ref
+Drawable *drawable;

  if (red_get_drawable(worker-mem_slots, ext_cmd.group_id,
   red_drawable, ext_cmd.cmd.data, 
ext_cmd.flags)) {
  break;
  }
-red_process_drawable(worker, red_drawable, ext_cmd.group_id);
-// release the red_drawable
+drawable = red_process_drawable(worker, red_drawable, 
ext_cmd.group_id);
+// release red_drawable first, it will not die because drawable 
holds a reference on it.
  put_red_drawable(worker, red_drawable, ext_cmd.group_id, NULL);
+release_drawable(worker, drawable);
  break;
  }
  case QXL_CMD_UPDATE: {


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 3/3] server/red_worker: fix possible leak of self_bitmap

2012-05-15 Thread Alon Levy 
On Tue, May 15, 2012 at 11:57:05AM +0300, Yonit Halperin wrote:
 Hi,
 
 Instead of this patch series and the previous self_bitmap patch, I think
 we should do the following:
 Both GLZDrawable and Drawable share references to RedDrawable and
 self_bitmap, and self_bitmap life time is equal to RedDrawable's one.
 So we need to have a new type which warps RedDrawable and self_bitmap, and
 move the ref count from RedDrawable to this new type.
 Then, Drawable and GlzDrawable will just hold a reference to
 RedDrawableWrapper, instead of two references to RedDrawable and
 self_bitmap, and they will just decrease the reference to RedDrawableWrapper
 when they are released.

Why can't we have GlzDrawable reference Drawable?

 
 Thanks,
 Yonit.
 On 05/14/2012 03:32 PM, Alon Levy wrote:
 After the previous patch moving self_bitmap freeing inside red_drawable
 ref count, we have a possible self_bitmap leak:
 
 red_process_commands
   red_get_drawable | red_drawable #1, red_drawable-self_bitmap == 1
red_process_drawable | rd #2, d #1, d-self_bitmap != NULL
 release_drawable | rd #1, d# = 0, try to release self_bitmap, but
  blocked by rd #1
put_red_drawable | rd #0
 
 This patch moves the call to release_drawable after the call to
 put_red_drawable, thereby fixing the above situation.
 ---
   server/red_worker.c |   12 +++-
   1 file changed, 7 insertions(+), 5 deletions(-)
 
 diff --git a/server/red_worker.c b/server/red_worker.c
 index e1c86fa..1adf4c9 100644
 --- a/server/red_worker.c
 +++ b/server/red_worker.c
 @@ -3867,8 +3867,8 @@ static inline void 
 red_inc_surfaces_drawable_dependencies(RedWorker *worker, Dra
   }
   }
 
 -static inline void red_process_drawable(RedWorker *worker, RedDrawable 
 *drawable,
 -uint32_t group_id)
 +static inline Drawable *red_process_drawable(RedWorker *worker, RedDrawable 
 *drawable,
 + uint32_t group_id)
   {
   int surface_id;
   Drawable *item = get_drawable(worker, drawable-effect, drawable, 
  group_id);
 @@ -3931,7 +3931,7 @@ static inline void red_process_drawable(RedWorker 
 *worker, RedDrawable *drawable
   #endif
   }
   cleanup:
 -release_drawable(worker, item);
 +return item;
   }
 
   static inline void red_create_surface(RedWorker *worker, uint32_t 
  surface_id,uint32_t width,
 @@ -4844,14 +4844,16 @@ static int red_process_commands(RedWorker *worker, 
 uint32_t max_pipe_size, int *
   switch (ext_cmd.cmd.type) {
   case QXL_CMD_DRAW: {
   RedDrawable *red_drawable = red_drawable_new(); // returns 
  with 1 ref
 +Drawable *drawable;
 
   if (red_get_drawable(worker-mem_slots, ext_cmd.group_id,
red_drawable, ext_cmd.cmd.data, 
  ext_cmd.flags)) {
   break;
   }
 -red_process_drawable(worker, red_drawable, ext_cmd.group_id);
 -// release the red_drawable
 +drawable = red_process_drawable(worker, red_drawable, 
 ext_cmd.group_id);
 +// release red_drawable first, it will not die because drawable 
 holds a reference on it.
   put_red_drawable(worker, red_drawable, ext_cmd.group_id, NULL);
 +release_drawable(worker, drawable);
   break;
   }
   case QXL_CMD_UPDATE: {
 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 3/3] server/red_worker: fix possible leak of self_bitmap

2012-05-15 Thread Uri Lublin 

On 05/15/2012 11:57 AM, Yonit Halperin wrote:

Hi,

Instead of this patch series and the previous self_bitmap patch, I 
think we should do the following:
Both GLZDrawable and Drawable share references to RedDrawable and 
self_bitmap, and self_bitmap life time is equal to RedDrawable's one.
So we need to have a new type which warps RedDrawable and self_bitmap, 
and move the ref count from RedDrawable to this new type.
Then, Drawable and GlzDrawable will just hold a reference to 
RedDrawableWrapper, instead of two references to RedDrawable and 
self_bitmap, and they will just decrease the reference to 
RedDrawableWrapper when they are released.


Can we just add self_bitmap into RedDrawable ?


___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 3/3] server/red_worker: fix possible leak of self_bitmap

2012-05-15 Thread Yonit Halperin 

On 05/15/2012 12:06 PM, Alon Levy wrote:

On Tue, May 15, 2012 at 11:57:05AM +0300, Yonit Halperin wrote:

Hi,

Instead of this patch series and the previous self_bitmap patch, I think
we should do the following:
Both GLZDrawable and Drawable share references to RedDrawable and
self_bitmap, and self_bitmap life time is equal to RedDrawable's one.
So we need to have a new type which warps RedDrawable and self_bitmap, and
move the ref count from RedDrawable to this new type.
Then, Drawable and GlzDrawable will just hold a reference to
RedDrawableWrapper, instead of two references to RedDrawable and
self_bitmap, and they will just decrease the reference to RedDrawableWrapper
when they are released.


Why can't we have GlzDrawable reference Drawable?

Currently Drawables are allocated on the stack. Making GLZDrawable and 
Drawable life time dependent, will lead to more calls for 
free_one_drawable, and will limit the glz dictionary as well. In 
addition, the current code assumes GLZDrawable and Drawable are 
independent, while RedDrawable life time is dependent on both of them. 
Making the change you suggest will require more risky refactoring to the 
worker.

Maybe we should move Drawables to the heap, as we already discussed,
and limit the drawables allocation not by number, but by the size their 
corresponding qxl drawables occupy in the device RAM. However, I think 
this should be done gradually, after fixing the self_bitmap double 
release bug.




Thanks,
Yonit.
On 05/14/2012 03:32 PM, Alon Levy wrote:

After the previous patch moving self_bitmap freeing inside red_drawable
ref count, we have a possible self_bitmap leak:

red_process_commands
  red_get_drawable | red_drawable #1, red_drawable-self_bitmap == 1
   red_process_drawable | rd #2, d #1, d-self_bitmap != NULL
release_drawable | rd #1, d# = 0, try to release self_bitmap, but
 blocked by rd #1
   put_red_drawable | rd #0

This patch moves the call to release_drawable after the call to
put_red_drawable, thereby fixing the above situation.
---
  server/red_worker.c |   12 +++-
  1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index e1c86fa..1adf4c9 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -3867,8 +3867,8 @@ static inline void 
red_inc_surfaces_drawable_dependencies(RedWorker *worker, Dra
  }
  }

-static inline void red_process_drawable(RedWorker *worker, RedDrawable 
*drawable,
-uint32_t group_id)
+static inline Drawable *red_process_drawable(RedWorker *worker, RedDrawable 
*drawable,
+ uint32_t group_id)
  {
  int surface_id;
  Drawable *item = get_drawable(worker, drawable-effect, drawable, 
group_id);
@@ -3931,7 +3931,7 @@ static inline void red_process_drawable(RedWorker 
*worker, RedDrawable *drawable
  #endif
  }
  cleanup:
-release_drawable(worker, item);
+return item;
  }

  static inline void red_create_surface(RedWorker *worker, uint32_t 
surface_id,uint32_t width,
@@ -4844,14 +4844,16 @@ static int red_process_commands(RedWorker *worker, 
uint32_t max_pipe_size, int *
  switch (ext_cmd.cmd.type) {
  case QXL_CMD_DRAW: {
  RedDrawable *red_drawable = red_drawable_new(); // returns with 1 
ref
+Drawable *drawable;

  if (red_get_drawable(worker-mem_slots, ext_cmd.group_id,
   red_drawable, ext_cmd.cmd.data, 
ext_cmd.flags)) {
  break;
  }
-red_process_drawable(worker, red_drawable, ext_cmd.group_id);
-// release the red_drawable
+drawable = red_process_drawable(worker, red_drawable, 
ext_cmd.group_id);
+// release red_drawable first, it will not die because drawable 
holds a reference on it.
  put_red_drawable(worker, red_drawable, ext_cmd.group_id, NULL);
+release_drawable(worker, drawable);
  break;
  }
  case QXL_CMD_UPDATE: {




___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 3/3] server/red_worker: fix possible leak of self_bitmap

2012-05-15 Thread Alon Levy 
On Tue, May 15, 2012 at 12:27:24PM +0300, Yonit Halperin wrote:
 On 05/15/2012 12:06 PM, Alon Levy wrote:
 On Tue, May 15, 2012 at 11:57:05AM +0300, Yonit Halperin wrote:
 Hi,
 
 Instead of this patch series and the previous self_bitmap patch, I think
 we should do the following:
 Both GLZDrawable and Drawable share references to RedDrawable and
 self_bitmap, and self_bitmap life time is equal to RedDrawable's one.
 So we need to have a new type which warps RedDrawable and self_bitmap, and
 move the ref count from RedDrawable to this new type.
 Then, Drawable and GlzDrawable will just hold a reference to
 RedDrawableWrapper, instead of two references to RedDrawable and
 self_bitmap, and they will just decrease the reference to RedDrawableWrapper
 when they are released.
 
 Why can't we have GlzDrawable reference Drawable?
 
 Currently Drawables are allocated on the stack. Making GLZDrawable and
 Drawable life time dependent, will lead to more calls for free_one_drawable,
 and will limit the glz dictionary as well. In addition, the current code
 assumes GLZDrawable and Drawable are independent, while RedDrawable life
 time is dependent on both of them. Making the change you suggest will
 require more risky refactoring to the worker.
 Maybe we should move Drawables to the heap, as we already discussed,
 and limit the drawables allocation not by number, but by the size their
 corresponding qxl drawables occupy in the device RAM. However, I think this
 should be done gradually, after fixing the self_bitmap double release bug.

OK, I'll do a new patch with self_bitmap moves to RedDrawable.

 
 
 Thanks,
 Yonit.
 On 05/14/2012 03:32 PM, Alon Levy wrote:
 After the previous patch moving self_bitmap freeing inside red_drawable
 ref count, we have a possible self_bitmap leak:
 
 red_process_commands
   red_get_drawable | red_drawable #1, red_drawable-self_bitmap == 1
red_process_drawable | rd #2, d #1, d-self_bitmap != NULL
 release_drawable | rd #1, d# = 0, try to release self_bitmap, but
  blocked by rd #1
put_red_drawable | rd #0
 
 This patch moves the call to release_drawable after the call to
 put_red_drawable, thereby fixing the above situation.
 ---
   server/red_worker.c |   12 +++-
   1 file changed, 7 insertions(+), 5 deletions(-)
 
 diff --git a/server/red_worker.c b/server/red_worker.c
 index e1c86fa..1adf4c9 100644
 --- a/server/red_worker.c
 +++ b/server/red_worker.c
 @@ -3867,8 +3867,8 @@ static inline void 
 red_inc_surfaces_drawable_dependencies(RedWorker *worker, Dra
   }
   }
 
 -static inline void red_process_drawable(RedWorker *worker, RedDrawable 
 *drawable,
 -uint32_t group_id)
 +static inline Drawable *red_process_drawable(RedWorker *worker, 
 RedDrawable *drawable,
 + uint32_t group_id)
   {
   int surface_id;
   Drawable *item = get_drawable(worker, drawable-effect, drawable, 
  group_id);
 @@ -3931,7 +3931,7 @@ static inline void red_process_drawable(RedWorker 
 *worker, RedDrawable *drawable
   #endif
   }
   cleanup:
 -release_drawable(worker, item);
 +return item;
   }
 
   static inline void red_create_surface(RedWorker *worker, uint32_t 
  surface_id,uint32_t width,
 @@ -4844,14 +4844,16 @@ static int red_process_commands(RedWorker *worker, 
 uint32_t max_pipe_size, int *
   switch (ext_cmd.cmd.type) {
   case QXL_CMD_DRAW: {
   RedDrawable *red_drawable = red_drawable_new(); // returns 
  with 1 ref
 +Drawable *drawable;
 
   if (red_get_drawable(worker-mem_slots, ext_cmd.group_id,
red_drawable, ext_cmd.cmd.data, 
  ext_cmd.flags)) {
   break;
   }
 -red_process_drawable(worker, red_drawable, ext_cmd.group_id);
 -// release the red_drawable
 +drawable = red_process_drawable(worker, red_drawable, 
 ext_cmd.group_id);
 +// release red_drawable first, it will not die because 
 drawable holds a reference on it.
   put_red_drawable(worker, red_drawable, ext_cmd.group_id, 
  NULL);
 +release_drawable(worker, drawable);
   break;
   }
   case QXL_CMD_UPDATE: {
 
 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 3/3] server/red_worker: fix possible leak of self_bitmap

2012-05-15 Thread Alon Levy 
On Tue, May 15, 2012 at 12:18:44PM +0300, Uri Lublin wrote:
 On 05/15/2012 11:57 AM, Yonit Halperin wrote:
 Hi,
 
 Instead of this patch series and the previous self_bitmap patch, I think
 we should do the following:
 Both GLZDrawable and Drawable share references to RedDrawable and
 self_bitmap, and self_bitmap life time is equal to RedDrawable's one.
 So we need to have a new type which warps RedDrawable and self_bitmap, and
 move the ref count from RedDrawable to this new type.
 Then, Drawable and GlzDrawable will just hold a reference to
 RedDrawableWrapper, instead of two references to RedDrawable and
 self_bitmap, and they will just decrease the reference to
 RedDrawableWrapper when they are released.
 
 Can we just add self_bitmap into RedDrawable ?

Just sent patches that do that.

 
 
 ___
 Spice-devel mailing list
 Spice-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH 3/3] server/red_worker: fix possible leak of self_bitmap

2012-05-14 Thread Alon Levy 
After the previous patch moving self_bitmap freeing inside red_drawable
ref count, we have a possible self_bitmap leak:

red_process_commands
 red_get_drawable | red_drawable #1, red_drawable-self_bitmap == 1
  red_process_drawable | rd #2, d #1, d-self_bitmap != NULL
   release_drawable | rd #1, d# = 0, try to release self_bitmap, but
blocked by rd #1
  put_red_drawable | rd #0

This patch moves the call to release_drawable after the call to
put_red_drawable, thereby fixing the above situation.
---
 server/red_worker.c |   12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/server/red_worker.c b/server/red_worker.c
index e1c86fa..1adf4c9 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -3867,8 +3867,8 @@ static inline void 
red_inc_surfaces_drawable_dependencies(RedWorker *worker, Dra
 }
 }
 
-static inline void red_process_drawable(RedWorker *worker, RedDrawable 
*drawable,
-uint32_t group_id)
+static inline Drawable *red_process_drawable(RedWorker *worker, RedDrawable 
*drawable,
+ uint32_t group_id)
 {
 int surface_id;
 Drawable *item = get_drawable(worker, drawable-effect, drawable, 
group_id);
@@ -3931,7 +3931,7 @@ static inline void red_process_drawable(RedWorker 
*worker, RedDrawable *drawable
 #endif
 }
 cleanup:
-release_drawable(worker, item);
+return item;
 }
 
 static inline void red_create_surface(RedWorker *worker, uint32_t 
surface_id,uint32_t width,
@@ -4844,14 +4844,16 @@ static int red_process_commands(RedWorker *worker, 
uint32_t max_pipe_size, int *
 switch (ext_cmd.cmd.type) {
 case QXL_CMD_DRAW: {
 RedDrawable *red_drawable = red_drawable_new(); // returns with 1 
ref
+Drawable *drawable;
 
 if (red_get_drawable(worker-mem_slots, ext_cmd.group_id,
  red_drawable, ext_cmd.cmd.data, 
ext_cmd.flags)) {
 break;
 }
-red_process_drawable(worker, red_drawable, ext_cmd.group_id);
-// release the red_drawable
+drawable = red_process_drawable(worker, red_drawable, 
ext_cmd.group_id);
+// release red_drawable first, it will not die because drawable 
holds a reference on it.
 put_red_drawable(worker, red_drawable, ext_cmd.group_id, NULL);
+release_drawable(worker, drawable);
 break;
 }
 case QXL_CMD_UPDATE: {
-- 
1.7.10.1

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel