[v2,2/4] mm/gup: handle ZONE_DEVICE pages in folio_fast_pin_allowed()

Commit Message

Martin Oliveira June 11, 2024, 6:27 p.m. UTC

folio_fast_pin_allowed() does not support ZONE_DEVICE pages because
currently it is impossible for that type of page to be used with
FOLL_LONGTERM. When this changes in a subsequent patch, this path will
attempt to read the mapping of a ZONE_DEVICE page which is not valid.

Instead, allow ZONE_DEVICE pages explicitly seeing they shouldn't pose
any problem with the fast path.

Co-developed-by: Logan Gunthorpe <[e-mail]>
Signed-off-by: Logan Gunthorpe <[e-mail]>
Signed-off-by: Martin Oliveira <[e-mail]>
---
 mm/gup.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

John Hubbard June 15, 2024, 2:40 a.m. UTC | #1

On 6/11/24 11:27 AM, Martin Oliveira wrote:
> folio_fast_pin_allowed() does not support ZONE_DEVICE pages because

s/folio_fast_pin_allowed/gup_fast_folio_allowed/ ?

> currently it is impossible for that type of page to be used with
> FOLL_LONGTERM. When this changes in a subsequent patch, this path will
> attempt to read the mapping of a ZONE_DEVICE page which is not valid.
> 
> Instead, allow ZONE_DEVICE pages explicitly seeing they shouldn't pose
> any problem with the fast path.
> 
> Co-developed-by: Logan Gunthorpe <[e-mail]>
> Signed-off-by: Logan Gunthorpe <[e-mail]>
> Signed-off-by: Martin Oliveira <[e-mail]>
> ---
>   mm/gup.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index ca0f5cedce9b2..00d0a77112f4f 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2847,6 +2847,10 @@ static bool gup_fast_folio_allowed(struct folio *folio, unsigned int flags)
>   	if (folio_test_hugetlb(folio))
>   		return true;
>   
> +	/* It makes no sense to access the mapping of ZONE_DEVICE pages */

This comment is very difficult, because it states that one cannot
do something, right before explicitly enable something else. And the
reader is given little help on connecting the two.

And there are several subtypes of ZONE_DEVICE. Is it really true that
none of them can be mapped to user space? For p2p BAR1 mappings, those
actually go to user space, yes? Confused, need help. :)

> +	if (folio_is_zone_device(folio))
> +		return true;
> +
>   	/*
>   	 * GUP-fast disables IRQs. When IRQS are disabled, RCU grace periods
>   	 * cannot proceed, which means no actions performed under RCU can

thanks,

Martin Oliveira June 26, 2024, 10:23 p.m. UTC | #2

Hi John,

Thanks for your comments and sorry for the delayed response, I was off the
last few days.

On 2024-06-14 20:40, John Hubbard wrote:
> s/folio_fast_pin_allowed/gup_fast_folio_allowed/ ?

Nice catch! That function got renamed from the original work
we did on this series.

> This comment is very difficult, because it states that one cannot
> do something, right before explicitly enable something else. And the
> reader is given little help on connecting the two.
> 
> And there are several subtypes of ZONE_DEVICE. Is it really true that
> none of them can be mapped to user space? For p2p BAR1 mappings, those
> actually go to user space, yes? Confused, need help. :)
This is a fair point, I had only looked at p2p but I can't say anything
about the other subtypes of ZONE_DEVICE.

For p2p, yes, they will go to userspace, however folio->mapping was NULL
for those cases. And hence fast path would reject it.

In any case, I think we could drop this patch for now and revisit when/if
required, the regular gup path was not measurably slower.

Thanks,
Martin

Patch

diff --git a/mm/gup.c b/mm/gup.c
index ca0f5cedce9b2..00d0a77112f4f 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2847,6 +2847,10 @@  static bool gup_fast_folio_allowed(struct folio *folio, unsigned int flags)
 	if (folio_test_hugetlb(folio))
 		return true;
 
+	/* It makes no sense to access the mapping of ZONE_DEVICE pages */
+	if (folio_is_zone_device(folio))
+		return true;
+
 	/*
 	 * GUP-fast disables IRQs. When IRQS are disabled, RCU grace periods
 	 * cannot proceed, which means no actions performed under RCU can