Re: [PATCH v8 20/20] device/dax: Properly refcount device dax pages when mapping

[Gerald Schaefer <gerald.schaefer@linux.ibm.com>]

On Tue, 18 Feb 2025 14:55:36 +1100
Alistair Popple <apopple@nvidia.com> wrote:

[...]
> diff --git a/mm/memremap.c b/mm/memremap.c
> index 9a8879b..532a52a 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -460,11 +460,10 @@ void free_zone_device_folio(struct folio *folio)
>  {
>  	struct dev_pagemap *pgmap = folio->pgmap;
>  
> -	if (WARN_ON_ONCE(!pgmap->ops))
> -		return;
> -
> -	if (WARN_ON_ONCE(pgmap->type != MEMORY_DEVICE_FS_DAX &&
> -			 !pgmap->ops->page_free))
> +	if (WARN_ON_ONCE((!pgmap->ops &&
> +			  pgmap->type != MEMORY_DEVICE_GENERIC) ||
> +			 (pgmap->ops && !pgmap->ops->page_free &&
> +			  pgmap->type != MEMORY_DEVICE_FS_DAX)))

Playing around with dcssblk, adding devm_memremap_pages() and
pgmap.type = MEMORY_DEVICE_FS_DAX, similar to the other two existing
FS_DAX drivers drivers/nvdimm/pmem.c and fs/fuse/virtio_fs.c, I hit
this warning when executing binaries from DAX-mounted fs.

I do not set up pgmap->ops, similar to fs/fuse/virtio_fs.c, and I don't see
why they would be needed here anyway, at least for MEMORY_DEVICE_FS_DAX.
drivers/nvdimm/pmem.c does set up pgmap->ops, but only ->memory_failure,
which is still good enough to not trigger the warning here, probably just
by chance.

Now I wonder:
1) What is this check / warning good for, when this function only ever
   calls pgmap->ops->page_free(), but not for MEMORY_DEVICE_FS_DAX and
   not for MEMORY_DEVICE_GENERIC (the latter only after this patch)?
2) Is the warning also seen for virtio DAX mappings (added Vivek and
   Stefan on CC)? No pgmap->ops set up there, so I would guess "yes",
   and already before this series, with the old check / warning.
3) Could this be changed to only check / warn if pgmap->ops (or maybe
   rather pgmap->ops->page_free) is not set up, but not for
   MEMORY_DEVICE_GENERIC and MEMORY_DEVICE_FS_DAX where this is not
   being called?
4) Or is there any reason why pgmap->ops would be required for
   MEMORY_DEVICE_FS_DAX?

Apart from the warning, we would also miss out on the
wake_up_var(&folio->page) in the MEMORY_DEVICE_FS_DAX case, when no
pgmap->ops was set up. IIUC, even before this change / series (i.e.
for virtio DAX only, since dcssblk was not using ZONE_DEVICE before,
and pmem seems to work by chance because they have ops->memory_failure).

>  		return;
>  
>  	mem_cgroup_uncharge(folio);
> @@ -494,7 +493,8 @@ void free_zone_device_folio(struct folio *folio)
>  	 * zero which indicating the page has been removed from the file
>  	 * system mapping.
>  	 */
> -	if (pgmap->type != MEMORY_DEVICE_FS_DAX)
> +	if (pgmap->type != MEMORY_DEVICE_FS_DAX &&
> +	    pgmap->type != MEMORY_DEVICE_GENERIC)
>  		folio->mapping = NULL;
>  
>  	switch (pgmap->type) {
> @@ -509,7 +509,6 @@ void free_zone_device_folio(struct folio *folio)
>  		 * Reset the refcount to 1 to prepare for handing out the page
>  		 * again.
>  		 */
> -		pgmap->ops->page_free(folio_page(folio, 0));

Ok, this is probably the reason why you adjusted the check above, since
no more pgmap->ops needed for MEMORY_DEVICE_GENERIC.
Still, the MEMORY_DEVICE_FS_DAX case also does not seem to need
pgmap->ops, and at least the existing virtio DAX should already be
affected, and of course future dcssblk DAX.

But maybe that should be addressed in a separate patch, since your changes
here seem consistent, and not change or worsen anything wrt !pgmap->ops
and MEMORY_DEVICE_FS_DAX.

>  		folio_set_count(folio, 1);
>  		break;
>  

For reference, this is call trace I see when I hit the warning:

[  283.567945] ------------[ cut here ]------------
[  283.567947] WARNING: CPU: 12 PID: 878 at mm/memremap.c:436 free_zone_device_folio+0x6e/0x140
[  283.567959] Modules linked in:
[  283.567963] CPU: 12 UID: 0 PID: 878 Comm: ls Not tainted 6.14.0-rc3-next-20250220-00012-gd072dabf62e8-dirty #44
[  283.567968] Hardware name: IBM 3931 A01 704 (z/VM 7.4.0)
[  283.567971] Krnl PSW : 0704d00180000000 000001ec0548b44a (free_zone_device_folio+0x72/0x140)
[  283.567978]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:1 PM:0 RI:0 EA:3
[  283.567982] Krnl GPRS: 0000000000000038 0000000000000000 0000000000000003 000001ec06cc42e8
[  283.567986]            00000004cc38e400 0000000000000000 0000000000000003 0000000093eacd00
[  283.567990]            000000009a68413f 0000016614010940 000000009553a640 0000016614010940
[  283.567994]            0000000000000000 0000000000000000 000001ec0548b416 0000016c05da3bf8
[  283.568004] Krnl Code: 000001ec0548b43e: a70e0003		chi	%r0,3
                          000001ec0548b442: a7840006		brc	8,000001ec0548b44e
                         #000001ec0548b446: af000000		mc	0,0
                         >000001ec0548b44a: a7f4005f		brc	15,000001ec0548b508
                          000001ec0548b44e: c00400000008	brcl	0,000001ec0548b45e
                          000001ec0548b454: b904002b		lgr	%r2,%r11
                          000001ec0548b458: c0e5001dcd84	brasl	%r14,000001ec05844f60
                          000001ec0548b45e: 9101b01f		tm	31(%r11),1
[  283.568035] Call Trace:
[  283.568038]  [<000001ec0548b44a>] free_zone_device_folio+0x72/0x140 
[  283.568042] ([<000001ec0548b416>] free_zone_device_folio+0x3e/0x140)
[  283.568045]  [<000001ec057a4c1c>] wp_page_copy+0x34c/0x6e0 
[  283.568050]  [<000001ec057ac640>] __handle_mm_fault+0x220/0x4d0 
[  283.568054]  [<000001ec057ac97e>] handle_mm_fault+0x8e/0x160 
[  283.568057]  [<000001ec054ca006>] do_exception+0x1a6/0x450 
[  283.568061]  [<000001ec06264992>] __do_pgm_check+0x132/0x1e0 
[  283.568065]  [<000001ec0627057e>] pgm_check_handler+0x11e/0x170 
[  283.568069] Last Breaking-Event-Address:
[  283.568070]  [<000001ec0548b428>] free_zone_device_folio+0x50/0x140
[  283.568074] ---[ end trace 0000000000000000 ]---