[PATCH] ttm: don't destroy old mm_node on memcpy failure

[PATCH] ttm: don't destroy old mm_node on memcpy failure

[Dave Airlie]

When we are using memcpy to move objects around, and we fail to memcpy
due to lack of memory to populate or failure to finish the copy, we don't
want to destroy the mm_node that has been copied into old_copy.

While working on a new kms driver that uses memcpy, if I overallocated bo's
up to the memory limits, and eviction failed, then machine would oops soon
after due to having an active bo with an already freed drm_mm embedded in it,
freeing it a second time didn't end well.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/ttm/ttm_bo_util.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index ed42323..a2aab88 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -344,8 +344,12 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
 
 	if (ttm->state == tt_unpopulated) {
 		ret = ttm->bdev->driver->ttm_tt_populate(ttm);
-		if (ret)
+		if (ret) {
+			/* if we fail here don't nuke the mm node
+			 * as the bo still owns it */
+			old_copy.mm_node = NULL;
 			goto out1;
+		}
 	}
 
 	add = 0;
@@ -371,8 +375,11 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
 						   prot);
 		} else
 			ret = ttm_copy_io_page(new_iomap, old_iomap, page);
-		if (ret)
+		if (ret) {
+			/* failing here, means keep old copy as-is */
+			old_copy.mm_node = NULL;
 			goto out1;
+		}
 	}
 	mb();
 out2:
-- 
1.8.1

Re: [PATCH] ttm: don't destroy old mm_node on memcpy failure

[Jerome Glisse]

On Tue, Jan 15, 2013 at 11:28 PM, Dave Airlie <airlied@gmail.com> wrote:
> When we are using memcpy to move objects around, and we fail to memcpy
> due to lack of memory to populate or failure to finish the copy, we don't
> want to destroy the mm_node that has been copied into old_copy.
>
> While working on a new kms driver that uses memcpy, if I overallocated bo's
> up to the memory limits, and eviction failed, then machine would oops soon
> after due to having an active bo with an already freed drm_mm embedded in it,
> freeing it a second time didn't end well.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Reviewed-by: Jerome Glisse <jglisse@redhat.com>

> ---
>  drivers/gpu/drm/ttm/ttm_bo_util.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index ed42323..a2aab88 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -344,8 +344,12 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
>
>         if (ttm->state == tt_unpopulated) {
>                 ret = ttm->bdev->driver->ttm_tt_populate(ttm);
> -               if (ret)
> +               if (ret) {
> +                       /* if we fail here don't nuke the mm node
> +                        * as the bo still owns it */
> +                       old_copy.mm_node = NULL;
>                         goto out1;
> +               }
>         }
>
>         add = 0;
> @@ -371,8 +375,11 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
>                                                    prot);
>                 } else
>                         ret = ttm_copy_io_page(new_iomap, old_iomap, page);
> -               if (ret)
> +               if (ret) {
> +                       /* failing here, means keep old copy as-is */
> +                       old_copy.mm_node = NULL;
>                         goto out1;
> +               }
>         }
>         mb();
>  out2:
> --
> 1.8.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel