diff mbox series

libnvdimm/region: Allow setting align attribute on regions without mappings

Message ID 20210326152645.85225-1-tyhicks@linux.microsoft.com (mailing list archive)
State New
Headers show
Series libnvdimm/region: Allow setting align attribute on regions without mappings | expand

Commit Message

Tyler Hicks March 26, 2021, 3:26 p.m. UTC
The alignment constraint for namespace creation in a region was
increased, from 2M to 16M, for non-PowerPC architectures in v5.7 with
commit 2522afb86a8c ("libnvdimm/region: Introduce an 'align'
attribute"). The thought behind the change was that region alignment
should be uniform across all architectures and, since PowerPC had the
largest alignment constraint of 16M, all architectures should conform to
that alignment.

The change regressed namespace creation in pre-defined regions that
relied on 2M alignment but a workaround was provided in the form of a
sysfs attribute, named 'align', that could be adjusted to a non-default
alignment value.

However, the sysfs attribute's store function returned an error (-ENXIO)
when userspace attempted to change the alignment of a region that had no
mappings. This affected 2M aligned regions of volatile memory that were
defined in a device tree using "pmem-region" and created by the
of_pmem_region_driver, since those regions do not contain mappings
(ndr_mappings is 0).

Allow userspace to set the align attribute on pre-existing regions that
do not have mappings so that namespaces can still be within those
regions, despite not being aligned to 16M.

Fixes: 2522afb86a8c ("libnvdimm/region: Introduce an 'align' attribute")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
 drivers/nvdimm/region_devs.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

Comments

Pavel Tatashin March 26, 2021, 10:03 p.m. UTC | #1
On Fri, Mar 26, 2021 at 11:27 AM Tyler Hicks
<tyhicks@linux.microsoft.com> wrote:
>
> The alignment constraint for namespace creation in a region was
> increased, from 2M to 16M, for non-PowerPC architectures in v5.7 with
> commit 2522afb86a8c ("libnvdimm/region: Introduce an 'align'
> attribute"). The thought behind the change was that region alignment
> should be uniform across all architectures and, since PowerPC had the
> largest alignment constraint of 16M, all architectures should conform to
> that alignment.
>
> The change regressed namespace creation in pre-defined regions that
> relied on 2M alignment but a workaround was provided in the form of a
> sysfs attribute, named 'align', that could be adjusted to a non-default
> alignment value.
>
> However, the sysfs attribute's store function returned an error (-ENXIO)
> when userspace attempted to change the alignment of a region that had no
> mappings. This affected 2M aligned regions of volatile memory that were
> defined in a device tree using "pmem-region" and created by the
> of_pmem_region_driver, since those regions do not contain mappings
> (ndr_mappings is 0).
>
> Allow userspace to set the align attribute on pre-existing regions that
> do not have mappings so that namespaces can still be within those
> regions, despite not being aligned to 16M.
>
> Fixes: 2522afb86a8c ("libnvdimm/region: Introduce an 'align' attribute")
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>

This solves the problem that I had in this thread:
https://lore.kernel.org/lkml/CA+CK2bCD13JBLMxn2mAuRyVQGKBS5ic2UqYSsxXTccszXCmHkA@mail.gmail.com/

Thank you Tyler for root causing and finding a proper fix.

Reviewed-by: Pavel Tatashin <pasha.tatashin@soleen.com>

> ---
>  drivers/nvdimm/region_devs.c | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> index ef23119db574..09cff8aa6b40 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -545,29 +545,32 @@ static ssize_t align_store(struct device *dev,
>                 struct device_attribute *attr, const char *buf, size_t len)
>  {
>         struct nd_region *nd_region = to_nd_region(dev);
> -       unsigned long val, dpa;
> -       u32 remainder;
> +       unsigned long val;
>         int rc;
>
>         rc = kstrtoul(buf, 0, &val);
>         if (rc)
>                 return rc;
>
> -       if (!nd_region->ndr_mappings)
> -               return -ENXIO;
> -
> -       /*
> -        * Ensure space-align is evenly divisible by the region
> -        * interleave-width because the kernel typically has no facility
> -        * to determine which DIMM(s), dimm-physical-addresses, would
> -        * contribute to the tail capacity in system-physical-address
> -        * space for the namespace.
> -        */
> -       dpa = div_u64_rem(val, nd_region->ndr_mappings, &remainder);
> -       if (!is_power_of_2(dpa) || dpa < PAGE_SIZE
> -                       || val > region_size(nd_region) || remainder)
> +       if (val > region_size(nd_region))
>                 return -EINVAL;
>
> +       if (nd_region->ndr_mappings) {
> +               unsigned long dpa;
> +               u32 remainder;
> +
> +               /*
> +                * Ensure space-align is evenly divisible by the region
> +                * interleave-width because the kernel typically has no facility
> +                * to determine which DIMM(s), dimm-physical-addresses, would
> +                * contribute to the tail capacity in system-physical-address
> +                * space for the namespace.
> +                */
> +               dpa = div_u64_rem(val, nd_region->ndr_mappings, &remainder);
> +               if (!is_power_of_2(dpa) || dpa < PAGE_SIZE || remainder)
> +                       return -EINVAL;
> +       }
> +
>         /*
>          * Given that space allocation consults this value multiple
>          * times ensure it does not change for the duration of the
> --
> 2.25.1
>
Aneesh Kumar K.V March 30, 2021, 11:02 a.m. UTC | #2
Tyler Hicks <tyhicks@linux.microsoft.com> writes:

> The alignment constraint for namespace creation in a region was
> increased, from 2M to 16M, for non-PowerPC architectures in v5.7 with
> commit 2522afb86a8c ("libnvdimm/region: Introduce an 'align'
> attribute"). The thought behind the change was that region alignment
> should be uniform across all architectures and, since PowerPC had the
> largest alignment constraint of 16M, all architectures should conform to
> that alignment.
>
> The change regressed namespace creation in pre-defined regions that
> relied on 2M alignment but a workaround was provided in the form of a
> sysfs attribute, named 'align', that could be adjusted to a non-default
> alignment value.
>
> However, the sysfs attribute's store function returned an error (-ENXIO)
> when userspace attempted to change the alignment of a region that had no
> mappings. This affected 2M aligned regions of volatile memory that were
> defined in a device tree using "pmem-region" and created by the
> of_pmem_region_driver, since those regions do not contain mappings
> (ndr_mappings is 0).
>
> Allow userspace to set the align attribute on pre-existing regions that
> do not have mappings so that namespaces can still be within those
> regions, despite not being aligned to 16M.
>
> Fixes: 2522afb86a8c ("libnvdimm/region: Introduce an 'align' attribute")
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> ---
>  drivers/nvdimm/region_devs.c | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> index ef23119db574..09cff8aa6b40 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -545,29 +545,32 @@ static ssize_t align_store(struct device *dev,
>  		struct device_attribute *attr, const char *buf, size_t len)
>  {
>  	struct nd_region *nd_region = to_nd_region(dev);
> -	unsigned long val, dpa;
> -	u32 remainder;
> +	unsigned long val;
>  	int rc;
>  
>  	rc = kstrtoul(buf, 0, &val);
>  	if (rc)
>  		return rc;
>  
> -	if (!nd_region->ndr_mappings)
> -		return -ENXIO;
> -
> -	/*
> -	 * Ensure space-align is evenly divisible by the region
> -	 * interleave-width because the kernel typically has no facility
> -	 * to determine which DIMM(s), dimm-physical-addresses, would
> -	 * contribute to the tail capacity in system-physical-address
> -	 * space for the namespace.
> -	 */
> -	dpa = div_u64_rem(val, nd_region->ndr_mappings, &remainder);
> -	if (!is_power_of_2(dpa) || dpa < PAGE_SIZE
> -			|| val > region_size(nd_region) || remainder)
> +	if (val > region_size(nd_region))
>  		return -EINVAL;
>  
> +	if (nd_region->ndr_mappings) {
> +		unsigned long dpa;
> +		u32 remainder;
> +
> +		/*
> +		 * Ensure space-align is evenly divisible by the region
> +		 * interleave-width because the kernel typically has no facility
> +		 * to determine which DIMM(s), dimm-physical-addresses, would
> +		 * contribute to the tail capacity in system-physical-address
> +		 * space for the namespace.
> +		 */
> +		dpa = div_u64_rem(val, nd_region->ndr_mappings, &remainder);
> +		if (!is_power_of_2(dpa) || dpa < PAGE_SIZE || remainder)
> +			return -EINVAL;
> +	}

We still want

else {

if (!is_power_of_2(val) || val < PAGE_SIZE)
         return -EINVAL?

}
> +
>  	/*
>  	 * Given that space allocation consults this value multiple
>  	 * times ensure it does not change for the duration of the
> -- 
> 2.25.1
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
Tyler Hicks March 30, 2021, 3:43 p.m. UTC | #3
On 2021-03-30 16:32:10, Aneesh Kumar K.V wrote:
> Tyler Hicks <tyhicks@linux.microsoft.com> writes:
> 
> > The alignment constraint for namespace creation in a region was
> > increased, from 2M to 16M, for non-PowerPC architectures in v5.7 with
> > commit 2522afb86a8c ("libnvdimm/region: Introduce an 'align'
> > attribute"). The thought behind the change was that region alignment
> > should be uniform across all architectures and, since PowerPC had the
> > largest alignment constraint of 16M, all architectures should conform to
> > that alignment.
> >
> > The change regressed namespace creation in pre-defined regions that
> > relied on 2M alignment but a workaround was provided in the form of a
> > sysfs attribute, named 'align', that could be adjusted to a non-default
> > alignment value.
> >
> > However, the sysfs attribute's store function returned an error (-ENXIO)
> > when userspace attempted to change the alignment of a region that had no
> > mappings. This affected 2M aligned regions of volatile memory that were
> > defined in a device tree using "pmem-region" and created by the
> > of_pmem_region_driver, since those regions do not contain mappings
> > (ndr_mappings is 0).
> >
> > Allow userspace to set the align attribute on pre-existing regions that
> > do not have mappings so that namespaces can still be within those
> > regions, despite not being aligned to 16M.
> >
> > Fixes: 2522afb86a8c ("libnvdimm/region: Introduce an 'align' attribute")
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > ---
> >  drivers/nvdimm/region_devs.c | 33 ++++++++++++++++++---------------
> >  1 file changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> > index ef23119db574..09cff8aa6b40 100644
> > --- a/drivers/nvdimm/region_devs.c
> > +++ b/drivers/nvdimm/region_devs.c
> > @@ -545,29 +545,32 @@ static ssize_t align_store(struct device *dev,
> >  		struct device_attribute *attr, const char *buf, size_t len)
> >  {
> >  	struct nd_region *nd_region = to_nd_region(dev);
> > -	unsigned long val, dpa;
> > -	u32 remainder;
> > +	unsigned long val;
> >  	int rc;
> >  
> >  	rc = kstrtoul(buf, 0, &val);
> >  	if (rc)
> >  		return rc;
> >  
> > -	if (!nd_region->ndr_mappings)
> > -		return -ENXIO;
> > -
> > -	/*
> > -	 * Ensure space-align is evenly divisible by the region
> > -	 * interleave-width because the kernel typically has no facility
> > -	 * to determine which DIMM(s), dimm-physical-addresses, would
> > -	 * contribute to the tail capacity in system-physical-address
> > -	 * space for the namespace.
> > -	 */
> > -	dpa = div_u64_rem(val, nd_region->ndr_mappings, &remainder);
> > -	if (!is_power_of_2(dpa) || dpa < PAGE_SIZE
> > -			|| val > region_size(nd_region) || remainder)
> > +	if (val > region_size(nd_region))
> >  		return -EINVAL;
> >  
> > +	if (nd_region->ndr_mappings) {
> > +		unsigned long dpa;
> > +		u32 remainder;
> > +
> > +		/*
> > +		 * Ensure space-align is evenly divisible by the region
> > +		 * interleave-width because the kernel typically has no facility
> > +		 * to determine which DIMM(s), dimm-physical-addresses, would
> > +		 * contribute to the tail capacity in system-physical-address
> > +		 * space for the namespace.
> > +		 */
> > +		dpa = div_u64_rem(val, nd_region->ndr_mappings, &remainder);
> > +		if (!is_power_of_2(dpa) || dpa < PAGE_SIZE || remainder)
> > +			return -EINVAL;
> > +	}
> 
> We still want
> 
> else {
> 
> if (!is_power_of_2(val) || val < PAGE_SIZE)
>          return -EINVAL?

Yes, very good point. I'll send out a v2 that ensures val is a power of
2 and at least page size.

Tyler

> 
> }
> > +
> >  	/*
> >  	 * Given that space allocation consults this value multiple
> >  	 * times ensure it does not change for the duration of the
> > -- 
> > 2.25.1
> > _______________________________________________
> > Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> > To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
>
diff mbox series

Patch

diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index ef23119db574..09cff8aa6b40 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -545,29 +545,32 @@  static ssize_t align_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t len)
 {
 	struct nd_region *nd_region = to_nd_region(dev);
-	unsigned long val, dpa;
-	u32 remainder;
+	unsigned long val;
 	int rc;
 
 	rc = kstrtoul(buf, 0, &val);
 	if (rc)
 		return rc;
 
-	if (!nd_region->ndr_mappings)
-		return -ENXIO;
-
-	/*
-	 * Ensure space-align is evenly divisible by the region
-	 * interleave-width because the kernel typically has no facility
-	 * to determine which DIMM(s), dimm-physical-addresses, would
-	 * contribute to the tail capacity in system-physical-address
-	 * space for the namespace.
-	 */
-	dpa = div_u64_rem(val, nd_region->ndr_mappings, &remainder);
-	if (!is_power_of_2(dpa) || dpa < PAGE_SIZE
-			|| val > region_size(nd_region) || remainder)
+	if (val > region_size(nd_region))
 		return -EINVAL;
 
+	if (nd_region->ndr_mappings) {
+		unsigned long dpa;
+		u32 remainder;
+
+		/*
+		 * Ensure space-align is evenly divisible by the region
+		 * interleave-width because the kernel typically has no facility
+		 * to determine which DIMM(s), dimm-physical-addresses, would
+		 * contribute to the tail capacity in system-physical-address
+		 * space for the namespace.
+		 */
+		dpa = div_u64_rem(val, nd_region->ndr_mappings, &remainder);
+		if (!is_power_of_2(dpa) || dpa < PAGE_SIZE || remainder)
+			return -EINVAL;
+	}
+
 	/*
 	 * Given that space allocation consults this value multiple
 	 * times ensure it does not change for the duration of the