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

[Alistair Popple <apopple@nvidia.com>]

On Thu, Feb 20, 2025 at 07:33:34PM +0100, Gerald Schaefer wrote:
> 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.

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

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

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