[PATCH 1/2] libnvdimm, pfn: use size is enough

[PATCH 1/2] libnvdimm, pfn: use size is enough

[Wei Yang]

When trying to see whether current nd_region intersects with others, we
have already calculated the *size* to be expanded to SECTION size.

So just pass size is enough.

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

diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
index becf0bb481b3..5eca050b3660 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -686,7 +686,7 @@ static void trim_pfn_device(struct nd_pfn *nd_pfn, u32 *start_pad, u32 *end_trun
 	if (region_intersects(start, size, IORESOURCE_SYSTEM_RAM,
 				IORES_DESC_NONE) == REGION_MIXED
 			|| !IS_ALIGNED(end, nd_pfn->align)
-			|| nd_region_conflict(nd_region, start, size + adjust))
+			|| nd_region_conflict(nd_region, start, size))
 		*end_trunc = end - phys_pmem_align_down(nd_pfn, end);
 }
 
-- 
2.19.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 2/2] libnvdimm, pfn: use PAGE_SIZE to calculate npfns

[Wei Yang]

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(-)

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);
-- 
2.19.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/2] libnvdimm, pfn: use PAGE_SIZE to calculate npfns

[Dan Williams]

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.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/2] libnvdimm, pfn: use size is enough

[Dan Williams]

On Mon, Jan 21, 2019 at 6:49 PM Wei Yang <richardw.yang@linux.intel.com> wrote:
>
> When trying to see whether current nd_region intersects with others, we
> have already calculated the *size* to be expanded to SECTION size.
>
> So just pass size is enough.
>
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  drivers/nvdimm/pfn_devs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
> index becf0bb481b3..5eca050b3660 100644
> --- a/drivers/nvdimm/pfn_devs.c
> +++ b/drivers/nvdimm/pfn_devs.c
> @@ -686,7 +686,7 @@ static void trim_pfn_device(struct nd_pfn *nd_pfn, u32 *start_pad, u32 *end_trun
>         if (region_intersects(start, size, IORESOURCE_SYSTEM_RAM,
>                                 IORES_DESC_NONE) == REGION_MIXED
>                         || !IS_ALIGNED(end, nd_pfn->align)
> -                       || nd_region_conflict(nd_region, start, size + adjust))
> +                       || nd_region_conflict(nd_region, start, size))

Good catch, thanks. I fixed up the changelog a bit and applied this:

    libnvdimm, pfn: Fix over-trim in trim_pfn_device()

    When trying to see whether current nd_region intersects with others,
    trim_pfn_device() has already calculated the *size* to be expanded to
    SECTION size.

    Do not double append 'adjust' to 'size' when calculating whether the end
    of a region collides with the next pmem region.

    Fixes: ae86cbfef381 "libnvdimm, pfn: Pad pfn namespaces relative
to other regions"
    Cc: <stable@vger.kernel.org>
    Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
    Signed-off-by: Dan Williams <dan.j.williams@intel.com>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/2] libnvdimm, pfn: use size is enough

[Wei Yang]

On Tue, Jan 22, 2019 at 05:28:39PM -0800, Dan Williams wrote:
>On Mon, Jan 21, 2019 at 6:49 PM Wei Yang <richardw.yang@linux.intel.com> wrote:
>>
>> When trying to see whether current nd_region intersects with others, we
>> have already calculated the *size* to be expanded to SECTION size.
>>
>> So just pass size is enough.
>>
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  drivers/nvdimm/pfn_devs.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
>> index becf0bb481b3..5eca050b3660 100644
>> --- a/drivers/nvdimm/pfn_devs.c
>> +++ b/drivers/nvdimm/pfn_devs.c
>> @@ -686,7 +686,7 @@ static void trim_pfn_device(struct nd_pfn *nd_pfn, u32 *start_pad, u32 *end_trun
>>         if (region_intersects(start, size, IORESOURCE_SYSTEM_RAM,
>>                                 IORES_DESC_NONE) == REGION_MIXED
>>                         || !IS_ALIGNED(end, nd_pfn->align)
>> -                       || nd_region_conflict(nd_region, start, size + adjust))
>> +                       || nd_region_conflict(nd_region, start, size))
>
>Good catch, thanks. I fixed up the changelog a bit and applied this:
>
>    libnvdimm, pfn: Fix over-trim in trim_pfn_device()
>
>    When trying to see whether current nd_region intersects with others,
>    trim_pfn_device() has already calculated the *size* to be expanded to
>    SECTION size.
>
>    Do not double append 'adjust' to 'size' when calculating whether the end
>    of a region collides with the next pmem region.

Looks much better :-)

Thanks

>
>    Fixes: ae86cbfef381 "libnvdimm, pfn: Pad pfn namespaces relative
>to other regions"
>    Cc: <stable@vger.kernel.org>
>    Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>    Signed-off-by: Dan Williams <dan.j.williams@intel.com>

-- 
Wei Yang
Help you, Help me
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/2] libnvdimm, pfn: use PAGE_SIZE to calculate npfns

[Wei Yang]

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.


-- 
Wei Yang
Help you, Help me
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/2] libnvdimm, pfn: use size is enough

[Wei Yang]

On Tue, Jan 22, 2019 at 05:28:39PM -0800, Dan Williams wrote:
>On Mon, Jan 21, 2019 at 6:49 PM Wei Yang <richardw.yang@linux.intel.com> wrote:
>>
>> When trying to see whether current nd_region intersects with others, we
>> have already calculated the *size* to be expanded to SECTION size.
>>
>> So just pass size is enough.
>>
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  drivers/nvdimm/pfn_devs.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
>> index becf0bb481b3..5eca050b3660 100644
>> --- a/drivers/nvdimm/pfn_devs.c
>> +++ b/drivers/nvdimm/pfn_devs.c
>> @@ -686,7 +686,7 @@ static void trim_pfn_device(struct nd_pfn *nd_pfn, u32 *start_pad, u32 *end_trun
>>         if (region_intersects(start, size, IORESOURCE_SYSTEM_RAM,
>>                                 IORES_DESC_NONE) == REGION_MIXED
>>                         || !IS_ALIGNED(end, nd_pfn->align)
>> -                       || nd_region_conflict(nd_region, start, size + adjust))
>> +                       || nd_region_conflict(nd_region, start, size))
>

Hi, Dan,

I got a question about the trim on start.

We check the alignment of nd_pfn->align on end, while we don't do this for
start. I lost why we would like to have this behavior.

Would we align start with nd_pfn->align too?

>Good catch, thanks. I fixed up the changelog a bit and applied this:
>
>    libnvdimm, pfn: Fix over-trim in trim_pfn_device()
>
>    When trying to see whether current nd_region intersects with others,
>    trim_pfn_device() has already calculated the *size* to be expanded to
>    SECTION size.
>
>    Do not double append 'adjust' to 'size' when calculating whether the end
>    of a region collides with the next pmem region.
>
>    Fixes: ae86cbfef381 "libnvdimm, pfn: Pad pfn namespaces relative
>to other regions"
>    Cc: <stable@vger.kernel.org>
>    Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>    Signed-off-by: Dan Williams <dan.j.williams@intel.com>

-- 
Wei Yang
Help you, Help me
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm