[PATCH 00/11] Miscellaneous stability patches

[PATCH 00/11] Miscellaneous stability patches

[Frediano Ziglio]

This set of patches mainly contains fix for some memory issues
using quite aggressively surfaces and other minor problems like
images going black after a while.

Frediano Ziglio (11):
  Do not cause spice-server to clean our objects
  Do not leak memory if qxl_release_list_add fails
  Fix print statement not using uninitialized variable
  Avoid double free on error
  Handle all errors in qxl_surface_evict
  Fix return for qxl_release_alloc
  Handle correctly failures in qxl_alloc_relase_reserved
  Remove format string errors
  Move main reference counter to GEM object instead of TTM ones
  Simplify cleaning qxl processing command
  Propagate correctly errors from qxlhw_handle_to_bo

 qxl/qxl_cmd.c     | 11 ++++++-----
 qxl/qxl_display.c |  2 +-
 qxl/qxl_drv.h     |  2 +-
 qxl/qxl_gem.c     | 10 ++++++++--
 qxl/qxl_ioctl.c   | 46 +++++++++++++++++-----------------------------
 qxl/qxl_object.c  | 11 ++++-------
 qxl/qxl_release.c | 13 +++++++++----
 7 files changed, 46 insertions(+), 49 deletions(-)

-- 
2.1.0

[PATCH 01/11] Do not cause spice-server to clean our objects

[Frediano Ziglio]

If objects are moved back from system memory to VRAM (and spice id
created again) memory is already initialized so we need to set flag
to not clear memory.
If you don't do it after a while using desktop many images turns to
black or transparents.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 qxl/qxl_cmd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qxl/qxl_cmd.c b/qxl/qxl_cmd.c
index bd5404e..85ed5dc 100644
--- a/qxl/qxl_cmd.c
+++ b/qxl/qxl_cmd.c
@@ -502,6 +502,7 @@ int qxl_hw_surface_alloc(struct qxl_device *qdev,
 
 	cmd = (struct qxl_surface_cmd *)qxl_release_map(qdev, release);
 	cmd->type = QXL_SURFACE_CMD_CREATE;
+	cmd->flags = QXL_SURF_FLAG_KEEP_DATA;
 	cmd->u.surface_create.format = surf->surf.format;
 	cmd->u.surface_create.width = surf->surf.width;
 	cmd->u.surface_create.height = surf->surf.height;
-- 
2.1.0

Re: [Spice-devel] [PATCH 01/11] Do not cause spice-server to clean our objects

[Dave Airlie]

On 27 May 2015 at 20:03, Frediano Ziglio <fziglio@redhat.com> wrote:
> If objects are moved back from system memory to VRAM (and spice id
> created again) memory is already initialized so we need to set flag
> to not clear memory.
> If you don't do it after a while using desktop many images turns to
> black or transparents.

Good point,

Reviewed-by: Dave Airlie <airlied@redhat.com>
>
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  qxl/qxl_cmd.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/qxl/qxl_cmd.c b/qxl/qxl_cmd.c
> index bd5404e..85ed5dc 100644
> --- a/qxl/qxl_cmd.c
> +++ b/qxl/qxl_cmd.c
> @@ -502,6 +502,7 @@ int qxl_hw_surface_alloc(struct qxl_device *qdev,
>
>         cmd = (struct qxl_surface_cmd *)qxl_release_map(qdev, release);
>         cmd->type = QXL_SURFACE_CMD_CREATE;
> +       cmd->flags = QXL_SURF_FLAG_KEEP_DATA;
>         cmd->u.surface_create.format = surf->surf.format;
>         cmd->u.surface_create.width = surf->surf.width;
>         cmd->u.surface_create.height = surf->surf.height;
> --
> 2.1.0
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

[PATCH 02/11] Do not leak memory if qxl_release_list_add fails

[Frediano Ziglio]

If the function fails reference counter to the object is not decremented
causing leaks.
This is hard to spot as it happens only on very low memory situations.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 qxl/qxl_ioctl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
index afd7297..e8b5207 100644
--- a/qxl/qxl_ioctl.c
+++ b/qxl/qxl_ioctl.c
@@ -122,8 +122,10 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
 	qobj = gem_to_qxl_bo(gobj);
 
 	ret = qxl_release_list_add(release, qobj);
-	if (ret)
+	if (ret) {
+		drm_gem_object_unreference_unlocked(gobj);
 		return NULL;
+	}
 
 	return qobj;
 }
-- 
2.1.0

Re: [Spice-devel] [PATCH 02/11] Do not leak memory if qxl_release_list_add fails

[Dave Airlie]

On 27 May 2015 at 20:03, Frediano Ziglio <fziglio@redhat.com> wrote:
> If the function fails reference counter to the object is not decremented
> causing leaks.
> This is hard to spot as it happens only on very low memory situations.
>
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>

Looks good,

Reviewed-by: Dave Airlie <airlied@redhat.com>

Dave.

[PATCH 03/11] Fix print statement not using uninitialized variable

[Frediano Ziglio]

reloc_info[i] is not still initialized in the print statement.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 qxl/qxl_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
index e8b5207..230ab84 100644
--- a/qxl/qxl_ioctl.c
+++ b/qxl/qxl_ioctl.c
@@ -212,7 +212,7 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 		/* add the bos to the list of bos to validate -
 		   need to validate first then process relocs? */
 		if (reloc.reloc_type != QXL_RELOC_TYPE_BO && reloc.reloc_type != QXL_RELOC_TYPE_SURF) {
-			DRM_DEBUG("unknown reloc type %d\n", reloc_info[i].type);
+			DRM_DEBUG("unknown reloc type %d\n", reloc.reloc_type);
 
 			ret = -EINVAL;
 			goto out_free_bos;
-- 
2.1.0

Re: [Spice-devel] [PATCH 03/11] Fix print statement not using uninitialized variable

[Dave Airlie]

On 27 May 2015 at 20:03, Frediano Ziglio <fziglio@redhat.com> wrote:
> reloc_info[i] is not still initialized in the print statement.
>
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>

Reviewed-by: Dave Airlie <airlied@redhat.com>

[PATCH 04/11] Avoid double free on error

[Frediano Ziglio]

Is we are not able to get source bo object from handle we free
destination bo object and call cleanup code however destination
object was already inserted in reloc_info array (num_relocs was
already incremented) so on cleanup we free destination again.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 qxl/qxl_ioctl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
index 230ab84..85b3808 100644
--- a/qxl/qxl_ioctl.c
+++ b/qxl/qxl_ioctl.c
@@ -240,8 +240,6 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 				qxlhw_handle_to_bo(qdev, file_priv,
 						   reloc.src_handle, release);
 			if (!reloc_info[i].src_bo) {
-				if (reloc_info[i].dst_bo != cmd_bo)
-					drm_gem_object_unreference_unlocked(&reloc_info[i].dst_bo->gem_base);
 				ret = -EINVAL;
 				goto out_free_bos;
 			}
-- 
2.1.0

Re: [Spice-devel] [PATCH 04/11] Avoid double free on error

[Dave Airlie]

On 27 May 2015 at 20:03, Frediano Ziglio <fziglio@redhat.com> wrote:
> Is we are not able to get source bo object from handle we free
> destination bo object and call cleanup code however destination
> object was already inserted in reloc_info array (num_relocs was
> already incremented) so on cleanup we free destination again.
>
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>

Reviewed-by: Dave Airlie <airlied@redhat.com>

[PATCH 05/11] Handle all errors in qxl_surface_evict

[Frediano Ziglio]

Only EBUSY error was handled. This could cause code to believe
reserve was successful while it failed.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 qxl/qxl_cmd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/qxl/qxl_cmd.c b/qxl/qxl_cmd.c
index 85ed5dc..b18f84c 100644
--- a/qxl/qxl_cmd.c
+++ b/qxl/qxl_cmd.c
@@ -612,8 +612,8 @@ static int qxl_reap_surf(struct qxl_device *qdev, struct qxl_bo *surf, bool stal
 	int ret;
 
 	ret = qxl_bo_reserve(surf, false);
-	if (ret == -EBUSY)
-		return -EBUSY;
+	if (ret)
+		return ret;
 
 	if (stall)
 		mutex_unlock(&qdev->surf_evict_mutex);
@@ -622,9 +622,9 @@ static int qxl_reap_surf(struct qxl_device *qdev, struct qxl_bo *surf, bool stal
 
 	if (stall)
 		mutex_lock(&qdev->surf_evict_mutex);
-	if (ret == -EBUSY) {
+	if (ret) {
 		qxl_bo_unreserve(surf);
-		return -EBUSY;
+		return ret;
 	}
 
 	qxl_surface_evict_locked(qdev, surf, true);
-- 
2.1.0

Re: [Spice-devel] [PATCH 05/11] Handle all errors in qxl_surface_evict

[Dave Airlie]

On 27 May 2015 at 20:04, Frediano Ziglio <fziglio@redhat.com> wrote:
> Only EBUSY error was handled. This could cause code to believe
> reserve was successful while it failed.
>
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>

This has been there since I wrote qxl, so I expect I had some reason
for it, but I can't remember it
now..

Most likely is something had the bo reserved already and we reentered
from somewhere we shouldn't
but I think a lot of the horrible code went away.

so because I can't justify the hack,
Reviewed-by: Dave Airlie <airlied@redhat.com>
Dave.

[PATCH 06/11] Fix return for qxl_release_alloc

[Frediano Ziglio]

This function return handle to allocated release object which is an int.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 qxl/qxl_release.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qxl/qxl_release.c b/qxl/qxl_release.c
index d9b2568..6fd8e50 100644
--- a/qxl/qxl_release.c
+++ b/qxl/qxl_release.c
@@ -122,7 +122,7 @@ static const struct fence_ops qxl_fence_ops = {
 	.wait = qxl_fence_wait,
 };
 
-static uint64_t
+static int
 qxl_release_alloc(struct qxl_device *qdev, int type,
 		  struct qxl_release **ret)
 {
-- 
2.1.0

Re: [Spice-devel] [PATCH 06/11] Fix return for qxl_release_alloc

[Dave Airlie]

On 27 May 2015 at 20:04, Frediano Ziglio <fziglio@redhat.com> wrote:
> This function return handle to allocated release object which is an int.

Reviewed-by: Dave Airlie <airlied@redhat.com>
>
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  qxl/qxl_release.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qxl/qxl_release.c b/qxl/qxl_release.c
> index d9b2568..6fd8e50 100644
> --- a/qxl/qxl_release.c
> +++ b/qxl/qxl_release.c
> @@ -122,7 +122,7 @@ static const struct fence_ops qxl_fence_ops = {
>         .wait = qxl_fence_wait,
>  };
>
> -static uint64_t
> +static int
>  qxl_release_alloc(struct qxl_device *qdev, int type,
>                   struct qxl_release **ret)
>  {
> --
> 2.1.0
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

[PATCH 07/11] Handle correctly failures in qxl_alloc_relase_reserved

[Frediano Ziglio]

Free resources correctly if function fails

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 qxl/qxl_release.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/qxl/qxl_release.c b/qxl/qxl_release.c
index 6fd8e50..00604ed 100644
--- a/qxl/qxl_release.c
+++ b/qxl/qxl_release.c
@@ -363,6 +363,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
 		ret = qxl_release_bo_alloc(qdev, &qdev->current_release_bo[cur_idx]);
 		if (ret) {
 			mutex_unlock(&qdev->release_mutex);
+			qxl_release_free(qdev, *release);
 			return ret;
 		}
 	}
@@ -377,13 +378,17 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
 
 	mutex_unlock(&qdev->release_mutex);
 
-	qxl_release_list_add(*release, bo);
+	ret = qxl_release_list_add(*release, bo);
+	qxl_bo_unref(&bo);
+	if (ret) {
+		qxl_release_free(qdev, *release);
+		return ret;
+	}
 
 	info = qxl_release_map(qdev, *release);
 	info->id = idr_ret;
 	qxl_release_unmap(qdev, *release, info);
 
-	qxl_bo_unref(&bo);
 	return ret;
 }
 
-- 
2.1.0

Re: [Spice-devel] [PATCH 07/11] Handle correctly failures in qxl_alloc_relase_reserved

[Dave Airlie]

On 27 May 2015 at 20:04, Frediano Ziglio <fziglio@redhat.com> wrote:
> Free resources correctly if function fails
>
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>

Reviewed-by: Dave Airlie <airlied@redhat.com>

[PATCH 08/11] Remove format string errors

[Frediano Ziglio]

Enable format string checks for qxl_io_log and remove resulting warnings
which could lead to memory errors on different platform or just printing
wrong information.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 qxl/qxl_cmd.c     | 2 +-
 qxl/qxl_display.c | 2 +-
 qxl/qxl_drv.h     | 2 +-
 qxl/qxl_release.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/qxl/qxl_cmd.c b/qxl/qxl_cmd.c
index b18f84c..edc1eec 100644
--- a/qxl/qxl_cmd.c
+++ b/qxl/qxl_cmd.c
@@ -248,7 +248,7 @@ int qxl_garbage_collect(struct qxl_device *qdev)
 		}
 	}
 
-	QXL_INFO(qdev, "%s: %lld\n", __func__, i);
+	QXL_INFO(qdev, "%s: %d\n", __func__, i);
 
 	return i;
 }
diff --git a/qxl/qxl_display.c b/qxl/qxl_display.c
index 4a0a8b2..a8dbb3e 100644
--- a/qxl/qxl_display.c
+++ b/qxl/qxl_display.c
@@ -67,7 +67,7 @@ static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
 	crc = crc32(0, (const uint8_t *)&qdev->rom->client_monitors_config,
 		  sizeof(qdev->rom->client_monitors_config));
 	if (crc != qdev->rom->client_monitors_config_crc) {
-		qxl_io_log(qdev, "crc mismatch: have %X (%d) != %X\n", crc,
+		qxl_io_log(qdev, "crc mismatch: have %X (%zd) != %X\n", crc,
 			   sizeof(qdev->rom->client_monitors_config),
 			   qdev->rom->client_monitors_config_crc);
 		return 1;
diff --git a/qxl/qxl_drv.h b/qxl/qxl_drv.h
index 6745c44..62ef8be 100644
--- a/qxl/qxl_drv.h
+++ b/qxl/qxl_drv.h
@@ -328,7 +328,7 @@ struct qxl_device {
 };
 
 /* forward declaration for QXL_INFO_IO */
-void qxl_io_log(struct qxl_device *qdev, const char *fmt, ...);
+__printf(2,3) void qxl_io_log(struct qxl_device *qdev, const char *fmt, ...);
 
 extern const struct drm_ioctl_desc qxl_ioctls[];
 extern int qxl_max_ioctl;
diff --git a/qxl/qxl_release.c b/qxl/qxl_release.c
index 00604ed..b66ec33 100644
--- a/qxl/qxl_release.c
+++ b/qxl/qxl_release.c
@@ -153,7 +153,7 @@ qxl_release_alloc(struct qxl_device *qdev, int type,
 		return handle;
 	}
 	*ret = release;
-	QXL_INFO(qdev, "allocated release %lld\n", handle);
+	QXL_INFO(qdev, "allocated release %d\n", handle);
 	release->id = handle;
 	return handle;
 }
-- 
2.1.0

Re: [Spice-devel] [PATCH 08/11] Remove format string errors

[Dave Airlie]

On 27 May 2015 at 20:04, Frediano Ziglio <fziglio@redhat.com> wrote:
> Enable format string checks for qxl_io_log and remove resulting warnings
> which could lead to memory errors on different platform or just printing
> wrong information.
>
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>

Reviewed-by: Dave Airlie <airlied@redhat.com>
> ---
>  qxl/qxl_cmd.c     | 2 +-
>  qxl/qxl_display.c | 2 +-
>  qxl/qxl_drv.h     | 2 +-
>  qxl/qxl_release.c | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/qxl/qxl_cmd.c b/qxl/qxl_cmd.c
> index b18f84c..edc1eec 100644
> --- a/qxl/qxl_cmd.c
> +++ b/qxl/qxl_cmd.c
> @@ -248,7 +248,7 @@ int qxl_garbage_collect(struct qxl_device *qdev)
>                 }
>         }
>
> -       QXL_INFO(qdev, "%s: %lld\n", __func__, i);
> +       QXL_INFO(qdev, "%s: %d\n", __func__, i);
>
>         return i;
>  }
> diff --git a/qxl/qxl_display.c b/qxl/qxl_display.c
> index 4a0a8b2..a8dbb3e 100644
> --- a/qxl/qxl_display.c
> +++ b/qxl/qxl_display.c
> @@ -67,7 +67,7 @@ static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
>         crc = crc32(0, (const uint8_t *)&qdev->rom->client_monitors_config,
>                   sizeof(qdev->rom->client_monitors_config));
>         if (crc != qdev->rom->client_monitors_config_crc) {
> -               qxl_io_log(qdev, "crc mismatch: have %X (%d) != %X\n", crc,
> +               qxl_io_log(qdev, "crc mismatch: have %X (%zd) != %X\n", crc,
>                            sizeof(qdev->rom->client_monitors_config),
>                            qdev->rom->client_monitors_config_crc);
>                 return 1;
> diff --git a/qxl/qxl_drv.h b/qxl/qxl_drv.h
> index 6745c44..62ef8be 100644
> --- a/qxl/qxl_drv.h
> +++ b/qxl/qxl_drv.h
> @@ -328,7 +328,7 @@ struct qxl_device {
>  };
>
>  /* forward declaration for QXL_INFO_IO */
> -void qxl_io_log(struct qxl_device *qdev, const char *fmt, ...);
> +__printf(2,3) void qxl_io_log(struct qxl_device *qdev, const char *fmt, ...);
>
>  extern const struct drm_ioctl_desc qxl_ioctls[];
>  extern int qxl_max_ioctl;
> diff --git a/qxl/qxl_release.c b/qxl/qxl_release.c
> index 00604ed..b66ec33 100644
> --- a/qxl/qxl_release.c
> +++ b/qxl/qxl_release.c
> @@ -153,7 +153,7 @@ qxl_release_alloc(struct qxl_device *qdev, int type,
>                 return handle;
>         }
>         *ret = release;
> -       QXL_INFO(qdev, "allocated release %lld\n", handle);
> +       QXL_INFO(qdev, "allocated release %d\n", handle);
>         release->id = handle;
>         return handle;
>  }
> --
> 2.1.0
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

[PATCH 09/11] Move main reference counter to GEM object instead of TTM ones

[Frediano Ziglio]

qxl_bo structure has two reference counters, one in the GEM object and
another in the TTM object. The GEM object keep a counter to the TTM object
so when GEM counter reached zero the TTM counter (using qxl_bo_unref) was
decremented. The qxl object is fully freed (both GEM and TTM part are cleaned)
when the TTM counter reach zero.
One issue was that surface idr structure has no owning on qxl_bo objects however
it contains a pointer to qxl_bo object. This caused some nasty race condition
for instance qxl_bo object was reaped even after counter was already zero.
This patch fix these races moving main counter (the one used by qxl_bo_(un)ref)
to GEM object which cleanup routine (qxl_gem_object_free) remove the idr pointer
(using qxl_surface_evict) when the counters are still valid.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 qxl/qxl_gem.c    | 10 ++++++++--
 qxl/qxl_object.c | 11 ++++-------
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/qxl/qxl_gem.c b/qxl/qxl_gem.c
index b96f0c9..d9746e9 100644
--- a/qxl/qxl_gem.c
+++ b/qxl/qxl_gem.c
@@ -31,9 +31,15 @@
 void qxl_gem_object_free(struct drm_gem_object *gobj)
 {
 	struct qxl_bo *qobj = gem_to_qxl_bo(gobj);
+	struct qxl_device *qdev;
+	struct ttm_buffer_object *tbo;
 
-	if (qobj)
-		qxl_bo_unref(&qobj);
+	qdev = (struct qxl_device *)gobj->dev->dev_private;
+
+	qxl_surface_evict(qdev, qobj, false);
+
+	tbo = &qobj->tbo;
+	ttm_bo_unref(&tbo);
 }
 
 int qxl_gem_object_create(struct qxl_device *qdev, int size,
diff --git a/qxl/qxl_object.c b/qxl/qxl_object.c
index cdeaf08..6d6f33d 100644
--- a/qxl/qxl_object.c
+++ b/qxl/qxl_object.c
@@ -208,19 +208,16 @@ void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev,
 
 void qxl_bo_unref(struct qxl_bo **bo)
 {
-	struct ttm_buffer_object *tbo;
-
 	if ((*bo) == NULL)
 		return;
-	tbo = &((*bo)->tbo);
-	ttm_bo_unref(&tbo);
-	if (tbo == NULL)
-		*bo = NULL;
+
+	drm_gem_object_unreference_unlocked(&(*bo)->gem_base);
+	*bo = NULL;
 }
 
 struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo)
 {
-	ttm_bo_reference(&bo->tbo);
+	drm_gem_object_reference(&bo->gem_base);
 	return bo;
 }
 
-- 
2.1.0

Re: [Spice-devel] [PATCH 09/11] Move main reference counter to GEM object instead of TTM ones

[Dave Airlie]

On 27 May 2015 at 20:04, Frediano Ziglio <fziglio@redhat.com> wrote:
> qxl_bo structure has two reference counters, one in the GEM object and
> another in the TTM object. The GEM object keep a counter to the TTM object
> so when GEM counter reached zero the TTM counter (using qxl_bo_unref) was
> decremented. The qxl object is fully freed (both GEM and TTM part are cleaned)
> when the TTM counter reach zero.
> One issue was that surface idr structure has no owning on qxl_bo objects however
> it contains a pointer to qxl_bo object. This caused some nasty race condition
> for instance qxl_bo object was reaped even after counter was already zero.
> This patch fix these races moving main counter (the one used by qxl_bo_(un)ref)
> to GEM object which cleanup routine (qxl_gem_object_free) remove the idr pointer
> (using qxl_surface_evict) when the counters are still valid.

Uggh, but yes, not sure I like this fix for the problem, but if it works,

Reviewed-by: Dave Airlie <airlied@redhat.com>

Re: [Spice-devel] [PATCH 09/11] Move main reference counter to GEM object instead of TTM ones

[Frediano Ziglio]

> On 27 May 2015 at 20:04, Frediano Ziglio <fziglio@redhat.com> wrote:
> > qxl_bo structure has two reference counters, one in the GEM object and
> > another in the TTM object. The GEM object keep a counter to the TTM object
> > so when GEM counter reached zero the TTM counter (using qxl_bo_unref) was
> > decremented. The qxl object is fully freed (both GEM and TTM part are
> > cleaned)
> > when the TTM counter reach zero.
> > One issue was that surface idr structure has no owning on qxl_bo objects
> > however
> > it contains a pointer to qxl_bo object. This caused some nasty race
> > condition
> > for instance qxl_bo object was reaped even after counter was already zero.
> > This patch fix these races moving main counter (the one used by
> > qxl_bo_(un)ref)
> > to GEM object which cleanup routine (qxl_gem_object_free) remove the idr
> > pointer
> > (using qxl_surface_evict) when the counters are still valid.
> 
> Uggh, but yes, not sure I like this fix for the problem, but if it works,
> 
> Reviewed-by: Dave Airlie <airlied@redhat.com>
> 

Well, the patch does not surely looks very clear but I can assure the problems it fixes are much less clear to understand.
Having a pointer to a object the is going to be deleted whenever another thread decide to causes difficult races. I tried to avoid this kind of change and fix the races instead but was a nightmare.
My first experimental patch added an additional counter on top of GEM and TTM one as the main counter but at the end was much more complicated and result was similar to move the main counter to GEM.
Mainly external references (from userspace and kernel) are pointers to GEM. Pointers to TTM are from memory mapped files. Deleting the spice id after GEM object has no references assure the not owning reference from spice id still refer to a valid object. As user can't retrieve a pointer from a mapping (at most can remap it) so there are no risks counter to GEM is incremented again.

Frediano

[PATCH 10/11] Simplify cleaning qxl processing command

[Frediano Ziglio]

In qxlhw_handle_to_bo we incremented counters twice, one time for release object
and one for reloc_info.
In the main function however reloc_info references was drop much earlier than
release so keeping the pointer only on release is safe and make cleaning
process easier.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 qxl/qxl_ioctl.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
index 85b3808..bb326ff 100644
--- a/qxl/qxl_ioctl.c
+++ b/qxl/qxl_ioctl.c
@@ -122,10 +122,9 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
 	qobj = gem_to_qxl_bo(gobj);
 
 	ret = qxl_release_list_add(release, qobj);
-	if (ret) {
-		drm_gem_object_unreference_unlocked(gobj);
+	drm_gem_object_unreference_unlocked(gobj);
+	if (ret)
 		return NULL;
-	}
 
 	return qobj;
 }
@@ -145,7 +144,7 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 	struct qxl_release *release;
 	struct qxl_bo *cmd_bo;
 	void *fb_cmd;
-	int i, j, ret, num_relocs;
+	int i, ret, num_relocs;
 	int unwritten;
 
 	switch (cmd->type) {
@@ -269,12 +268,6 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 		qxl_release_fence_buffer_objects(release);
 
 out_free_bos:
-	for (j = 0; j < num_relocs; j++) {
-		if (reloc_info[j].dst_bo != cmd_bo)
-			drm_gem_object_unreference_unlocked(&reloc_info[j].dst_bo->gem_base);
-		if (reloc_info[j].src_bo && reloc_info[j].src_bo != cmd_bo)
-			drm_gem_object_unreference_unlocked(&reloc_info[j].src_bo->gem_base);
-	}
 out_free_release:
 	if (ret)
 		qxl_release_free(qdev, release);
-- 
2.1.0

Re: [Spice-devel] [PATCH 10/11] Simplify cleaning qxl processing command

[Dave Airlie]

On 27 May 2015 at 20:04, Frediano Ziglio <fziglio@redhat.com> wrote:
> In qxlhw_handle_to_bo we incremented counters twice, one time for release object
> and one for reloc_info.
> In the main function however reloc_info references was drop much earlier than
> release so keeping the pointer only on release is safe and make cleaning
> process easier.

Seems fine,

Reviewed-by: Dave Airlie <airlied@redhat.com>
>
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  qxl/qxl_ioctl.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
> index 85b3808..bb326ff 100644
> --- a/qxl/qxl_ioctl.c
> +++ b/qxl/qxl_ioctl.c
> @@ -122,10 +122,9 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
>         qobj = gem_to_qxl_bo(gobj);
>
>         ret = qxl_release_list_add(release, qobj);
> -       if (ret) {
> -               drm_gem_object_unreference_unlocked(gobj);
> +       drm_gem_object_unreference_unlocked(gobj);
> +       if (ret)
>                 return NULL;
> -       }
>
>         return qobj;
>  }
> @@ -145,7 +144,7 @@ static int qxl_process_single_command(struct qxl_device *qdev,
>         struct qxl_release *release;
>         struct qxl_bo *cmd_bo;
>         void *fb_cmd;
> -       int i, j, ret, num_relocs;
> +       int i, ret, num_relocs;
>         int unwritten;
>
>         switch (cmd->type) {
> @@ -269,12 +268,6 @@ static int qxl_process_single_command(struct qxl_device *qdev,
>                 qxl_release_fence_buffer_objects(release);
>
>  out_free_bos:
> -       for (j = 0; j < num_relocs; j++) {
> -               if (reloc_info[j].dst_bo != cmd_bo)
> -                       drm_gem_object_unreference_unlocked(&reloc_info[j].dst_bo->gem_base);
> -               if (reloc_info[j].src_bo && reloc_info[j].src_bo != cmd_bo)
> -                       drm_gem_object_unreference_unlocked(&reloc_info[j].src_bo->gem_base);
> -       }
>  out_free_release:
>         if (ret)
>                 qxl_release_free(qdev, release);
> --
> 2.1.0
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

[PATCH 11/11] Propagate correctly errors from qxlhw_handle_to_bo

[Frediano Ziglio]

This function could return a NULL pointer in case of handle not
present and in case of out of memory conditions however caller
function always returned EINVAL error hiding a possible ENOMEM.
This patch change the function to return the error instead to
be able to propagate the error instead of assuming EINVAL.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 qxl/qxl_ioctl.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
index bb326ff..37f1faf 100644
--- a/qxl/qxl_ioctl.c
+++ b/qxl/qxl_ioctl.c
@@ -107,9 +107,9 @@ apply_surf_reloc(struct qxl_device *qdev, struct qxl_reloc_info *info)
 }
 
 /* return holding the reference to this object */
-static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
-					 struct drm_file *file_priv, uint64_t handle,
-					 struct qxl_release *release)
+static int qxlhw_handle_to_bo(struct qxl_device *qdev,
+			      struct drm_file *file_priv, uint64_t handle,
+			      struct qxl_release *release, struct qxl_bo **qbo_p)
 {
 	struct drm_gem_object *gobj;
 	struct qxl_bo *qobj;
@@ -117,16 +117,17 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
 
 	gobj = drm_gem_object_lookup(qdev->ddev, file_priv, handle);
 	if (!gobj)
-		return NULL;
+		return -EINVAL;
 
 	qobj = gem_to_qxl_bo(gobj);
 
 	ret = qxl_release_list_add(release, qobj);
 	drm_gem_object_unreference_unlocked(gobj);
 	if (ret)
-		return NULL;
+		return ret;
 
-	return qobj;
+	*qbo_p = qobj;
+	return 0;
 }
 
 /*
@@ -219,13 +220,10 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 		reloc_info[i].type = reloc.reloc_type;
 
 		if (reloc.dst_handle) {
-			reloc_info[i].dst_bo = qxlhw_handle_to_bo(qdev, file_priv,
-								  reloc.dst_handle, release);
-			if (!reloc_info[i].dst_bo) {
-				ret = -EINVAL;
-				reloc_info[i].src_bo = NULL;
+			ret = qxlhw_handle_to_bo(qdev, file_priv, reloc.dst_handle, release,
+						 &reloc_info[i].dst_bo);
+			if (ret)
 				goto out_free_bos;
-			}
 			reloc_info[i].dst_offset = reloc.dst_offset;
 		} else {
 			reloc_info[i].dst_bo = cmd_bo;
@@ -234,14 +232,11 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 		num_relocs++;
 
 		/* reserve and validate the reloc dst bo */
-		if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle > 0) {
-			reloc_info[i].src_bo =
-				qxlhw_handle_to_bo(qdev, file_priv,
-						   reloc.src_handle, release);
-			if (!reloc_info[i].src_bo) {
-				ret = -EINVAL;
+		if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle) {
+			ret = qxlhw_handle_to_bo(qdev, file_priv, reloc.src_handle, release,
+						 &reloc_info[i].src_bo);
+			if (ret)
 				goto out_free_bos;
-			}
 			reloc_info[i].src_offset = reloc.src_offset;
 		} else {
 			reloc_info[i].src_bo = NULL;
-- 
2.1.0

Re: [Spice-devel] [PATCH 11/11] Propagate correctly errors from qxlhw_handle_to_bo

[Dave Airlie]

On 27 May 2015 at 20:04, Frediano Ziglio <fziglio@redhat.com> wrote:
> This function could return a NULL pointer in case of handle not
> present and in case of out of memory conditions however caller
> function always returned EINVAL error hiding a possible ENOMEM.
> This patch change the function to return the error instead to
> be able to propagate the error instead of assuming EINVAL.
>
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>

Reviewed-by: Dave Airlie <airlied@redhat.com>
> ---
>  qxl/qxl_ioctl.c | 33 ++++++++++++++-------------------
>  1 file changed, 14 insertions(+), 19 deletions(-)
>
> diff --git a/qxl/qxl_ioctl.c b/qxl/qxl_ioctl.c
> index bb326ff..37f1faf 100644
> --- a/qxl/qxl_ioctl.c
> +++ b/qxl/qxl_ioctl.c
> @@ -107,9 +107,9 @@ apply_surf_reloc(struct qxl_device *qdev, struct qxl_reloc_info *info)
>  }
>
>  /* return holding the reference to this object */
> -static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
> -                                        struct drm_file *file_priv, uint64_t handle,
> -                                        struct qxl_release *release)
> +static int qxlhw_handle_to_bo(struct qxl_device *qdev,
> +                             struct drm_file *file_priv, uint64_t handle,
> +                             struct qxl_release *release, struct qxl_bo **qbo_p)
>  {
>         struct drm_gem_object *gobj;
>         struct qxl_bo *qobj;
> @@ -117,16 +117,17 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
>
>         gobj = drm_gem_object_lookup(qdev->ddev, file_priv, handle);
>         if (!gobj)
> -               return NULL;
> +               return -EINVAL;
>
>         qobj = gem_to_qxl_bo(gobj);
>
>         ret = qxl_release_list_add(release, qobj);
>         drm_gem_object_unreference_unlocked(gobj);
>         if (ret)
> -               return NULL;
> +               return ret;
>
> -       return qobj;
> +       *qbo_p = qobj;
> +       return 0;
>  }
>
>  /*
> @@ -219,13 +220,10 @@ static int qxl_process_single_command(struct qxl_device *qdev,
>                 reloc_info[i].type = reloc.reloc_type;
>
>                 if (reloc.dst_handle) {
> -                       reloc_info[i].dst_bo = qxlhw_handle_to_bo(qdev, file_priv,
> -                                                                 reloc.dst_handle, release);
> -                       if (!reloc_info[i].dst_bo) {
> -                               ret = -EINVAL;
> -                               reloc_info[i].src_bo = NULL;
> +                       ret = qxlhw_handle_to_bo(qdev, file_priv, reloc.dst_handle, release,
> +                                                &reloc_info[i].dst_bo);
> +                       if (ret)
>                                 goto out_free_bos;
> -                       }
>                         reloc_info[i].dst_offset = reloc.dst_offset;
>                 } else {
>                         reloc_info[i].dst_bo = cmd_bo;
> @@ -234,14 +232,11 @@ static int qxl_process_single_command(struct qxl_device *qdev,
>                 num_relocs++;
>
>                 /* reserve and validate the reloc dst bo */
> -               if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle > 0) {
> -                       reloc_info[i].src_bo =
> -                               qxlhw_handle_to_bo(qdev, file_priv,
> -                                                  reloc.src_handle, release);
> -                       if (!reloc_info[i].src_bo) {
> -                               ret = -EINVAL;
> +               if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle) {
> +                       ret = qxlhw_handle_to_bo(qdev, file_priv, reloc.src_handle, release,
> +                                                &reloc_info[i].src_bo);
> +                       if (ret)
>                                 goto out_free_bos;
> -                       }
>                         reloc_info[i].src_offset = reloc.src_offset;
>                 } else {
>                         reloc_info[i].src_bo = NULL;
> --
> 2.1.0
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [PATCH 00/11] Miscellaneous stability patches

[Josh Boyer]

On Wed, May 27, 2015 at 6:03 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
> This set of patches mainly contains fix for some memory issues
> using quite aggressively surfaces and other minor problems like
> images going black after a while.
>
> Frediano Ziglio (11):
>   Do not cause spice-server to clean our objects
>   Do not leak memory if qxl_release_list_add fails
>   Fix print statement not using uninitialized variable
>   Avoid double free on error
>   Handle all errors in qxl_surface_evict
>   Fix return for qxl_release_alloc
>   Handle correctly failures in qxl_alloc_relase_reserved
>   Remove format string errors
>   Move main reference counter to GEM object instead of TTM ones
>   Simplify cleaning qxl processing command
>   Propagate correctly errors from qxlhw_handle_to_bo
>
>  qxl/qxl_cmd.c     | 11 ++++++-----
>  qxl/qxl_display.c |  2 +-
>  qxl/qxl_drv.h     |  2 +-
>  qxl/qxl_gem.c     | 10 ++++++++--
>  qxl/qxl_ioctl.c   | 46 +++++++++++++++++-----------------------------
>  qxl/qxl_object.c  | 11 ++++-------
>  qxl/qxl_release.c | 13 +++++++++----
>  7 files changed, 46 insertions(+), 49 deletions(-)

The strip level on these patches is rather odd.  Normally one would
see a strip level of 1 at the top of the kernel dir.  E.g.

drivers/gpu/drm/qxl/qxl_gem.c

in the diffstat, etc.

josh

Re: [PATCH 00/11] Miscellaneous stability patches

[Josh Boyer]

On Wed, May 27, 2015 at 8:47 AM, Josh Boyer <jwboyer@fedoraproject.org> wrote:
> On Wed, May 27, 2015 at 6:03 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
>> This set of patches mainly contains fix for some memory issues
>> using quite aggressively surfaces and other minor problems like
>> images going black after a while.
>>
>> Frediano Ziglio (11):
>>   Do not cause spice-server to clean our objects
>>   Do not leak memory if qxl_release_list_add fails
>>   Fix print statement not using uninitialized variable
>>   Avoid double free on error
>>   Handle all errors in qxl_surface_evict
>>   Fix return for qxl_release_alloc
>>   Handle correctly failures in qxl_alloc_relase_reserved
>>   Remove format string errors
>>   Move main reference counter to GEM object instead of TTM ones
>>   Simplify cleaning qxl processing command
>>   Propagate correctly errors from qxlhw_handle_to_bo
>>
>>  qxl/qxl_cmd.c     | 11 ++++++-----
>>  qxl/qxl_display.c |  2 +-
>>  qxl/qxl_drv.h     |  2 +-
>>  qxl/qxl_gem.c     | 10 ++++++++--
>>  qxl/qxl_ioctl.c   | 46 +++++++++++++++++-----------------------------
>>  qxl/qxl_object.c  | 11 ++++-------
>>  qxl/qxl_release.c | 13 +++++++++----
>>  7 files changed, 46 insertions(+), 49 deletions(-)
>
> The strip level on these patches is rather odd.  Normally one would
> see a strip level of 1 at the top of the kernel dir.  E.g.
>
> drivers/gpu/drm/qxl/qxl_gem.c
>
> in the diffstat, etc.

(Sorry for the double reply.)

Also, are any of these commits something that should be queued for
stable kernel releases?  There are a handful that look like they
should be to me.

josh

Re: [PATCH 00/11] Miscellaneous stability patches

[Frediano Ziglio]

> 
> On Wed, May 27, 2015 at 8:47 AM, Josh Boyer <jwboyer@fedoraproject.org>
> wrote:
> > On Wed, May 27, 2015 at 6:03 AM, Frediano Ziglio <fziglio@redhat.com>
> > wrote:
> >> This set of patches mainly contains fix for some memory issues
> >> using quite aggressively surfaces and other minor problems like
> >> images going black after a while.
> >>
> >> Frediano Ziglio (11):
> >>   Do not cause spice-server to clean our objects
> >>   Do not leak memory if qxl_release_list_add fails
> >>   Fix print statement not using uninitialized variable
> >>   Avoid double free on error
> >>   Handle all errors in qxl_surface_evict
> >>   Fix return for qxl_release_alloc
> >>   Handle correctly failures in qxl_alloc_relase_reserved
> >>   Remove format string errors
> >>   Move main reference counter to GEM object instead of TTM ones
> >>   Simplify cleaning qxl processing command
> >>   Propagate correctly errors from qxlhw_handle_to_bo
> >>
> >>  qxl/qxl_cmd.c     | 11 ++++++-----
> >>  qxl/qxl_display.c |  2 +-
> >>  qxl/qxl_drv.h     |  2 +-
> >>  qxl/qxl_gem.c     | 10 ++++++++--
> >>  qxl/qxl_ioctl.c   | 46 +++++++++++++++++-----------------------------
> >>  qxl/qxl_object.c  | 11 ++++-------
> >>  qxl/qxl_release.c | 13 +++++++++----
> >>  7 files changed, 46 insertions(+), 49 deletions(-)
> >
> > The strip level on these patches is rather odd.  Normally one would
> > see a strip level of 1 at the top of the kernel dir.  E.g.
> >
> > drivers/gpu/drm/qxl/qxl_gem.c
> >
> > in the diffstat, etc.
> 
> (Sorry for the double reply.)
> 
> Also, are any of these commits something that should be queued for
> stable kernel releases?  There are a handful that look like they
> should be to me.
> 
> josh
> 

Hi,
  no problem for double reply.

I was using a different repository with only QXL driver. I tested and all patches apply and compile perfectly even with Linus master branch.

About which patches should be applied surely (attempting to put a priority)
- "Move main reference counter to GEM object instead of TTM ones" this can causes memory corruption even not wanting to;
- "Avoid double free on error" this can be cause leaks in kernel if user space wants, mitigated by the fact that usually DRM inodes are owned by root;
- "Handle all errors in qxl_surface_evict" could cause corruption too, not really probable but taking into account that Xorg implementation use a lot signals is not so impossible;
- "Handle correctly failures in qxl_alloc_relase_reserved", "Do not leak memory if qxl_release_list_add fails" just cause leaks on situation where memory is already REALLY low, can be omitted;
- "Fix print statement not using uninitialized variable", "Remove format string errors" should just print garbage and debugging is disabled by default, not necessary.

Frediano

Re: [Spice-devel] [PATCH 00/11] Miscellaneous stability patches

[Dave Airlie]

> I was using a different repository with only QXL driver. I tested and all patches apply and compile perfectly even with Linus master branch.

Lets only post patches people can apply, it makes it harder to figure
out stuff. I'll take a look at the patches, but it would be good to
get them resent base on drm-next.

also indicating in each patch what is a right now fix and what isn't.

Dave.

Re: [Spice-devel] [PATCH 00/11] Miscellaneous stability patches

[Frediano Ziglio]

> 
> > I was using a different repository with only QXL driver. I tested and all
> > patches apply and compile perfectly even with Linus master branch.
> 
> Lets only post patches people can apply, it makes it harder to figure
> out stuff. I'll take a look at the patches, but it would be good to
> get them resent base on drm-next.
> 

I'll do.

> also indicating in each patch what is a right now fix and what isn't.
> 

What do you mean by right fix or not ?
In the cover specify the sort of information I give to Josh ?

> Dave.
> 

Frediano

Re: [Spice-devel] [PATCH 00/11] Miscellaneous stability patches

[Frans Klaver]

On Thu, May 28, 2015 at 4:10 PM, Frediano Ziglio <fziglio@redhat.com> wrote:
>
>> also indicating in each patch what is a right now fix and what isn't.
>
> What do you mean by right fix or not ?

He probably meant indicating whether it is an urgent fix.

Frans