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

["Christian König" <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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