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 <[email protected]> > > > >> Signed-off-by: Christian König <[email protected]> > >> --- > >> 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 > >> [email protected] > >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
