[PATCH] ndctl, check: Ensure mmap of BTT sections work with 64K page-size

[PATCH] ndctl, check: Ensure mmap of BTT sections work with 64K page-size

[Vaibhav Jain]

Presently on PPC64 which uses a 64K page-size, ndtl-check command
fails on a BTT device with following error:

$sudo ndctl check-namespace namespace0.0 -vv
namespace0.0: namespace_check: checking namespace0.0
namespace0.0: btt_discover_arenas: found 1 BTT arena
namespace0.0: btt_create_mappings: mmap arena[0].info [sz = 0x1000, off = 0x1000] failed: Invalid argument
error checking namespaces: Invalid argument
checked 0 namespaces

Error happens when btt_create_mappings() tries to mmap the sections of
the BTT device which are usually 4K offset aligned. However the mmap()
syscall expects the 'offset' argument to be in multiples of page-size,
hence it returns EINVAL causing the btt_create_mappings() to error
out.

As a fix for the issue this patch proposes addition of two new
functions to 'check.c' namely btt_mmap/btt_unmap that can be used to
map/unmap parts of BTT device to ndctl process address-space. The
functions tweaks the requested 'offset' argument to mmap() ensuring
that its page-size aligned and then fix-ups the returned pointer such
that it points to the requested offset within m-mapped region.

Reported-by: harish@linux.ibm.com
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
 ndctl/check.c | 71 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 56 insertions(+), 15 deletions(-)

diff --git a/ndctl/check.c b/ndctl/check.c
index 8a7125053865..18d259048616 100644
--- a/ndctl/check.c
+++ b/ndctl/check.c
@@ -907,6 +907,47 @@ static int btt_discover_arenas(struct btt_chk *bttc)
 	return ret;
 }
 
+/* Mmap requested btt region so it works with non 4-K page sizes */
+static void *btt_mmap(struct btt_chk *bttc, void *addr, size_t length,
+		      int prot, int flags, off_t offset)
+{
+	off_t shift;
+
+	/* Calculate the shift back needed to make offset page aligned */
+	shift = offset - rounddown(offset, bttc->sys_page_size);
+
+	/* Update the offset and length with the shift calculated above */
+	offset -= shift;
+	length += shift;
+
+	addr = mmap(addr, length, prot, flags, bttc->fd, offset);
+
+	/* If needed fixup the return pointer to correct offset request */
+	if (addr != MAP_FAILED)
+		addr = (void *) ((uintptr_t)addr + shift);
+
+	dbg(bttc, "mmap: addr=%p length=0x%lx offset=0x%lx shift=0x%lx\n",
+	    addr, length, offset, shift);
+
+	return addr;
+}
+
+static void btt_unmap(struct btt_chk *bttc, void *ptr, size_t length)
+{
+	uintptr_t addr = ptr;
+	off_t shift;
+
+	/* Calculate the shift back needed to make offset page aligned */
+	shift = addr - rounddown(addr, bttc->sys_page_size);
+
+	addr -= shift;
+	length += shift;
+
+	munmap((void *)addr, length);
+	dbg(bttc, "unmap: addr=%p length=0x%lx shift=0x%lx\n",
+	    addr, length, shift);
+}
+
 static int btt_create_mappings(struct btt_chk *bttc)
 {
 	struct arena_info *a;
@@ -921,8 +962,8 @@ static int btt_create_mappings(struct btt_chk *bttc)
 	for (i = 0; i < bttc->num_arenas; i++) {
 		a = &bttc->arena[i];
 		a->map.info_len = BTT_INFO_SIZE;
-		a->map.info = mmap(NULL, a->map.info_len, mmap_flags,
-			MAP_SHARED, bttc->fd, a->infooff);
+		a->map.info = btt_mmap(bttc, NULL, a->map.info_len, mmap_flags,
+				       MAP_SHARED, a->infooff);
 		if (a->map.info == MAP_FAILED) {
 			err(bttc, "mmap arena[%d].info [sz = %#lx, off = %#lx] failed: %s\n",
 				i, a->map.info_len, a->infooff, strerror(errno));
@@ -930,8 +971,8 @@ static int btt_create_mappings(struct btt_chk *bttc)
 		}
 
 		a->map.data_len = a->mapoff - a->dataoff;
-		a->map.data = mmap(NULL, a->map.data_len, mmap_flags,
-			MAP_SHARED, bttc->fd, a->dataoff);
+		a->map.data = btt_mmap(bttc, NULL, a->map.data_len, mmap_flags,
+				       MAP_SHARED, a->dataoff);
 		if (a->map.data == MAP_FAILED) {
 			err(bttc, "mmap arena[%d].data [sz = %#lx, off = %#lx] failed: %s\n",
 				i, a->map.data_len, a->dataoff, strerror(errno));
@@ -939,8 +980,8 @@ static int btt_create_mappings(struct btt_chk *bttc)
 		}
 
 		a->map.map_len = a->logoff - a->mapoff;
-		a->map.map = mmap(NULL, a->map.map_len, mmap_flags,
-			MAP_SHARED, bttc->fd, a->mapoff);
+		a->map.map = btt_mmap(bttc, NULL, a->map.map_len, mmap_flags,
+				      MAP_SHARED, a->mapoff);
 		if (a->map.map == MAP_FAILED) {
 			err(bttc, "mmap arena[%d].map [sz = %#lx, off = %#lx] failed: %s\n",
 				i, a->map.map_len, a->mapoff, strerror(errno));
@@ -948,8 +989,8 @@ static int btt_create_mappings(struct btt_chk *bttc)
 		}
 
 		a->map.log_len = a->info2off - a->logoff;
-		a->map.log = mmap(NULL, a->map.log_len, mmap_flags,
-			MAP_SHARED, bttc->fd, a->logoff);
+		a->map.log = btt_mmap(bttc, NULL, a->map.log_len, mmap_flags,
+				  MAP_SHARED, a->logoff);
 		if (a->map.log == MAP_FAILED) {
 			err(bttc, "mmap arena[%d].log [sz = %#lx, off = %#lx] failed: %s\n",
 				i, a->map.log_len, a->logoff, strerror(errno));
@@ -957,8 +998,8 @@ static int btt_create_mappings(struct btt_chk *bttc)
 		}
 
 		a->map.info2_len = BTT_INFO_SIZE;
-		a->map.info2 = mmap(NULL, a->map.info2_len, mmap_flags,
-			MAP_SHARED, bttc->fd, a->info2off);
+		a->map.info2 = btt_mmap(bttc, NULL, a->map.info2_len,
+					mmap_flags, MAP_SHARED, a->info2off);
 		if (a->map.info2 == MAP_FAILED) {
 			err(bttc, "mmap arena[%d].info2 [sz = %#lx, off = %#lx] failed: %s\n",
 				i, a->map.info2_len, a->info2off, strerror(errno));
@@ -977,15 +1018,15 @@ static void btt_remove_mappings(struct btt_chk *bttc)
 	for (i = 0; i < bttc->num_arenas; i++) {
 		a = &bttc->arena[i];
 		if (a->map.info)
-			munmap(a->map.info, a->map.info_len);
+			btt_unmap(bttc, a->map.info, a->map.info_len);
 		if (a->map.data)
-			munmap(a->map.data, a->map.data_len);
+			btt_unmap(bttc, a->map.data, a->map.data_len);
 		if (a->map.map)
-			munmap(a->map.map, a->map.map_len);
+			btt_unmap(bttc, a->map.map, a->map.map_len);
 		if (a->map.log)
-			munmap(a->map.log, a->map.log_len);
+			btt_unmap(bttc, a->map.log, a->map.log_len);
 		if (a->map.info2)
-			munmap(a->map.info2, a->map.info2_len);
+			btt_unmap(bttc, a->map.info2, a->map.info2_len);
 	}
 }
 
-- 
2.21.0

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

Re: [PATCH] ndctl, check: Ensure mmap of BTT sections work with 64K page-size

[Verma, Vishal L]

On Thu, 2019-07-04 at 08:21 +0530, Vaibhav Jain wrote:
> Presently on PPC64 which uses a 64K page-size, ndtl-check command

Drop 'Presently'. Maybe something like "On PPC64, ... ndctl-check-
namespace would fail.."

Also s/ndtl/ndctl/

> fails on a BTT device with following error:

on a BTT namespace.

> 
> $sudo ndctl check-namespace namespace0.0 -vv
> namespace0.0: namespace_check: checking namespace0.0
> namespace0.0: btt_discover_arenas: found 1 BTT arena
> namespace0.0: btt_create_mappings: mmap arena[0].info [sz = 0x1000, off = 0x1000] failed: Invalid argument
> error checking namespaces: Invalid argument
> checked 0 namespaces

Perhaps indent the above by two spaces so it is clearly visible as a
copy-pasted session.

> 
> Error happens when btt_create_mappings() tries to mmap the sections of
> the BTT device which are usually 4K offset aligned. However the mmap()
> syscall expects the 'offset' argument to be in multiples of page-size,
> hence it returns EINVAL causing the btt_create_mappings() to error
> out.
> 
> As a fix for the issue this patch proposes addition of two new
> functions to 'check.c' namely btt_mmap/btt_unmap that can be used to
> map/unmap parts of BTT device to ndctl process address-space. The
> functions tweaks the requested 'offset' argument to mmap() ensuring
> that its page-size aligned and then fix-ups the returned pointer such
> that it points to the requested offset within m-mapped region.

'mmaped region'

> 
> Reported-by: harish@linux.ibm.com

Could you make this the canonical Name <email> format?

> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
>  ndctl/check.c | 71 ++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 56 insertions(+), 15 deletions(-)
> 
> diff --git a/ndctl/check.c b/ndctl/check.c
> index 8a7125053865..18d259048616 100644
> --- a/ndctl/check.c
> +++ b/ndctl/check.c
> @@ -907,6 +907,47 @@ static int btt_discover_arenas(struct btt_chk *bttc)
>  	return ret;
>  }
>  
> +/* Mmap requested btt region so it works with non 4-K page sizes */

Maybe something like "Wrap mmap(2) so that the underlying system call
can use system page sizes for the mappings, but the checking code
doesn't have to worry about that detail, and can map smaller-than-page-
size sections"

> +static void *btt_mmap(struct btt_chk *bttc, void *addr, size_t length,
> +		      int prot, int flags, off_t offset)

I see you tried to keep the argument list similar to mmap(2), but I
suspect it can be made much cleaner if we only pass in just what's
needed.

Since we're passing in bttc, the bttc->opts->repair check to determine
mmap_flags could be moved into this helper. The NULL and MAP_SHARED
arguments (addr and prot) are always the same, so no need to pass those.
With this, the callers look much cleaner:

a->map_info = btt_mmap(bttc, a->map.info_len, a->infooff);

> +{
> +	off_t shift;

In both of thse wrappers, and especially since we are printing 'shift ='
in a debug message - 

'shift' sounds more like an arithmatic left/right shift operation, and
it might be confusing to the user what it means. I'd suggest naming this
something like 'page_offset', or 'page_start_pad', or something along
those lines.

> +
> +	/* Calculate the shift back needed to make offset page aligned */

"Calculate the offset from system page size boundary"

> +	shift = offset - rounddown(offset, bttc->sys_page_size);
> +
> +	/* Update the offset and length with the shift calculated above */
> +	offset -= shift;
> +	length += shift;
> +
> +	addr = mmap(addr, length, prot, flags, bttc->fd, offset);
> +
> +	/* If needed fixup the return pointer to correct offset request */

requested

> +	if (addr != MAP_FAILED)
> +		addr = (void *) ((uintptr_t)addr + shift);

The (uintptr_t) cast should be ok to drop, for v66 we are removing the
pointer arithmatic warning:
https://patchwork.kernel.org/patch/11062697/

In fact, since 'shift' is in bytes, isn't an unsigned int cast actually *wrong*?

> +
> +	dbg(bttc, "mmap: addr=%p length=0x%lx offset=0x%lx shift=0x%lx\n",
> +	    addr, length, offset, shift);

It would be nice to make this print more consistent with the err()
prints in btt_create_mappings - so spaces sround the '=', and note you
can use %#lx instead of 0x%lx

Also the function name 'btt_map' will automatically get prefixed by
dbg(), so no need for another 'mmap' at the start.

	dbg(bttc, "addr = %p, length = %#lx, offset = %#lx, page_offset = %#lx\n",
		addr, length, offset, page_offset);

> +
> +	return addr;
> +}
> +
> +static void btt_unmap(struct btt_chk *bttc, void *ptr, size_t length)
> +{
> +	uintptr_t addr = ptr;
> +	off_t shift;
> +
> +	/* Calculate the shift back needed to make offset page aligned */
> +	shift = addr - rounddown(addr, bttc->sys_page_size);
> +
> +	addr -= shift;
> +	length += shift;
> +
> +	munmap((void *)addr, length);
> +	dbg(bttc, "unmap: addr=%p length=0x%lx shift=0x%lx\n",
> +	    addr, length, shift);

Similar comments about the print as above.

> +}
> +
>  static int btt_create_mappings(struct btt_chk *bttc)
>  {
>  	struct arena_info *a;
> @@ -921,8 +962,8 @@ static int btt_create_mappings(struct btt_chk *bttc)
>  	for (i = 0; i < bttc->num_arenas; i++) {
>  		a = &bttc->arena[i];
>  		a->map.info_len = BTT_INFO_SIZE;
> -		a->map.info = mmap(NULL, a->map.info_len, mmap_flags,
> -			MAP_SHARED, bttc->fd, a->infooff);
> +		a->map.info = btt_mmap(bttc, NULL, a->map.info_len, mmap_flags,
> +				       MAP_SHARED, a->infooff);
>  		if (a->map.info == MAP_FAILED) {

I wonder if it will also be cleaner to sequester away the MAP_FAILED
detail of the mmap API into the wrapper. The wrapper can just return
NULL for MAP_FAILED, and then this check just becomes if (!a->map.info)

>  			err(bttc, "mmap arena[%d].info [sz = %#lx, off = %#lx] failed: %s\n",
>  				i, a->map.info_len, a->infooff, strerror(errno));
> @@ -930,8 +971,8 @@ static int btt_create_mappings(struct btt_chk *bttc)
>  		}
>  
>  		a->map.data_len = a->mapoff - a->dataoff;
> -		a->map.data = mmap(NULL, a->map.data_len, mmap_flags,
> -			MAP_SHARED, bttc->fd, a->dataoff);
> +		a->map.data = btt_mmap(bttc, NULL, a->map.data_len, mmap_flags,
> +				       MAP_SHARED, a->dataoff);
>  		if (a->map.data == MAP_FAILED) {
>  			err(bttc, "mmap arena[%d].data [sz = %#lx, off = %#lx] failed: %s\n",
>  				i, a->map.data_len, a->dataoff, strerror(errno));
> @@ -939,8 +980,8 @@ static int btt_create_mappings(struct btt_chk *bttc)
>  		}
>  
>  		a->map.map_len = a->logoff - a->mapoff;
> -		a->map.map = mmap(NULL, a->map.map_len, mmap_flags,
> -			MAP_SHARED, bttc->fd, a->mapoff);
> +		a->map.map = btt_mmap(bttc, NULL, a->map.map_len, mmap_flags,
> +				      MAP_SHARED, a->mapoff);
>  		if (a->map.map == MAP_FAILED) {
>  			err(bttc, "mmap arena[%d].map [sz = %#lx, off = %#lx] failed: %s\n",
>  				i, a->map.map_len, a->mapoff, strerror(errno));
> @@ -948,8 +989,8 @@ static int btt_create_mappings(struct btt_chk *bttc)
>  		}
>  
>  		a->map.log_len = a->info2off - a->logoff;
> -		a->map.log = mmap(NULL, a->map.log_len, mmap_flags,
> -			MAP_SHARED, bttc->fd, a->logoff);
> +		a->map.log = btt_mmap(bttc, NULL, a->map.log_len, mmap_flags,
> +				  MAP_SHARED, a->logoff);
>  		if (a->map.log == MAP_FAILED) {
>  			err(bttc, "mmap arena[%d].log [sz = %#lx, off = %#lx] failed: %s\n",
>  				i, a->map.log_len, a->logoff, strerror(errno));
> @@ -957,8 +998,8 @@ static int btt_create_mappings(struct btt_chk *bttc)
>  		}
>  
>  		a->map.info2_len = BTT_INFO_SIZE;
> -		a->map.info2 = mmap(NULL, a->map.info2_len, mmap_flags,
> -			MAP_SHARED, bttc->fd, a->info2off);
> +		a->map.info2 = btt_mmap(bttc, NULL, a->map.info2_len,
> +					mmap_flags, MAP_SHARED, a->info2off);
>  		if (a->map.info2 == MAP_FAILED) {
>  			err(bttc, "mmap arena[%d].info2 [sz = %#lx, off = %#lx] failed: %s\n",
>  				i, a->map.info2_len, a->info2off, strerror(errno));
> @@ -977,15 +1018,15 @@ static void btt_remove_mappings(struct btt_chk *bttc)
>  	for (i = 0; i < bttc->num_arenas; i++) {
>  		a = &bttc->arena[i];
>  		if (a->map.info)
> -			munmap(a->map.info, a->map.info_len);
> +			btt_unmap(bttc, a->map.info, a->map.info_len);
>  		if (a->map.data)
> -			munmap(a->map.data, a->map.data_len);
> +			btt_unmap(bttc, a->map.data, a->map.data_len);
>  		if (a->map.map)
> -			munmap(a->map.map, a->map.map_len);
> +			btt_unmap(bttc, a->map.map, a->map.map_len);
>  		if (a->map.log)
> -			munmap(a->map.log, a->map.log_len);
> +			btt_unmap(bttc, a->map.log, a->map.log_len);
>  		if (a->map.info2)
> -			munmap(a->map.info2, a->map.info2_len);
> +			btt_unmap(bttc, a->map.info2, a->map.info2_len);
>  	}
>  }
>  


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

Re: [PATCH] ndctl, check: Ensure mmap of BTT sections work with 64K page-size

[Vaibhav Jain]

Thanks for reviewing this patch Vishal. I have prepped a v2 adressing
all your review comments with one exception below:

"Verma, Vishal L" <vishal.l.verma@intel.com> writes:

> On Thu, 2019-07-04 at 08:21 +0530, Vaibhav Jain wrote:
>> Presently on PPC64 which uses a 64K page-size, ndtl-check command
>
> Drop 'Presently'. Maybe something like "On PPC64, ... ndctl-check-
> namespace would fail.."
>
> Also s/ndtl/ndctl/
>
>> fails on a BTT device with following error:
>
> on a BTT namespace.
>
>> 
>> $sudo ndctl check-namespace namespace0.0 -vv
>> namespace0.0: namespace_check: checking namespace0.0
>> namespace0.0: btt_discover_arenas: found 1 BTT arena
>> namespace0.0: btt_create_mappings: mmap arena[0].info [sz = 0x1000, off = 0x1000] failed: Invalid argument
>> error checking namespaces: Invalid argument
>> checked 0 namespaces
>
> Perhaps indent the above by two spaces so it is clearly visible as a
> copy-pasted session.
>
>> 
>> Error happens when btt_create_mappings() tries to mmap the sections of
>> the BTT device which are usually 4K offset aligned. However the mmap()
>> syscall expects the 'offset' argument to be in multiples of page-size,
>> hence it returns EINVAL causing the btt_create_mappings() to error
>> out.
>> 
>> As a fix for the issue this patch proposes addition of two new
>> functions to 'check.c' namely btt_mmap/btt_unmap that can be used to
>> map/unmap parts of BTT device to ndctl process address-space. The
>> functions tweaks the requested 'offset' argument to mmap() ensuring
>> that its page-size aligned and then fix-ups the returned pointer such
>> that it points to the requested offset within m-mapped region.
>
> 'mmaped region'
>
>> 
>> Reported-by: harish@linux.ibm.com
>
> Could you make this the canonical Name <email> format?
>
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>>  ndctl/check.c | 71 ++++++++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 56 insertions(+), 15 deletions(-)
>> 
>> diff --git a/ndctl/check.c b/ndctl/check.c
>> index 8a7125053865..18d259048616 100644
>> --- a/ndctl/check.c
>> +++ b/ndctl/check.c
>> @@ -907,6 +907,47 @@ static int btt_discover_arenas(struct btt_chk *bttc)
>>  	return ret;
>>  }
>>  
>> +/* Mmap requested btt region so it works with non 4-K page sizes */
>
> Maybe something like "Wrap mmap(2) so that the underlying system call
> can use system page sizes for the mappings, but the checking code
> doesn't have to worry about that detail, and can map smaller-than-page-
> size sections"
>
>> +static void *btt_mmap(struct btt_chk *bttc, void *addr, size_t length,
>> +		      int prot, int flags, off_t offset)
>
> I see you tried to keep the argument list similar to mmap(2), but I
> suspect it can be made much cleaner if we only pass in just what's
> needed.
>
> Since we're passing in bttc, the bttc->opts->repair check to determine
> mmap_flags could be moved into this helper. The NULL and MAP_SHARED
> arguments (addr and prot) are always the same, so no need to pass those.
> With this, the callers look much cleaner:
>
> a->map_info = btt_mmap(bttc, a->map.info_len, a->infooff);
>
>> +{
>> +	off_t shift;
>
> In both of thse wrappers, and especially since we are printing 'shift ='
> in a debug message - 
>
> 'shift' sounds more like an arithmatic left/right shift operation, and
> it might be confusing to the user what it means. I'd suggest naming this
> something like 'page_offset', or 'page_start_pad', or something along
> those lines.
>
>> +
>> +	/* Calculate the shift back needed to make offset page aligned */
>
> "Calculate the offset from system page size boundary"
>
>> +	shift = offset - rounddown(offset, bttc->sys_page_size);
>> +
>> +	/* Update the offset and length with the shift calculated above */
>> +	offset -= shift;
>> +	length += shift;
>> +
>> +	addr = mmap(addr, length, prot, flags, bttc->fd, offset);
>> +
>> +	/* If needed fixup the return pointer to correct offset request */
>
> requested
>
>> +	if (addr != MAP_FAILED)
>> +		addr = (void *) ((uintptr_t)addr + shift);
>
> The (uintptr_t) cast should be ok to drop, for v66 we are removing the
> pointer arithmatic warning:
> https://patchwork.kernel.org/patch/11062697/
>
> In fact, since 'shift' is in bytes, isn't an unsigned int cast
> actually *wrong*?
Not sure if I understand your review comment correctly. With uintptr_t
cast and 'shift' in bytes, addr will be assigned 'addr + shift' instead
of 'addr + shift * sizeof (unsigned int)'

So I think the arithmetic I am doing here is correct.

>
>> +
>> +	dbg(bttc, "mmap: addr=%p length=0x%lx offset=0x%lx shift=0x%lx\n",
>> +	    addr, length, offset, shift);
>
> It would be nice to make this print more consistent with the err()
> prints in btt_create_mappings - so spaces sround the '=', and note you
> can use %#lx instead of 0x%lx
>
> Also the function name 'btt_map' will automatically get prefixed by
> dbg(), so no need for another 'mmap' at the start.
>
> 	dbg(bttc, "addr = %p, length = %#lx, offset = %#lx, page_offset = %#lx\n",
> 		addr, length, offset, page_offset);
>
>> +
>> +	return addr;
>> +}
>> +
>> +static void btt_unmap(struct btt_chk *bttc, void *ptr, size_t length)
>> +{
>> +	uintptr_t addr = ptr;
>> +	off_t shift;
>> +
>> +	/* Calculate the shift back needed to make offset page aligned */
>> +	shift = addr - rounddown(addr, bttc->sys_page_size);
>> +
>> +	addr -= shift;
>> +	length += shift;
>> +
>> +	munmap((void *)addr, length);
>> +	dbg(bttc, "unmap: addr=%p length=0x%lx shift=0x%lx\n",
>> +	    addr, length, shift);
>
> Similar comments about the print as above.
>
>> +}
>> +
>>  static int btt_create_mappings(struct btt_chk *bttc)
>>  {
>>  	struct arena_info *a;
>> @@ -921,8 +962,8 @@ static int btt_create_mappings(struct btt_chk *bttc)
>>  	for (i = 0; i < bttc->num_arenas; i++) {
>>  		a = &bttc->arena[i];
>>  		a->map.info_len = BTT_INFO_SIZE;
>> -		a->map.info = mmap(NULL, a->map.info_len, mmap_flags,
>> -			MAP_SHARED, bttc->fd, a->infooff);
>> +		a->map.info = btt_mmap(bttc, NULL, a->map.info_len, mmap_flags,
>> +				       MAP_SHARED, a->infooff);
>>  		if (a->map.info == MAP_FAILED) {
>
> I wonder if it will also be cleaner to sequester away the MAP_FAILED
> detail of the mmap API into the wrapper. The wrapper can just return
> NULL for MAP_FAILED, and then this check just becomes if (!a->map.info)
>
>>  			err(bttc, "mmap arena[%d].info [sz = %#lx, off = %#lx] failed: %s\n",
>>  				i, a->map.info_len, a->infooff, strerror(errno));
>> @@ -930,8 +971,8 @@ static int btt_create_mappings(struct btt_chk *bttc)
>>  		}
>>  
>>  		a->map.data_len = a->mapoff - a->dataoff;
>> -		a->map.data = mmap(NULL, a->map.data_len, mmap_flags,
>> -			MAP_SHARED, bttc->fd, a->dataoff);
>> +		a->map.data = btt_mmap(bttc, NULL, a->map.data_len, mmap_flags,
>> +				       MAP_SHARED, a->dataoff);
>>  		if (a->map.data == MAP_FAILED) {
>>  			err(bttc, "mmap arena[%d].data [sz = %#lx, off = %#lx] failed: %s\n",
>>  				i, a->map.data_len, a->dataoff, strerror(errno));
>> @@ -939,8 +980,8 @@ static int btt_create_mappings(struct btt_chk *bttc)
>>  		}
>>  
>>  		a->map.map_len = a->logoff - a->mapoff;
>> -		a->map.map = mmap(NULL, a->map.map_len, mmap_flags,
>> -			MAP_SHARED, bttc->fd, a->mapoff);
>> +		a->map.map = btt_mmap(bttc, NULL, a->map.map_len, mmap_flags,
>> +				      MAP_SHARED, a->mapoff);
>>  		if (a->map.map == MAP_FAILED) {
>>  			err(bttc, "mmap arena[%d].map [sz = %#lx, off = %#lx] failed: %s\n",
>>  				i, a->map.map_len, a->mapoff, strerror(errno));
>> @@ -948,8 +989,8 @@ static int btt_create_mappings(struct btt_chk *bttc)
>>  		}
>>  
>>  		a->map.log_len = a->info2off - a->logoff;
>> -		a->map.log = mmap(NULL, a->map.log_len, mmap_flags,
>> -			MAP_SHARED, bttc->fd, a->logoff);
>> +		a->map.log = btt_mmap(bttc, NULL, a->map.log_len, mmap_flags,
>> +				  MAP_SHARED, a->logoff);
>>  		if (a->map.log == MAP_FAILED) {
>>  			err(bttc, "mmap arena[%d].log [sz = %#lx, off = %#lx] failed: %s\n",
>>  				i, a->map.log_len, a->logoff, strerror(errno));
>> @@ -957,8 +998,8 @@ static int btt_create_mappings(struct btt_chk *bttc)
>>  		}
>>  
>>  		a->map.info2_len = BTT_INFO_SIZE;
>> -		a->map.info2 = mmap(NULL, a->map.info2_len, mmap_flags,
>> -			MAP_SHARED, bttc->fd, a->info2off);
>> +		a->map.info2 = btt_mmap(bttc, NULL, a->map.info2_len,
>> +					mmap_flags, MAP_SHARED, a->info2off);
>>  		if (a->map.info2 == MAP_FAILED) {
>>  			err(bttc, "mmap arena[%d].info2 [sz = %#lx, off = %#lx] failed: %s\n",
>>  				i, a->map.info2_len, a->info2off, strerror(errno));
>> @@ -977,15 +1018,15 @@ static void btt_remove_mappings(struct btt_chk *bttc)
>>  	for (i = 0; i < bttc->num_arenas; i++) {
>>  		a = &bttc->arena[i];
>>  		if (a->map.info)
>> -			munmap(a->map.info, a->map.info_len);
>> +			btt_unmap(bttc, a->map.info, a->map.info_len);
>>  		if (a->map.data)
>> -			munmap(a->map.data, a->map.data_len);
>> +			btt_unmap(bttc, a->map.data, a->map.data_len);
>>  		if (a->map.map)
>> -			munmap(a->map.map, a->map.map_len);
>> +			btt_unmap(bttc, a->map.map, a->map.map_len);
>>  		if (a->map.log)
>> -			munmap(a->map.log, a->map.log_len);
>> +			btt_unmap(bttc, a->map.log, a->map.log_len);
>>  		if (a->map.info2)
>> -			munmap(a->map.info2, a->map.info2_len);
>> +			btt_unmap(bttc, a->map.info2, a->map.info2_len);
>>  	}
>>  }
>>  
>
>

Cheers,
-- 
Vaibhav Jain <vaibhav@linux.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.

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

Re: [PATCH] ndctl, check: Ensure mmap of BTT sections work with 64K page-size

[Verma, Vishal L]

On Mon, 2019-08-05 at 18:39 +0530, Vaibhav Jain wrote:
> Thanks for reviewing this patch Vishal. I have prepped a v2 adressing
> all your review comments with one exception below:

No problem, thanks for fixing this.

> 
> "Verma, Vishal L" <vishal.l.verma@intel.com> writes:
> 
> > On Thu, 2019-07-04 at 08:21 +0530, Vaibhav Jain wrote:
> > > 
> > 
> > > +	if (addr != MAP_FAILED)
> > > +		addr = (void *) ((uintptr_t)addr + shift);
> > 
> > The (uintptr_t) cast should be ok to drop, for v66 we are removing
> > the
> > pointer arithmatic warning:
> > https://patchwork.kernel.org/patch/11062697/
> > 
> > In fact, since 'shift' is in bytes, isn't an unsigned int cast
> > actually *wrong*?
> Not sure if I understand your review comment correctly. With uintptr_t
> cast and 'shift' in bytes, addr will be assigned 'addr + shift'
> instead
> of 'addr + shift * sizeof (unsigned int)'
> 
> So I think the arithmetic I am doing here is correct.
> 
Yes you're right - I conflated a (uintptr_t) cast with an (unsigned int)
cast. The latter would actually be wrong, but I see now that uintptr_t
is correct. However, given the above patch where we drop the pointer
arithmetic warning, I think it should be OK to manipulate the void
pointer directly, and this makes the line cleaner and more concise.

Thanks,
	-Vishal


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

Re: [PATCH] ndctl, check: Ensure mmap of BTT sections work with 64K page-size

[Vaibhav Jain]

"Verma, Vishal L" <vishal.l.verma@intel.com> writes:

> On Mon, 2019-08-05 at 18:39 +0530, Vaibhav Jain wrote:
>> Thanks for reviewing this patch Vishal. I have prepped a v2 adressing
>> all your review comments with one exception below:
>
> No problem, thanks for fixing this.
>
>> 
>> "Verma, Vishal L" <vishal.l.verma@intel.com> writes:
>> 
>> > On Thu, 2019-07-04 at 08:21 +0530, Vaibhav Jain wrote:
>> > > 
>> > 
>> > > +	if (addr != MAP_FAILED)
>> > > +		addr = (void *) ((uintptr_t)addr + shift);
>> > 
>> > The (uintptr_t) cast should be ok to drop, for v66 we are removing
>> > the
>> > pointer arithmatic warning:
>> > https://patchwork.kernel.org/patch/11062697/
>> > 
>> > In fact, since 'shift' is in bytes, isn't an unsigned int cast
>> > actually *wrong*?
>> Not sure if I understand your review comment correctly. With uintptr_t
>> cast and 'shift' in bytes, addr will be assigned 'addr + shift'
>> instead
>> of 'addr + shift * sizeof (unsigned int)'
>> 
>> So I think the arithmetic I am doing here is correct.
>> 
> Yes you're right - I conflated a (uintptr_t) cast with an (unsigned int)
> cast. The latter would actually be wrong, but I see now that uintptr_t
> is correct. However, given the above patch where we drop the pointer
> arithmetic warning, I think it should be OK to manipulate the void
> pointer directly, and this makes the line cleaner and more concise.
>
> Thanks,
> 	-Vishal

Sure agreed. I have sent out a v2 patch titled "[PATCH v2] ndctl, check:
Ensure mmap of BTT sections work with 64K page-sizes" with the changes.

Thanks,
-- 
Vaibhav Jain <vaibhav@linux.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.

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