| Message ID | e0f979deab3d5785bc7d7dab64b962131a8491d1.1404367199.git.hutao@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 07/03/2014 12:10 AM, Hu Tao wrote: > This patch fixes two problems of memory-backend-file: > > 1. If user adds a memory-backend-file object using object_add command, > specifying a non-existing directory for property mem-path, qemu > will core dump with message: > > /nonexistingdir: No such file or directory > Bad ram offset fffffffffffff000 > Aborted (core dumped) > > 2. If user adds a memory-backend-file object using object_add command, > specifying a size that is less than huge page size, qemu > will core dump with message: > > Bad ram offset fffffffffffff000 > Aborted (core dumped) > Might be nice if the commit message also shows the new message issued for the same cases after the patch is applied. > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> > --- > exec.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > > if (memory < hpagesize) { > - return NULL; > + error_setg(errp, "memory size 0x" RAM_ADDR_FMT " should be larger " > + "than huge page size 0x%" PRIx64, memory, hpagesize); > + goto error; Isn't exactly equal also allowed? Maybe a better wording is "should be a multiple of the huge page size"
On Thu, Jul 03, 2014 at 06:33:28AM -0600, Eric Blake wrote: > On 07/03/2014 12:10 AM, Hu Tao wrote: > > This patch fixes two problems of memory-backend-file: > > > > 1. If user adds a memory-backend-file object using object_add command, > > specifying a non-existing directory for property mem-path, qemu > > will core dump with message: > > > > /nonexistingdir: No such file or directory > > Bad ram offset fffffffffffff000 > > Aborted (core dumped) > > > > 2. If user adds a memory-backend-file object using object_add command, > > specifying a size that is less than huge page size, qemu > > will core dump with message: > > > > Bad ram offset fffffffffffff000 > > Aborted (core dumped) > > > > Might be nice if the commit message also shows the new message issued > for the same cases after the patch is applied. OK. > > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> > > --- > > exec.c | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > > > > if (memory < hpagesize) { > > - return NULL; > > + error_setg(errp, "memory size 0x" RAM_ADDR_FMT " should be larger " > > + "than huge page size 0x%" PRIx64, memory, hpagesize); > > + goto error; > > Isn't exactly equal also allowed? Maybe a better wording is "should be > a multiple of the huge page size" Yes. I'll change the error message. Thanks, Hu > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
On 07/03/2014 08:56 PM, Hu Tao wrote: > On Thu, Jul 03, 2014 at 06:33:28AM -0600, Eric Blake wrote: >> On 07/03/2014 12:10 AM, Hu Tao wrote: >>> This patch fixes two problems of memory-backend-file: >>> >>> 1. If user adds a memory-backend-file object using object_add command, >>> specifying a non-existing directory for property mem-path, qemu >>> will core dump with message: >>> >>> /nonexistingdir: No such file or directory >>> Bad ram offset fffffffffffff000 >>> Aborted (core dumped) >>> >>> 2. If user adds a memory-backend-file object using object_add command, >>> specifying a size that is less than huge page size, qemu >>> will core dump with message: >>> >>> Bad ram offset fffffffffffff000 >>> Aborted (core dumped) >>> >> >> Might be nice if the commit message also shows the new message issued >> for the same cases after the patch is applied. > > OK. > >>> >>> if (memory < hpagesize) { >>> - return NULL; >>> + error_setg(errp, "memory size 0x" RAM_ADDR_FMT " should be larger " >>> + "than huge page size 0x%" PRIx64, memory, hpagesize); >>> + goto error; >> >> Isn't exactly equal also allowed? Maybe a better wording is "should be >> a multiple of the huge page size" > > Yes. I'll change the error message. > Thinking about it more, should you also enforce that it is a multiple? As in: if (!memory || memory % hpageszie) { error... }
On Thu, Jul 03, 2014 at 10:05:56PM -0600, Eric Blake wrote: > On 07/03/2014 08:56 PM, Hu Tao wrote: > > On Thu, Jul 03, 2014 at 06:33:28AM -0600, Eric Blake wrote: > >> On 07/03/2014 12:10 AM, Hu Tao wrote: > >>> This patch fixes two problems of memory-backend-file: > >>> > >>> 1. If user adds a memory-backend-file object using object_add command, > >>> specifying a non-existing directory for property mem-path, qemu > >>> will core dump with message: > >>> > >>> /nonexistingdir: No such file or directory > >>> Bad ram offset fffffffffffff000 > >>> Aborted (core dumped) > >>> > >>> 2. If user adds a memory-backend-file object using object_add command, > >>> specifying a size that is less than huge page size, qemu > >>> will core dump with message: > >>> > >>> Bad ram offset fffffffffffff000 > >>> Aborted (core dumped) > >>> > >> > >> Might be nice if the commit message also shows the new message issued > >> for the same cases after the patch is applied. > > > > OK. > > > > >>> > >>> if (memory < hpagesize) { > >>> - return NULL; > >>> + error_setg(errp, "memory size 0x" RAM_ADDR_FMT " should be larger " > >>> + "than huge page size 0x%" PRIx64, memory, hpagesize); > >>> + goto error; > >> > >> Isn't exactly equal also allowed? Maybe a better wording is "should be > >> a multiple of the huge page size" > > > > Yes. I'll change the error message. > > > > Thinking about it more, should you also enforce that it is a multiple? > > As in: if (!memory || memory % hpageszie) { error... } The memory size is rounded up to hpagesize when allocating memory, we can waste at most hpagesize-1 bytes memory. I don't think it's a problem. In the other side, do you think we should give an error in the case like -object memory-backend-ram,size=1111M(given 2M hpagesize)? Regards, Hu
Il 04/07/2014 09:43, Hu Tao ha scritto: > The memory size is rounded up to hpagesize when allocating memory, we > can waste at most hpagesize-1 bytes memory. I don't think it's a > problem. > > In the other side, do you think we should give an error in the case like > -object memory-backend-ram,size=1111M(given 2M hpagesize)? I think for gb pages it can make sense to waste a little memory (or even not so little, like if you have a 2.5 GB guest). Paolo
On Fri, Jul 04, 2014 at 09:47:43AM +0200, Paolo Bonzini wrote: > Il 04/07/2014 09:43, Hu Tao ha scritto: > >The memory size is rounded up to hpagesize when allocating memory, we > >can waste at most hpagesize-1 bytes memory. I don't think it's a > >problem. > > > >In the other side, do you think we should give an error in the case like > >-object memory-backend-ram,size=1111M(given 2M hpagesize)? > > I think for gb pages it can make sense to waste a little memory (or even not > so little, like if you have a 2.5 GB guest). > > Paolo For 2.1, all we can take at this point is obvious bugfixes. Propagating error up the stack instead of aborting might be fine if it's ready before rc1, that is Monday at the latest. Afterwards, I'll only merge high priority bugfixes, improving handling of user errors doesn't count as there's an easy workaround. Whether rounding up to page size is a good idea or not, I'm inclined to say this is 2.2 material.
On Sun, Jul 06, 2014 at 09:42:42AM +0300, Michael S. Tsirkin wrote: > On Fri, Jul 04, 2014 at 09:47:43AM +0200, Paolo Bonzini wrote: > > Il 04/07/2014 09:43, Hu Tao ha scritto: > > >The memory size is rounded up to hpagesize when allocating memory, we > > >can waste at most hpagesize-1 bytes memory. I don't think it's a > > >problem. > > > > > >In the other side, do you think we should give an error in the case like > > >-object memory-backend-ram,size=1111M(given 2M hpagesize)? > > > > I think for gb pages it can make sense to waste a little memory (or even not > > so little, like if you have a 2.5 GB guest). > > > > Paolo > > For 2.1, all we can take at this point is obvious bugfixes. > Propagating error up the stack instead of aborting > might be fine if it's ready before rc1, that is Monday at the latest. > > Afterwards, I'll only merge high priority bugfixes, improving > handling of user errors doesn't count as there's > an easy workaround. > > Whether rounding up to page size is a good idea or not, > I'm inclined to say this is 2.2 material. I'll send patches not including this part today. Regards, Hu
diff --git a/exec.c b/exec.c index 8c2a91d..35c2dcb 100644 --- a/exec.c +++ b/exec.c @@ -996,7 +996,7 @@ void qemu_mutex_unlock_ramlist(void) #define HUGETLBFS_MAGIC 0x958458f6 -static long gethugepagesize(const char *path) +static long gethugepagesize(const char *path, Error **errp) { struct statfs fs; int ret; @@ -1006,7 +1006,7 @@ static long gethugepagesize(const char *path) } while (ret != 0 && errno == EINTR); if (ret != 0) { - perror(path); + error_setg_errno(errp, errno, "failed to stat file %s", path); return 0; } @@ -1024,17 +1024,19 @@ static void *file_ram_alloc(RAMBlock *block, char *filename; char *sanitized_name; char *c; - void *area; + void *area = NULL; int fd; unsigned long hpagesize; - hpagesize = gethugepagesize(path); + hpagesize = gethugepagesize(path, errp); if (!hpagesize) { goto error; } if (memory < hpagesize) { - return NULL; + error_setg(errp, "memory size 0x" RAM_ADDR_FMT " should be larger " + "than huge page size 0x%" PRIx64, memory, hpagesize); + goto error; } if (kvm_enabled() && !kvm_has_sync_mmu()) { @@ -1094,8 +1096,8 @@ static void *file_ram_alloc(RAMBlock *block, return area; error: - if (mem_prealloc) { - exit(1); + if (area && area != MAP_FAILED) { + munmap(area, memory); } return NULL; }
This patch fixes two problems of memory-backend-file: 1. If user adds a memory-backend-file object using object_add command, specifying a non-existing directory for property mem-path, qemu will core dump with message: /nonexistingdir: No such file or directory Bad ram offset fffffffffffff000 Aborted (core dumped) 2. If user adds a memory-backend-file object using object_add command, specifying a size that is less than huge page size, qemu will core dump with message: Bad ram offset fffffffffffff000 Aborted (core dumped) Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- exec.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)