diff mbox series

[v2,17/46] hugetlbfs: do a full walk to check if vma maps a page

Message ID 20230218002819.1486479-18-jthoughton@google.com
State New
Headers
Series hugetlb: introduce HugeTLB high-granularity mapping |

Commit Message

James Houghton Feb. 18, 2023, 12:27 a.m. UTC 
  Because it is safe to do so, do a full high-granularity page table walk
to check if the page is mapped.

Signed-off-by: James Houghton <jthoughton@google.com>

Comments

James Houghton Feb. 22, 2023, 3:46 p.m. UTC | #1 
On Fri, Feb 17, 2023 at 4:29 PM James Houghton <jthoughton@google.com> wrote:
>
> Because it is safe to do so, do a full high-granularity page table walk
> to check if the page is mapped.
>
> Signed-off-by: James Houghton <jthoughton@google.com>
>
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index cfd09f95551b..c0ee69f0418e 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -386,17 +386,24 @@ static void hugetlb_delete_from_page_cache(struct folio *folio)
>  static bool hugetlb_vma_maps_page(struct vm_area_struct *vma,
>                                 unsigned long addr, struct page *page)
>  {
> -       pte_t *ptep, pte;
> +       pte_t pte;
> +       struct hugetlb_pte hpte;
>
> -       ptep = hugetlb_walk(vma, addr, huge_page_size(hstate_vma(vma)));
> -       if (!ptep)
> +       if (hugetlb_full_walk(&hpte, vma, addr))
>                 return false;
>
> -       pte = huge_ptep_get(ptep);
> +       pte = huge_ptep_get(hpte.ptep);
>         if (huge_pte_none(pte) || !pte_present(pte))
>                 return false;
>
> -       if (pte_page(pte) == page)
> +       if (unlikely(!hugetlb_pte_present_leaf(&hpte, pte)))
> +               /*
> +                * We raced with someone splitting us, and the only case
> +                * where this is impossible is when the pte was none.
> +                */
> +               return false;
> +
> +       if (compound_head(pte_page(pte)) == page)
>                 return true;
>
>         return false;
> --
> 2.39.2.637.g21b0678d19-goog
>

I think this patch is actually incorrect.

This function is *supposed* to check if the page is mapped at all in
this VMA, but really we're only checking if the base address of the
page is mapped. If we did the 'hugetlb_vma_maybe_maps_page' approach
that I did previously and returned 'true' if
!hugetlb_pte_present_leaf(), then this code would be correct again.

But what I really think this function should do is just call
page_vma_mapped_walk(). We're sort of reimplementing it here anyway.
Unless someone disagrees, I'll do this for v3.
Mike Kravetz Feb. 28, 2023, 11:52 p.m. UTC | #2 
On 02/22/23 07:46, James Houghton wrote:
> On Fri, Feb 17, 2023 at 4:29 PM James Houghton <jthoughton@google.com> wrote:
> >
> > Because it is safe to do so, do a full high-granularity page table walk
> > to check if the page is mapped.
> >
> > Signed-off-by: James Houghton <jthoughton@google.com>
> >
> > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> > index cfd09f95551b..c0ee69f0418e 100644
> > --- a/fs/hugetlbfs/inode.c
> > +++ b/fs/hugetlbfs/inode.c
> > @@ -386,17 +386,24 @@ static void hugetlb_delete_from_page_cache(struct folio *folio)
> >  static bool hugetlb_vma_maps_page(struct vm_area_struct *vma,
> >                                 unsigned long addr, struct page *page)
> >  {
> > -       pte_t *ptep, pte;
> > +       pte_t pte;
> > +       struct hugetlb_pte hpte;
> >
> > -       ptep = hugetlb_walk(vma, addr, huge_page_size(hstate_vma(vma)));
> > -       if (!ptep)
> > +       if (hugetlb_full_walk(&hpte, vma, addr))
> >                 return false;
> >
> > -       pte = huge_ptep_get(ptep);
> > +       pte = huge_ptep_get(hpte.ptep);
> >         if (huge_pte_none(pte) || !pte_present(pte))
> >                 return false;
> >
> > -       if (pte_page(pte) == page)
> > +       if (unlikely(!hugetlb_pte_present_leaf(&hpte, pte)))
> > +               /*
> > +                * We raced with someone splitting us, and the only case
> > +                * where this is impossible is when the pte was none.
> > +                */
> > +               return false;
> > +
> > +       if (compound_head(pte_page(pte)) == page)
> >                 return true;
> >
> >         return false;
> > --
> > 2.39.2.637.g21b0678d19-goog
> >
> 
> I think this patch is actually incorrect.
> 
> This function is *supposed* to check if the page is mapped at all in
> this VMA, but really we're only checking if the base address of the
> page is mapped.

The function is/was only checking if the page is mapped at the specific
address.  That is because when walking the interval tree, we know where
it would be mapped and only check there.

I suppose it would still be functionally correct if we checked for the
page being mapped anywhere in the vma.

>                 If we did the 'hugetlb_vma_maybe_maps_page' approach
> that I did previously and returned 'true' if
> !hugetlb_pte_present_leaf(), then this code would be correct again.
> 
> But what I really think this function should do is just call
> page_vma_mapped_walk(). We're sort of reimplementing it here anyway.
> Unless someone disagrees, I'll do this for v3.

Yes, I think page_vma_mapped_walk would provide the same functionality.
I did not consider this when writing hugetlb_vma_maps_page, and
hugetlb_vma_maps_page was pretty simple for the current hugetlb
possibilities.  Things get more complicated with HGM.
diff mbox series

Patch

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index cfd09f95551b..c0ee69f0418e 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -386,17 +386,24 @@  static void hugetlb_delete_from_page_cache(struct folio *folio)
 static bool hugetlb_vma_maps_page(struct vm_area_struct *vma,
 				unsigned long addr, struct page *page)
 {
-	pte_t *ptep, pte;
+	pte_t pte;
+	struct hugetlb_pte hpte;
 
-	ptep = hugetlb_walk(vma, addr, huge_page_size(hstate_vma(vma)));
-	if (!ptep)
+	if (hugetlb_full_walk(&hpte, vma, addr))
 		return false;
 
-	pte = huge_ptep_get(ptep);
+	pte = huge_ptep_get(hpte.ptep);
 	if (huge_pte_none(pte) || !pte_present(pte))
 		return false;
 
-	if (pte_page(pte) == page)
+	if (unlikely(!hugetlb_pte_present_leaf(&hpte, pte)))
+		/*
+		 * We raced with someone splitting us, and the only case
+		 * where this is impossible is when the pte was none.
+		 */
+		return false;
+
+	if (compound_head(pte_page(pte)) == page)
 		return true;
 
 	return false;