[PATCH] drm/ttm: once more fix ttm_bo_bulk_move_lru_tail

[PATCH] drm/ttm: once more fix ttm_bo_bulk_move_lru_tail

[Christian König]

While cutting the lists we sometimes accidentally added a list_head from
the stack to the LRUs, effectively corrupting the list.

Remove the list cutting and use explicit list manipulation instead.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 51 ++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 138c98902033..b2a33bf1ef10 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -247,23 +247,18 @@ void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo,
 }
 EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
 
-static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
-				    struct list_head *lru, bool is_swap)
+static void ttm_list_move_bulk_tail(struct list_head *list,
+				    struct list_head *first,
+				    struct list_head *last)
 {
-	struct list_head *list;
-	LIST_HEAD(entries);
-	LIST_HEAD(before);
+	first->prev->next = last->next;
+	last->next->prev = first->prev;
 
-	reservation_object_assert_held(pos->last->resv);
-	list = is_swap ? &pos->last->swap : &pos->last->lru;
-	list_cut_position(&entries, lru, list);
+	list->prev->next = first;
+	first->prev = list->prev;
 
-	reservation_object_assert_held(pos->first->resv);
-	list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
-	list_cut_position(&before, &entries, list);
-
-	list_splice(&before, lru);
-	list_splice_tail(&entries, lru);
+	last->next = list;
+	list->prev = last;
 }
 
 void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
@@ -271,23 +266,33 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
 	unsigned i;
 
 	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
+		struct ttm_lru_bulk_move_pos *pos = &bulk->tt[i];
 		struct ttm_mem_type_manager *man;
 
-		if (!bulk->tt[i].first)
+		if (!pos->first)
 			continue;
 
-		man = &bulk->tt[i].first->bdev->man[TTM_PL_TT];
-		ttm_bo_bulk_move_helper(&bulk->tt[i], &man->lru[i], false);
+		reservation_object_assert_held(pos->first->resv);
+		reservation_object_assert_held(pos->last->resv);
+
+		man = &pos->first->bdev->man[TTM_PL_TT];
+		ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
+					&pos->last->lru);
 	}
 
 	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
+		struct ttm_lru_bulk_move_pos *pos = &bulk->vram[i];
 		struct ttm_mem_type_manager *man;
 
-		if (!bulk->vram[i].first)
+		if (!pos->first)
 			continue;
 
-		man = &bulk->vram[i].first->bdev->man[TTM_PL_VRAM];
-		ttm_bo_bulk_move_helper(&bulk->vram[i], &man->lru[i], false);
+		reservation_object_assert_held(pos->first->resv);
+		reservation_object_assert_held(pos->last->resv);
+
+		man = &pos->first->bdev->man[TTM_PL_VRAM];
+		ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
+					&pos->last->lru);
 	}
 
 	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
@@ -297,8 +302,12 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
 		if (!pos->first)
 			continue;
 
+		reservation_object_assert_held(pos->first->resv);
+		reservation_object_assert_held(pos->last->resv);
+
 		lru = &pos->first->bdev->glob->swap_lru[i];
-		ttm_bo_bulk_move_helper(&bulk->swap[i], lru, true);
+		ttm_list_move_bulk_tail(lru, &pos->first->swap,
+					&pos->last->swap);
 	}
 }
 EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail);
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/ttm: once more fix ttm_bo_bulk_move_lru_tail

[Alex Deucher]

On Wed, Sep 12, 2018 at 3:25 PM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> While cutting the lists we sometimes accidentally added a list_head from
> the stack to the LRUs, effectively corrupting the list.
>
> Remove the list cutting and use explicit list manipulation instead.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>

Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/ttm/ttm_bo.c | 51 ++++++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 138c98902033..b2a33bf1ef10 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -247,23 +247,18 @@ void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo,
>  }
>  EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>
> -static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
> -                                   struct list_head *lru, bool is_swap)
> +static void ttm_list_move_bulk_tail(struct list_head *list,
> +                                   struct list_head *first,
> +                                   struct list_head *last)
>  {
> -       struct list_head *list;
> -       LIST_HEAD(entries);
> -       LIST_HEAD(before);
> +       first->prev->next = last->next;
> +       last->next->prev = first->prev;
>
> -       reservation_object_assert_held(pos->last->resv);
> -       list = is_swap ? &pos->last->swap : &pos->last->lru;
> -       list_cut_position(&entries, lru, list);
> +       list->prev->next = first;
> +       first->prev = list->prev;
>
> -       reservation_object_assert_held(pos->first->resv);
> -       list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
> -       list_cut_position(&before, &entries, list);
> -
> -       list_splice(&before, lru);
> -       list_splice_tail(&entries, lru);
> +       last->next = list;
> +       list->prev = last;
>  }
>
>  void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
> @@ -271,23 +266,33 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>         unsigned i;
>
>         for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> +               struct ttm_lru_bulk_move_pos *pos = &bulk->tt[i];
>                 struct ttm_mem_type_manager *man;
>
> -               if (!bulk->tt[i].first)
> +               if (!pos->first)
>                         continue;
>
> -               man = &bulk->tt[i].first->bdev->man[TTM_PL_TT];
> -               ttm_bo_bulk_move_helper(&bulk->tt[i], &man->lru[i], false);
> +               reservation_object_assert_held(pos->first->resv);
> +               reservation_object_assert_held(pos->last->resv);
> +
> +               man = &pos->first->bdev->man[TTM_PL_TT];
> +               ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
> +                                       &pos->last->lru);
>         }
>
>         for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> +               struct ttm_lru_bulk_move_pos *pos = &bulk->vram[i];
>                 struct ttm_mem_type_manager *man;
>
> -               if (!bulk->vram[i].first)
> +               if (!pos->first)
>                         continue;
>
> -               man = &bulk->vram[i].first->bdev->man[TTM_PL_VRAM];
> -               ttm_bo_bulk_move_helper(&bulk->vram[i], &man->lru[i], false);
> +               reservation_object_assert_held(pos->first->resv);
> +               reservation_object_assert_held(pos->last->resv);
> +
> +               man = &pos->first->bdev->man[TTM_PL_VRAM];
> +               ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
> +                                       &pos->last->lru);
>         }
>
>         for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> @@ -297,8 +302,12 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>                 if (!pos->first)
>                         continue;
>
> +               reservation_object_assert_held(pos->first->resv);
> +               reservation_object_assert_held(pos->last->resv);
> +
>                 lru = &pos->first->bdev->glob->swap_lru[i];
> -               ttm_bo_bulk_move_helper(&bulk->swap[i], lru, true);
> +               ttm_list_move_bulk_tail(lru, &pos->first->swap,
> +                                       &pos->last->swap);
>         }
>  }
>  EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail);
> --
> 2.14.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/ttm: once more fix ttm_bo_bulk_move_lru_tail

[Huang Rui]

On Wed, Sep 12, 2018 at 09:23:55PM +0200, Christian König wrote:
> While cutting the lists we sometimes accidentally added a list_head from
> the stack to the LRUs, effectively corrupting the list.
> 
> Remove the list cutting and use explicit list manipulation instead.

This patch actually fixes the corruption bug. Was it a defect of
list_cut_position or list_splice handlers?

Reviewed-and-Tested: Huang Rui <ray.huang@amd.com>

> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/ttm/ttm_bo.c | 51 ++++++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 138c98902033..b2a33bf1ef10 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -247,23 +247,18 @@ void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo,
>  }
>  EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>  
> -static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
> -				    struct list_head *lru, bool is_swap)
> +static void ttm_list_move_bulk_tail(struct list_head *list,
> +				    struct list_head *first,
> +				    struct list_head *last)
>  {
> -	struct list_head *list;
> -	LIST_HEAD(entries);
> -	LIST_HEAD(before);
> +	first->prev->next = last->next;
> +	last->next->prev = first->prev;
>  
> -	reservation_object_assert_held(pos->last->resv);
> -	list = is_swap ? &pos->last->swap : &pos->last->lru;
> -	list_cut_position(&entries, lru, list);
> +	list->prev->next = first;
> +	first->prev = list->prev;
>  
> -	reservation_object_assert_held(pos->first->resv);
> -	list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
> -	list_cut_position(&before, &entries, list);
> -
> -	list_splice(&before, lru);
> -	list_splice_tail(&entries, lru);
> +	last->next = list;
> +	list->prev = last;
>  }
>  
>  void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
> @@ -271,23 +266,33 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>  	unsigned i;
>  
>  	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> +		struct ttm_lru_bulk_move_pos *pos = &bulk->tt[i];
>  		struct ttm_mem_type_manager *man;
>  
> -		if (!bulk->tt[i].first)
> +		if (!pos->first)
>  			continue;
>  
> -		man = &bulk->tt[i].first->bdev->man[TTM_PL_TT];
> -		ttm_bo_bulk_move_helper(&bulk->tt[i], &man->lru[i], false);
> +		reservation_object_assert_held(pos->first->resv);
> +		reservation_object_assert_held(pos->last->resv);
> +
> +		man = &pos->first->bdev->man[TTM_PL_TT];
> +		ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
> +					&pos->last->lru);
>  	}
>  
>  	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> +		struct ttm_lru_bulk_move_pos *pos = &bulk->vram[i];
>  		struct ttm_mem_type_manager *man;
>  
> -		if (!bulk->vram[i].first)
> +		if (!pos->first)
>  			continue;
>  
> -		man = &bulk->vram[i].first->bdev->man[TTM_PL_VRAM];
> -		ttm_bo_bulk_move_helper(&bulk->vram[i], &man->lru[i], false);
> +		reservation_object_assert_held(pos->first->resv);
> +		reservation_object_assert_held(pos->last->resv);
> +
> +		man = &pos->first->bdev->man[TTM_PL_VRAM];
> +		ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
> +					&pos->last->lru);
>  	}
>  
>  	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> @@ -297,8 +302,12 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>  		if (!pos->first)
>  			continue;
>  
> +		reservation_object_assert_held(pos->first->resv);
> +		reservation_object_assert_held(pos->last->resv);
> +
>  		lru = &pos->first->bdev->glob->swap_lru[i];
> -		ttm_bo_bulk_move_helper(&bulk->swap[i], lru, true);
> +		ttm_list_move_bulk_tail(lru, &pos->first->swap,
> +					&pos->last->swap);
>  	}
>  }
>  EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail);
> -- 
> 2.14.1
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/ttm: once more fix ttm_bo_bulk_move_lru_tail

[Michel Dänzer]

[ Moving to dri-devel, where TTM patches are reviewed ]

On 2018-09-12 9:23 p.m., Christian König wrote:
> While cutting the lists we sometimes accidentally added a list_head from
> the stack to the LRUs, effectively corrupting the list.
> 
> Remove the list cutting and use explicit list manipulation instead.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/ttm/ttm_bo.c | 51 ++++++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 138c98902033..b2a33bf1ef10 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -247,23 +247,18 @@ void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo,
>  }
>  EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>  
> -static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
> -				    struct list_head *lru, bool is_swap)
> +static void ttm_list_move_bulk_tail(struct list_head *list,
> +				    struct list_head *first,
> +				    struct list_head *last)
>  {
> -	struct list_head *list;
> -	LIST_HEAD(entries);
> -	LIST_HEAD(before);
> +	first->prev->next = last->next;
> +	last->next->prev = first->prev;
>  
> -	reservation_object_assert_held(pos->last->resv);
> -	list = is_swap ? &pos->last->swap : &pos->last->lru;
> -	list_cut_position(&entries, lru, list);
> +	list->prev->next = first;
> +	first->prev = list->prev;
>  
> -	reservation_object_assert_held(pos->first->resv);
> -	list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
> -	list_cut_position(&before, &entries, list);
> -
> -	list_splice(&before, lru);
> -	list_splice_tail(&entries, lru);
> +	last->next = list;
> +	list->prev = last;
>  }
>  
>  void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
> @@ -271,23 +266,33 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>  	unsigned i;
>  
>  	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> +		struct ttm_lru_bulk_move_pos *pos = &bulk->tt[i];
>  		struct ttm_mem_type_manager *man;
>  
> -		if (!bulk->tt[i].first)
> +		if (!pos->first)
>  			continue;
>  
> -		man = &bulk->tt[i].first->bdev->man[TTM_PL_TT];
> -		ttm_bo_bulk_move_helper(&bulk->tt[i], &man->lru[i], false);
> +		reservation_object_assert_held(pos->first->resv);
> +		reservation_object_assert_held(pos->last->resv);
> +
> +		man = &pos->first->bdev->man[TTM_PL_TT];
> +		ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
> +					&pos->last->lru);
>  	}
>  
>  	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> +		struct ttm_lru_bulk_move_pos *pos = &bulk->vram[i];
>  		struct ttm_mem_type_manager *man;
>  
> -		if (!bulk->vram[i].first)
> +		if (!pos->first)
>  			continue;
>  
> -		man = &bulk->vram[i].first->bdev->man[TTM_PL_VRAM];
> -		ttm_bo_bulk_move_helper(&bulk->vram[i], &man->lru[i], false);
> +		reservation_object_assert_held(pos->first->resv);
> +		reservation_object_assert_held(pos->last->resv);
> +
> +		man = &pos->first->bdev->man[TTM_PL_VRAM];
> +		ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
> +					&pos->last->lru);
>  	}
>  
>  	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> @@ -297,8 +302,12 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>  		if (!pos->first)
>  			continue;
>  
> +		reservation_object_assert_held(pos->first->resv);
> +		reservation_object_assert_held(pos->last->resv);
> +
>  		lru = &pos->first->bdev->glob->swap_lru[i];
> -		ttm_bo_bulk_move_helper(&bulk->swap[i], lru, true);
> +		ttm_list_move_bulk_tail(lru, &pos->first->swap,
> +					&pos->last->swap);
>  	}
>  }
>  EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail);
> 

Seems like more code could be kept in / moved into the shared helper
function. That would keep the ttm_bo_bulk_move_lru_tail code leaner, and
might make the helper function changes easier to review as well.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ttm: once more fix ttm_bo_bulk_move_lru_tail

[Michel Dänzer]

[ Moving to dri-devel, where TTM patches are reviewed ]

On 2018-09-12 9:23 p.m., Christian König wrote:
> While cutting the lists we sometimes accidentally added a list_head from
> the stack to the LRUs, effectively corrupting the list.
> 
> Remove the list cutting and use explicit list manipulation instead.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/ttm/ttm_bo.c | 51 ++++++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 138c98902033..b2a33bf1ef10 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -247,23 +247,18 @@ void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo,
>  }
>  EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>  
> -static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
> -				    struct list_head *lru, bool is_swap)
> +static void ttm_list_move_bulk_tail(struct list_head *list,
> +				    struct list_head *first,
> +				    struct list_head *last)
>  {
> -	struct list_head *list;
> -	LIST_HEAD(entries);
> -	LIST_HEAD(before);
> +	first->prev->next = last->next;
> +	last->next->prev = first->prev;
>  
> -	reservation_object_assert_held(pos->last->resv);
> -	list = is_swap ? &pos->last->swap : &pos->last->lru;
> -	list_cut_position(&entries, lru, list);
> +	list->prev->next = first;
> +	first->prev = list->prev;
>  
> -	reservation_object_assert_held(pos->first->resv);
> -	list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
> -	list_cut_position(&before, &entries, list);
> -
> -	list_splice(&before, lru);
> -	list_splice_tail(&entries, lru);
> +	last->next = list;
> +	list->prev = last;
>  }
>  
>  void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
> @@ -271,23 +266,33 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>  	unsigned i;
>  
>  	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> +		struct ttm_lru_bulk_move_pos *pos = &bulk->tt[i];
>  		struct ttm_mem_type_manager *man;
>  
> -		if (!bulk->tt[i].first)
> +		if (!pos->first)
>  			continue;
>  
> -		man = &bulk->tt[i].first->bdev->man[TTM_PL_TT];
> -		ttm_bo_bulk_move_helper(&bulk->tt[i], &man->lru[i], false);
> +		reservation_object_assert_held(pos->first->resv);
> +		reservation_object_assert_held(pos->last->resv);
> +
> +		man = &pos->first->bdev->man[TTM_PL_TT];
> +		ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
> +					&pos->last->lru);
>  	}
>  
>  	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> +		struct ttm_lru_bulk_move_pos *pos = &bulk->vram[i];
>  		struct ttm_mem_type_manager *man;
>  
> -		if (!bulk->vram[i].first)
> +		if (!pos->first)
>  			continue;
>  
> -		man = &bulk->vram[i].first->bdev->man[TTM_PL_VRAM];
> -		ttm_bo_bulk_move_helper(&bulk->vram[i], &man->lru[i], false);
> +		reservation_object_assert_held(pos->first->resv);
> +		reservation_object_assert_held(pos->last->resv);
> +
> +		man = &pos->first->bdev->man[TTM_PL_VRAM];
> +		ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
> +					&pos->last->lru);
>  	}
>  
>  	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> @@ -297,8 +302,12 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>  		if (!pos->first)
>  			continue;
>  
> +		reservation_object_assert_held(pos->first->resv);
> +		reservation_object_assert_held(pos->last->resv);
> +
>  		lru = &pos->first->bdev->glob->swap_lru[i];
> -		ttm_bo_bulk_move_helper(&bulk->swap[i], lru, true);
> +		ttm_list_move_bulk_tail(lru, &pos->first->swap,
> +					&pos->last->swap);
>  	}
>  }
>  EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail);
> 

Seems like more code could be kept in / moved into the shared helper
function. That would keep the ttm_bo_bulk_move_lru_tail code leaner, and
might make the helper function changes easier to review as well.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/ttm: once more fix ttm_bo_bulk_move_lru_tail

[Christian König]

Am 13.09.2018 um 10:35 schrieb Michel Dänzer:
> [ Moving to dri-devel, where TTM patches are reviewed ]
>
> On 2018-09-12 9:23 p.m., Christian König wrote:
>> While cutting the lists we sometimes accidentally added a list_head from
>> the stack to the LRUs, effectively corrupting the list.
>>
>> Remove the list cutting and use explicit list manipulation instead.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/ttm/ttm_bo.c | 51 ++++++++++++++++++++++++++------------------
>>   1 file changed, 30 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>> index 138c98902033..b2a33bf1ef10 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -247,23 +247,18 @@ void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo,
>>   }
>>   EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>>   
>> -static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
>> -				    struct list_head *lru, bool is_swap)
>> +static void ttm_list_move_bulk_tail(struct list_head *list,
>> +				    struct list_head *first,
>> +				    struct list_head *last)
>>   {
>> -	struct list_head *list;
>> -	LIST_HEAD(entries);
>> -	LIST_HEAD(before);
>> +	first->prev->next = last->next;
>> +	last->next->prev = first->prev;
>>   
>> -	reservation_object_assert_held(pos->last->resv);
>> -	list = is_swap ? &pos->last->swap : &pos->last->lru;
>> -	list_cut_position(&entries, lru, list);
>> +	list->prev->next = first;
>> +	first->prev = list->prev;
>>   
>> -	reservation_object_assert_held(pos->first->resv);
>> -	list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>> -	list_cut_position(&before, &entries, list);
>> -
>> -	list_splice(&before, lru);
>> -	list_splice_tail(&entries, lru);
>> +	last->next = list;
>> +	list->prev = last;
>>   }
>>   
>>   void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>> @@ -271,23 +266,33 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>>   	unsigned i;
>>   
>>   	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>> +		struct ttm_lru_bulk_move_pos *pos = &bulk->tt[i];
>>   		struct ttm_mem_type_manager *man;
>>   
>> -		if (!bulk->tt[i].first)
>> +		if (!pos->first)
>>   			continue;
>>   
>> -		man = &bulk->tt[i].first->bdev->man[TTM_PL_TT];
>> -		ttm_bo_bulk_move_helper(&bulk->tt[i], &man->lru[i], false);
>> +		reservation_object_assert_held(pos->first->resv);
>> +		reservation_object_assert_held(pos->last->resv);
>> +
>> +		man = &pos->first->bdev->man[TTM_PL_TT];
>> +		ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
>> +					&pos->last->lru);
>>   	}
>>   
>>   	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>> +		struct ttm_lru_bulk_move_pos *pos = &bulk->vram[i];
>>   		struct ttm_mem_type_manager *man;
>>   
>> -		if (!bulk->vram[i].first)
>> +		if (!pos->first)
>>   			continue;
>>   
>> -		man = &bulk->vram[i].first->bdev->man[TTM_PL_VRAM];
>> -		ttm_bo_bulk_move_helper(&bulk->vram[i], &man->lru[i], false);
>> +		reservation_object_assert_held(pos->first->resv);
>> +		reservation_object_assert_held(pos->last->resv);
>> +
>> +		man = &pos->first->bdev->man[TTM_PL_VRAM];
>> +		ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
>> +					&pos->last->lru);
>>   	}
>>   
>>   	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>> @@ -297,8 +302,12 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>>   		if (!pos->first)
>>   			continue;
>>   
>> +		reservation_object_assert_held(pos->first->resv);
>> +		reservation_object_assert_held(pos->last->resv);
>> +
>>   		lru = &pos->first->bdev->glob->swap_lru[i];
>> -		ttm_bo_bulk_move_helper(&bulk->swap[i], lru, true);
>> +		ttm_list_move_bulk_tail(lru, &pos->first->swap,
>> +					&pos->last->swap);
>>   	}
>>   }
>>   EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail);
>>
> Seems like more code could be kept in / moved into the shared helper
> function. That would keep the ttm_bo_bulk_move_lru_tail code leaner, and
> might make the helper function changes easier to review as well.

Yeah, actually only wanted to send that to Rui and you for testing.

Going to clean that up and send it for upstream review today.

Christian.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ttm: once more fix ttm_bo_bulk_move_lru_tail

[Michel Dänzer]

On 2018-09-13 10:55 a.m., Christian König wrote:
> Am 13.09.2018 um 10:35 schrieb Michel Dänzer:
>> [ Moving to dri-devel, where TTM patches are reviewed ]
>>
>> On 2018-09-12 9:23 p.m., Christian König wrote:
>>> While cutting the lists we sometimes accidentally added a list_head from
>>> the stack to the LRUs, effectively corrupting the list.
>>>
>>> Remove the list cutting and use explicit list manipulation instead.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/ttm/ttm_bo.c | 51
>>> ++++++++++++++++++++++++++------------------
>>>   1 file changed, 30 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>>> index 138c98902033..b2a33bf1ef10 100644
>>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>> @@ -247,23 +247,18 @@ void ttm_bo_move_to_lru_tail(struct
>>> ttm_buffer_object *bo,
>>>   }
>>>   EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>>>   -static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos
>>> *pos,
>>> -                    struct list_head *lru, bool is_swap)
>>> +static void ttm_list_move_bulk_tail(struct list_head *list,
>>> +                    struct list_head *first,
>>> +                    struct list_head *last)
>>>   {
>>> -    struct list_head *list;
>>> -    LIST_HEAD(entries);
>>> -    LIST_HEAD(before);
>>> +    first->prev->next = last->next;
>>> +    last->next->prev = first->prev;
>>>   -    reservation_object_assert_held(pos->last->resv);
>>> -    list = is_swap ? &pos->last->swap : &pos->last->lru;
>>> -    list_cut_position(&entries, lru, list);
>>> +    list->prev->next = first;
>>> +    first->prev = list->prev;
>>>   -    reservation_object_assert_held(pos->first->resv);
>>> -    list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>>> -    list_cut_position(&before, &entries, list);
>>> -
>>> -    list_splice(&before, lru);
>>> -    list_splice_tail(&entries, lru);
>>> +    last->next = list;
>>> +    list->prev = last;
>>>   }
>>>     void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>>> @@ -271,23 +266,33 @@ void ttm_bo_bulk_move_lru_tail(struct
>>> ttm_lru_bulk_move *bulk)
>>>       unsigned i;
>>>         for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>>> +        struct ttm_lru_bulk_move_pos *pos = &bulk->tt[i];
>>>           struct ttm_mem_type_manager *man;
>>>   -        if (!bulk->tt[i].first)
>>> +        if (!pos->first)
>>>               continue;
>>>   -        man = &bulk->tt[i].first->bdev->man[TTM_PL_TT];
>>> -        ttm_bo_bulk_move_helper(&bulk->tt[i], &man->lru[i], false);
>>> +        reservation_object_assert_held(pos->first->resv);
>>> +        reservation_object_assert_held(pos->last->resv);
>>> +
>>> +        man = &pos->first->bdev->man[TTM_PL_TT];
>>> +        ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
>>> +                    &pos->last->lru);
>>>       }
>>>         for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>>> +        struct ttm_lru_bulk_move_pos *pos = &bulk->vram[i];
>>>           struct ttm_mem_type_manager *man;
>>>   -        if (!bulk->vram[i].first)
>>> +        if (!pos->first)
>>>               continue;
>>>   -        man = &bulk->vram[i].first->bdev->man[TTM_PL_VRAM];
>>> -        ttm_bo_bulk_move_helper(&bulk->vram[i], &man->lru[i], false);
>>> +        reservation_object_assert_held(pos->first->resv);
>>> +        reservation_object_assert_held(pos->last->resv);
>>> +
>>> +        man = &pos->first->bdev->man[TTM_PL_VRAM];
>>> +        ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
>>> +                    &pos->last->lru);
>>>       }
>>>         for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>>> @@ -297,8 +302,12 @@ void ttm_bo_bulk_move_lru_tail(struct
>>> ttm_lru_bulk_move *bulk)
>>>           if (!pos->first)
>>>               continue;
>>>   +        reservation_object_assert_held(pos->first->resv);
>>> +        reservation_object_assert_held(pos->last->resv);
>>> +
>>>           lru = &pos->first->bdev->glob->swap_lru[i];
>>> -        ttm_bo_bulk_move_helper(&bulk->swap[i], lru, true);
>>> +        ttm_list_move_bulk_tail(lru, &pos->first->swap,
>>> +                    &pos->last->swap);
>>>       }
>>>   }
>>>   EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail);
>>>
>> Seems like more code could be kept in / moved into the shared helper
>> function. That would keep the ttm_bo_bulk_move_lru_tail code leaner, and
>> might make the helper function changes easier to review as well.
> 
> Yeah, actually only wanted to send that to Rui and you for testing.
> 
> Going to clean that up and send it for upstream review today.

Sounds good, but Tom needs to test it as well.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/ttm: once more fix ttm_bo_bulk_move_lru_tail

[Christian König]

Am 13.09.2018 um 10:31 schrieb Huang Rui:
> On Wed, Sep 12, 2018 at 09:23:55PM +0200, Christian König wrote:
>> While cutting the lists we sometimes accidentally added a list_head from
>> the stack to the LRUs, effectively corrupting the list.
>>
>> Remove the list cutting and use explicit list manipulation instead.
> This patch actually fixes the corruption bug. Was it a defect of
> list_cut_position or list_splice handlers?

We somehow did something illegal with list_cut_position. I haven't 
narrowed it down till the end, but we ended up with list_heads from the 
stack to the lru.

Anyway adding a specialized list bulk move function is much simpler and 
avoids the issue.

I've just split that up as Michel suggested and send it out to the 
mailing lists, please review that version once more.

Thanks,
Christian.

>
> Reviewed-and-Tested: Huang Rui <ray.huang@amd.com>
>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/ttm/ttm_bo.c | 51 ++++++++++++++++++++++++++------------------
>>   1 file changed, 30 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>> index 138c98902033..b2a33bf1ef10 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -247,23 +247,18 @@ void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo,
>>   }
>>   EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>>   
>> -static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
>> -				    struct list_head *lru, bool is_swap)
>> +static void ttm_list_move_bulk_tail(struct list_head *list,
>> +				    struct list_head *first,
>> +				    struct list_head *last)
>>   {
>> -	struct list_head *list;
>> -	LIST_HEAD(entries);
>> -	LIST_HEAD(before);
>> +	first->prev->next = last->next;
>> +	last->next->prev = first->prev;
>>   
>> -	reservation_object_assert_held(pos->last->resv);
>> -	list = is_swap ? &pos->last->swap : &pos->last->lru;
>> -	list_cut_position(&entries, lru, list);
>> +	list->prev->next = first;
>> +	first->prev = list->prev;
>>   
>> -	reservation_object_assert_held(pos->first->resv);
>> -	list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>> -	list_cut_position(&before, &entries, list);
>> -
>> -	list_splice(&before, lru);
>> -	list_splice_tail(&entries, lru);
>> +	last->next = list;
>> +	list->prev = last;
>>   }
>>   
>>   void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>> @@ -271,23 +266,33 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>>   	unsigned i;
>>   
>>   	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>> +		struct ttm_lru_bulk_move_pos *pos = &bulk->tt[i];
>>   		struct ttm_mem_type_manager *man;
>>   
>> -		if (!bulk->tt[i].first)
>> +		if (!pos->first)
>>   			continue;
>>   
>> -		man = &bulk->tt[i].first->bdev->man[TTM_PL_TT];
>> -		ttm_bo_bulk_move_helper(&bulk->tt[i], &man->lru[i], false);
>> +		reservation_object_assert_held(pos->first->resv);
>> +		reservation_object_assert_held(pos->last->resv);
>> +
>> +		man = &pos->first->bdev->man[TTM_PL_TT];
>> +		ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
>> +					&pos->last->lru);
>>   	}
>>   
>>   	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>> +		struct ttm_lru_bulk_move_pos *pos = &bulk->vram[i];
>>   		struct ttm_mem_type_manager *man;
>>   
>> -		if (!bulk->vram[i].first)
>> +		if (!pos->first)
>>   			continue;
>>   
>> -		man = &bulk->vram[i].first->bdev->man[TTM_PL_VRAM];
>> -		ttm_bo_bulk_move_helper(&bulk->vram[i], &man->lru[i], false);
>> +		reservation_object_assert_held(pos->first->resv);
>> +		reservation_object_assert_held(pos->last->resv);
>> +
>> +		man = &pos->first->bdev->man[TTM_PL_VRAM];
>> +		ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
>> +					&pos->last->lru);
>>   	}
>>   
>>   	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>> @@ -297,8 +302,12 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>>   		if (!pos->first)
>>   			continue;
>>   
>> +		reservation_object_assert_held(pos->first->resv);
>> +		reservation_object_assert_held(pos->last->resv);
>> +
>>   		lru = &pos->first->bdev->glob->swap_lru[i];
>> -		ttm_bo_bulk_move_helper(&bulk->swap[i], lru, true);
>> +		ttm_list_move_bulk_tail(lru, &pos->first->swap,
>> +					&pos->last->swap);
>>   	}
>>   }
>>   EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail);
>> -- 
>> 2.14.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/ttm: once more fix ttm_bo_bulk_move_lru_tail

[Alex Deucher]

On Wed, Sep 12, 2018 at 3:25 PM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> While cutting the lists we sometimes accidentally added a list_head from
> the stack to the LRUs, effectively corrupting the list.
>
> Remove the list cutting and use explicit list manipulation instead.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>

Can you apply this patch first and then do the clean up as a later
series or temporarily disable the feature?  I want to send out a -next
pull this week and I don't want to leave this broken.

Alex

> ---
>  drivers/gpu/drm/ttm/ttm_bo.c | 51 ++++++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 138c98902033..b2a33bf1ef10 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -247,23 +247,18 @@ void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo,
>  }
>  EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>
> -static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
> -                                   struct list_head *lru, bool is_swap)
> +static void ttm_list_move_bulk_tail(struct list_head *list,
> +                                   struct list_head *first,
> +                                   struct list_head *last)
>  {
> -       struct list_head *list;
> -       LIST_HEAD(entries);
> -       LIST_HEAD(before);
> +       first->prev->next = last->next;
> +       last->next->prev = first->prev;
>
> -       reservation_object_assert_held(pos->last->resv);
> -       list = is_swap ? &pos->last->swap : &pos->last->lru;
> -       list_cut_position(&entries, lru, list);
> +       list->prev->next = first;
> +       first->prev = list->prev;
>
> -       reservation_object_assert_held(pos->first->resv);
> -       list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
> -       list_cut_position(&before, &entries, list);
> -
> -       list_splice(&before, lru);
> -       list_splice_tail(&entries, lru);
> +       last->next = list;
> +       list->prev = last;
>  }
>
>  void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
> @@ -271,23 +266,33 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>         unsigned i;
>
>         for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> +               struct ttm_lru_bulk_move_pos *pos = &bulk->tt[i];
>                 struct ttm_mem_type_manager *man;
>
> -               if (!bulk->tt[i].first)
> +               if (!pos->first)
>                         continue;
>
> -               man = &bulk->tt[i].first->bdev->man[TTM_PL_TT];
> -               ttm_bo_bulk_move_helper(&bulk->tt[i], &man->lru[i], false);
> +               reservation_object_assert_held(pos->first->resv);
> +               reservation_object_assert_held(pos->last->resv);
> +
> +               man = &pos->first->bdev->man[TTM_PL_TT];
> +               ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
> +                                       &pos->last->lru);
>         }
>
>         for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> +               struct ttm_lru_bulk_move_pos *pos = &bulk->vram[i];
>                 struct ttm_mem_type_manager *man;
>
> -               if (!bulk->vram[i].first)
> +               if (!pos->first)
>                         continue;
>
> -               man = &bulk->vram[i].first->bdev->man[TTM_PL_VRAM];
> -               ttm_bo_bulk_move_helper(&bulk->vram[i], &man->lru[i], false);
> +               reservation_object_assert_held(pos->first->resv);
> +               reservation_object_assert_held(pos->last->resv);
> +
> +               man = &pos->first->bdev->man[TTM_PL_VRAM];
> +               ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
> +                                       &pos->last->lru);
>         }
>
>         for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> @@ -297,8 +302,12 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>                 if (!pos->first)
>                         continue;
>
> +               reservation_object_assert_held(pos->first->resv);
> +               reservation_object_assert_held(pos->last->resv);
> +
>                 lru = &pos->first->bdev->glob->swap_lru[i];
> -               ttm_bo_bulk_move_helper(&bulk->swap[i], lru, true);
> +               ttm_list_move_bulk_tail(lru, &pos->first->swap,
> +                                       &pos->last->swap);
>         }
>  }
>  EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail);
> --
> 2.14.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/ttm: once more fix ttm_bo_bulk_move_lru_tail

[Christian König]

Am 13.09.2018 um 16:25 schrieb Alex Deucher:
> On Wed, Sep 12, 2018 at 3:25 PM Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>> While cutting the lists we sometimes accidentally added a list_head from
>> the stack to the LRUs, effectively corrupting the list.
>>
>> Remove the list cutting and use explicit list manipulation instead.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
> Can you apply this patch first and then do the clean up as a later
> series or temporarily disable the feature?  I want to send out a -next
> pull this week and I don't want to leave this broken.

Yeah, that should work as well.

Christian.

>
> Alex
>
>> ---
>>   drivers/gpu/drm/ttm/ttm_bo.c | 51 ++++++++++++++++++++++++++------------------
>>   1 file changed, 30 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>> index 138c98902033..b2a33bf1ef10 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -247,23 +247,18 @@ void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo,
>>   }
>>   EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>>
>> -static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
>> -                                   struct list_head *lru, bool is_swap)
>> +static void ttm_list_move_bulk_tail(struct list_head *list,
>> +                                   struct list_head *first,
>> +                                   struct list_head *last)
>>   {
>> -       struct list_head *list;
>> -       LIST_HEAD(entries);
>> -       LIST_HEAD(before);
>> +       first->prev->next = last->next;
>> +       last->next->prev = first->prev;
>>
>> -       reservation_object_assert_held(pos->last->resv);
>> -       list = is_swap ? &pos->last->swap : &pos->last->lru;
>> -       list_cut_position(&entries, lru, list);
>> +       list->prev->next = first;
>> +       first->prev = list->prev;
>>
>> -       reservation_object_assert_held(pos->first->resv);
>> -       list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>> -       list_cut_position(&before, &entries, list);
>> -
>> -       list_splice(&before, lru);
>> -       list_splice_tail(&entries, lru);
>> +       last->next = list;
>> +       list->prev = last;
>>   }
>>
>>   void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>> @@ -271,23 +266,33 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>>          unsigned i;
>>
>>          for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>> +               struct ttm_lru_bulk_move_pos *pos = &bulk->tt[i];
>>                  struct ttm_mem_type_manager *man;
>>
>> -               if (!bulk->tt[i].first)
>> +               if (!pos->first)
>>                          continue;
>>
>> -               man = &bulk->tt[i].first->bdev->man[TTM_PL_TT];
>> -               ttm_bo_bulk_move_helper(&bulk->tt[i], &man->lru[i], false);
>> +               reservation_object_assert_held(pos->first->resv);
>> +               reservation_object_assert_held(pos->last->resv);
>> +
>> +               man = &pos->first->bdev->man[TTM_PL_TT];
>> +               ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
>> +                                       &pos->last->lru);
>>          }
>>
>>          for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>> +               struct ttm_lru_bulk_move_pos *pos = &bulk->vram[i];
>>                  struct ttm_mem_type_manager *man;
>>
>> -               if (!bulk->vram[i].first)
>> +               if (!pos->first)
>>                          continue;
>>
>> -               man = &bulk->vram[i].first->bdev->man[TTM_PL_VRAM];
>> -               ttm_bo_bulk_move_helper(&bulk->vram[i], &man->lru[i], false);
>> +               reservation_object_assert_held(pos->first->resv);
>> +               reservation_object_assert_held(pos->last->resv);
>> +
>> +               man = &pos->first->bdev->man[TTM_PL_VRAM];
>> +               ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
>> +                                       &pos->last->lru);
>>          }
>>
>>          for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>> @@ -297,8 +302,12 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
>>                  if (!pos->first)
>>                          continue;
>>
>> +               reservation_object_assert_held(pos->first->resv);
>> +               reservation_object_assert_held(pos->last->resv);
>> +
>>                  lru = &pos->first->bdev->glob->swap_lru[i];
>> -               ttm_bo_bulk_move_helper(&bulk->swap[i], lru, true);
>> +               ttm_list_move_bulk_tail(lru, &pos->first->swap,
>> +                                       &pos->last->swap);
>>          }
>>   }
>>   EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail);
>> --
>> 2.14.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/ttm: once more fix ttm_bo_bulk_move_lru_tail

[Huang Rui]

On Thu, Sep 13, 2018 at 07:32:24PM +0800, Christian König wrote:
> Am 13.09.2018 um 10:31 schrieb Huang Rui:
> > On Wed, Sep 12, 2018 at 09:23:55PM +0200, Christian König wrote:
> >> While cutting the lists we sometimes accidentally added a list_head from
> >> the stack to the LRUs, effectively corrupting the list.
> >>
> >> Remove the list cutting and use explicit list manipulation instead.
> > This patch actually fixes the corruption bug. Was it a defect of
> > list_cut_position or list_splice handlers?
> 
> We somehow did something illegal with list_cut_position. I haven't 
> narrowed it down till the end, but we ended up with list_heads from the 
> stack to the lru.
> 

I am confused, in theory, even we do any manipulation with list helper, it
should not trigger the list corruption. The usage of those helpers should
ensure the list operation safely...

> Anyway adding a specialized list bulk move function is much simpler and 
> avoids the issue.
> 
> I've just split that up as Michel suggested and send it out to the 
> mailing lists, please review that version once more.
> 

Sure, already reviewed.

> Thanks,
> Christian.
> 
> >
> > Reviewed-and-Tested: Huang Rui <ray.huang@amd.com>
> >
> >> Signed-off-by: Christian König <christian.koenig@amd.com>
> >> ---
> >>   drivers/gpu/drm/ttm/ttm_bo.c | 51 ++++++++++++++++++++++++++------------------
> >>   1 file changed, 30 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> >> index 138c98902033..b2a33bf1ef10 100644
> >> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> >> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> >> @@ -247,23 +247,18 @@ void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo,
> >>   }
> >>   EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
> >>   
> >> -static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
> >> -				    struct list_head *lru, bool is_swap)
> >> +static void ttm_list_move_bulk_tail(struct list_head *list,
> >> +				    struct list_head *first,
> >> +				    struct list_head *last)
> >>   {
> >> -	struct list_head *list;
> >> -	LIST_HEAD(entries);
> >> -	LIST_HEAD(before);
> >> +	first->prev->next = last->next;
> >> +	last->next->prev = first->prev;
> >>   
> >> -	reservation_object_assert_held(pos->last->resv);
> >> -	list = is_swap ? &pos->last->swap : &pos->last->lru;
> >> -	list_cut_position(&entries, lru, list);
> >> +	list->prev->next = first;
> >> +	first->prev = list->prev;
> >>   
> >> -	reservation_object_assert_held(pos->first->resv);
> >> -	list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
> >> -	list_cut_position(&before, &entries, list);
> >> -
> >> -	list_splice(&before, lru);
> >> -	list_splice_tail(&entries, lru);
> >> +	last->next = list;
> >> +	list->prev = last;
> >>   }
> >>   
> >>   void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
> >> @@ -271,23 +266,33 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
> >>   	unsigned i;
> >>   
> >>   	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> >> +		struct ttm_lru_bulk_move_pos *pos = &bulk->tt[i];
> >>   		struct ttm_mem_type_manager *man;
> >>   
> >> -		if (!bulk->tt[i].first)
> >> +		if (!pos->first)
> >>   			continue;
> >>   
> >> -		man = &bulk->tt[i].first->bdev->man[TTM_PL_TT];
> >> -		ttm_bo_bulk_move_helper(&bulk->tt[i], &man->lru[i], false);
> >> +		reservation_object_assert_held(pos->first->resv);
> >> +		reservation_object_assert_held(pos->last->resv);
> >> +
> >> +		man = &pos->first->bdev->man[TTM_PL_TT];
> >> +		ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
> >> +					&pos->last->lru);
> >>   	}
> >>   
> >>   	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> >> +		struct ttm_lru_bulk_move_pos *pos = &bulk->vram[i];
> >>   		struct ttm_mem_type_manager *man;
> >>   
> >> -		if (!bulk->vram[i].first)
> >> +		if (!pos->first)
> >>   			continue;
> >>   
> >> -		man = &bulk->vram[i].first->bdev->man[TTM_PL_VRAM];
> >> -		ttm_bo_bulk_move_helper(&bulk->vram[i], &man->lru[i], false);
> >> +		reservation_object_assert_held(pos->first->resv);
> >> +		reservation_object_assert_held(pos->last->resv);
> >> +
> >> +		man = &pos->first->bdev->man[TTM_PL_VRAM];
> >> +		ttm_list_move_bulk_tail(&man->lru[i], &pos->first->lru,
> >> +					&pos->last->lru);
> >>   	}
> >>   
> >>   	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> >> @@ -297,8 +302,12 @@ void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk)
> >>   		if (!pos->first)
> >>   			continue;
> >>   
> >> +		reservation_object_assert_held(pos->first->resv);
> >> +		reservation_object_assert_held(pos->last->resv);
> >> +
> >>   		lru = &pos->first->bdev->glob->swap_lru[i];
> >> -		ttm_bo_bulk_move_helper(&bulk->swap[i], lru, true);
> >> +		ttm_list_move_bulk_tail(lru, &pos->first->swap,
> >> +					&pos->last->swap);
> >>   	}
> >>   }
> >>   EXPORT_SYMBOL(ttm_bo_bulk_move_lru_tail);
> >> -- 
> >> 2.14.1
> >>
> >> _______________________________________________
> >> amd-gfx mailing list
> >> amd-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/ttm: once more fix ttm_bo_bulk_move_lru_tail

[Michel Dänzer]

On 2018-09-14 10:22 a.m., Huang Rui wrote:
> On Thu, Sep 13, 2018 at 07:32:24PM +0800, Christian König wrote:
>> Am 13.09.2018 um 10:31 schrieb Huang Rui:
>>> On Wed, Sep 12, 2018 at 09:23:55PM +0200, Christian König wrote:
>>>> While cutting the lists we sometimes accidentally added a list_head from
>>>> the stack to the LRUs, effectively corrupting the list.
>>>>
>>>> Remove the list cutting and use explicit list manipulation instead.
>>> This patch actually fixes the corruption bug. Was it a defect of
>>> list_cut_position or list_splice handlers?
>>
>> We somehow did something illegal with list_cut_position. I haven't 
>> narrowed it down till the end, but we ended up with list_heads from the 
>> stack to the lru.
> 
> I am confused, in theory, even we do any manipulation with list helper, it
> should not trigger the list corruption. The usage of those helpers should
> ensure the list operation safely...

There's nothing the helpers can do about being passed in pointers to
stack memory. It's a bug in the code using the helpers.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/ttm: once more fix ttm_bo_bulk_move_lru_tail

[Christian König]

Am 14.09.2018 um 11:22 schrieb Michel Dänzer:
> On 2018-09-14 10:22 a.m., Huang Rui wrote:
>> On Thu, Sep 13, 2018 at 07:32:24PM +0800, Christian König wrote:
>>> Am 13.09.2018 um 10:31 schrieb Huang Rui:
>>>> On Wed, Sep 12, 2018 at 09:23:55PM +0200, Christian König wrote:
>>>>> While cutting the lists we sometimes accidentally added a list_head from
>>>>> the stack to the LRUs, effectively corrupting the list.
>>>>>
>>>>> Remove the list cutting and use explicit list manipulation instead.
>>>> This patch actually fixes the corruption bug. Was it a defect of
>>>> list_cut_position or list_splice handlers?
>>> We somehow did something illegal with list_cut_position. I haven't
>>> narrowed it down till the end, but we ended up with list_heads from the
>>> stack to the lru.
>> I am confused, in theory, even we do any manipulation with list helper, it
>> should not trigger the list corruption. The usage of those helpers should
>> ensure the list operation safely...
> There's nothing the helpers can do about being passed in pointers to
> stack memory. It's a bug in the code using the helpers.

Actually I'm not 100% sure of that. To me it looks like we hit a corner 
case list_cut_position doesn't support.

Or we indeed had a logic error in how we called it, anyway the explicit 
implementation only uses 6 assignments and so is much easier to handle.

Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/ttm: once more fix ttm_bo_bulk_move_lru_tail

[Huang Rui]

On Fri, Sep 14, 2018 at 05:22:16PM +0800, Michel Dänzer wrote:
> On 2018-09-14 10:22 a.m., Huang Rui wrote:
> > On Thu, Sep 13, 2018 at 07:32:24PM +0800, Christian König wrote:
> >> Am 13.09.2018 um 10:31 schrieb Huang Rui:
> >>> On Wed, Sep 12, 2018 at 09:23:55PM +0200, Christian König wrote:
> >>>> While cutting the lists we sometimes accidentally added a list_head from
> >>>> the stack to the LRUs, effectively corrupting the list.
> >>>>
> >>>> Remove the list cutting and use explicit list manipulation instead.
> >>> This patch actually fixes the corruption bug. Was it a defect of
> >>> list_cut_position or list_splice handlers?
> >>
> >> We somehow did something illegal with list_cut_position. I haven't 
> >> narrowed it down till the end, but we ended up with list_heads from the 
> >> stack to the lru.
> > 
> > I am confused, in theory, even we do any manipulation with list helper, it
> > should not trigger the list corruption. The usage of those helpers should
> > ensure the list operation safely...
> 
> There's nothing the helpers can do about being passed in pointers to
> stack memory. It's a bug in the code using the helpers.
> 

Actually, I was checking carefully with list cut and splice in our case,
and it didn't find any illegal use. However, I also agree with the explicit
list manipulation is more clear and simple for now.

Thanks,
Ray
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx