| Message ID | 1446455617-129562-9-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02.11.2015 12:13, Xiao Guangrong wrote: > Currently file_ram_alloc() is designed for hugetlbfs, however, the memory > of nvdimm can come from either raw pmem device eg, /dev/pmem, or the file > locates at DAX enabled filesystem > > So this patch let it work on any kind of path No, this patch doesn't change any logic, but only fix variable name and some error messages. > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- > exec.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/exec.c b/exec.c > index 9de38be..9075f4d 100644 > --- a/exec.c > +++ b/exec.c > @@ -1184,25 +1184,25 @@ static void *file_ram_alloc(RAMBlock *block, > char *c; > void *area; > int fd; > - uint64_t hpagesize; > + uint64_t pagesize; > Error *local_err = NULL; > > - hpagesize = qemu_file_get_page_size(path, &local_err); > + pagesize = qemu_file_get_page_size(path, &local_err); > if (local_err) { > error_propagate(errp, local_err); > goto error; > } > > - if (hpagesize == getpagesize()) { > - fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); > + if (pagesize == getpagesize()) { > + fprintf(stderr, "Memory is not allocated from HugeTlbfs.\n"); Why do you remove path from error message? It is good additional information.. What if we have several memory file backends? > } > > - block->mr->align = hpagesize; > + block->mr->align = pagesize; > > - if (memory < hpagesize) { > + if (memory < pagesize) { > error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to " > - "or larger than huge page size 0x%" PRIx64, > - memory, hpagesize); > + "or larger than page size 0x%" PRIx64, > + memory, pagesize); > goto error; > } > > @@ -1226,14 +1226,14 @@ static void *file_ram_alloc(RAMBlock *block, > fd = mkstemp(filename); > if (fd < 0) { > error_setg_errno(errp, errno, > - "unable to create backing store for hugepages"); > + "unable to create backing store for path %s", path); > g_free(filename); > goto error; > } > unlink(filename); > g_free(filename); > > - memory = ROUND_UP(memory, hpagesize); > + memory = ROUND_UP(memory, pagesize); > > /* > * ftruncate is not supported by hugetlbfs in older > @@ -1245,10 +1245,10 @@ static void *file_ram_alloc(RAMBlock *block, > perror("ftruncate"); > } > > - area = qemu_ram_mmap(fd, memory, hpagesize, block->flags & RAM_SHARED); > + area = qemu_ram_mmap(fd, memory, pagesize, block->flags & RAM_SHARED); > if (area == MAP_FAILED) { > error_setg_errno(errp, errno, > - "unable to map backing store for hugepages"); > + "unable to map backing store for path %s", path); > close(fd); > goto error; > } >
On 11/02/2015 10:51 PM, Vladimir Sementsov-Ogievskiy wrote: > On 02.11.2015 12:13, Xiao Guangrong wrote: >> Currently file_ram_alloc() is designed for hugetlbfs, however, the memory >> of nvdimm can come from either raw pmem device eg, /dev/pmem, or the file >> locates at DAX enabled filesystem >> >> So this patch let it work on any kind of path > > No, this patch doesn't change any logic, but only fix variable name and some error messages. Yes, it is. 'let it work' in my thought exactly was "fix variable name and some error messages"... okay, if it confused you, how about change it to: "This patch fixes variable name and some error messages to let it be aware of normal path" > >> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >> --- >> exec.c | 24 ++++++++++++------------ >> 1 file changed, 12 insertions(+), 12 deletions(-) >> >> diff --git a/exec.c b/exec.c >> index 9de38be..9075f4d 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -1184,25 +1184,25 @@ static void *file_ram_alloc(RAMBlock *block, >> char *c; >> void *area; >> int fd; >> - uint64_t hpagesize; >> + uint64_t pagesize; >> Error *local_err = NULL; >> - hpagesize = qemu_file_get_page_size(path, &local_err); >> + pagesize = qemu_file_get_page_size(path, &local_err); >> if (local_err) { >> error_propagate(errp, local_err); >> goto error; >> } >> - if (hpagesize == getpagesize()) { >> - fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); >> + if (pagesize == getpagesize()) { >> + fprintf(stderr, "Memory is not allocated from HugeTlbfs.\n"); > > Why do you remove path from error message? It is good additional information.. What if we have > several memory file backends? Good catch, will change it to: fprintf(stderr, "Memory is not allocated from HugeTlbfs on path %s.\n", path); BTW, if no other big change in the further, i will post the new version just for of this patch, > >> } >> - block->mr->align = hpagesize; >> + block->mr->align = pagesize; >> - if (memory < hpagesize) { >> + if (memory < pagesize) { >> error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to " >> - "or larger than huge page size 0x%" PRIx64, >> - memory, hpagesize); >> + "or larger than page size 0x%" PRIx64, >> + memory, pagesize); >> goto error; >> } >> @@ -1226,14 +1226,14 @@ static void *file_ram_alloc(RAMBlock *block, >> fd = mkstemp(filename); >> if (fd < 0) { >> error_setg_errno(errp, errno, >> - "unable to create backing store for hugepages"); >> + "unable to create backing store for path %s", path); >> g_free(filename); >> goto error; >> } >> unlink(filename); >> g_free(filename); >> - memory = ROUND_UP(memory, hpagesize); >> + memory = ROUND_UP(memory, pagesize); >> /* >> * ftruncate is not supported by hugetlbfs in older >> @@ -1245,10 +1245,10 @@ static void *file_ram_alloc(RAMBlock *block, >> perror("ftruncate"); >> } >> - area = qemu_ram_mmap(fd, memory, hpagesize, block->flags & RAM_SHARED); >> + area = qemu_ram_mmap(fd, memory, pagesize, block->flags & RAM_SHARED); >> if (area == MAP_FAILED) { >> error_setg_errno(errp, errno, >> - "unable to map backing store for hugepages"); >> + "unable to map backing store for path %s", path); >> close(fd); >> goto error; >> } >> > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02.11.2015 18:22, Xiao Guangrong wrote: > > > On 11/02/2015 10:51 PM, Vladimir Sementsov-Ogievskiy wrote: >> On 02.11.2015 12:13, Xiao Guangrong wrote: >>> Currently file_ram_alloc() is designed for hugetlbfs, however, the >>> memory >>> of nvdimm can come from either raw pmem device eg, /dev/pmem, or the >>> file >>> locates at DAX enabled filesystem >>> >>> So this patch let it work on any kind of path >> >> No, this patch doesn't change any logic, but only fix variable name >> and some error messages. > > Yes, it is. > > 'let it work' in my thought exactly was "fix variable name and some > error messages"... okay, > if it confused you, how about change it to: > > "This patch fixes variable name and some error messages to let it be > aware of normal > path" My english is not very good, I don't know figures of speech. For me "patch let it work" means that without this patch it will not work)) Your new variant is ok for me, or better (imo) "This patch fixes variable name and some error messages to be suitable for any kind of path" > >> >>> >>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >>> --- >>> exec.c | 24 ++++++++++++------------ >>> 1 file changed, 12 insertions(+), 12 deletions(-) >>> >>> diff --git a/exec.c b/exec.c >>> index 9de38be..9075f4d 100644 >>> --- a/exec.c >>> +++ b/exec.c >>> @@ -1184,25 +1184,25 @@ static void *file_ram_alloc(RAMBlock *block, >>> char *c; >>> void *area; >>> int fd; >>> - uint64_t hpagesize; >>> + uint64_t pagesize; >>> Error *local_err = NULL; >>> - hpagesize = qemu_file_get_page_size(path, &local_err); >>> + pagesize = qemu_file_get_page_size(path, &local_err); >>> if (local_err) { >>> error_propagate(errp, local_err); >>> goto error; >>> } >>> - if (hpagesize == getpagesize()) { >>> - fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); >>> + if (pagesize == getpagesize()) { >>> + fprintf(stderr, "Memory is not allocated from HugeTlbfs.\n"); >> >> Why do you remove path from error message? It is good additional >> information.. What if we have >> several memory file backends? > > Good catch, will change it to: > fprintf(stderr, "Memory is not allocated from HugeTlbfs on path > %s.\n", path); > > BTW, if no other big change in the further, i will post the new > version just for of this patch, > >> >>> } >>> - block->mr->align = hpagesize; >>> + block->mr->align = pagesize; >>> - if (memory < hpagesize) { >>> + if (memory < pagesize) { >>> error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be >>> equal to " >>> - "or larger than huge page size 0x%" PRIx64, >>> - memory, hpagesize); >>> + "or larger than page size 0x%" PRIx64, >>> + memory, pagesize); >>> goto error; >>> } >>> @@ -1226,14 +1226,14 @@ static void *file_ram_alloc(RAMBlock *block, >>> fd = mkstemp(filename); >>> if (fd < 0) { >>> error_setg_errno(errp, errno, >>> - "unable to create backing store for >>> hugepages"); >>> + "unable to create backing store for path >>> %s", path); >>> g_free(filename); >>> goto error; >>> } >>> unlink(filename); >>> g_free(filename); >>> - memory = ROUND_UP(memory, hpagesize); >>> + memory = ROUND_UP(memory, pagesize); >>> /* >>> * ftruncate is not supported by hugetlbfs in older >>> @@ -1245,10 +1245,10 @@ static void *file_ram_alloc(RAMBlock *block, >>> perror("ftruncate"); >>> } >>> - area = qemu_ram_mmap(fd, memory, hpagesize, block->flags & >>> RAM_SHARED); >>> + area = qemu_ram_mmap(fd, memory, pagesize, block->flags & >>> RAM_SHARED); >>> if (area == MAP_FAILED) { >>> error_setg_errno(errp, errno, >>> - "unable to map backing store for hugepages"); >>> + "unable to map backing store for path %s", >>> path); >>> close(fd); >>> goto error; >>> } >>> >> With these two fixes (any commit message variant): Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On Mon, Nov 02, 2015 at 05:13:10PM +0800, Xiao Guangrong wrote: > Currently file_ram_alloc() is designed for hugetlbfs, however, the memory > of nvdimm can come from either raw pmem device eg, /dev/pmem, or the file > locates at DAX enabled filesystem > > So this patch let it work on any kind of path > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- > exec.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/exec.c b/exec.c > index 9de38be..9075f4d 100644 > --- a/exec.c > +++ b/exec.c > @@ -1184,25 +1184,25 @@ static void *file_ram_alloc(RAMBlock *block, > char *c; > void *area; > int fd; > - uint64_t hpagesize; > + uint64_t pagesize; > Error *local_err = NULL; > > - hpagesize = qemu_file_get_page_size(path, &local_err); > + pagesize = qemu_file_get_page_size(path, &local_err); > if (local_err) { > error_propagate(errp, local_err); > goto error; > } > > - if (hpagesize == getpagesize()) { > - fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); > + if (pagesize == getpagesize()) { > + fprintf(stderr, "Memory is not allocated from HugeTlbfs.\n"); If the point of this patch is to allow file_ram_alloc() to not be specific to hugetlbfs anymore, this warning can simply go away. (And in case if you really want to keep the warning, I don't see the point of the changes you made to it.)
On 11/04/2015 07:00 AM, Eduardo Habkost wrote: > On Mon, Nov 02, 2015 at 05:13:10PM +0800, Xiao Guangrong wrote: >> Currently file_ram_alloc() is designed for hugetlbfs, however, the memory >> of nvdimm can come from either raw pmem device eg, /dev/pmem, or the file >> locates at DAX enabled filesystem >> >> So this patch let it work on any kind of path >> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >> --- >> exec.c | 24 ++++++++++++------------ >> 1 file changed, 12 insertions(+), 12 deletions(-) >> >> diff --git a/exec.c b/exec.c >> index 9de38be..9075f4d 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -1184,25 +1184,25 @@ static void *file_ram_alloc(RAMBlock *block, >> char *c; >> void *area; >> int fd; >> - uint64_t hpagesize; >> + uint64_t pagesize; >> Error *local_err = NULL; >> >> - hpagesize = qemu_file_get_page_size(path, &local_err); >> + pagesize = qemu_file_get_page_size(path, &local_err); >> if (local_err) { >> error_propagate(errp, local_err); >> goto error; >> } >> >> - if (hpagesize == getpagesize()) { >> - fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); >> + if (pagesize == getpagesize()) { >> + fprintf(stderr, "Memory is not allocated from HugeTlbfs.\n"); > > If the point of this patch is to allow file_ram_alloc() to not be > specific to hugetlbfs anymore, this warning can simply go away. > > (And in case if you really want to keep the warning, I don't see the > point of the changes you made to it.) > This is the history why we did it like this: https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02862.html Q: | What this *actually* is trying to warn against is that | mapping a regular file (as opposed to hugetlbfs) | means transparent huge pages don't work. | So I don't think we should drop this warning completely. | Either let's add the nvdimm magic, or simply check the | page size. A: | Check the page size sounds good, will check: | if (pagesize != getpagesize()) { | ...print something... |} | I agree with you that showing the info is needed, however, | 'Warning' might scare some users, how about drop this word or | just show?“Memory is not allocated from HugeTlbfs”? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 04, 2015 at 11:12:41AM +0800, Xiao Guangrong wrote: > On 11/04/2015 07:00 AM, Eduardo Habkost wrote: > >On Mon, Nov 02, 2015 at 05:13:10PM +0800, Xiao Guangrong wrote: > >>Currently file_ram_alloc() is designed for hugetlbfs, however, the memory > >>of nvdimm can come from either raw pmem device eg, /dev/pmem, or the file > >>locates at DAX enabled filesystem > >> > >>So this patch let it work on any kind of path > >> > >>Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > >>--- > >> exec.c | 24 ++++++++++++------------ > >> 1 file changed, 12 insertions(+), 12 deletions(-) > >> > >>diff --git a/exec.c b/exec.c > >>index 9de38be..9075f4d 100644 > >>--- a/exec.c > >>+++ b/exec.c > >>@@ -1184,25 +1184,25 @@ static void *file_ram_alloc(RAMBlock *block, > >> char *c; > >> void *area; > >> int fd; > >>- uint64_t hpagesize; > >>+ uint64_t pagesize; > >> Error *local_err = NULL; > >> > >>- hpagesize = qemu_file_get_page_size(path, &local_err); > >>+ pagesize = qemu_file_get_page_size(path, &local_err); > >> if (local_err) { > >> error_propagate(errp, local_err); > >> goto error; > >> } > >> > >>- if (hpagesize == getpagesize()) { > >>- fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); > >>+ if (pagesize == getpagesize()) { > >>+ fprintf(stderr, "Memory is not allocated from HugeTlbfs.\n"); > > > >If the point of this patch is to allow file_ram_alloc() to not be > >specific to hugetlbfs anymore, this warning can simply go away. > > > >(And in case if you really want to keep the warning, I don't see the > >point of the changes you made to it.) > > > > This is the history why we did it like this: > https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02862.html The rule I am trying to follow is simple: if there are valid use cases (e.g. nvdimm, tmpfs) where the warning will be printed every single time QEMU runs, the warning is not appropriate. If you really want to keep a warning, please make it not be printed on all other valid use cases (nvdimm and tmpfs). Personally, I don't think adding those additional checks would be worth the trouble, that's why I suggest removing the warning. > > Q: > | What this *actually* is trying to warn against is that > | mapping a regular file (as opposed to hugetlbfs) > | means transparent huge pages don't work. I don't think the author of that warning even thought about transparent huge pages (did THP even existed when it was written?). I believe they just assumed that the only reason for using -mem-path would be hugetlbfs and wanted to warn about it. That assumption isn't true anymore. > > | So I don't think we should drop this warning completely. > | Either let's add the nvdimm magic, or simply check the > | page size. > > A: > | Check the page size sounds good, will check: > | if (pagesize != getpagesize()) { > | ...print something... > |} > > | I agree with you that showing the info is needed, however, > | 'Warning' might scare some users, how about drop this word or > | just show?“Memory is not allocated from HugeTlbfs”? With "warning:", I know it may be OK to do what I am doing and the software is just trying to warn me. If there's no "warning:", I don't even know if something is really broken in my config, or if it's just a warning, and I would be very confused.
On 11/04/2015 08:40 PM, Eduardo Habkost wrote: > On Wed, Nov 04, 2015 at 11:12:41AM +0800, Xiao Guangrong wrote: >> On 11/04/2015 07:00 AM, Eduardo Habkost wrote: >>> On Mon, Nov 02, 2015 at 05:13:10PM +0800, Xiao Guangrong wrote: >>>> Currently file_ram_alloc() is designed for hugetlbfs, however, the memory >>>> of nvdimm can come from either raw pmem device eg, /dev/pmem, or the file >>>> locates at DAX enabled filesystem >>>> >>>> So this patch let it work on any kind of path >>>> >>>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >>>> --- >>>> exec.c | 24 ++++++++++++------------ >>>> 1 file changed, 12 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/exec.c b/exec.c >>>> index 9de38be..9075f4d 100644 >>>> --- a/exec.c >>>> +++ b/exec.c >>>> @@ -1184,25 +1184,25 @@ static void *file_ram_alloc(RAMBlock *block, >>>> char *c; >>>> void *area; >>>> int fd; >>>> - uint64_t hpagesize; >>>> + uint64_t pagesize; >>>> Error *local_err = NULL; >>>> >>>> - hpagesize = qemu_file_get_page_size(path, &local_err); >>>> + pagesize = qemu_file_get_page_size(path, &local_err); >>>> if (local_err) { >>>> error_propagate(errp, local_err); >>>> goto error; >>>> } >>>> >>>> - if (hpagesize == getpagesize()) { >>>> - fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); >>>> + if (pagesize == getpagesize()) { >>>> + fprintf(stderr, "Memory is not allocated from HugeTlbfs.\n"); >>> >>> If the point of this patch is to allow file_ram_alloc() to not be >>> specific to hugetlbfs anymore, this warning can simply go away. >>> >>> (And in case if you really want to keep the warning, I don't see the >>> point of the changes you made to it.) >>> >> >> This is the history why we did it like this: >> https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02862.html > > The rule I am trying to follow is simple: if there are valid use cases > (e.g. nvdimm, tmpfs) where the warning will be printed every single time > QEMU runs, the warning is not appropriate. > > If you really want to keep a warning, please make it not be printed on > all other valid use cases (nvdimm and tmpfs). Personally, I don't think > adding those additional checks would be worth the trouble, that's why I > suggest removing the warning. > >> >> Q: >> | What this *actually* is trying to warn against is that >> | mapping a regular file (as opposed to hugetlbfs) >> | means transparent huge pages don't work. > > I don't think the author of that warning even thought about transparent > huge pages (did THP even existed when it was written?). I believe they > just assumed that the only reason for using -mem-path would be hugetlbfs > and wanted to warn about it. That assumption isn't true anymore. Michael, your idea? If Michael will not beat me, i will drop this. :) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/exec.c b/exec.c index 9de38be..9075f4d 100644 --- a/exec.c +++ b/exec.c @@ -1184,25 +1184,25 @@ static void *file_ram_alloc(RAMBlock *block, char *c; void *area; int fd; - uint64_t hpagesize; + uint64_t pagesize; Error *local_err = NULL; - hpagesize = qemu_file_get_page_size(path, &local_err); + pagesize = qemu_file_get_page_size(path, &local_err); if (local_err) { error_propagate(errp, local_err); goto error; } - if (hpagesize == getpagesize()) { - fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); + if (pagesize == getpagesize()) { + fprintf(stderr, "Memory is not allocated from HugeTlbfs.\n"); } - block->mr->align = hpagesize; + block->mr->align = pagesize; - if (memory < hpagesize) { + if (memory < pagesize) { error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to " - "or larger than huge page size 0x%" PRIx64, - memory, hpagesize); + "or larger than page size 0x%" PRIx64, + memory, pagesize); goto error; } @@ -1226,14 +1226,14 @@ static void *file_ram_alloc(RAMBlock *block, fd = mkstemp(filename); if (fd < 0) { error_setg_errno(errp, errno, - "unable to create backing store for hugepages"); + "unable to create backing store for path %s", path); g_free(filename); goto error; } unlink(filename); g_free(filename); - memory = ROUND_UP(memory, hpagesize); + memory = ROUND_UP(memory, pagesize); /* * ftruncate is not supported by hugetlbfs in older @@ -1245,10 +1245,10 @@ static void *file_ram_alloc(RAMBlock *block, perror("ftruncate"); } - area = qemu_ram_mmap(fd, memory, hpagesize, block->flags & RAM_SHARED); + area = qemu_ram_mmap(fd, memory, pagesize, block->flags & RAM_SHARED); if (area == MAP_FAILED) { error_setg_errno(errp, errno, - "unable to map backing store for hugepages"); + "unable to map backing store for path %s", path); close(fd); goto error; }
Currently file_ram_alloc() is designed for hugetlbfs, however, the memory of nvdimm can come from either raw pmem device eg, /dev/pmem, or the file locates at DAX enabled filesystem So this patch let it work on any kind of path Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> --- exec.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)