Thanks for reviewing this patch Vishal. I have prepped a v2 adressing all your review comments with one exception below:
"Verma, Vishal L" <[email protected]> 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: [email protected] > > Could you make this the canonical Name <email> format? > >> Signed-off-by: Vaibhav Jain <[email protected]> >> --- >> 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 <[email protected]> Linux Technology Center, IBM India Pvt. Ltd. _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
