[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b51769db-0344-5817-7daf-a2e39c5d2644@nvidia.com>
Date: Sun, 4 Sep 2022 15:21:57 -0700
From: John Hubbard <jhubbard@...dia.com>
To: Peter Xu <peterx@...hat.com>, Yang Shi <shy828301@...il.com>
Cc: david@...hat.com, kirill.shutemov@...ux.intel.com, jgg@...dia.com,
hughd@...gle.com, akpm@...ux-foundation.org, linux-mm@...ck.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: gup: fix the fast GUP race against THP collapse
On 9/2/22 08:59, Peter Xu wrote:
...
> Would you mind rewording this comment a bit? I feel it a bit weird to
> suddenly mention about thp collapse especially its details.
>
> Maybe some statement on the whole history of why check pte, and in what
> case pmd check is needed (where the thp collapse example can be moved to,
> imho)?
>
> One of my attempt for reference..
>
> /*
> * Fast-gup relies on pte change detection to avoid
> * concurrent pgtable operations.
> *
> * To pin the page, fast-gup needs to do below in order:
> * (1) pin the page (by prefetching pte), then (2) check
> * pte not changed.
> *
> * For the rest of pgtable operations where pgtable updates
> * can be racy with fast-gup, we need to do (1) clear pte,
> * then (2) check whether page is pinned.
> *
> * Above will work for all pte-level operations, including
> * thp split.
> *
> * For thp collapse, it's a bit more complicated because
> * with RCU pgtable free fast-gup can be walking a pgtable
> * page that is being freed (so pte is still valid but pmd
> * can be cleared already). To avoid race in such
> * condition, we need to also check pmd here to make sure
> * pmd doesn't change (corresponds to pmdp_collapse_flush()
> * in the thp collide code path).
> */
>
> If you agree with the comment change, feel free to add:
>
> Acked-by: Peter Xu <peterx@...hat.com>
>
This seems fine, but I'd like to additionally request that we move it
to function-level documentation. Because it expands the length of the
function, which previously fit neatly on my screen. So I think it's
time to move it up a level.
thanks,
--
John Hubbard
NVIDIA
Powered by blists - more mailing lists