| Message ID | 156530042781.2068700.8733813683117819799.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/memremap: Fix reuse of pgmap instances with internal references | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
Dan Williams <dan.j.williams@intel.com> writes: > Currently, attempts to shutdown and re-enable a device-dax instance > trigger: What does "shutdown and re-enable" translate to? If I disable and re-enable a device-dax namespace, I don't see this behavior. -Jeff > > Missing reference count teardown definition > WARNING: CPU: 37 PID: 1608 at mm/memremap.c:211 devm_memremap_pages+0x234/0x850 > [..] > RIP: 0010:devm_memremap_pages+0x234/0x850 > [..] > Call Trace: > dev_dax_probe+0x66/0x190 [device_dax] > really_probe+0xef/0x390 > driver_probe_device+0xb4/0x100 > device_driver_attach+0x4f/0x60 > > Given that the setup path initializes pgmap->ref, arrange for it to be > also torn down so devm_memremap_pages() is ready to be called again and > not be mistaken for the 3rd-party per-cpu-ref case. > > Fixes: 24917f6b1041 ("memremap: provide an optional internal refcount in struct dev_pagemap") > Reported-by: Fan Du <fan.du@intel.com> > Tested-by: Vishal Verma <vishal.l.verma@intel.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Jason Gunthorpe <jgg@mellanox.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > > Andrew, I have another dax fix pending, so I'm ok to take this through > the nvdimm tree, holler if you want it in -mm. > > mm/memremap.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/mm/memremap.c b/mm/memremap.c > index 6ee03a816d67..86432650f829 100644 > --- a/mm/memremap.c > +++ b/mm/memremap.c > @@ -91,6 +91,12 @@ static void dev_pagemap_cleanup(struct dev_pagemap *pgmap) > wait_for_completion(&pgmap->done); > percpu_ref_exit(pgmap->ref); > } > + /* > + * Undo the pgmap ref assignment for the internal case as the > + * caller may re-enable the same pgmap. > + */ > + if (pgmap->ref == &pgmap->internal_ref) > + pgmap->ref = NULL; > } > > static void devm_memremap_pages_release(void *data) > > _______________________________________________ > Linux-nvdimm mailing list > Linux-nvdimm@lists.01.org > https://lists.01.org/mailman/listinfo/linux-nvdimm
On Mon, Aug 12, 2019 at 8:51 AM Jeff Moyer <jmoyer@redhat.com> wrote: > > Dan Williams <dan.j.williams@intel.com> writes: > > > Currently, attempts to shutdown and re-enable a device-dax instance > > trigger: > > What does "shutdown and re-enable" translate to? If I disable and > re-enable a device-dax namespace, I don't see this behavior. I was not seeing this either until I made sure I was in 'bus" device model mode. # cat /etc/modprobe.d/daxctl.conf blacklist dax_pmem_compat alias nd:t7* dax_pmem # make TESTS="daxctl-devices.sh" check -j 40 2>out # dmesg | grep WARN.*devm [ 225.588651] WARNING: CPU: 10 PID: 9103 at mm/memremap.c:211 devm_memremap_pages+0x234/0x850 [ 225.679828] WARNING: CPU: 10 PID: 9103 at mm/memremap.c:211 devm_memremap_pages+0x234/0x850
Dan Williams <dan.j.williams@intel.com> writes: > On Mon, Aug 12, 2019 at 8:51 AM Jeff Moyer <jmoyer@redhat.com> wrote: >> >> Dan Williams <dan.j.williams@intel.com> writes: >> >> > Currently, attempts to shutdown and re-enable a device-dax instance >> > trigger: >> >> What does "shutdown and re-enable" translate to? If I disable and >> re-enable a device-dax namespace, I don't see this behavior. > > I was not seeing this either until I made sure I was in 'bus" device model mode. > > # cat /etc/modprobe.d/daxctl.conf > blacklist dax_pmem_compat > alias nd:t7* dax_pmem > > # make TESTS="daxctl-devices.sh" check -j 40 2>out > > # dmesg | grep WARN.*devm > [ 225.588651] WARNING: CPU: 10 PID: 9103 at mm/memremap.c:211 > devm_memremap_pages+0x234/0x850 > [ 225.679828] WARNING: CPU: 10 PID: 9103 at mm/memremap.c:211 > devm_memremap_pages+0x234/0x850 Ah, you see this when reconfiguring the device. So, the lifetime of the pgmap is tied to the character device, which doesn't get torn down. The fix looks good to me, and tests out fine. Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
diff --git a/mm/memremap.c b/mm/memremap.c index 6ee03a816d67..86432650f829 100644 --- a/mm/memremap.c +++ b/mm/memremap.c @@ -91,6 +91,12 @@ static void dev_pagemap_cleanup(struct dev_pagemap *pgmap) wait_for_completion(&pgmap->done); percpu_ref_exit(pgmap->ref); } + /* + * Undo the pgmap ref assignment for the internal case as the + * caller may re-enable the same pgmap. + */ + if (pgmap->ref == &pgmap->internal_ref) + pgmap->ref = NULL; } static void devm_memremap_pages_release(void *data)
Currently, attempts to shutdown and re-enable a device-dax instance trigger: Missing reference count teardown definition WARNING: CPU: 37 PID: 1608 at mm/memremap.c:211 devm_memremap_pages+0x234/0x850 [..] RIP: 0010:devm_memremap_pages+0x234/0x850 [..] Call Trace: dev_dax_probe+0x66/0x190 [device_dax] really_probe+0xef/0x390 driver_probe_device+0xb4/0x100 device_driver_attach+0x4f/0x60 Given that the setup path initializes pgmap->ref, arrange for it to be also torn down so devm_memremap_pages() is ready to be called again and not be mistaken for the 3rd-party per-cpu-ref case. Fixes: 24917f6b1041 ("memremap: provide an optional internal refcount in struct dev_pagemap") Reported-by: Fan Du <fan.du@intel.com> Tested-by: Vishal Verma <vishal.l.verma@intel.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Christoph Hellwig <hch@lst.de> Cc: Ira Weiny <ira.weiny@intel.com> Cc: Jason Gunthorpe <jgg@mellanox.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- Andrew, I have another dax fix pending, so I'm ok to take this through the nvdimm tree, holler if you want it in -mm. mm/memremap.c | 6 ++++++ 1 file changed, 6 insertions(+)