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