* [PATCH] libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated
@ 2019-01-18 3:35 Wei Yang
2019-01-18 4:31 ` Dan Williams
0 siblings, 1 reply; 10+ messages in thread
From: Wei Yang @ 2019-01-18 3:35 UTC (permalink / raw)
To: linux-nvdimm; +Cc: zwisler
In current implementation, we might re-allocate nd_pfn->pfn_sb.
For example:
nd_dax_probe()
nd_pfn->pfn_sb = devm_kzalloc()
dax_pmem_probe()
nvdimm_setup_pfn()
nd_pfn_init()
nd_pfn->pfn_sb = devm_kzalloc()
This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init().
Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
drivers/nvdimm/pfn_devs.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
index 6f22272e8d80..1f4f07007483 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -687,21 +687,25 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn)
u32 dax_label_reserve = is_nd_dax(&nd_pfn->dev) ? SZ_128K : 0;
struct nd_namespace_common *ndns = nd_pfn->ndns;
struct nd_namespace_io *nsio = to_nd_namespace_io(&ndns->dev);
+ struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
resource_size_t start, size;
struct nd_region *nd_region;
u32 start_pad, end_trunc;
- struct nd_pfn_sb *pfn_sb;
unsigned long npfns;
phys_addr_t offset;
const char *sig;
u64 checksum;
int rc;
- pfn_sb = devm_kzalloc(&nd_pfn->dev, sizeof(*pfn_sb), GFP_KERNEL);
- if (!pfn_sb)
- return -ENOMEM;
+ if (!pfn_sb) {
+ pfn_sb = devm_kzalloc(&nd_pfn->dev,
+ sizeof(*pfn_sb), GFP_KERNEL);
+ if (!pfn_sb)
+ return -ENOMEM;
+
+ nd_pfn->pfn_sb = pfn_sb;
+ }
- nd_pfn->pfn_sb = pfn_sb;
if (is_nd_dax(&nd_pfn->dev))
sig = DAX_SIG;
else
--
2.19.1
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated 2019-01-18 3:35 [PATCH] libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated Wei Yang @ 2019-01-18 4:31 ` Dan Williams 2019-01-18 5:03 ` Wei Yang 0 siblings, 1 reply; 10+ messages in thread From: Dan Williams @ 2019-01-18 4:31 UTC (permalink / raw) To: Wei Yang; +Cc: Ross Zwisler, linux-nvdimm On Thu, Jan 17, 2019 at 7:36 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > > In current implementation, we might re-allocate nd_pfn->pfn_sb. > > For example: > > nd_dax_probe() > nd_pfn->pfn_sb = devm_kzalloc() > > dax_pmem_probe() > nvdimm_setup_pfn() > nd_pfn_init() > nd_pfn->pfn_sb = devm_kzalloc() > > This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init(). Ugh, this patch is unfortunately uncovering a deeper problem. nvdimm_setup_pfn() should not be calling nd_pfn_init(). This effectively is always re-initializing the info-block, benign but unnecessary. Instead it needs to be using nd_pfn_validate() followed by an __nvdimm_setup_pfn() in the dax_pmem_probe() path. Good find! _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated 2019-01-18 4:31 ` Dan Williams @ 2019-01-18 5:03 ` Wei Yang 2019-01-18 5:05 ` Dan Williams 0 siblings, 1 reply; 10+ messages in thread From: Wei Yang @ 2019-01-18 5:03 UTC (permalink / raw) To: Dan Williams; +Cc: Ross Zwisler, linux-nvdimm On Thu, Jan 17, 2019 at 08:31:56PM -0800, Dan Williams wrote: >On Thu, Jan 17, 2019 at 7:36 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> >> In current implementation, we might re-allocate nd_pfn->pfn_sb. >> >> For example: >> >> nd_dax_probe() >> nd_pfn->pfn_sb = devm_kzalloc() >> >> dax_pmem_probe() >> nvdimm_setup_pfn() >> nd_pfn_init() >> nd_pfn->pfn_sb = devm_kzalloc() >> >> This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init(). > >Ugh, this patch is unfortunately uncovering a deeper problem. >nvdimm_setup_pfn() should not be calling nd_pfn_init(). This >effectively is always re-initializing the info-block, benign but >unnecessary. Instead it needs to be using nd_pfn_validate() followed >by an __nvdimm_setup_pfn() in the dax_pmem_probe() path. > >Good find! So we should fix this like: dax_pmem_probe() nd_pfn_validate() __nvdimm_setup_pfn() Is my understanding correct? -- Wei Yang Help you, Help me _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated 2019-01-18 5:03 ` Wei Yang @ 2019-01-18 5:05 ` Dan Williams 2019-01-18 7:31 ` Wei Yang 0 siblings, 1 reply; 10+ messages in thread From: Dan Williams @ 2019-01-18 5:05 UTC (permalink / raw) To: Wei Yang; +Cc: Ross Zwisler, linux-nvdimm On Thu, Jan 17, 2019 at 9:03 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > > On Thu, Jan 17, 2019 at 08:31:56PM -0800, Dan Williams wrote: > >On Thu, Jan 17, 2019 at 7:36 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > >> > >> In current implementation, we might re-allocate nd_pfn->pfn_sb. > >> > >> For example: > >> > >> nd_dax_probe() > >> nd_pfn->pfn_sb = devm_kzalloc() > >> > >> dax_pmem_probe() > >> nvdimm_setup_pfn() > >> nd_pfn_init() > >> nd_pfn->pfn_sb = devm_kzalloc() > >> > >> This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init(). > > > >Ugh, this patch is unfortunately uncovering a deeper problem. > >nvdimm_setup_pfn() should not be calling nd_pfn_init(). This > >effectively is always re-initializing the info-block, benign but > >unnecessary. Instead it needs to be using nd_pfn_validate() followed > >by an __nvdimm_setup_pfn() in the dax_pmem_probe() path. > > > >Good find! > > So we should fix this like: > > dax_pmem_probe() > nd_pfn_validate() > __nvdimm_setup_pfn() > > Is my understanding correct? Yes, at first glance, but it would need a deeper review and testing. The usage of nvdimm_setup_pfn() in drivers/nvdimm/pmem.c needs a similar fixup. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated 2019-01-18 5:05 ` Dan Williams @ 2019-01-18 7:31 ` Wei Yang 2019-01-18 8:38 ` Dan Williams 0 siblings, 1 reply; 10+ messages in thread From: Wei Yang @ 2019-01-18 7:31 UTC (permalink / raw) To: Dan Williams; +Cc: Ross Zwisler, linux-nvdimm On Thu, Jan 17, 2019 at 09:05:54PM -0800, Dan Williams wrote: >On Thu, Jan 17, 2019 at 9:03 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> >> On Thu, Jan 17, 2019 at 08:31:56PM -0800, Dan Williams wrote: >> >On Thu, Jan 17, 2019 at 7:36 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> >> >> >> In current implementation, we might re-allocate nd_pfn->pfn_sb. >> >> >> >> For example: >> >> >> >> nd_dax_probe() >> >> nd_pfn->pfn_sb = devm_kzalloc() >> >> >> >> dax_pmem_probe() >> >> nvdimm_setup_pfn() >> >> nd_pfn_init() >> >> nd_pfn->pfn_sb = devm_kzalloc() >> >> >> >> This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init(). >> > >> >Ugh, this patch is unfortunately uncovering a deeper problem. >> >nvdimm_setup_pfn() should not be calling nd_pfn_init(). This >> >effectively is always re-initializing the info-block, benign but >> >unnecessary. Instead it needs to be using nd_pfn_validate() followed >> >by an __nvdimm_setup_pfn() in the dax_pmem_probe() path. >> > >> >Good find! >> >> So we should fix this like: >> >> dax_pmem_probe() >> nd_pfn_validate() >> __nvdimm_setup_pfn() >> >> Is my understanding correct? > >Yes, at first glance, but it would need a deeper review and testing. >The usage of nvdimm_setup_pfn() in drivers/nvdimm/pmem.c needs a >similar fixup. I went through the code again and found I may remove the allocation in nd_pfn_init(). Below is my analysis. nvdimm_setup_pfn() is invoked in two places: * dax_pmem_probe() * pmem_attach_disk(), when it is_nd_pfn() And before these two function called, corresponding device should be allocated and created. And we can see these two corresponding places to create the devices are: * nd_dax_probe() * nd_pfn_probe() Both of them allocate pfn_sb. This means it is not necessary to allocate it again when we do the probe function. Do you think it looks good to you? -- Wei Yang Help you, Help me _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated 2019-01-18 7:31 ` Wei Yang @ 2019-01-18 8:38 ` Dan Williams 2019-01-18 18:59 ` Dan Williams 2019-01-21 0:24 ` Wei Yang 0 siblings, 2 replies; 10+ messages in thread From: Dan Williams @ 2019-01-18 8:38 UTC (permalink / raw) To: Wei Yang; +Cc: Ross Zwisler, linux-nvdimm On Thu, Jan 17, 2019 at 11:31 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > > On Thu, Jan 17, 2019 at 09:05:54PM -0800, Dan Williams wrote: > >On Thu, Jan 17, 2019 at 9:03 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > >> > >> On Thu, Jan 17, 2019 at 08:31:56PM -0800, Dan Williams wrote: > >> >On Thu, Jan 17, 2019 at 7:36 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > >> >> > >> >> In current implementation, we might re-allocate nd_pfn->pfn_sb. > >> >> > >> >> For example: > >> >> > >> >> nd_dax_probe() > >> >> nd_pfn->pfn_sb = devm_kzalloc() > >> >> > >> >> dax_pmem_probe() > >> >> nvdimm_setup_pfn() > >> >> nd_pfn_init() > >> >> nd_pfn->pfn_sb = devm_kzalloc() > >> >> > >> >> This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init(). > >> > > >> >Ugh, this patch is unfortunately uncovering a deeper problem. > >> >nvdimm_setup_pfn() should not be calling nd_pfn_init(). This > >> >effectively is always re-initializing the info-block, benign but > >> >unnecessary. Instead it needs to be using nd_pfn_validate() followed > >> >by an __nvdimm_setup_pfn() in the dax_pmem_probe() path. > >> > > >> >Good find! > >> > >> So we should fix this like: > >> > >> dax_pmem_probe() > >> nd_pfn_validate() > >> __nvdimm_setup_pfn() > >> > >> Is my understanding correct? > > > >Yes, at first glance, but it would need a deeper review and testing. > >The usage of nvdimm_setup_pfn() in drivers/nvdimm/pmem.c needs a > >similar fixup. > > I went through the code again and found I may remove the allocation in > nd_pfn_init(). > > Below is my analysis. > > nvdimm_setup_pfn() is invoked in two places: > > * dax_pmem_probe() > * pmem_attach_disk(), when it is_nd_pfn() > > And before these two function called, corresponding device should be allocated > and created. And we can see these two corresponding places to create the > devices are: > > * nd_dax_probe() > * nd_pfn_probe() > > Both of them allocate pfn_sb. This means it is not necessary to allocate it > again when we do the probe function. > > Do you think it looks good to you? > No it doesn't because the real issue is that we should not be recreating the info block. So the fix is not avoiding the allocation, the fix is avoiding nd_pfn_init() in the probe path altogether. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated 2019-01-18 8:38 ` Dan Williams @ 2019-01-18 18:59 ` Dan Williams 2019-01-21 0:50 ` Wei Yang 2019-01-21 0:24 ` Wei Yang 1 sibling, 1 reply; 10+ messages in thread From: Dan Williams @ 2019-01-18 18:59 UTC (permalink / raw) To: Wei Yang; +Cc: Ross Zwisler, linux-nvdimm On Fri, Jan 18, 2019 at 12:38 AM Dan Williams <dan.j.williams@intel.com> wrote: > > On Thu, Jan 17, 2019 at 11:31 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > > > > On Thu, Jan 17, 2019 at 09:05:54PM -0800, Dan Williams wrote: > > >On Thu, Jan 17, 2019 at 9:03 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > > >> > > >> On Thu, Jan 17, 2019 at 08:31:56PM -0800, Dan Williams wrote: > > >> >On Thu, Jan 17, 2019 at 7:36 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > > >> >> > > >> >> In current implementation, we might re-allocate nd_pfn->pfn_sb. > > >> >> > > >> >> For example: > > >> >> > > >> >> nd_dax_probe() > > >> >> nd_pfn->pfn_sb = devm_kzalloc() > > >> >> > > >> >> dax_pmem_probe() > > >> >> nvdimm_setup_pfn() > > >> >> nd_pfn_init() > > >> >> nd_pfn->pfn_sb = devm_kzalloc() > > >> >> > > >> >> This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init(). > > >> > > > >> >Ugh, this patch is unfortunately uncovering a deeper problem. > > >> >nvdimm_setup_pfn() should not be calling nd_pfn_init(). This > > >> >effectively is always re-initializing the info-block, benign but > > >> >unnecessary. Instead it needs to be using nd_pfn_validate() followed > > >> >by an __nvdimm_setup_pfn() in the dax_pmem_probe() path. > > >> > > > >> >Good find! > > >> > > >> So we should fix this like: > > >> > > >> dax_pmem_probe() > > >> nd_pfn_validate() > > >> __nvdimm_setup_pfn() > > >> > > >> Is my understanding correct? > > > > > >Yes, at first glance, but it would need a deeper review and testing. > > >The usage of nvdimm_setup_pfn() in drivers/nvdimm/pmem.c needs a > > >similar fixup. > > > > I went through the code again and found I may remove the allocation in > > nd_pfn_init(). > > > > Below is my analysis. > > > > nvdimm_setup_pfn() is invoked in two places: > > > > * dax_pmem_probe() > > * pmem_attach_disk(), when it is_nd_pfn() > > > > And before these two function called, corresponding device should be allocated > > and created. And we can see these two corresponding places to create the > > devices are: > > > > * nd_dax_probe() > > * nd_pfn_probe() > > > > Both of them allocate pfn_sb. This means it is not necessary to allocate it > > again when we do the probe function. > > > > Do you think it looks good to you? > > > > No it doesn't because the real issue is that we should not be > recreating the info block. So the fix is not avoiding the allocation, > the fix is avoiding nd_pfn_init() in the probe path altogether. I was wrong, thankfully! nd_pfn_init() *does* call nd_pfn_validate(), I was just overlooking it. I also don't see that this fix is needed. nd_dax_probe() devm-allocates pfn_sb and performs a validation. If the validation succeeds nd_dax_probe() returns 0 which creates a new device and then causes nd_pmem_probe() to fail. At that point devm frees the pfn_sb allocation. The allocation is also freed if the validation step failed. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated 2019-01-18 18:59 ` Dan Williams @ 2019-01-21 0:50 ` Wei Yang 2019-01-21 3:28 ` Dan Williams 0 siblings, 1 reply; 10+ messages in thread From: Wei Yang @ 2019-01-21 0:50 UTC (permalink / raw) To: Dan Williams; +Cc: Ross Zwisler, linux-nvdimm On Fri, Jan 18, 2019 at 10:59:23AM -0800, Dan Williams wrote: >On Fri, Jan 18, 2019 at 12:38 AM Dan Williams <dan.j.williams@intel.com> wrote: >> >> On Thu, Jan 17, 2019 at 11:31 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> > >> > On Thu, Jan 17, 2019 at 09:05:54PM -0800, Dan Williams wrote: >> > >On Thu, Jan 17, 2019 at 9:03 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> > >> >> > >> On Thu, Jan 17, 2019 at 08:31:56PM -0800, Dan Williams wrote: >> > >> >On Thu, Jan 17, 2019 at 7:36 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> > >> >> >> > >> >> In current implementation, we might re-allocate nd_pfn->pfn_sb. >> > >> >> >> > >> >> For example: >> > >> >> >> > >> >> nd_dax_probe() >> > >> >> nd_pfn->pfn_sb = devm_kzalloc() >> > >> >> >> > >> >> dax_pmem_probe() >> > >> >> nvdimm_setup_pfn() >> > >> >> nd_pfn_init() >> > >> >> nd_pfn->pfn_sb = devm_kzalloc() >> > >> >> >> > >> >> This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init(). >> > >> > >> > >> >Ugh, this patch is unfortunately uncovering a deeper problem. >> > >> >nvdimm_setup_pfn() should not be calling nd_pfn_init(). This >> > >> >effectively is always re-initializing the info-block, benign but >> > >> >unnecessary. Instead it needs to be using nd_pfn_validate() followed >> > >> >by an __nvdimm_setup_pfn() in the dax_pmem_probe() path. >> > >> > >> > >> >Good find! >> > >> >> > >> So we should fix this like: >> > >> >> > >> dax_pmem_probe() >> > >> nd_pfn_validate() >> > >> __nvdimm_setup_pfn() >> > >> >> > >> Is my understanding correct? >> > > >> > >Yes, at first glance, but it would need a deeper review and testing. >> > >The usage of nvdimm_setup_pfn() in drivers/nvdimm/pmem.c needs a >> > >similar fixup. >> > >> > I went through the code again and found I may remove the allocation in >> > nd_pfn_init(). >> > >> > Below is my analysis. >> > >> > nvdimm_setup_pfn() is invoked in two places: >> > >> > * dax_pmem_probe() >> > * pmem_attach_disk(), when it is_nd_pfn() >> > >> > And before these two function called, corresponding device should be allocated >> > and created. And we can see these two corresponding places to create the >> > devices are: >> > >> > * nd_dax_probe() >> > * nd_pfn_probe() >> > >> > Both of them allocate pfn_sb. This means it is not necessary to allocate it >> > again when we do the probe function. >> > >> > Do you think it looks good to you? >> > >> >> No it doesn't because the real issue is that we should not be >> recreating the info block. So the fix is not avoiding the allocation, >> the fix is avoiding nd_pfn_init() in the probe path altogether. > >I was wrong, thankfully! nd_pfn_init() *does* call nd_pfn_validate(), >I was just overlooking it. > >I also don't see that this fix is needed. nd_dax_probe() >devm-allocates pfn_sb and performs a validation. If the validation >succeeds nd_dax_probe() returns 0 which creates a new device and then >causes nd_pmem_probe() to fail. At that point devm frees the pfn_sb If I am correct, nd_dax_probe() creates a device whose driver is dax_pmem_driver. So the probe function is dax_pmem_probe. Based on my test, one more pfn_sb is allocated in dax_pmem_probe and the driver attached to the device successfully. >allocation. The allocation is also freed if the validation step >failed. nd_pmem_probe() will be called on device created by nd_btt_probe() and nd_pfn_probe(), if I am right. So in which case these two driver will fail to to attach the device and release the memory? -- Wei Yang Help you, Help me _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated 2019-01-21 0:50 ` Wei Yang @ 2019-01-21 3:28 ` Dan Williams 0 siblings, 0 replies; 10+ messages in thread From: Dan Williams @ 2019-01-21 3:28 UTC (permalink / raw) To: Wei Yang; +Cc: Ross Zwisler, linux-nvdimm On Sun, Jan 20, 2019 at 4:51 PM Wei Yang <richardw.yang@linux.intel.com> wrote: [..] > >I also don't see that this fix is needed. nd_dax_probe() > >devm-allocates pfn_sb and performs a validation. If the validation > >succeeds nd_dax_probe() returns 0 which creates a new device and then > >causes nd_pmem_probe() to fail. At that point devm frees the pfn_sb > > If I am correct, nd_dax_probe() creates a device whose driver is > dax_pmem_driver. So the probe function is dax_pmem_probe. > > Based on my test, one more pfn_sb is allocated in dax_pmem_probe and the > driver attached to the device successfully. > > >allocation. The allocation is also freed if the validation step > >failed. > > nd_pmem_probe() will be called on device created by nd_btt_probe() and > nd_pfn_probe(), if I am right. > > So in which case these two driver will fail to to attach the device and > release the memory? See nd_pmem_probe(): /* if we find a valid info-block we'll come back as that personality */ if (nd_btt_probe(dev, ndns) == 0 || nd_pfn_probe(dev, ndns) == 0 || nd_dax_probe(dev, ndns) == 0) return -ENXIO; ...all devm allocations are released due to that "return -ENXIO". _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated 2019-01-18 8:38 ` Dan Williams 2019-01-18 18:59 ` Dan Williams @ 2019-01-21 0:24 ` Wei Yang 1 sibling, 0 replies; 10+ messages in thread From: Wei Yang @ 2019-01-21 0:24 UTC (permalink / raw) To: Dan Williams; +Cc: Ross Zwisler, linux-nvdimm On Fri, Jan 18, 2019 at 12:38:18AM -0800, Dan Williams wrote: >On Thu, Jan 17, 2019 at 11:31 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> >> On Thu, Jan 17, 2019 at 09:05:54PM -0800, Dan Williams wrote: >> >On Thu, Jan 17, 2019 at 9:03 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> >> >> >> On Thu, Jan 17, 2019 at 08:31:56PM -0800, Dan Williams wrote: >> >> >On Thu, Jan 17, 2019 at 7:36 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> >> >> >> >> >> In current implementation, we might re-allocate nd_pfn->pfn_sb. >> >> >> >> >> >> For example: >> >> >> >> >> >> nd_dax_probe() >> >> >> nd_pfn->pfn_sb = devm_kzalloc() >> >> >> >> >> >> dax_pmem_probe() >> >> >> nvdimm_setup_pfn() >> >> >> nd_pfn_init() >> >> >> nd_pfn->pfn_sb = devm_kzalloc() >> >> >> >> >> >> This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init(). >> >> > >> >> >Ugh, this patch is unfortunately uncovering a deeper problem. >> >> >nvdimm_setup_pfn() should not be calling nd_pfn_init(). This >> >> >effectively is always re-initializing the info-block, benign but >> >> >unnecessary. Instead it needs to be using nd_pfn_validate() followed >> >> >by an __nvdimm_setup_pfn() in the dax_pmem_probe() path. >> >> > >> >> >Good find! >> >> >> >> So we should fix this like: >> >> >> >> dax_pmem_probe() >> >> nd_pfn_validate() >> >> __nvdimm_setup_pfn() >> >> >> >> Is my understanding correct? >> > >> >Yes, at first glance, but it would need a deeper review and testing. >> >The usage of nvdimm_setup_pfn() in drivers/nvdimm/pmem.c needs a >> >similar fixup. >> >> I went through the code again and found I may remove the allocation in >> nd_pfn_init(). >> >> Below is my analysis. >> >> nvdimm_setup_pfn() is invoked in two places: >> >> * dax_pmem_probe() >> * pmem_attach_disk(), when it is_nd_pfn() >> >> And before these two function called, corresponding device should be allocated >> and created. And we can see these two corresponding places to create the >> devices are: >> >> * nd_dax_probe() >> * nd_pfn_probe() >> >> Both of them allocate pfn_sb. This means it is not necessary to allocate it >> again when we do the probe function. >> >> Do you think it looks good to you? >> > >No it doesn't because the real issue is that we should not be >recreating the info block. So the fix is not avoiding the allocation, >the fix is avoiding nd_pfn_init() in the probe path altogether. Hmm... I may lost here. Which info block we recreated? For one particular device type, nvdimm_setup_pfn() is just called once. By preventing re-allocation here is enough to me. I may not get your point here. Would you mind share some light on this place? -- Wei Yang Help you, Help me _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-01-21 3:28 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-01-18 3:35 [PATCH] libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated Wei Yang 2019-01-18 4:31 ` Dan Williams 2019-01-18 5:03 ` Wei Yang 2019-01-18 5:05 ` Dan Williams 2019-01-18 7:31 ` Wei Yang 2019-01-18 8:38 ` Dan Williams 2019-01-18 18:59 ` Dan Williams 2019-01-21 0:50 ` Wei Yang 2019-01-21 3:28 ` Dan Williams 2019-01-21 0:24 ` Wei Yang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.