<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=windows-1252"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">Am 07.09.2018 um 13:02 schrieb Huang, Ray: </div> <blockquote type="cite" cite="mid:CY1PR12MB0043539F7EE805B3A98E1FE2EC000@CY1PR12MB0043.namprd12.prod.outlook.com"> <pre wrap=""> </pre> <blockquote type="cite"> <blockquote type="cite"> <pre wrap=""> Yes, that was one problem. Another was that the cutting code was buggy and determined one of the positions to cut at the wrong time. </pre> </blockquote> </blockquote> <pre wrap=""> I went through again about the list cutting behavior and wrote down with the attached picture. After do the second list_cut_position, the list2 should be point the end of "before" list. And list2 won't be used anymore after list cutting. May I know am I something missed? </pre> </blockquote> Let's take a look at the original code: <blockquote type="cite">list1 = is_swap ? &pos->last->swap : &pos->last->lru; list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev; list_cut_position(&entries, lru, list1); list_cut_position(&before, &entries, list2); </blockquote> As far as I can see the problem is that the first list_cur_position could modify the value of pos->first->lru.prev and so make the second list_cut_position work on the wrong position. Regards, Christian. <blockquote type="cite" cite="mid:CY1PR12MB0043539F7EE805B3A98E1FE2EC000@CY1PR12MB0043.namprd12.prod.outlook.com"> <pre wrap=""> Thanks, Ray From: amd-gfx <a class="moz-txt-link-rfc2396E" href="mailto:amd-gfx-bounces@lists.freedesktop.org"><amd-gfx-bounces@lists.freedesktop.org></a> on behalf of Christian König <a class="moz-txt-link-rfc2396E" href="mailto:christian.koenig@amd.com"><christian.koenig@amd.com></a> Sent: Thursday, September 6, 2018 6:06 PM To: Huang, Ray Cc: Michel Dänzer; <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> Subject: Re: [PATCH 1/3] drm/ttm: fix ttm_bo_bulk_move_helper Am 06.09.2018 um 12:02 schrieb Huang Rui: </pre> <blockquote type="cite"> <pre wrap="">On Fri, Aug 31, 2018 at 05:17:33PM +0200, Christian König wrote: </pre> <blockquote type="cite"> <pre wrap="">Am 31.08.2018 um 17:15 schrieb Michel Dänzer: </pre> <blockquote type="cite"> <pre wrap="">On 2018-08-31 3:10 p.m., Christian König wrote: </pre> <blockquote type="cite"> <pre wrap="">Staring at the function for six hours, just to essentially move one line of code. </pre> </blockquote> <pre wrap="">That sucks, but the commit log should describe what the problem was and how this patch solves it. </pre> <blockquote type="cite"> <pre wrap="">Signed-off-by: Christian König <a class="moz-txt-link-rfc2396E" href="mailto:christian.koenig@amd.com"><christian.koenig@amd.com></a> --- drivers/gpu/drm/ttm/ttm_bo.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 35d53d81f486..138c98902033 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -250,15 +250,18 @@ 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) { + struct list_head *list; LIST_HEAD(entries); LIST_HEAD(before); - struct list_head *list1, *list2; - list1 = is_swap ? &pos->last->swap : &pos->last->lru; - list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev; + reservation_object_assert_held(pos->last->resv); + list = is_swap ? &pos->last->swap : &pos->last->lru; + list_cut_position(&entries, lru, list); + + reservation_object_assert_held(pos->first->resv); + list = is_swap ? pos->first->swap.prev : pos->first->lru.prev; + list_cut_position(&before, &entries, list); </pre> </blockquote> <pre wrap="">So the problem was that the first list_cut_position call could result in list2 pointing to la-la-land? Good catch! </pre> </blockquote> <pre wrap="">Yes, exactly. Thought that would be obvious, but going to add that to the commit log. Can I get a tested-by? You where much better at reproducing that than I'm. </pre> </blockquote> <pre wrap="">Michel, Christian, thanks so much to take care of this when I was on vacation. And sorry to let you take a long time for finding the cause. Is that because I didn't hold the resveration before cut the list from position "first" and "last"? </pre> </blockquote> <pre wrap=""> Yes, that was one problem. Another was that the cutting code was buggy and determined one of the positions to cut at the wrong time. </pre> <blockquote type="cite"> <pre wrap=""> May I know in which cases, we must hold the bo's reservation firstly? </pre> </blockquote> <pre wrap=""> BOs are reserved to prevent moving them. E.g. when the BO isn't reserved it can move around and so the LRU where you want to add/remove it could change. Christian. </pre> <blockquote type="cite"> <pre wrap=""> Thanks, Ray </pre> </blockquote> <pre wrap=""> _______________________________________________ amd-gfx mailing list <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> <a class="moz-txt-link-freetext" href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a> amd-gfx Info Page - freedesktop.org lists.freedesktop.org To see the collection of prior postings to the list, visit the amd-gfx Archives.. Using amd-gfx: To post a message to all the list members, send email to <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a>. You can subscribe to the list, or change your existing subscription, in the sections below. </pre> <fieldset class="mimeAttachmentHeader"></fieldset> <pre wrap="">_______________________________________________ amd-gfx mailing list <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> <a class="moz-txt-link-freetext" href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a> </pre> </blockquote> </body> </html>