[2/2] libnvdimm, pfn: use PAGE_SIZE to calculate npfns
diff mbox series

Message ID 20190122024810.4448-2-richardw.yang@linux.intel.com
State New
Headers show
Series
  • [1/2] libnvdimm, pfn: use size is enough
Related show

Commit Message

Wei Yang Jan. 22, 2019, 2:48 a.m. UTC
There are two places to calculate npfns in nd_pfn_init(), while they use
difference size to calculate.

Use PAGE_SIZE would be more proper for calculation.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 drivers/nvdimm/pfn_devs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Dan Williams Jan. 23, 2019, 1:27 a.m. UTC | #1
On Mon, Jan 21, 2019 at 6:49 PM Wei Yang <richardw.yang@linux.intel.com> wrote:
>
> There are two places to calculate npfns in nd_pfn_init(), while they use
> difference size to calculate.
>
> Use PAGE_SIZE would be more proper for calculation.

No, this would make the kernel have different output based on
PAGE_SIZE. It should be possible for a PAGE_SIZE==64K system to create
a valid info-block for PAGE_SIZE==4K system. This would need to encode
the PAGE_SIZE in the info-block if it were to ever support non-4K
PAGE_SIZE systems.

Another consideration is that a PAGE_SIZE==4K infoblock is compatible
with a PAGE_SIZE==64K system. All that happens is that the memmap
reserve area is oversized and portions go unused. The reverse is not
true.
Wei Yang Jan. 23, 2019, 6:40 a.m. UTC | #2
On Tue, Jan 22, 2019 at 05:27:29PM -0800, Dan Williams wrote:
>On Mon, Jan 21, 2019 at 6:49 PM Wei Yang <richardw.yang@linux.intel.com> wrote:
>>
>> There are two places to calculate npfns in nd_pfn_init(), while they use
>> difference size to calculate.
>>
>> Use PAGE_SIZE would be more proper for calculation.
>
>No, this would make the kernel have different output based on
>PAGE_SIZE. It should be possible for a PAGE_SIZE==64K system to create
>a valid info-block for PAGE_SIZE==4K system. This would need to encode
>the PAGE_SIZE in the info-block if it were to ever support non-4K
>PAGE_SIZE systems.
>

I am confused.

Generally, npfns is used in two functions: nd_pfn_init() and
__nvdimm_setup_pfn().

The code flow looks like this:

  nvdimm_setup_pfn()

    nd_pfn_init()
      npfns = SECTION_ALIGN(trim_size / PAGE_SIZE)
      offset = start + npfns * page_struct
      npfns = (trim_size - offset) / SZ_4K
      pfn_sb->npfns

    __nvdimm_setup_pfn()
      nd_pfn->npfns = pfn_sb->npfns
      or
      nd_pfn->npfns = (trim_size - offset) / PAGE_SIZE

My questions are:

1. offset = start + npfns * page_struct
   This means the number of page struct we reserve in meta-data area is
   calculated with PAGE_SIZE page.

   But we set pfn_sb->npfns = (trim_size - offset) / SZ_4K, which
   means we tell hardware the number of pages is calculated with 4K size.
   
   Would this be a conflict?  Or at least we need to reserve more meta-data
   area to hold page struct?

2. When mode == PFN_MODE_PMEM and PAGE_SIZE == 64K,
   nd_pfn->pfn_sb->npfns is sure to be bigger than nd_pfn->npfns. Because 
   nd_pfn->pfn_sb->npfns = (trim_size - offset) / 4K
   nd_pfn->npfns         = (trim_size - offset) / 64K

   If we are sure for the final result, why we would like to have this
   calculation again?

>Another consideration is that a PAGE_SIZE==4K infoblock is compatible
>with a PAGE_SIZE==64K system. All that happens is that the memmap
>reserve area is oversized and portions go unused. The reverse is not
>true.

The oversized memap reserve area is SECTION size aligned? If so, it looks we
took that into consideration when we calculate offset.

Patch
diff mbox series

diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
index 5eca050b3660..383f01bedd40 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -770,7 +770,7 @@  static int nd_pfn_init(struct nd_pfn *nd_pfn)
 		return -ENXIO;
 	}
 
-	npfns = (size - offset - start_pad - end_trunc) / SZ_4K;
+	npfns = (size - offset - start_pad - end_trunc) / PAGE_SIZE;
 	pfn_sb->mode = cpu_to_le32(nd_pfn->mode);
 	pfn_sb->dataoff = cpu_to_le64(offset);
 	pfn_sb->npfns = cpu_to_le64(npfns);