* [PATCH v3] ndctl, check: Ensure mmap of BTT sections work with 64K page-sizes
@ 2019-08-06 10:50 Vaibhav Jain
2019-08-12 21:46 ` Jeff Moyer
0 siblings, 1 reply; 4+ messages in thread
From: Vaibhav Jain @ 2019-08-06 10:50 UTC (permalink / raw)
To: linux-nvdimm, Verma, Vishal L
Cc: Harish Sriram, Vaibhav Jain, Aneesh Kumar K.V
On PPC64 which uses a 64K page-size, ndtl-check command fails on a BTT
namespace 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
An 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 mmapped region.
With these newly introduced functions the patch updates the call-sites
in 'check.c' to use these functions instead of mmap/unmap syscalls.
Also since btt_mmap returns NULL if mmap operation fails, the
error check call-sites can be made against NULL instead of MAP_FAILED.
Reported-by: Harish Sriram <harish@linux.ibm.com>
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
Changelog:
v3:
* Fixed a log string that was splitted across multiple lines [Vishal]
v2:
* Updated patch description to include canonical email address of bug
reporter. [Vishal]
* Improved the comment describing function btt_mmap() in 'check.c'
[Vishal]
* Simplified the argument list for btt_mmap() to just accept bttc,
length and offset. Other arguments for mmap() are derived from these
args. [Vishal]
* Renamed 'shift' variable in btt_mmap()/unmap() to 'page_offset'
[Vishal]
* Improved the dbg() messages logged inside
btt_mmap()/unmap(). [Vishal]
* Return NULL from btt_mmap() in case of an error. [Vishal]
* Changed the all sites to btt_mmap() to perform error check against
NULL return value instead of MAP_FAILED. [Vishal]
---
ndctl/check.c | 93 +++++++++++++++++++++++++++++++++++++--------------
1 file changed, 67 insertions(+), 26 deletions(-)
diff --git a/ndctl/check.c b/ndctl/check.c
index 8a7125053865..5969012eba84 100644
--- a/ndctl/check.c
+++ b/ndctl/check.c
@@ -907,59 +907,100 @@ static int btt_discover_arenas(struct btt_chk *bttc)
return ret;
}
-static int btt_create_mappings(struct btt_chk *bttc)
+/*
+ * Wrap call to mmap(2) to work with btt device offsets that are not aligned
+ * to system page boundary. It works by rounding down the requested offset
+ * to sys_page_size when calling mmap(2) and then returning a fixed-up pointer
+ * to the correct offset in the mmaped region.
+ */
+static void *btt_mmap(struct btt_chk *bttc, size_t length, off_t offset)
{
- struct arena_info *a;
- int mmap_flags;
- int i;
+ off_t page_offset;
+ int prot_flags;
+ uint8_t *addr;
if (!bttc->opts->repair)
- mmap_flags = PROT_READ;
+ prot_flags = PROT_READ;
else
- mmap_flags = PROT_READ|PROT_WRITE;
+ prot_flags = PROT_READ|PROT_WRITE;
+
+ /* Calculate the page_offset from the system page boundary */
+ page_offset = offset - rounddown(offset, bttc->sys_page_size);
+
+ /* Update the offset and length with the page_offset calculated above */
+ offset -= page_offset;
+ length += page_offset;
+
+ addr = mmap(NULL, length, prot_flags, MAP_SHARED, bttc->fd, offset);
+
+ /* If needed fixup the return pointer to correct offset requested */
+ if (addr != MAP_FAILED)
+ addr += page_offset;
+
+ dbg(bttc, "addr = %p, length = %#lx, offset = %#lx, page_offset = %#lx\n",
+ (void *) addr, length, offset, page_offset);
+
+ return addr == MAP_FAILED ? NULL : addr;
+}
+
+static void btt_unmap(struct btt_chk *bttc, void *ptr, size_t length)
+{
+ off_t page_offset;
+ uintptr_t addr = (uintptr_t) ptr;
+
+ /* Calculate the page_offset from system page boundary */
+ page_offset = addr - rounddown(addr, bttc->sys_page_size);
+
+ addr -= page_offset;
+ length += page_offset;
+
+ munmap((void *) addr, length);
+ dbg(bttc, "addr = %p, length = %#lx, page_offset = %#lx\n",
+ (void *) addr, length, page_offset);
+}
+
+static int btt_create_mappings(struct btt_chk *bttc)
+{
+ struct arena_info *a;
+ int i;
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);
- if (a->map.info == MAP_FAILED) {
+ a->map.info = btt_mmap(bttc, a->map.info_len, a->infooff);
+ 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));
return -errno;
}
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);
- if (a->map.data == MAP_FAILED) {
+ a->map.data = btt_mmap(bttc, a->map.data_len, a->dataoff);
+ if (!a->map.data) {
err(bttc, "mmap arena[%d].data [sz = %#lx, off = %#lx] failed: %s\n",
i, a->map.data_len, a->dataoff, strerror(errno));
return -errno;
}
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);
- if (a->map.map == MAP_FAILED) {
+ a->map.map = btt_mmap(bttc, a->map.map_len, a->mapoff);
+ if (!a->map.map) {
err(bttc, "mmap arena[%d].map [sz = %#lx, off = %#lx] failed: %s\n",
i, a->map.map_len, a->mapoff, strerror(errno));
return -errno;
}
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);
- if (a->map.log == MAP_FAILED) {
+ a->map.log = btt_mmap(bttc, a->map.log_len, a->logoff);
+ if (!a->map.log) {
err(bttc, "mmap arena[%d].log [sz = %#lx, off = %#lx] failed: %s\n",
i, a->map.log_len, a->logoff, strerror(errno));
return -errno;
}
a->map.info2_len = BTT_INFO_SIZE;
- a->map.info2 = mmap(NULL, a->map.info2_len, mmap_flags,
- MAP_SHARED, bttc->fd, a->info2off);
- if (a->map.info2 == MAP_FAILED) {
+ a->map.info2 = btt_mmap(bttc, a->map.info2_len, a->info2off);
+ if (!a->map.info2) {
err(bttc, "mmap arena[%d].info2 [sz = %#lx, off = %#lx] failed: %s\n",
i, a->map.info2_len, a->info2off, strerror(errno));
return -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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] ndctl, check: Ensure mmap of BTT sections work with 64K page-sizes
2019-08-06 10:50 [PATCH v3] ndctl, check: Ensure mmap of BTT sections work with 64K page-sizes Vaibhav Jain
@ 2019-08-12 21:46 ` Jeff Moyer
2019-08-13 6:29 ` Vaibhav Jain
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Moyer @ 2019-08-12 21:46 UTC (permalink / raw)
To: Vaibhav Jain; +Cc: Harish Sriram, Aneesh Kumar K.V, linux-nvdimm
Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> On PPC64 which uses a 64K page-size, ndtl-check command fails on a BTT
> namespace 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
>
> An 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 mmapped region.
>
> With these newly introduced functions the patch updates the call-sites
> in 'check.c' to use these functions instead of mmap/unmap syscalls.
> Also since btt_mmap returns NULL if mmap operation fails, the
> error check call-sites can be made against NULL instead of MAP_FAILED.
>
> Reported-by: Harish Sriram <harish@linux.ibm.com>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Changelog:
> v3:
> * Fixed a log string that was splitted across multiple lines [Vishal]
>
> v2:
> * Updated patch description to include canonical email address of bug
> reporter. [Vishal]
> * Improved the comment describing function btt_mmap() in 'check.c'
> [Vishal]
> * Simplified the argument list for btt_mmap() to just accept bttc,
> length and offset. Other arguments for mmap() are derived from these
> args. [Vishal]
> * Renamed 'shift' variable in btt_mmap()/unmap() to 'page_offset'
> [Vishal]
> * Improved the dbg() messages logged inside
> btt_mmap()/unmap(). [Vishal]
> * Return NULL from btt_mmap() in case of an error. [Vishal]
> * Changed the all sites to btt_mmap() to perform error check against
> NULL return value instead of MAP_FAILED. [Vishal]
> ---
> ndctl/check.c | 93 +++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 67 insertions(+), 26 deletions(-)
>
> diff --git a/ndctl/check.c b/ndctl/check.c
> index 8a7125053865..5969012eba84 100644
> --- a/ndctl/check.c
> +++ b/ndctl/check.c
> @@ -907,59 +907,100 @@ static int btt_discover_arenas(struct btt_chk *bttc)
> return ret;
> }
>
> -static int btt_create_mappings(struct btt_chk *bttc)
> +/*
> + * Wrap call to mmap(2) to work with btt device offsets that are not aligned
> + * to system page boundary. It works by rounding down the requested offset
> + * to sys_page_size when calling mmap(2) and then returning a fixed-up pointer
> + * to the correct offset in the mmaped region.
> + */
> +static void *btt_mmap(struct btt_chk *bttc, size_t length, off_t offset)
> {
> - struct arena_info *a;
> - int mmap_flags;
> - int i;
> + off_t page_offset;
> + int prot_flags;
> + uint8_t *addr;
>
> if (!bttc->opts->repair)
> - mmap_flags = PROT_READ;
> + prot_flags = PROT_READ;
> else
> - mmap_flags = PROT_READ|PROT_WRITE;
> + prot_flags = PROT_READ|PROT_WRITE;
> +
> + /* Calculate the page_offset from the system page boundary */
> + page_offset = offset - rounddown(offset, bttc->sys_page_size);
> +
> + /* Update the offset and length with the page_offset calculated above */
> + offset -= page_offset;
> + length += page_offset;
Don't you have to ensure that the length is also a multiple of the
system page size?
-Jeff
> +
> + addr = mmap(NULL, length, prot_flags, MAP_SHARED, bttc->fd, offset);
> +
> + /* If needed fixup the return pointer to correct offset requested */
> + if (addr != MAP_FAILED)
> + addr += page_offset;
> +
> + dbg(bttc, "addr = %p, length = %#lx, offset = %#lx, page_offset = %#lx\n",
> + (void *) addr, length, offset, page_offset);
> +
> + return addr == MAP_FAILED ? NULL : addr;
> +}
> +
> +static void btt_unmap(struct btt_chk *bttc, void *ptr, size_t length)
> +{
> + off_t page_offset;
> + uintptr_t addr = (uintptr_t) ptr;
> +
> + /* Calculate the page_offset from system page boundary */
> + page_offset = addr - rounddown(addr, bttc->sys_page_size);
> +
> + addr -= page_offset;
> + length += page_offset;
> +
> + munmap((void *) addr, length);
> + dbg(bttc, "addr = %p, length = %#lx, page_offset = %#lx\n",
> + (void *) addr, length, page_offset);
> +}
> +
> +static int btt_create_mappings(struct btt_chk *bttc)
> +{
> + struct arena_info *a;
> + int i;
>
> 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);
> - if (a->map.info == MAP_FAILED) {
> + a->map.info = btt_mmap(bttc, a->map.info_len, a->infooff);
> + 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));
> return -errno;
> }
>
> 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);
> - if (a->map.data == MAP_FAILED) {
> + a->map.data = btt_mmap(bttc, a->map.data_len, a->dataoff);
> + if (!a->map.data) {
> err(bttc, "mmap arena[%d].data [sz = %#lx, off = %#lx] failed: %s\n",
> i, a->map.data_len, a->dataoff, strerror(errno));
> return -errno;
> }
>
> 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);
> - if (a->map.map == MAP_FAILED) {
> + a->map.map = btt_mmap(bttc, a->map.map_len, a->mapoff);
> + if (!a->map.map) {
> err(bttc, "mmap arena[%d].map [sz = %#lx, off = %#lx] failed: %s\n",
> i, a->map.map_len, a->mapoff, strerror(errno));
> return -errno;
> }
>
> 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);
> - if (a->map.log == MAP_FAILED) {
> + a->map.log = btt_mmap(bttc, a->map.log_len, a->logoff);
> + if (!a->map.log) {
> err(bttc, "mmap arena[%d].log [sz = %#lx, off = %#lx] failed: %s\n",
> i, a->map.log_len, a->logoff, strerror(errno));
> return -errno;
> }
>
> a->map.info2_len = BTT_INFO_SIZE;
> - a->map.info2 = mmap(NULL, a->map.info2_len, mmap_flags,
> - MAP_SHARED, bttc->fd, a->info2off);
> - if (a->map.info2 == MAP_FAILED) {
> + a->map.info2 = btt_mmap(bttc, a->map.info2_len, a->info2off);
> + if (!a->map.info2) {
> err(bttc, "mmap arena[%d].info2 [sz = %#lx, off = %#lx] failed: %s\n",
> i, a->map.info2_len, a->info2off, strerror(errno));
> return -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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] ndctl, check: Ensure mmap of BTT sections work with 64K page-sizes
2019-08-12 21:46 ` Jeff Moyer
@ 2019-08-13 6:29 ` Vaibhav Jain
2019-08-13 20:52 ` Jeff Moyer
0 siblings, 1 reply; 4+ messages in thread
From: Vaibhav Jain @ 2019-08-13 6:29 UTC (permalink / raw)
To: Jeff Moyer; +Cc: Harish Sriram, Aneesh Kumar K.V, linux-nvdimm
Thanks for reviewing this patch Jeff.
Jeff Moyer <jmoyer@redhat.com> writes:
> Vaibhav Jain <vaibhav@linux.ibm.com> writes:
>
>> On PPC64 which uses a 64K page-size, ndtl-check command fails on a BTT
>> namespace 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
>>
>> An 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 mmapped region.
>>
>> With these newly introduced functions the patch updates the call-sites
>> in 'check.c' to use these functions instead of mmap/unmap syscalls.
>> Also since btt_mmap returns NULL if mmap operation fails, the
>> error check call-sites can be made against NULL instead of MAP_FAILED.
>>
>> Reported-by: Harish Sriram <harish@linux.ibm.com>
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>> Changelog:
>> v3:
>> * Fixed a log string that was splitted across multiple lines [Vishal]
>>
>> v2:
>> * Updated patch description to include canonical email address of bug
>> reporter. [Vishal]
>> * Improved the comment describing function btt_mmap() in 'check.c'
>> [Vishal]
>> * Simplified the argument list for btt_mmap() to just accept bttc,
>> length and offset. Other arguments for mmap() are derived from these
>> args. [Vishal]
>> * Renamed 'shift' variable in btt_mmap()/unmap() to 'page_offset'
>> [Vishal]
>> * Improved the dbg() messages logged inside
>> btt_mmap()/unmap(). [Vishal]
>> * Return NULL from btt_mmap() in case of an error. [Vishal]
>> * Changed the all sites to btt_mmap() to perform error check against
>> NULL return value instead of MAP_FAILED. [Vishal]
>> ---
>> ndctl/check.c | 93 +++++++++++++++++++++++++++++++++++++--------------
>> 1 file changed, 67 insertions(+), 26 deletions(-)
>>
>> diff --git a/ndctl/check.c b/ndctl/check.c
>> index 8a7125053865..5969012eba84 100644
>> --- a/ndctl/check.c
>> +++ b/ndctl/check.c
>> @@ -907,59 +907,100 @@ static int btt_discover_arenas(struct btt_chk *bttc)
>> return ret;
>> }
>>
>> -static int btt_create_mappings(struct btt_chk *bttc)
>> +/*
>> + * Wrap call to mmap(2) to work with btt device offsets that are not aligned
>> + * to system page boundary. It works by rounding down the requested offset
>> + * to sys_page_size when calling mmap(2) and then returning a fixed-up pointer
>> + * to the correct offset in the mmaped region.
>> + */
>> +static void *btt_mmap(struct btt_chk *bttc, size_t length, off_t offset)
>> {
>> - struct arena_info *a;
>> - int mmap_flags;
>> - int i;
>> + off_t page_offset;
>> + int prot_flags;
>> + uint8_t *addr;
>>
>> if (!bttc->opts->repair)
>> - mmap_flags = PROT_READ;
>> + prot_flags = PROT_READ;
>> else
>> - mmap_flags = PROT_READ|PROT_WRITE;
>> + prot_flags = PROT_READ|PROT_WRITE;
>> +
>> + /* Calculate the page_offset from the system page boundary */
>> + page_offset = offset - rounddown(offset, bttc->sys_page_size);
>> +
>> + /* Update the offset and length with the page_offset calculated above */
>> + offset -= page_offset;
>> + length += page_offset;
>
> Don't you have to ensure that the length is also a multiple of the
> system page size?
>
> -Jeff
No, as the BTT info header is 4K in size which isnt in multiple of page
size on PPC64 where PAGE_SIZE == 64K.
Also I see 'do_mmap()' in kernel always rounding up the 'length' to
PAGE_SIZE. So any unaligned value for 'length' arg will be handled by the
kernel.
Finally mmap(2) doesn't put any constraint on the 'length' argument to
mmap except it should > 0. The 'offset' arg on other hand has a
constraint which needs to be in multiple of PAGE_SIZE.
>
>> +
>> + addr = mmap(NULL, length, prot_flags, MAP_SHARED, bttc->fd, offset);
>> +
>> + /* If needed fixup the return pointer to correct offset requested */
>> + if (addr != MAP_FAILED)
>> + addr += page_offset;
>> +
>> + dbg(bttc, "addr = %p, length = %#lx, offset = %#lx, page_offset = %#lx\n",
>> + (void *) addr, length, offset, page_offset);
>> +
>> + return addr == MAP_FAILED ? NULL : addr;
>> +}
>> +
>> +static void btt_unmap(struct btt_chk *bttc, void *ptr, size_t length)
>> +{
>> + off_t page_offset;
>> + uintptr_t addr = (uintptr_t) ptr;
>> +
>> + /* Calculate the page_offset from system page boundary */
>> + page_offset = addr - rounddown(addr, bttc->sys_page_size);
>> +
>> + addr -= page_offset;
>> + length += page_offset;
>> +
>> + munmap((void *) addr, length);
>> + dbg(bttc, "addr = %p, length = %#lx, page_offset = %#lx\n",
>> + (void *) addr, length, page_offset);
>> +}
>> +
>> +static int btt_create_mappings(struct btt_chk *bttc)
>> +{
>> + struct arena_info *a;
>> + int i;
>>
>> 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);
>> - if (a->map.info == MAP_FAILED) {
>> + a->map.info = btt_mmap(bttc, a->map.info_len, a->infooff);
>> + 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));
>> return -errno;
>> }
>>
>> 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);
>> - if (a->map.data == MAP_FAILED) {
>> + a->map.data = btt_mmap(bttc, a->map.data_len, a->dataoff);
>> + if (!a->map.data) {
>> err(bttc, "mmap arena[%d].data [sz = %#lx, off = %#lx] failed: %s\n",
>> i, a->map.data_len, a->dataoff, strerror(errno));
>> return -errno;
>> }
>>
>> 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);
>> - if (a->map.map == MAP_FAILED) {
>> + a->map.map = btt_mmap(bttc, a->map.map_len, a->mapoff);
>> + if (!a->map.map) {
>> err(bttc, "mmap arena[%d].map [sz = %#lx, off = %#lx] failed: %s\n",
>> i, a->map.map_len, a->mapoff, strerror(errno));
>> return -errno;
>> }
>>
>> 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);
>> - if (a->map.log == MAP_FAILED) {
>> + a->map.log = btt_mmap(bttc, a->map.log_len, a->logoff);
>> + if (!a->map.log) {
>> err(bttc, "mmap arena[%d].log [sz = %#lx, off = %#lx] failed: %s\n",
>> i, a->map.log_len, a->logoff, strerror(errno));
>> return -errno;
>> }
>>
>> a->map.info2_len = BTT_INFO_SIZE;
>> - a->map.info2 = mmap(NULL, a->map.info2_len, mmap_flags,
>> - MAP_SHARED, bttc->fd, a->info2off);
>> - if (a->map.info2 == MAP_FAILED) {
>> + a->map.info2 = btt_mmap(bttc, a->map.info2_len, a->info2off);
>> + if (!a->map.info2) {
>> err(bttc, "mmap arena[%d].info2 [sz = %#lx, off = %#lx] failed: %s\n",
>> i, a->map.info2_len, a->info2off, strerror(errno));
>> return -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);
>> }
>> }
>
--
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] ndctl, check: Ensure mmap of BTT sections work with 64K page-sizes
2019-08-13 6:29 ` Vaibhav Jain
@ 2019-08-13 20:52 ` Jeff Moyer
0 siblings, 0 replies; 4+ messages in thread
From: Jeff Moyer @ 2019-08-13 20:52 UTC (permalink / raw)
To: Vaibhav Jain; +Cc: Harish Sriram, Aneesh Kumar K.V, linux-nvdimm
Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> Thanks for reviewing this patch Jeff.
>
> Jeff Moyer <jmoyer@redhat.com> writes:
>
>> Vaibhav Jain <vaibhav@linux.ibm.com> writes:
>>
>>> On PPC64 which uses a 64K page-size, ndtl-check command fails on a BTT
>>> namespace 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
>>>
>>> An 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 mmapped region.
>>>
>>> With these newly introduced functions the patch updates the call-sites
>>> in 'check.c' to use these functions instead of mmap/unmap syscalls.
>>> Also since btt_mmap returns NULL if mmap operation fails, the
>>> error check call-sites can be made against NULL instead of MAP_FAILED.
>>>
>>> Reported-by: Harish Sriram <harish@linux.ibm.com>
>>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>>> ---
>>> Changelog:
>>> v3:
>>> * Fixed a log string that was splitted across multiple lines [Vishal]
>>>
>>> v2:
>>> * Updated patch description to include canonical email address of bug
>>> reporter. [Vishal]
>>> * Improved the comment describing function btt_mmap() in 'check.c'
>>> [Vishal]
>>> * Simplified the argument list for btt_mmap() to just accept bttc,
>>> length and offset. Other arguments for mmap() are derived from these
>>> args. [Vishal]
>>> * Renamed 'shift' variable in btt_mmap()/unmap() to 'page_offset'
>>> [Vishal]
>>> * Improved the dbg() messages logged inside
>>> btt_mmap()/unmap(). [Vishal]
>>> * Return NULL from btt_mmap() in case of an error. [Vishal]
>>> * Changed the all sites to btt_mmap() to perform error check against
>>> NULL return value instead of MAP_FAILED. [Vishal]
>>> ---
>>> ndctl/check.c | 93 +++++++++++++++++++++++++++++++++++++--------------
>>> 1 file changed, 67 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/ndctl/check.c b/ndctl/check.c
>>> index 8a7125053865..5969012eba84 100644
>>> --- a/ndctl/check.c
>>> +++ b/ndctl/check.c
>>> @@ -907,59 +907,100 @@ static int btt_discover_arenas(struct btt_chk *bttc)
>>> return ret;
>>> }
>>>
>>> -static int btt_create_mappings(struct btt_chk *bttc)
>>> +/*
>>> + * Wrap call to mmap(2) to work with btt device offsets that are not aligned
>>> + * to system page boundary. It works by rounding down the requested offset
>>> + * to sys_page_size when calling mmap(2) and then returning a fixed-up pointer
>>> + * to the correct offset in the mmaped region.
>>> + */
>>> +static void *btt_mmap(struct btt_chk *bttc, size_t length, off_t offset)
>>> {
>>> - struct arena_info *a;
>>> - int mmap_flags;
>>> - int i;
>>> + off_t page_offset;
>>> + int prot_flags;
>>> + uint8_t *addr;
>>>
>>> if (!bttc->opts->repair)
>>> - mmap_flags = PROT_READ;
>>> + prot_flags = PROT_READ;
>>> else
>>> - mmap_flags = PROT_READ|PROT_WRITE;
>>> + prot_flags = PROT_READ|PROT_WRITE;
>>> +
>>> + /* Calculate the page_offset from the system page boundary */
>>> + page_offset = offset - rounddown(offset, bttc->sys_page_size);
>>> +
>>> + /* Update the offset and length with the page_offset calculated above */
>>> + offset -= page_offset;
>>> + length += page_offset;
>>
>> Don't you have to ensure that the length is also a multiple of the
>> system page size?
>>
>> -Jeff
>
> No, as the BTT info header is 4K in size which isnt in multiple of page
> size on PPC64 where PAGE_SIZE == 64K.
>
> Also I see 'do_mmap()' in kernel always rounding up the 'length' to
> PAGE_SIZE. So any unaligned value for 'length' arg will be handled by the
> kernel.
>
> Finally mmap(2) doesn't put any constraint on the 'length' argument to
> mmap except it should > 0. The 'offset' arg on other hand has a
> constraint which needs to be in multiple of PAGE_SIZE.
Right, this is the part I forgot. I'd probaby write the map and unmap
routines a bit differently, but what you wrote works.
Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
>>
>>> +
>>> + addr = mmap(NULL, length, prot_flags, MAP_SHARED, bttc->fd, offset);
>>> +
>>> + /* If needed fixup the return pointer to correct offset requested */
>>> + if (addr != MAP_FAILED)
>>> + addr += page_offset;
>>> +
>>> + dbg(bttc, "addr = %p, length = %#lx, offset = %#lx, page_offset = %#lx\n",
>>> + (void *) addr, length, offset, page_offset);
>>> +
>>> + return addr == MAP_FAILED ? NULL : addr;
>>> +}
>>> +
>>> +static void btt_unmap(struct btt_chk *bttc, void *ptr, size_t length)
>>> +{
>>> + off_t page_offset;
>>> + uintptr_t addr = (uintptr_t) ptr;
>>> +
>>> + /* Calculate the page_offset from system page boundary */
>>> + page_offset = addr - rounddown(addr, bttc->sys_page_size);
>>> +
>>> + addr -= page_offset;
>>> + length += page_offset;
>>> +
>>> + munmap((void *) addr, length);
>>> + dbg(bttc, "addr = %p, length = %#lx, page_offset = %#lx\n",
>>> + (void *) addr, length, page_offset);
>>> +}
>>> +
>>> +static int btt_create_mappings(struct btt_chk *bttc)
>>> +{
>>> + struct arena_info *a;
>>> + int i;
>>>
>>> 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);
>>> - if (a->map.info == MAP_FAILED) {
>>> + a->map.info = btt_mmap(bttc, a->map.info_len, a->infooff);
>>> + 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));
>>> return -errno;
>>> }
>>>
>>> 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);
>>> - if (a->map.data == MAP_FAILED) {
>>> + a->map.data = btt_mmap(bttc, a->map.data_len, a->dataoff);
>>> + if (!a->map.data) {
>>> err(bttc, "mmap arena[%d].data [sz = %#lx, off = %#lx] failed: %s\n",
>>> i, a->map.data_len, a->dataoff, strerror(errno));
>>> return -errno;
>>> }
>>>
>>> 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);
>>> - if (a->map.map == MAP_FAILED) {
>>> + a->map.map = btt_mmap(bttc, a->map.map_len, a->mapoff);
>>> + if (!a->map.map) {
>>> err(bttc, "mmap arena[%d].map [sz = %#lx, off = %#lx] failed: %s\n",
>>> i, a->map.map_len, a->mapoff, strerror(errno));
>>> return -errno;
>>> }
>>>
>>> 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);
>>> - if (a->map.log == MAP_FAILED) {
>>> + a->map.log = btt_mmap(bttc, a->map.log_len, a->logoff);
>>> + if (!a->map.log) {
>>> err(bttc, "mmap arena[%d].log [sz = %#lx, off = %#lx] failed: %s\n",
>>> i, a->map.log_len, a->logoff, strerror(errno));
>>> return -errno;
>>> }
>>>
>>> a->map.info2_len = BTT_INFO_SIZE;
>>> - a->map.info2 = mmap(NULL, a->map.info2_len, mmap_flags,
>>> - MAP_SHARED, bttc->fd, a->info2off);
>>> - if (a->map.info2 == MAP_FAILED) {
>>> + a->map.info2 = btt_mmap(bttc, a->map.info2_len, a->info2off);
>>> + if (!a->map.info2) {
>>> err(bttc, "mmap arena[%d].info2 [sz = %#lx, off = %#lx] failed: %s\n",
>>> i, a->map.info2_len, a->info2off, strerror(errno));
>>> return -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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-08-13 20:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-06 10:50 [PATCH v3] ndctl, check: Ensure mmap of BTT sections work with 64K page-sizes Vaibhav Jain
2019-08-12 21:46 ` Jeff Moyer
2019-08-13 6:29 ` Vaibhav Jain
2019-08-13 20:52 ` Jeff Moyer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).