From: Wei Yang <richardw.yang@linux.intel.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Ross Zwisler <zwisler@kernel.org>,
linux-nvdimm <linux-nvdimm@lists.01.org>
Subject: Re: [PATCH] libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated
Date: Fri, 18 Jan 2019 15:31:17 +0800 [thread overview]
Message-ID: <20190118073117.GA4628@richard> (raw)
In-Reply-To: <CAPcyv4j6Ud_cMkJneN13tv4_WjdAWM41RUhAOJsPPupZ-5KNmg@mail.gmail.com>
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
next prev parent reply other threads:[~2019-01-18 7:31 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190118073117.GA4628@richard \
--to=richardw.yang@linux.intel.com \
--cc=dan.j.williams@intel.com \
--cc=linux-nvdimm@lists.01.org \
--cc=zwisler@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.