Previously dax pages were skipped by the pagewalk code as pud_special() or
vm_normal_page{_pmd}() would be false for DAX pages. Now that dax pages are
refcounted normally that is no longer the case, so add explicit checks to
skip them.

Signed-off-by: Alistair Popple <apopple@nvidia.com>
---
 include/linux/memremap.h | 11 +++++++++++
 mm/pagewalk.c            | 12 ++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/include/linux/memremap.h b/include/linux/memremap.h
index 4aa1519..54e8b57 100644
--- a/include/linux/memremap.h
+++ b/include/linux/memremap.h
@@ -198,6 +198,17 @@ static inline bool folio_is_fsdax(const struct folio *folio)
 	return is_fsdax_page(&folio->page);
 }
 
+static inline bool is_devdax_page(const struct page *page)
+{
+	return is_zone_device_page(page) &&
+		page_pgmap(page)->type == MEMORY_DEVICE_GENERIC;
+}
+
+static inline bool folio_is_devdax(const struct folio *folio)
+{
+	return is_devdax_page(&folio->page);
+}
+
 #ifdef CONFIG_ZONE_DEVICE
 void zone_device_page_init(struct page *page);
 void *memremap_pages(struct dev_pagemap *pgmap, int nid);
diff --git a/mm/pagewalk.c b/mm/pagewalk.c
index e478777..0dfb9c2 100644
--- a/mm/pagewalk.c
+++ b/mm/pagewalk.c
@@ -884,6 +884,12 @@ struct folio *folio_walk_start(struct folio_walk *fw,
 		 * support PUD mappings in VM_PFNMAP|VM_MIXEDMAP VMAs.
 		 */
 		page = pud_page(pud);
+
+		if (is_devdax_page(page)) {
+			spin_unlock(ptl);
+			goto not_found;
+		}
+
 		goto found;
 	}
 
@@ -911,7 +917,8 @@ struct folio *folio_walk_start(struct folio_walk *fw,
 			goto pte_table;
 		} else if (pmd_present(pmd)) {
 			page = vm_normal_page_pmd(vma, addr, pmd);
-			if (page) {
+			if (page && !is_devdax_page(page) &&
+			    !is_fsdax_page(page)) {
 				goto found;
 			} else if ((flags & FW_ZEROPAGE) &&
 				    is_huge_zero_pmd(pmd)) {
@@ -945,7 +952,8 @@ struct folio *folio_walk_start(struct folio_walk *fw,
 
 	if (pte_present(pte)) {
 		page = vm_normal_page(vma, addr, pte);
-		if (page)
+		if (page && !is_devdax_page(page) &&
+		    !is_fsdax_page(page))
 			goto found;
 		if ((flags & FW_ZEROPAGE) &&
 		    is_zero_pfn(pte_pfn(pte))) {
-- 
git-series 0.9.1

On Thu, May 29, 2025 at 04:32:04PM +1000, Alistair Popple wrote:
> Previously dax pages were skipped by the pagewalk code as pud_special() or
> vm_normal_page{_pmd}() would be false for DAX pages. Now that dax pages are
> refcounted normally that is no longer the case, so add explicit checks to
> skip them.
>
> Signed-off-by: Alistair Popple <apopple@nvidia.com>
> ---
>  include/linux/memremap.h | 11 +++++++++++
>  mm/pagewalk.c            | 12 ++++++++++--
>  2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/memremap.h b/include/linux/memremap.h
> index 4aa1519..54e8b57 100644
> --- a/include/linux/memremap.h
> +++ b/include/linux/memremap.h
> @@ -198,6 +198,17 @@ static inline bool folio_is_fsdax(const struct folio *folio)
>  	return is_fsdax_page(&folio->page);
>  }
>
> +static inline bool is_devdax_page(const struct page *page)
> +{
> +	return is_zone_device_page(page) &&
> +		page_pgmap(page)->type == MEMORY_DEVICE_GENERIC;
> +}
> +
> +static inline bool folio_is_devdax(const struct folio *folio)
> +{
> +	return is_devdax_page(&folio->page);
> +}
> +
>  #ifdef CONFIG_ZONE_DEVICE
>  void zone_device_page_init(struct page *page);
>  void *memremap_pages(struct dev_pagemap *pgmap, int nid);
> diff --git a/mm/pagewalk.c b/mm/pagewalk.c
> index e478777..0dfb9c2 100644
> --- a/mm/pagewalk.c
> +++ b/mm/pagewalk.c
> @@ -884,6 +884,12 @@ struct folio *folio_walk_start(struct folio_walk *fw,
>  		 * support PUD mappings in VM_PFNMAP|VM_MIXEDMAP VMAs.
>  		 */
>  		page = pud_page(pud);
> +
> +		if (is_devdax_page(page)) {

Is it only devdax that can exist at PUD leaf level, not fsdax?

> +			spin_unlock(ptl);
> +			goto not_found;
> +		}
> +
>  		goto found;
>  	}
>
> @@ -911,7 +917,8 @@ struct folio *folio_walk_start(struct folio_walk *fw,
>  			goto pte_table;
>  		} else if (pmd_present(pmd)) {
>  			page = vm_normal_page_pmd(vma, addr, pmd);
> -			if (page) {
> +			if (page && !is_devdax_page(page) &&
> +			    !is_fsdax_page(page)) {
>  				goto found;
>  			} else if ((flags & FW_ZEROPAGE) &&
>  				    is_huge_zero_pmd(pmd)) {
> @@ -945,7 +952,8 @@ struct folio *folio_walk_start(struct folio_walk *fw,
>
>  	if (pte_present(pte)) {
>  		page = vm_normal_page(vma, addr, pte);
> -		if (page)
> +		if (page && !is_devdax_page(page) &&
> +		    !is_fsdax_page(page))
>  			goto found;
>  		if ((flags & FW_ZEROPAGE) &&
>  		    is_zero_pfn(pte_pfn(pte))) {

I'm probably echoing others here (and I definitely particularly like Dan's
suggestion of a helper function here, and Jason's suggestion of explanatory
comments), but would also be nice to not have to do this separately at each page
table level and instead have something that you can say 'get me normal non-dax
page at page table level <parameter>'.

> --
> git-series 0.9.1

On Thu, Jun 12, 2025 at 03:15:31PM +0100, Lorenzo Stoakes wrote:
> On Thu, May 29, 2025 at 04:32:04PM +1000, Alistair Popple wrote:
> > Previously dax pages were skipped by the pagewalk code as pud_special() or
> > vm_normal_page{_pmd}() would be false for DAX pages. Now that dax pages are
> > refcounted normally that is no longer the case, so add explicit checks to
> > skip them.
> >
> > Signed-off-by: Alistair Popple <apopple@nvidia.com>
> > ---
> >  include/linux/memremap.h | 11 +++++++++++
> >  mm/pagewalk.c            | 12 ++++++++++--
> >  2 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/memremap.h b/include/linux/memremap.h
> > index 4aa1519..54e8b57 100644
> > --- a/include/linux/memremap.h
> > +++ b/include/linux/memremap.h
> > @@ -198,6 +198,17 @@ static inline bool folio_is_fsdax(const struct folio *folio)
> >  	return is_fsdax_page(&folio->page);
> >  }
> >
> > +static inline bool is_devdax_page(const struct page *page)
> > +{
> > +	return is_zone_device_page(page) &&
> > +		page_pgmap(page)->type == MEMORY_DEVICE_GENERIC;
> > +}
> > +
> > +static inline bool folio_is_devdax(const struct folio *folio)
> > +{
> > +	return is_devdax_page(&folio->page);
> > +}
> > +
> >  #ifdef CONFIG_ZONE_DEVICE
> >  void zone_device_page_init(struct page *page);
> >  void *memremap_pages(struct dev_pagemap *pgmap, int nid);
> > diff --git a/mm/pagewalk.c b/mm/pagewalk.c
> > index e478777..0dfb9c2 100644
> > --- a/mm/pagewalk.c
> > +++ b/mm/pagewalk.c
> > @@ -884,6 +884,12 @@ struct folio *folio_walk_start(struct folio_walk *fw,
> >  		 * support PUD mappings in VM_PFNMAP|VM_MIXEDMAP VMAs.
> >  		 */
> >  		page = pud_page(pud);
> > +
> > +		if (is_devdax_page(page)) {
> 
> Is it only devdax that can exist at PUD leaf level, not fsdax?

Correct.

> > +			spin_unlock(ptl);
> > +			goto not_found;
> > +		}
> > +
> >  		goto found;
> >  	}
> >
> > @@ -911,7 +917,8 @@ struct folio *folio_walk_start(struct folio_walk *fw,
> >  			goto pte_table;
> >  		} else if (pmd_present(pmd)) {
> >  			page = vm_normal_page_pmd(vma, addr, pmd);
> > -			if (page) {
> > +			if (page && !is_devdax_page(page) &&
> > +			    !is_fsdax_page(page)) {
> >  				goto found;
> >  			} else if ((flags & FW_ZEROPAGE) &&
> >  				    is_huge_zero_pmd(pmd)) {
> > @@ -945,7 +952,8 @@ struct folio *folio_walk_start(struct folio_walk *fw,
> >
> >  	if (pte_present(pte)) {
> >  		page = vm_normal_page(vma, addr, pte);
> > -		if (page)
> > +		if (page && !is_devdax_page(page) &&
> > +		    !is_fsdax_page(page))
> >  			goto found;
> >  		if ((flags & FW_ZEROPAGE) &&
> >  		    is_zero_pfn(pte_pfn(pte))) {
> 
> I'm probably echoing others here (and I definitely particularly like Dan's
> suggestion of a helper function here, and Jason's suggestion of explanatory
> comments), but would also be nice to not have to do this separately at each page
> table level and instead have something that you can say 'get me normal non-dax
> page at page table level <parameter>'.

I did the filtering here because I was trying to avoid unintended behavioural
changes and was being lazy by not auditing the callers. Turns out naming is
harder than doing this properly so I'm going to go with Jason and David's
suggestion and drop the filtering entirely. It will then be up to callers to
define what is "normal" for them by filtering out folio types they don't care
about. Most already do filter out zone device folios or DAX VMA's anyway, and I
will add some commentary to this effect in the respin.

> > --
> > git-series 0.9.1
>

Alistair Popple wrote:
> Previously dax pages were skipped by the pagewalk code as pud_special() or
> vm_normal_page{_pmd}() would be false for DAX pages. Now that dax pages are
> refcounted normally that is no longer the case, so add explicit checks to
> skip them.
> 
> Signed-off-by: Alistair Popple <apopple@nvidia.com>
> ---
>  include/linux/memremap.h | 11 +++++++++++
>  mm/pagewalk.c            | 12 ++++++++++--
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/memremap.h b/include/linux/memremap.h
> index 4aa1519..54e8b57 100644
> --- a/include/linux/memremap.h
> +++ b/include/linux/memremap.h
> @@ -198,6 +198,17 @@ static inline bool folio_is_fsdax(const struct folio *folio)
>  	return is_fsdax_page(&folio->page);
>  }
>  
> +static inline bool is_devdax_page(const struct page *page)
> +{
> +	return is_zone_device_page(page) &&
> +		page_pgmap(page)->type == MEMORY_DEVICE_GENERIC;
> +}
> +
> +static inline bool folio_is_devdax(const struct folio *folio)
> +{
> +	return is_devdax_page(&folio->page);
> +}
> +
>  #ifdef CONFIG_ZONE_DEVICE
>  void zone_device_page_init(struct page *page);
>  void *memremap_pages(struct dev_pagemap *pgmap, int nid);
> diff --git a/mm/pagewalk.c b/mm/pagewalk.c
> index e478777..0dfb9c2 100644
> --- a/mm/pagewalk.c
> +++ b/mm/pagewalk.c
> @@ -884,6 +884,12 @@ struct folio *folio_walk_start(struct folio_walk *fw,
>  		 * support PUD mappings in VM_PFNMAP|VM_MIXEDMAP VMAs.
>  		 */
>  		page = pud_page(pud);
> +
> +		if (is_devdax_page(page)) {
> +			spin_unlock(ptl);
> +			goto not_found;
> +		}
> +
>  		goto found;
>  	}
>  
> @@ -911,7 +917,8 @@ struct folio *folio_walk_start(struct folio_walk *fw,
>  			goto pte_table;
>  		} else if (pmd_present(pmd)) {
>  			page = vm_normal_page_pmd(vma, addr, pmd);
> -			if (page) {
> +			if (page && !is_devdax_page(page) &&
> +			    !is_fsdax_page(page)) {

It just looks awkward to say "yup, normal page, but not *that*
'normal'".

What about something like the below? Either way you can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 12d96659e8b4..4e549669166b 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2471,6 +2471,27 @@ struct folio *vm_normal_folio_pmd(struct vm_area_struct *vma,
 struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr,
 				pmd_t pmd);
 
+/* return normal pages backed by the page allocator */
+static inline struct page *vm_normal_gfp_pmd(struct vm_area_struct *vma,
+					     unsigned long addr, pmd_t pmd)
+{
+	struct page *page = vm_normal_page_pmd(vma, addr, pmd);
+
+	if (!is_devdax_page(page) && !is_fsdax_page(page))
+		return page;
+	return NULL;
+}
+
+static inline struct page *vm_normal_gfp_pte(struct vm_area_struct *vma,
+					     unsigned long addr, pte_t pte)
+{
+	struct page *page = vm_normal_page(vma, addr, pte);
+
+	if (!is_devdax_page(page) && !is_fsdax_page(page))
+		return page;
+	return NULL;
+}
+
 void zap_vma_ptes(struct vm_area_struct *vma, unsigned long address,
 		  unsigned long size);
 void zap_page_range_single(struct vm_area_struct *vma, unsigned long address,
diff --git a/mm/pagewalk.c b/mm/pagewalk.c
index cca170fe5be5..54bfece05323 100644
--- a/mm/pagewalk.c
+++ b/mm/pagewalk.c
@@ -914,9 +914,8 @@ struct folio *folio_walk_start(struct folio_walk *fw,
 			spin_unlock(ptl);
 			goto pte_table;
 		} else if (pmd_present(pmd)) {
-			page = vm_normal_page_pmd(vma, addr, pmd);
-			if (page && !is_devdax_page(page) &&
-			    !is_fsdax_page(page)) {
+			page = vm_normal_gfp_pmd(vma, addr, pmd);
+			if (page) {
 				goto found;
 			} else if ((flags & FW_ZEROPAGE) &&
 				    is_huge_zero_pmd(pmd)) {
@@ -949,9 +948,8 @@ struct folio *folio_walk_start(struct folio_walk *fw,
 	fw->pte = pte;
 
 	if (pte_present(pte)) {
-		page = vm_normal_page(vma, addr, pte);
-		if (page && !is_devdax_page(page) &&
-		    !is_fsdax_page(page))
+		page = vm_normal_gfp_pte(vma, addr, pte);
+		if (page)
 			goto found;
 		if ((flags & FW_ZEROPAGE) &&
 		    is_zero_pfn(pte_pfn(pte))) {

On Wed, Jun 04, 2025 at 06:59:09PM -0700, Dan Williams wrote:
> +/* return normal pages backed by the page allocator */
> +static inline struct page *vm_normal_gfp_pmd(struct vm_area_struct *vma,
> +					     unsigned long addr, pmd_t pmd)
> +{
> +	struct page *page = vm_normal_page_pmd(vma, addr, pmd);
> +
> +	if (!is_devdax_page(page) && !is_fsdax_page(page))
> +		return page;
> +	return NULL;

If you go for this make it more straight forward by having the
normal path in the main flow:

	if (is_devdax_page(page) || is_fsdax_page(page))
		return NULL;
	return page;

> +static inline struct page *vm_normal_gfp_pte(struct vm_area_struct *vma,
> +					     unsigned long addr, pte_t pte)
> +{
> +	struct page *page = vm_normal_page(vma, addr, pte);
> +
> +	if (!is_devdax_page(page) && !is_fsdax_page(page))
> +		return page;
> +	return NULL;

Same here.

On 05.06.25 09:46, Christoph Hellwig wrote:
> On Wed, Jun 04, 2025 at 06:59:09PM -0700, Dan Williams wrote:
>> +/* return normal pages backed by the page allocator */
>> +static inline struct page *vm_normal_gfp_pmd(struct vm_area_struct *vma,
>> +					     unsigned long addr, pmd_t pmd)
>> +{
>> +	struct page *page = vm_normal_page_pmd(vma, addr, pmd);
>> +
>> +	if (!is_devdax_page(page) && !is_fsdax_page(page))
>> +		return page;
>> +	return NULL;
> 
> If you go for this make it more straight forward by having the
> normal path in the main flow:
> 
> 	if (is_devdax_page(page) || is_fsdax_page(page))
> 		return NULL;
> 	return page;

+1

But I'd defer introducing that for now if avoidable. I find the naming 
rather ... suboptimal :)

-- 
Cheers,

David / dhildenb

David Hildenbrand wrote:
> On 05.06.25 09:46, Christoph Hellwig wrote:
> > On Wed, Jun 04, 2025 at 06:59:09PM -0700, Dan Williams wrote:
> >> +/* return normal pages backed by the page allocator */
> >> +static inline struct page *vm_normal_gfp_pmd(struct vm_area_struct *vma,
> >> +					     unsigned long addr, pmd_t pmd)
> >> +{
> >> +	struct page *page = vm_normal_page_pmd(vma, addr, pmd);
> >> +
> >> +	if (!is_devdax_page(page) && !is_fsdax_page(page))
> >> +		return page;
> >> +	return NULL;
> > 
> > If you go for this make it more straight forward by having the
> > normal path in the main flow:
> > 
> > 	if (is_devdax_page(page) || is_fsdax_page(page))
> > 		return NULL;
> > 	return page;
> 
> +1
> 
> But I'd defer introducing that for now if avoidable. I find the naming 
> rather ... suboptimal :)

Agree, that was a "for lack of a better term" suggestion, but the
suggestion is indeed lacking.

On Thu, Jun 05, 2025 at 09:21:28AM -0700, Dan Williams wrote:
> David Hildenbrand wrote:
> > On 05.06.25 09:46, Christoph Hellwig wrote:
> > > On Wed, Jun 04, 2025 at 06:59:09PM -0700, Dan Williams wrote:
> > >> +/* return normal pages backed by the page allocator */
> > >> +static inline struct page *vm_normal_gfp_pmd(struct vm_area_struct *vma,
> > >> +					     unsigned long addr, pmd_t pmd)
> > >> +{
> > >> +	struct page *page = vm_normal_page_pmd(vma, addr, pmd);
> > >> +
> > >> +	if (!is_devdax_page(page) && !is_fsdax_page(page))
> > >> +		return page;
> > >> +	return NULL;
> > > 
> > > If you go for this make it more straight forward by having the
> > > normal path in the main flow:
> > > 
> > > 	if (is_devdax_page(page) || is_fsdax_page(page))
> > > 		return NULL;
> > > 	return page;
> > 
> > +1
> > 
> > But I'd defer introducing that for now if avoidable. I find the naming 
> > rather ... suboptimal :)
> 
> Agree, that was a "for lack of a better term" suggestion, but the
> suggestion is indeed lacking.

I don't like the naming either ... maybe that is motivation enough for me to
audit the callers and have them explicitly filter the pages they don't want.

On Thu, Jun 12, 2025 at 05:02:13PM +1000, Alistair Popple wrote:
> On Thu, Jun 05, 2025 at 09:21:28AM -0700, Dan Williams wrote:
> > David Hildenbrand wrote:
> > > On 05.06.25 09:46, Christoph Hellwig wrote:
> > > > On Wed, Jun 04, 2025 at 06:59:09PM -0700, Dan Williams wrote:
> > > >> +/* return normal pages backed by the page allocator */
> > > >> +static inline struct page *vm_normal_gfp_pmd(struct vm_area_struct *vma,
> > > >> +					     unsigned long addr, pmd_t pmd)
> > > >> +{
> > > >> +	struct page *page = vm_normal_page_pmd(vma, addr, pmd);
> > > >> +
> > > >> +	if (!is_devdax_page(page) && !is_fsdax_page(page))
> > > >> +		return page;
> > > >> +	return NULL;
> > > > 
> > > > If you go for this make it more straight forward by having the
> > > > normal path in the main flow:
> > > > 
> > > > 	if (is_devdax_page(page) || is_fsdax_page(page))
> > > > 		return NULL;
> > > > 	return page;
> > > 
> > > +1
> > > 
> > > But I'd defer introducing that for now if avoidable. I find the naming 
> > > rather ... suboptimal :)
> > 
> > Agree, that was a "for lack of a better term" suggestion, but the
> > suggestion is indeed lacking.
> 
> I don't like the naming either ... maybe that is motivation enough for me to
> audit the callers and have them explicitly filter the pages they don't want.

Which actually most of them already do. The only ones I'm unsure about are both
in s390 so I'll be conservative and add checks for folio_is_zone_device() there.

On Thu, May 29, 2025 at 04:32:04PM +1000, Alistair Popple wrote:
> Previously dax pages were skipped by the pagewalk code as pud_special() or
> vm_normal_page{_pmd}() would be false for DAX pages. Now that dax pages are
> refcounted normally that is no longer the case, so add explicit checks to
> skip them.
> 
> Signed-off-by: Alistair Popple <apopple@nvidia.com>
> ---
>  include/linux/memremap.h | 11 +++++++++++
>  mm/pagewalk.c            | 12 ++++++++++--
>  2 files changed, 21 insertions(+), 2 deletions(-)

But why do we want to skip them?

Like hmm uses pagewalk and it would like to see DAX pages?

I guess it makes sense from the perspective of not changing things,
but it seems like a comment should be left behind explaining that this
is just for legacy reasons until someone audits the callers.

Jason

On 29.05.25 08:32, Alistair Popple wrote:
> Previously dax pages were skipped by the pagewalk code as pud_special() or
> vm_normal_page{_pmd}() would be false for DAX pages. Now that dax pages are
> refcounted normally that is no longer the case, so add explicit checks to
> skip them.

Is this really what we want, though? If these are now just "normal" 
pages, they shall be handled as being normal.

I would assume that we want to check that in the callers instead.

E.g., in get_mergeable_page() we already have a folio_is_zone_device() 
check.

> 
> Signed-off-by: Alistair Popple <apopple@nvidia.com>
> ---
>   include/linux/memremap.h | 11 +++++++++++
>   mm/pagewalk.c            | 12 ++++++++++--
>   2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/memremap.h b/include/linux/memremap.h
> index 4aa1519..54e8b57 100644
> --- a/include/linux/memremap.h
> +++ b/include/linux/memremap.h
> @@ -198,6 +198,17 @@ static inline bool folio_is_fsdax(const struct folio *folio)
>   	return is_fsdax_page(&folio->page);
>   }
>   
> +static inline bool is_devdax_page(const struct page *page)
> +{
> +	return is_zone_device_page(page) &&
> +		page_pgmap(page)->type == MEMORY_DEVICE_GENERIC;
> +}
> +
> +static inline bool folio_is_devdax(const struct folio *folio)
> +{
> +	return is_devdax_page(&folio->page);
> +}

Hm, nobody uses folio_is_devdax() in this patch :)


-- 
Cheers,

David / dhildenb