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@xxxxxxxxxx> > 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
