On 12/09/22 11:02, James Houghton wrote: > On Wed, Dec 7, 2022 at 7:46 PM Mina Almasry <almasrymina@xxxxxxxxxx> wrote: > > On Fri, Oct 21, 2022 at 9:37 AM James Houghton <jthoughton@xxxxxxxxxx> wrote: > > > > > > +bool hugetlb_pte_present_leaf(const struct hugetlb_pte *hpte, pte_t pte) > > > > I also don't know if this is obvious to other readers, but I'm quite > > confused that we pass both hugetlb_pte and pte_t here, especially when > > hpte has a pte_t inside of it. Maybe a comment would help. > > It's possible for the value of the pte to change if we haven't locked > the PTL; we only store a pte_t* in hugetlb_pte, not the value itself. I had comments similar to Mina and Peter on other parts of this patch. Calling this without some type of locking is 'interesting'. I have not yet looked at callers (without locking), but I assume such callers can handle stale results. > Thinking about this... we *do* store `shift` which technically depends > on the value of the PTE. If the PTE is pte_none, the true `shift` of > the PTE is ambiguous, and so we just provide what the user asked for. > That could lead to a scenario where UFFDIO_CONTINUE(some 4K page) then > UFFDIO_CONTINUE(CONT_PTE_SIZE range around that page) can both succeed > because we merely check if the *first* PTE in the contiguous bunch is > none/has changed. Right, Yuck! > > So, in the case of a contiguous PTE where we *think* we're overwriting > a bunch of none PTEs, we need to check that each PTE we're overwriting > is still none while holding the PTL. That means that the PTL we use > for cont PTEs and non-cont PTEs of the same level must be the same. > > So for the next version, I'll: > - add some requirement that contiguous and non-contiguous PTEs on the > same level must use the same PTL > - think up some kind of API like all_contig_ptes_none(), but it only > really applies for arm64, so I think actually putting it in can wait. > I'll at least put a comment in hugetlb_mcopy_atomic_pte and > hugetlb_no_page (near the final huge_pte_none() and pte_same() > checks). > -- Mike Kravetz
