| Message ID | 20140307004004.GA6114@amt.cnet |
|---|---|
| State | New |
| Headers | show |
On 03/06/2014 05:40 PM, Marcelo Tosatti wrote: > > Failing initialization in case hugepage path has > hugepage smaller than specified. > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > diff --git a/exec.c b/exec.c > index b69fd29..c95a0f3 100644 > --- a/exec.c > +++ b/exec.c > }; > > +static QemuOptsList qemu_mempath_opts = { > + .name = "mem-path", > - case QEMU_OPTION_mempath: > - mem_path = optarg; > + case QEMU_OPTION_mempath: { > + opts = qemu_opts_parse(qemu_find_opts("mem-path"), optarg, 1); Pre-existing, but this is yet another inconsistent naming between C objects and the command line. If we were consistent, it should be named QEMU_OPTION_mem_path, and qemu_mem_path_options. (See my recent complaint about other misnamed options: https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg01131.html)
Il 07/03/2014 01:40, Marcelo Tosatti ha scritto: > > Failing initialization in case hugepage path has > hugepage smaller than specified. > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> Why is this needed? Isn't it just operator error? Perhaps libvirt could add an attribute to its <hugepages/> XML element, and could use it to find the appropriate hugetlbfs mount. But I don't think this check belongs in QEMU. Also, see the series I posted recently for a complete (and more powerful + more extensible) replacement of -mem-path and -mem-prealloc. Paolo > diff --git a/exec.c b/exec.c > index b69fd29..c95a0f3 100644 > --- a/exec.c > +++ b/exec.c > @@ -1034,6 +1034,13 @@ static void *file_ram_alloc(RAMBlock *block, > return NULL; > } > > + if (mem_path_min_hpagesize && hpagesize < mem_path_min_hpagesize) { > + fprintf(stderr, "mount point (%s) has page size " > + "(%ld) < (%ld) = min_hpagesize\n", path, hpagesize, > + mem_path_min_hpagesize); > + exit(1); > + } > + > if (memory < hpagesize) { > return NULL; > } > diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h > index 4cb4b4a..cc9e28a 100644 > --- a/include/exec/cpu-all.h > +++ b/include/exec/cpu-all.h > @@ -470,6 +470,7 @@ extern RAMList ram_list; > > extern const char *mem_path; > extern int mem_prealloc; > +extern unsigned long int mem_path_min_hpagesize; > > /* Flags stored in the low bits of the TLB virtual address. These are > defined so that fast path ram access is all zeros. */ > diff --git a/qemu-options.hx b/qemu-options.hx > index 56e5fdf..36743e1 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -221,9 +221,9 @@ gigabytes respectively. > ETEXI > > DEF("mem-path", HAS_ARG, QEMU_OPTION_mempath, > - "-mem-path FILE provide backing storage for guest RAM\n", QEMU_ARCH_ALL) > + "-mem-path [mem-path=]file[,min-hpage-size=value] provide backing storage for guest RAM\n", QEMU_ARCH_ALL) > STEXI > -@item -mem-path @var{path} > +@item -mem-path [mem-path=]@var{path}[,min-hpage-size=@var{min-hpage-size}] > @findex -mem-path > Allocate guest RAM from a temporarily created file in @var{path}. > ETEXI > diff --git a/vl.c b/vl.c > index 1d27b34..08f9bee 100644 > --- a/vl.c > +++ b/vl.c > @@ -136,6 +136,7 @@ static int display_remote; > const char* keyboard_layout = NULL; > ram_addr_t ram_size; > const char *mem_path = NULL; > +unsigned long int mem_path_min_hpagesize; > int mem_prealloc = 0; /* force preallocation of physical target memory */ > int nb_nics; > NICInfo nd_table[MAX_NICS]; > @@ -479,6 +480,22 @@ static QemuOptsList qemu_msg_opts = { > }, > }; > > +static QemuOptsList qemu_mempath_opts = { > + .name = "mem-path", > + .implied_opt_name = "mem-path", > + .head = QTAILQ_HEAD_INITIALIZER(qemu_mempath_opts.head), > + .desc = { > + { > + .name = "mem-path", > + .type = QEMU_OPT_STRING, > + },{ > + .name = "min-hpage-size", > + .type = QEMU_OPT_SIZE, > + }, > + { /* end of list */ } > + }, > +}; > + > /** > * Get machine options > * > @@ -2863,6 +2880,7 @@ int main(int argc, char **argv, char **envp) > qemu_add_opts(&qemu_tpmdev_opts); > qemu_add_opts(&qemu_realtime_opts); > qemu_add_opts(&qemu_msg_opts); > + qemu_add_opts(&qemu_mempath_opts); > > runstate_init(); > > @@ -3189,9 +3207,16 @@ int main(int argc, char **argv, char **envp) > } > break; > #endif > - case QEMU_OPTION_mempath: > - mem_path = optarg; > + case QEMU_OPTION_mempath: { > + opts = qemu_opts_parse(qemu_find_opts("mem-path"), optarg, 1); > + if (!opts) { > + exit(1); > + } > + mem_path = qemu_opt_get(opts, "mem-path"); > + mem_path_min_hpagesize = qemu_opt_get_size(opts, > + "min-hpage-size", 0); > break; > + } > case QEMU_OPTION_mem_prealloc: > mem_prealloc = 1; > break; > >
On Thu, Mar 06, 2014 at 09:21:10PM -0700, Eric Blake wrote: > On 03/06/2014 05:40 PM, Marcelo Tosatti wrote: > > > > Failing initialization in case hugepage path has > > hugepage smaller than specified. > > > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > > > diff --git a/exec.c b/exec.c > > index b69fd29..c95a0f3 100644 > > --- a/exec.c > > +++ b/exec.c > > > }; > > > > +static QemuOptsList qemu_mempath_opts = { > > + .name = "mem-path", > > > - case QEMU_OPTION_mempath: > > - mem_path = optarg; > > + case QEMU_OPTION_mempath: { > > + opts = qemu_opts_parse(qemu_find_opts("mem-path"), optarg, 1); > > Pre-existing, but this is yet another inconsistent naming between C > objects and the command line. If we were consistent, it should be named > QEMU_OPTION_mem_path, and qemu_mem_path_options. (See my recent > complaint about other misnamed options: > https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg01131.html) Hi Erik, What is the practical effect of the mismatch?
On Fri, Mar 07, 2014 at 08:53:50AM +0100, Paolo Bonzini wrote: > Il 07/03/2014 01:40, Marcelo Tosatti ha scritto: > > > >Failing initialization in case hugepage path has > >hugepage smaller than specified. > > > >Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > > Why is this needed? Isn't it just operator error? Libvirt can be responsible for setting up the hugetlbfs mount. Are you suggesting that enforcement of this property be moved to the software on top of libvirt? > Perhaps libvirt could add an attribute to its <hugepages/> XML > element, and could use it to find the appropriate hugetlbfs mount. > But I don't think this check belongs in QEMU. http://www.spinics.net/linux/fedora/libvir/msg92526.html > Also, see the series I posted recently for a complete (and more > powerful + more extensible) replacement of -mem-path and > -mem-prealloc. > > Paolo > > >diff --git a/exec.c b/exec.c > >index b69fd29..c95a0f3 100644 > >--- a/exec.c > >+++ b/exec.c > >@@ -1034,6 +1034,13 @@ static void *file_ram_alloc(RAMBlock *block, > > return NULL; > > } > > > >+ if (mem_path_min_hpagesize && hpagesize < mem_path_min_hpagesize) { > >+ fprintf(stderr, "mount point (%s) has page size " > >+ "(%ld) < (%ld) = min_hpagesize\n", path, hpagesize, > >+ mem_path_min_hpagesize); > >+ exit(1); > >+ } > >+ > > if (memory < hpagesize) { > > return NULL; > > } > >diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h > >index 4cb4b4a..cc9e28a 100644 > >--- a/include/exec/cpu-all.h > >+++ b/include/exec/cpu-all.h > >@@ -470,6 +470,7 @@ extern RAMList ram_list; > > > > extern const char *mem_path; > > extern int mem_prealloc; > >+extern unsigned long int mem_path_min_hpagesize; > > > > /* Flags stored in the low bits of the TLB virtual address. These are > > defined so that fast path ram access is all zeros. */ > >diff --git a/qemu-options.hx b/qemu-options.hx > >index 56e5fdf..36743e1 100644 > >--- a/qemu-options.hx > >+++ b/qemu-options.hx > >@@ -221,9 +221,9 @@ gigabytes respectively. > > ETEXI > > > > DEF("mem-path", HAS_ARG, QEMU_OPTION_mempath, > >- "-mem-path FILE provide backing storage for guest RAM\n", QEMU_ARCH_ALL) > >+ "-mem-path [mem-path=]file[,min-hpage-size=value] provide backing storage for guest RAM\n", QEMU_ARCH_ALL) > > STEXI > >-@item -mem-path @var{path} > >+@item -mem-path [mem-path=]@var{path}[,min-hpage-size=@var{min-hpage-size}] > > @findex -mem-path > > Allocate guest RAM from a temporarily created file in @var{path}. > > ETEXI > >diff --git a/vl.c b/vl.c > >index 1d27b34..08f9bee 100644 > >--- a/vl.c > >+++ b/vl.c > >@@ -136,6 +136,7 @@ static int display_remote; > > const char* keyboard_layout = NULL; > > ram_addr_t ram_size; > > const char *mem_path = NULL; > >+unsigned long int mem_path_min_hpagesize; > > int mem_prealloc = 0; /* force preallocation of physical target memory */ > > int nb_nics; > > NICInfo nd_table[MAX_NICS]; > >@@ -479,6 +480,22 @@ static QemuOptsList qemu_msg_opts = { > > }, > > }; > > > >+static QemuOptsList qemu_mempath_opts = { > >+ .name = "mem-path", > >+ .implied_opt_name = "mem-path", > >+ .head = QTAILQ_HEAD_INITIALIZER(qemu_mempath_opts.head), > >+ .desc = { > >+ { > >+ .name = "mem-path", > >+ .type = QEMU_OPT_STRING, > >+ },{ > >+ .name = "min-hpage-size", > >+ .type = QEMU_OPT_SIZE, > >+ }, > >+ { /* end of list */ } > >+ }, > >+}; > >+ > > /** > > * Get machine options > > * > >@@ -2863,6 +2880,7 @@ int main(int argc, char **argv, char **envp) > > qemu_add_opts(&qemu_tpmdev_opts); > > qemu_add_opts(&qemu_realtime_opts); > > qemu_add_opts(&qemu_msg_opts); > >+ qemu_add_opts(&qemu_mempath_opts); > > > > runstate_init(); > > > >@@ -3189,9 +3207,16 @@ int main(int argc, char **argv, char **envp) > > } > > break; > > #endif > >- case QEMU_OPTION_mempath: > >- mem_path = optarg; > >+ case QEMU_OPTION_mempath: { > >+ opts = qemu_opts_parse(qemu_find_opts("mem-path"), optarg, 1); > >+ if (!opts) { > >+ exit(1); > >+ } > >+ mem_path = qemu_opt_get(opts, "mem-path"); > >+ mem_path_min_hpagesize = qemu_opt_get_size(opts, > >+ "min-hpage-size", 0); > > break; > >+ } > > case QEMU_OPTION_mem_prealloc: > > mem_prealloc = 1; > > break; > > > >
On 03/07/2014 08:13 AM, Marcelo Tosatti wrote: > On Thu, Mar 06, 2014 at 09:21:10PM -0700, Eric Blake wrote: >> On 03/06/2014 05:40 PM, Marcelo Tosatti wrote: >>> >>> Failing initialization in case hugepage path has >>> hugepage smaller than specified. >>> >>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> >>> >>> diff --git a/exec.c b/exec.c >>> index b69fd29..c95a0f3 100644 >>> --- a/exec.c >>> +++ b/exec.c >> >>> }; >>> >>> +static QemuOptsList qemu_mempath_opts = { >>> + .name = "mem-path", >> >>> - case QEMU_OPTION_mempath: >>> - mem_path = optarg; >>> + case QEMU_OPTION_mempath: { >>> + opts = qemu_opts_parse(qemu_find_opts("mem-path"), optarg, 1); >> >> Pre-existing, but this is yet another inconsistent naming between C >> objects and the command line. If we were consistent, it should be named >> QEMU_OPTION_mem_path, and qemu_mem_path_options. (See my recent >> complaint about other misnamed options: >> https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg01131.html) > > Hi Erik, It's Eric, but you're not the first to be affected by finger memory :) > > What is the practical effect of the mismatch? Harder to grep for things like 'mem.path' (to see both -mem-path strings and mem_path variable names) when looking for all places in the code base related to a given command line or QMP spelling.
diff --git a/exec.c b/exec.c index b69fd29..c95a0f3 100644 --- a/exec.c +++ b/exec.c @@ -1034,6 +1034,13 @@ static void *file_ram_alloc(RAMBlock *block, return NULL; } + if (mem_path_min_hpagesize && hpagesize < mem_path_min_hpagesize) { + fprintf(stderr, "mount point (%s) has page size " + "(%ld) < (%ld) = min_hpagesize\n", path, hpagesize, + mem_path_min_hpagesize); + exit(1); + } + if (memory < hpagesize) { return NULL; } diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h index 4cb4b4a..cc9e28a 100644 --- a/include/exec/cpu-all.h +++ b/include/exec/cpu-all.h @@ -470,6 +470,7 @@ extern RAMList ram_list; extern const char *mem_path; extern int mem_prealloc; +extern unsigned long int mem_path_min_hpagesize; /* Flags stored in the low bits of the TLB virtual address. These are defined so that fast path ram access is all zeros. */ diff --git a/qemu-options.hx b/qemu-options.hx index 56e5fdf..36743e1 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -221,9 +221,9 @@ gigabytes respectively. ETEXI DEF("mem-path", HAS_ARG, QEMU_OPTION_mempath, - "-mem-path FILE provide backing storage for guest RAM\n", QEMU_ARCH_ALL) + "-mem-path [mem-path=]file[,min-hpage-size=value] provide backing storage for guest RAM\n", QEMU_ARCH_ALL) STEXI -@item -mem-path @var{path} +@item -mem-path [mem-path=]@var{path}[,min-hpage-size=@var{min-hpage-size}] @findex -mem-path Allocate guest RAM from a temporarily created file in @var{path}. ETEXI diff --git a/vl.c b/vl.c index 1d27b34..08f9bee 100644 --- a/vl.c +++ b/vl.c @@ -136,6 +136,7 @@ static int display_remote; const char* keyboard_layout = NULL; ram_addr_t ram_size; const char *mem_path = NULL; +unsigned long int mem_path_min_hpagesize; int mem_prealloc = 0; /* force preallocation of physical target memory */ int nb_nics; NICInfo nd_table[MAX_NICS]; @@ -479,6 +480,22 @@ static QemuOptsList qemu_msg_opts = { }, }; +static QemuOptsList qemu_mempath_opts = { + .name = "mem-path", + .implied_opt_name = "mem-path", + .head = QTAILQ_HEAD_INITIALIZER(qemu_mempath_opts.head), + .desc = { + { + .name = "mem-path", + .type = QEMU_OPT_STRING, + },{ + .name = "min-hpage-size", + .type = QEMU_OPT_SIZE, + }, + { /* end of list */ } + }, +}; + /** * Get machine options * @@ -2863,6 +2880,7 @@ int main(int argc, char **argv, char **envp) qemu_add_opts(&qemu_tpmdev_opts); qemu_add_opts(&qemu_realtime_opts); qemu_add_opts(&qemu_msg_opts); + qemu_add_opts(&qemu_mempath_opts); runstate_init(); @@ -3189,9 +3207,16 @@ int main(int argc, char **argv, char **envp) } break; #endif - case QEMU_OPTION_mempath: - mem_path = optarg; + case QEMU_OPTION_mempath: { + opts = qemu_opts_parse(qemu_find_opts("mem-path"), optarg, 1); + if (!opts) { + exit(1); + } + mem_path = qemu_opt_get(opts, "mem-path"); + mem_path_min_hpagesize = qemu_opt_get_size(opts, + "min-hpage-size", 0); break; + } case QEMU_OPTION_mem_prealloc: mem_prealloc = 1; break;
Failing initialization in case hugepage path has hugepage smaller than specified. Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>