Re: [PATCH] mm/memremap: Fix reuse of pgmap instances with internal references
Dan Williams writes: > On Mon, Aug 12, 2019 at 8:51 AM Jeff Moyer wrote: >> >> Dan Williams 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 ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] mm/memremap: Fix reuse of pgmap instances with internal references
On Mon, Aug 12, 2019 at 8:51 AM Jeff Moyer wrote: > > Dan Williams 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 ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] mm/memremap: Fix reuse of pgmap instances with internal references
Dan Williams 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
> Tested-by: Vishal Verma
> Cc: Andrew Morton
> Cc: Christoph Hellwig
> Cc: Ira Weiny
> Cc: Jason Gunthorpe
> Signed-off-by: Dan Williams
> ---
>
> 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(>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 == >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
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH] mm/memremap: Fix reuse of pgmap instances with internal references
Looks good: Reviewed-by: Christoph Hellwig ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH] mm/memremap: Fix reuse of pgmap instances with internal references
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
Tested-by: Vishal Verma
Cc: Andrew Morton
Cc: Christoph Hellwig
Cc: Ira Weiny
Cc: Jason Gunthorpe
Signed-off-by: Dan Williams
---
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(>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 == >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
