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

On Thu, Feb 20, 2025 at 07:33:34PM +0100, Gerald Schaefer wrote:

On Tue, 18 Feb 2025 14:55:36 +1100
Alistair Popple [off-list ref] wrote:

[...]

quoted

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.

Yes, I think so. And you can guess which driver I've done all my testing
with.

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.

Right, I simply updated the warning to reflect what should have been
happening prior to this change. However looking again I don't think
free_zone_device_folio() is ever called for MEMORY_DEVICE_FS_DAX pages. Instead
put_devmap_managed_folio_refs() would have returned false and cause most paths
to skip calling free_zone_device_folio().

The only path that doesn't do that appears to be `folio_put()`. That probably
should also be calling put_devmap_managed_folio_refs(), I'm not sure why it
doesn't.

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?

Oh I think I know what actually happened. Earlier versions of my patch series
did define a pgmap->ops->page_free() callback for MEMORY_DEVICE_FS_DAX but
review comments suggested I just do all the was required directly in the switch
statement. Obviously I forgot to update the check when I removed the need
for pgmap->ops->page_free and having pgmap->ops->memory_failure defined was
sufficient to (accidentally) get past the check.

So yeah, the check is wrong. It shouldn't require pgmap->ops to be defined for
MEMORY_DEVICE_FS_DAX or MEMORY_DEVICE_GENERIC.

4) Or is there any reason why pgmap->ops would be required for
   MEMORY_DEVICE_FS_DAX?

Nope.

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).

See __put_devmap_managed_folio_refs() - the wake_up_var() was there to intercept
the 2->1 refcount transition. Now the wake_up_var() needs to happen on 1->0,
hence why it got moved to free_zone_device_page().

quoted

 		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.

Nah, I think the check is wrong and needs fixing here.

quoted

 		folio_set_count(folio, 1);
 		break;
 

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

Well thanks for testing this and for posting these results.

[  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 ]---