Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1350732 > unrolled thread

Re: [PATCH v2 2/3] libnvdimm, pmem: adjust for section collisions with 'System RAM'

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 2/3] libnvdimm, pmem: adjust for section collisions  with 'System RAM' Toshi Kani <toshi.kani@hpe.com> - 2016-03-05 03:00 +0100
    Re: [PATCH v2 2/3] libnvdimm, pmem: adjust for section collisions  with 'System RAM' Dan Williams <dan.j.williams@intel.com> - 2016-03-05 03:30 +0100
      Re: [PATCH v2 2/3] libnvdimm, pmem: adjust for section collisions  with 'System RAM' Toshi Kani <toshi.kani@hpe.com> - 2016-03-07 18:10 +0100
        Re: [PATCH v2 2/3] libnvdimm, pmem: adjust for section collisions  with 'System RAM' Dan Williams <dan.j.williams@intel.com> - 2016-03-07 18:20 +0100
          Re: [PATCH v2 2/3] libnvdimm, pmem: adjust for section collisions  with 'System RAM' Toshi Kani <toshi.kani@hpe.com> - 2016-03-07 19:10 +0100
            Re: [PATCH v2 2/3] libnvdimm, pmem: adjust for section collisions  with 'System RAM' Dan Williams <dan.j.williams@intel.com> - 2016-03-07 19:30 +0100
            Re: [PATCH v2 2/3] libnvdimm, pmem: adjust for section collisions  with 'System RAM' Dan Williams <dan.j.williams@intel.com> - 2016-03-07 19:40 +0100

#1350732 — Re: [PATCH v2 2/3] libnvdimm, pmem: adjust for section collisions with 'System RAM'

On Thu, 2016-03-03 at 13:53 -0800, Dan Williams wrote:
> On a platform where 'Persistent Memory' and 'System RAM' are mixed
> within a given sparsemem section, trim the namespace and notify about the
> sub-optimal alignment.
> 
> Cc: Toshi Kani <toshi.kani@hpe.com>
> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/nvdimm/namespace_devs.c |    7 ++
>  drivers/nvdimm/pfn.h            |   10 ++-
>  drivers/nvdimm/pfn_devs.c       |    5 ++
>  drivers/nvdimm/pmem.c           |  125 ++++++++++++++++++++++++++++-----
> ------
>  4 files changed, 111 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/nvdimm/namespace_devs.c
> b/drivers/nvdimm/namespace_devs.c
> index 8ebfcaae3f5a..463756ca2d4b 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
> @@ -133,6 +133,7 @@ bool nd_is_uuid_unique(struct device *dev, u8 *uuid)
>  bool pmem_should_map_pages(struct device *dev)
>  {
>  	struct nd_region *nd_region = to_nd_region(dev->parent);
> +	struct nd_namespace_io *nsio;
>  
>  	if (!IS_ENABLED(CONFIG_ZONE_DEVICE))
>  		return false;
> @@ -143,6 +144,12 @@ bool pmem_should_map_pages(struct device *dev)
>  	if (is_nd_pfn(dev) || is_nd_btt(dev))
>  		return false;
>  
> +	nsio = to_nd_namespace_io(dev);
> +	if (region_intersects(nsio->res.start, resource_size(&nsio-
> >res),
> +				IORESOURCE_SYSTEM_RAM,
> +				IORES_DESC_NONE) == REGION_MIXED)

Should this be != REGION_DISJOINT for safe?

> +		return false;
> +

 :

> @@ -304,21 +311,56 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn)
>  	}
>  
>  	memset(pfn_sb, 0, sizeof(*pfn_sb));
> -	npfns = (pmem->size - SZ_8K) / SZ_4K;
> +
> +	/*
> +	 * Check if pmem collides with 'System RAM' when section aligned
> and
> +	 * trim it accordingly
> +	 */
> +	nsio = to_nd_namespace_io(&ndns->dev);
> +	start = PHYS_SECTION_ALIGN_DOWN(nsio->res.start);
> +	size = resource_size(&nsio->res);
> +	if (region_intersects(start, size, IORESOURCE_SYSTEM_RAM,
> +				IORES_DESC_NONE) == REGION_MIXED) {
> +
> +		start = nsio->res.start;
> +		start_pad = PHYS_SECTION_ALIGN_UP(start) - start;
> +	}
> +
> +	start = nsio->res.start;
> +	size = PHYS_SECTION_ALIGN_UP(start + size) - start;
> +	if (region_intersects(start, size, IORESOURCE_SYSTEM_RAM,
> +				IORES_DESC_NONE) == REGION_MIXED) {
> +		size = resource_size(&nsio->res);
> +		end_trunc = start + size - PHYS_SECTION_ALIGN_DOWN(start
> + size);
> +	}

This check seems to assume that guest's regular memory layout does not
change.  That is, if there is no collision at first, there won't be any
later.  Is this a valid assumption?

Thanks,
-Toshi

[toc] | [next] | [standalone]

#1350749

On Fri, Mar 4, 2016 at 6:48 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> On Thu, 2016-03-03 at 13:53 -0800, Dan Williams wrote:
>> On a platform where 'Persistent Memory' and 'System RAM' are mixed
>> within a given sparsemem section, trim the namespace and notify about the
>> sub-optimal alignment.
>>
>> Cc: Toshi Kani <toshi.kani@hpe.com>
>> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>>  drivers/nvdimm/namespace_devs.c |    7 ++
>>  drivers/nvdimm/pfn.h            |   10 ++-
>>  drivers/nvdimm/pfn_devs.c       |    5 ++
>>  drivers/nvdimm/pmem.c           |  125 ++++++++++++++++++++++++++++-----
>> ------
>>  4 files changed, 111 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/nvdimm/namespace_devs.c
>> b/drivers/nvdimm/namespace_devs.c
>> index 8ebfcaae3f5a..463756ca2d4b 100644
>> --- a/drivers/nvdimm/namespace_devs.c
>> +++ b/drivers/nvdimm/namespace_devs.c
>> @@ -133,6 +133,7 @@ bool nd_is_uuid_unique(struct device *dev, u8 *uuid)
>>  bool pmem_should_map_pages(struct device *dev)
>>  {
>>       struct nd_region *nd_region = to_nd_region(dev->parent);
>> +     struct nd_namespace_io *nsio;
>>
>>       if (!IS_ENABLED(CONFIG_ZONE_DEVICE))
>>               return false;
>> @@ -143,6 +144,12 @@ bool pmem_should_map_pages(struct device *dev)
>>       if (is_nd_pfn(dev) || is_nd_btt(dev))
>>               return false;
>>
>> +     nsio = to_nd_namespace_io(dev);
>> +     if (region_intersects(nsio->res.start, resource_size(&nsio-
>> >res),
>> +                             IORESOURCE_SYSTEM_RAM,
>> +                             IORES_DESC_NONE) == REGION_MIXED)
>
> Should this be != REGION_DISJOINT for safe?

Acutally, it's ok.  It doesn't need to be disjoint.  The problem is
mixing an mm-zone within a given section.  If the region intersects
system-ram then devm_memremap_pages() is a no-op and we can use the
existing page allocation and linear mapping.

>
>> +             return false;
>> +
>
>  :
>
>> @@ -304,21 +311,56 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn)
>>       }
>>
>>       memset(pfn_sb, 0, sizeof(*pfn_sb));
>> -     npfns = (pmem->size - SZ_8K) / SZ_4K;
>> +
>> +     /*
>> +      * Check if pmem collides with 'System RAM' when section aligned
>> and
>> +      * trim it accordingly
>> +      */
>> +     nsio = to_nd_namespace_io(&ndns->dev);
>> +     start = PHYS_SECTION_ALIGN_DOWN(nsio->res.start);
>> +     size = resource_size(&nsio->res);
>> +     if (region_intersects(start, size, IORESOURCE_SYSTEM_RAM,
>> +                             IORES_DESC_NONE) == REGION_MIXED) {
>> +
>> +             start = nsio->res.start;
>> +             start_pad = PHYS_SECTION_ALIGN_UP(start) - start;
>> +     }
>> +
>> +     start = nsio->res.start;
>> +     size = PHYS_SECTION_ALIGN_UP(start + size) - start;
>> +     if (region_intersects(start, size, IORESOURCE_SYSTEM_RAM,
>> +                             IORES_DESC_NONE) == REGION_MIXED) {
>> +             size = resource_size(&nsio->res);
>> +             end_trunc = start + size - PHYS_SECTION_ALIGN_DOWN(start
>> + size);
>> +     }
>
> This check seems to assume that guest's regular memory layout does not
> change.  That is, if there is no collision at first, there won't be any
> later.  Is this a valid assumption?

If platform firmware changes the physical alignment during the
lifetime of the namespace there's not much we can do.  Another problem
not addressed by this patch is firmware choosing to hot plug system
ram into the same section as persistent memory.  As far as I can see
all we do is ask firmware implementations to respect Linux section
boundaries and otherwise not change alignments.

[toc] | [prev] | [next] | [standalone]

#1351809

On Fri, 2016-03-04 at 18:23 -0800, Dan Williams wrote:
> On Fri, Mar 4, 2016 at 6:48 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
> > On Thu, 2016-03-03 at 13:53 -0800, Dan Williams wrote:
> > > On a platform where 'Persistent Memory' and 'System RAM' are mixed
> > > within a given sparsemem section, trim the namespace and notify about
> > > the
> > > sub-optimal alignment.
> > > 
> > > Cc: Toshi Kani <toshi.kani@hpe.com>
> > > Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > ---
> > >  drivers/nvdimm/namespace_devs.c |    7 ++
> > >  drivers/nvdimm/pfn.h            |   10 ++-
> > >  drivers/nvdimm/pfn_devs.c       |    5 ++
> > >  drivers/nvdimm/pmem.c           |  125 ++++++++++++++++++++++++++++-
> > > ----
> > > ------
> > >  4 files changed, 111 insertions(+), 36 deletions(-)
> > > 
> > > diff --git a/drivers/nvdimm/namespace_devs.c
> > > b/drivers/nvdimm/namespace_devs.c
> > > index 8ebfcaae3f5a..463756ca2d4b 100644
> > > --- a/drivers/nvdimm/namespace_devs.c
> > > +++ b/drivers/nvdimm/namespace_devs.c
> > > @@ -133,6 +133,7 @@ bool nd_is_uuid_unique(struct device *dev, u8
> > > *uuid)
> > >  bool pmem_should_map_pages(struct device *dev)
> > >  {
> > >       struct nd_region *nd_region = to_nd_region(dev->parent);
> > > +     struct nd_namespace_io *nsio;
> > > 
> > >       if (!IS_ENABLED(CONFIG_ZONE_DEVICE))
> > >               return false;
> > > @@ -143,6 +144,12 @@ bool pmem_should_map_pages(struct device *dev)
> > >       if (is_nd_pfn(dev) || is_nd_btt(dev))
> > >               return false;
> > > 
> > > +     nsio = to_nd_namespace_io(dev);
> > > +     if (region_intersects(nsio->res.start, resource_size(&nsio-
> > > > res),
> > > +                             IORESOURCE_SYSTEM_RAM,
> > > +                             IORES_DESC_NONE) == REGION_MIXED)
> > 
> > Should this be != REGION_DISJOINT for safe?
> 
> Acutally, it's ok.  It doesn't need to be disjoint.  The problem is
> mixing an mm-zone within a given section.  If the region intersects
> system-ram then devm_memremap_pages() is a no-op and we can use the
> existing page allocation and linear mapping.

Oh, I see.

> > 
> > > +             return false;
> > > +
> > 
> >  :
> > 
> > > @@ -304,21 +311,56 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn)
> > >       }
> > > 
> > >       memset(pfn_sb, 0, sizeof(*pfn_sb));
> > > -     npfns = (pmem->size - SZ_8K) / SZ_4K;
> > > +
> > > +     /*
> > > +      * Check if pmem collides with 'System RAM' when section
> > > aligned
> > > and
> > > +      * trim it accordingly
> > > +      */
> > > +     nsio = to_nd_namespace_io(&ndns->dev);
> > > +     start = PHYS_SECTION_ALIGN_DOWN(nsio->res.start);
> > > +     size = resource_size(&nsio->res);
> > > +     if (region_intersects(start, size, IORESOURCE_SYSTEM_RAM,
> > > +                             IORES_DESC_NONE) == REGION_MIXED) {
> > > +
> > > +             start = nsio->res.start;
> > > +             start_pad = PHYS_SECTION_ALIGN_UP(start) - start;
> > > +     }
> > > +
> > > +     start = nsio->res.start;
> > > +     size = PHYS_SECTION_ALIGN_UP(start + size) - start;
> > > +     if (region_intersects(start, size, IORESOURCE_SYSTEM_RAM,
> > > +                             IORES_DESC_NONE) == REGION_MIXED) {
> > > +             size = resource_size(&nsio->res);
> > > +             end_trunc = start + size -
> > > PHYS_SECTION_ALIGN_DOWN(start
> > > + size);
> > > +     }
> > 
> > This check seems to assume that guest's regular memory layout does not
> > change.  That is, if there is no collision at first, there won't be any
> > later.  Is this a valid assumption?
> 
> If platform firmware changes the physical alignment during the
> lifetime of the namespace there's not much we can do.  

The physical alignment can be changed as long as it is large enough (see
below).

> Another problem
> not addressed by this patch is firmware choosing to hot plug system
> ram into the same section as persistent memory.  

Yes, and it does not have to be a hot-plug operation.  Memory size may be
changed off-line.  Data image can be copied to different guests for instant
deployment, or may be migrated to a different guest.

> As far as I can see
> all we do is ask firmware implementations to respect Linux section
> boundaries and otherwise not change alignments.

In addition to the requirement that pmem range alignment may not change,
the code also requires a regular memory range does not change to intersect
with a pmem section later.  This seems fragile to me since guest config may
vary / change as I mentioned above.

So, shouldn't the driver fails to attach when the range is not aligned by
the section size?  Since we need to place a requirement to firmware anyway,
we can simply state that it must be aligned by 128MiB (at least) on x86.
 Then, memory and pmem physical layouts can be changed as long as this
requirement is met.

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1351811

On Mon, Mar 7, 2016 at 9:56 AM, Toshi Kani <toshi.kani@hpe.com> wrote:
> On Fri, 2016-03-04 at 18:23 -0800, Dan Williams wrote:
>> On Fri, Mar 4, 2016 at 6:48 PM, Toshi Kani <toshi.kani@hpe.com> wrote:
[..]
>> As far as I can see
>> all we do is ask firmware implementations to respect Linux section
>> boundaries and otherwise not change alignments.
>
> In addition to the requirement that pmem range alignment may not change,
> the code also requires a regular memory range does not change to intersect
> with a pmem section later.  This seems fragile to me since guest config may
> vary / change as I mentioned above.
>
> So, shouldn't the driver fails to attach when the range is not aligned by
> the section size?  Since we need to place a requirement to firmware anyway,
> we can simply state that it must be aligned by 128MiB (at least) on x86.
>  Then, memory and pmem physical layouts can be changed as long as this
> requirement is met.

We can state that it must be aligned, but without a hard specification
I don't see how we can guarantee it.  We will fail the driver load
with a warning if our alignment fixups end up getting invalidated by a
later configuration change, but in the meantime we cover the gap of a
BIOS that has generated a problematic configuration.

[toc] | [prev] | [next] | [standalone]

#1351844

On Mon, 2016-03-07 at 09:18 -0800, Dan Williams wrote:
> On Mon, Mar 7, 2016 at 9:56 AM, Toshi Kani <toshi.kani@hpe.com> wrote:
> > On Fri, 2016-03-04 at 18:23 -0800, Dan Williams wrote:
> > > On Fri, Mar 4, 2016 at 6:48 PM, Toshi Kani <toshi.kani@hpe.com>
> > > wrote:
> [..]
> > > As far as I can see
> > > all we do is ask firmware implementations to respect Linux section
> > > boundaries and otherwise not change alignments.
> > 
> > In addition to the requirement that pmem range alignment may not
> > change, the code also requires a regular memory range does not change
> > to intersect with a pmem section later.  This seems fragile to me since
> > guest config may vary / change as I mentioned above.
> > 
> > So, shouldn't the driver fails to attach when the range is not aligned
> > by the section size?  Since we need to place a requirement to firmware
> > anyway, we can simply state that it must be aligned by 128MiB (at
> > least) on x86.  Then, memory and pmem physical layouts can be changed
> > as long as this requirement is met.
> 
> We can state that it must be aligned, but without a hard specification
> I don't see how we can guarantee it.  We will fail the driver load
> with a warning if our alignment fixups end up getting invalidated by a
> later configuration change, but in the meantime we cover the gap of a
> BIOS that has generated a problematic configuration.

I do not think it has to be stated in the spec (although it may be a good
idea to state it as an implementation note :-).

This is an OS-unique requirement (and the size is x86-specific) that if it
wants to support Linux pmem pfn, then the alignment needs to be at least
128MiB.  Regular pmem does not have this restriction, but it needs to be
aligned by 2MiB or 1GiB for using huge page mapping, which does not have to
be stated in the spec, either.

For KVM to support the pmem pfn feature on x86, it needs to guarantee this
128MiB alignment.  Otherwise, this feature is not supported.  (I do not
worry about NVDIMM-N since it is naturally aligned by its size.)

If we allow unaligned cases, then the driver needs to detect change from
the initial condition and fail to attach for protecting data.  I did not
see such check in the code, but I may have overlooked.  We cannot check if
KVM has any guarantee to keep the alignment at the initial setup, though.

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1351864

On Mon, Mar 7, 2016 at 10:58 AM, Toshi Kani <toshi.kani@hpe.com> wrote:
> On Mon, 2016-03-07 at 09:18 -0800, Dan Williams wrote:
>> On Mon, Mar 7, 2016 at 9:56 AM, Toshi Kani <toshi.kani@hpe.com> wrote:
>> > On Fri, 2016-03-04 at 18:23 -0800, Dan Williams wrote:
>> > > On Fri, Mar 4, 2016 at 6:48 PM, Toshi Kani <toshi.kani@hpe.com>
>> > > wrote:
>> [..]
>> > > As far as I can see
>> > > all we do is ask firmware implementations to respect Linux section
>> > > boundaries and otherwise not change alignments.
>> >
>> > In addition to the requirement that pmem range alignment may not
>> > change, the code also requires a regular memory range does not change
>> > to intersect with a pmem section later.  This seems fragile to me since
>> > guest config may vary / change as I mentioned above.
>> >
>> > So, shouldn't the driver fails to attach when the range is not aligned
>> > by the section size?  Since we need to place a requirement to firmware
>> > anyway, we can simply state that it must be aligned by 128MiB (at
>> > least) on x86.  Then, memory and pmem physical layouts can be changed
>> > as long as this requirement is met.
>>
>> We can state that it must be aligned, but without a hard specification
>> I don't see how we can guarantee it.  We will fail the driver load
>> with a warning if our alignment fixups end up getting invalidated by a
>> later configuration change, but in the meantime we cover the gap of a
>> BIOS that has generated a problematic configuration.
>
> I do not think it has to be stated in the spec (although it may be a good
> idea to state it as an implementation note :-).
>
> This is an OS-unique requirement (and the size is x86-specific) that if it
> wants to support Linux pmem pfn, then the alignment needs to be at least
> 128MiB.  Regular pmem does not have this restriction, but it needs to be
> aligned by 2MiB or 1GiB for using huge page mapping, which does not have to
> be stated in the spec, either.

We can check that the alignment is correct when the namespace is first
instantiated, but we're still stuck if the configuration ever changes.

> For KVM to support the pmem pfn feature on x86, it needs to guarantee this
> 128MiB alignment.  Otherwise, this feature is not supported.  (I do not
> worry about NVDIMM-N since it is naturally aligned by its size.)
>
> If we allow unaligned cases, then the driver needs to detect change from
> the initial condition and fail to attach for protecting data.  I did not
> see such check in the code, but I may have overlooked.  We cannot check if
> KVM has any guarantee to keep the alignment at the initial setup, though.

devm_memremap_pages() will fail if the driver tries to pass in an
unaligned address [1] ...and now that I look again, that patch
mishandles the aligning 'size', will fix.

[1]: https://lists.01.org/pipermail/linux-nvdimm/2016-February/004729.html

[toc] | [prev] | [next] | [standalone]

#1351870

[ adding Haozhong and Xiao for the alignment concerns below ]

On Mon, Mar 7, 2016 at 10:58 AM, Toshi Kani <toshi.kani@hpe.com> wrote:
> On Mon, 2016-03-07 at 09:18 -0800, Dan Williams wrote:
>> On Mon, Mar 7, 2016 at 9:56 AM, Toshi Kani <toshi.kani@hpe.com> wrote:
>> > On Fri, 2016-03-04 at 18:23 -0800, Dan Williams wrote:
>> > > On Fri, Mar 4, 2016 at 6:48 PM, Toshi Kani <toshi.kani@hpe.com>
>> > > wrote:
>> [..]
>> > > As far as I can see
>> > > all we do is ask firmware implementations to respect Linux section
>> > > boundaries and otherwise not change alignments.
>> >
>> > In addition to the requirement that pmem range alignment may not
>> > change, the code also requires a regular memory range does not change
>> > to intersect with a pmem section later.  This seems fragile to me since
>> > guest config may vary / change as I mentioned above.
>> >
>> > So, shouldn't the driver fails to attach when the range is not aligned
>> > by the section size?  Since we need to place a requirement to firmware
>> > anyway, we can simply state that it must be aligned by 128MiB (at
>> > least) on x86.  Then, memory and pmem physical layouts can be changed
>> > as long as this requirement is met.
>>
>> We can state that it must be aligned, but without a hard specification
>> I don't see how we can guarantee it.  We will fail the driver load
>> with a warning if our alignment fixups end up getting invalidated by a
>> later configuration change, but in the meantime we cover the gap of a
>> BIOS that has generated a problematic configuration.
>
> I do not think it has to be stated in the spec (although it may be a good
> idea to state it as an implementation note :-).
>
> This is an OS-unique requirement (and the size is x86-specific) that if it
> wants to support Linux pmem pfn, then the alignment needs to be at least
> 128MiB.  Regular pmem does not have this restriction, but it needs to be
> aligned by 2MiB or 1GiB for using huge page mapping, which does not have to
> be stated in the spec, either.
>
> For KVM to support the pmem pfn feature on x86, it needs to guarantee this
> 128MiB alignment.  Otherwise, this feature is not supported.  (I do not
> worry about NVDIMM-N since it is naturally aligned by its size.)
>
> If we allow unaligned cases, then the driver needs to detect change from
> the initial condition and fail to attach for protecting data.  I did not
> see such check in the code, but I may have overlooked.  We cannot check if
> KVM has any guarantee to keep the alignment at the initial setup, though.
>
> Thanks,
> -Toshi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web