[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 at 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.