| Message ID | 1469008443-72059-1-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Igor Mammedov <imammedo@redhat.com> writes: > When adding hostmem backend at runtime, QEMU might exit with error: > "os_mem_prealloc: Insufficient free host memory pages available to allocate guest RAM" > > It happens due to os_mem_prealloc() not handling errors gracefully. > > Fix it by passing errp argument so that os_mem_prealloc() could > report error to callers and undo performed allocation when > os_mem_prealloc() fails. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > include/qemu/osdep.h | 2 +- > backends/hostmem.c | 18 ++++++++++++++---- > exec.c | 10 ++++++++-- > util/oslib-posix.c | 25 ++++++++++++------------- > util/oslib-win32.c | 2 +- > 5 files changed, 36 insertions(+), 21 deletions(-) > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > index fbb8759..d7c111d 100644 > --- a/include/qemu/osdep.h > +++ b/include/qemu/osdep.h > @@ -379,7 +379,7 @@ unsigned long qemu_getauxval(unsigned long type); > > void qemu_set_tty_echo(int fd, bool echo); > > -void os_mem_prealloc(int fd, char *area, size_t sz); > +void os_mem_prealloc(int fd, char *area, size_t sz, Error **errp); > > int qemu_read_password(char *buf, int buf_size); > > diff --git a/backends/hostmem.c b/backends/hostmem.c > index ac80257..b7a208d 100644 > --- a/backends/hostmem.c > +++ b/backends/hostmem.c > @@ -203,6 +203,7 @@ static bool host_memory_backend_get_prealloc(Object *obj, Error **errp) > static void host_memory_backend_set_prealloc(Object *obj, bool value, > Error **errp) > { > + Error *local_err = NULL; > HostMemoryBackend *backend = MEMORY_BACKEND(obj); > > if (backend->force_prealloc) { > @@ -223,7 +224,11 @@ static void host_memory_backend_set_prealloc(Object *obj, bool value, > void *ptr = memory_region_get_ram_ptr(&backend->mr); > uint64_t sz = memory_region_size(&backend->mr); > > - os_mem_prealloc(fd, ptr, sz); > + os_mem_prealloc(fd, ptr, sz, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > backend->prealloc = true; > } > } > @@ -286,8 +291,7 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp) > if (bc->alloc) { > bc->alloc(backend, &local_err); > if (local_err) { > - error_propagate(errp, local_err); > - return; > + goto out; I'd leave the tail merging optimization to the compiler, i.e. don't touch the code here, and ... > } > > ptr = memory_region_get_ram_ptr(&backend->mr); > @@ -343,9 +347,15 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp) > * specified NUMA policy in place. > */ > if (backend->prealloc) { > - os_mem_prealloc(memory_region_get_fd(&backend->mr), ptr, sz); > + os_mem_prealloc(memory_region_get_fd(&backend->mr), ptr, sz, > + &local_err); > + if (local_err) { > + goto out; ... have the obvious error_propagate() + return here. Matter of taste, between you and the maintainer. Except there is none. Inexcusable for a file created in 2014. Suggest you appoint yourself. > + } > } > } > +out: > + error_propagate(errp, local_err); > } > > static bool > diff --git a/exec.c b/exec.c > index 60cf46a..bf1446c 100644 > --- a/exec.c > +++ b/exec.c > @@ -1268,7 +1268,7 @@ static void *file_ram_alloc(RAMBlock *block, > char *filename; > char *sanitized_name; > char *c; > - void *area; > + void *area = MAP_FAILED; > int fd = -1; > int64_t page_size; > > @@ -1356,13 +1356,19 @@ static void *file_ram_alloc(RAMBlock *block, > } > > if (mem_prealloc) { > - os_mem_prealloc(fd, area, memory); > + os_mem_prealloc(fd, area, memory, errp); > + if (errp && *errp) { > + goto error; > + } > } > > block->fd = fd; > return area; > > error: > + if (area != MAP_FAILED) { > + qemu_ram_munmap(area, memory); > + } > if (unlink_on_error) { > unlink(path); > } > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index 6d70d9a..a60ac97 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -318,7 +318,7 @@ static void sigbus_handler(int signal) > siglongjmp(sigjump, 1); > } > > -void os_mem_prealloc(int fd, char *area, size_t memory) > +void os_mem_prealloc(int fd, char *area, size_t memory, Error **errp) > { > int ret; > struct sigaction act, oldact; > @@ -330,8 +330,9 @@ void os_mem_prealloc(int fd, char *area, size_t memory) > > ret = sigaction(SIGBUS, &act, &oldact); > if (ret) { > - perror("os_mem_prealloc: failed to install signal handler"); > - exit(1); > + error_setg_errno(errp, errno, > + "os_mem_prealloc: failed to install signal handler"); > + return; > } > > /* unblock SIGBUS */ > @@ -340,9 +341,8 @@ void os_mem_prealloc(int fd, char *area, size_t memory) > pthread_sigmask(SIG_UNBLOCK, &set, &oldset); > > if (sigsetjmp(sigjump, 1)) { > - fprintf(stderr, "os_mem_prealloc: Insufficient free host memory " > - "pages available to allocate guest RAM\n"); > - exit(1); > + error_setg(errp, "os_mem_prealloc: Insufficient free host memory " > + "pages available to allocate guest RAM\n"); > } else { > int i; > size_t hpagesize = qemu_fd_getpagesize(fd); > @@ -352,15 +352,14 @@ void os_mem_prealloc(int fd, char *area, size_t memory) > for (i = 0; i < numpages; i++) { > memset(area + (hpagesize * i), 0, 1); > } > + } > > - ret = sigaction(SIGBUS, &oldact, NULL); > - if (ret) { > - perror("os_mem_prealloc: failed to reinstall signal handler"); > - exit(1); > - } > - > - pthread_sigmask(SIG_SETMASK, &oldset, NULL); > + ret = sigaction(SIGBUS, &oldact, NULL); > + if (ret) { > + perror("os_mem_prealloc: failed to reinstall signal handler"); > + exit(1); I guess you're keeping the exit() here, because you can't recover cleanly from this error. I should never happen anyway. Worth a comment, I think. > } > + pthread_sigmask(SIG_SETMASK, &oldset, NULL); > } > > > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > index 6debc2b..4c1dcf1 100644 > --- a/util/oslib-win32.c > +++ b/util/oslib-win32.c > @@ -539,7 +539,7 @@ int getpagesize(void) > return system_info.dwPageSize; > } > > -void os_mem_prealloc(int fd, char *area, size_t memory) > +void os_mem_prealloc(int fd, char *area, size_t memory, Error **errp) > { > int i; > size_t pagesize = getpagesize(); Reviewed-by: Markus Armbruster <armbru@redhat.com>
On 21/07/2016 10:36, Markus Armbruster wrote: > ... have the obvious error_propagate() + return here. Matter of taste, > between you and the maintainer. Except there is none. Inexcusable for > a file created in 2014. Suggest you appoint yourself. For now I've queued the patch, but I suggest that memory backends go together with numa.c. Paolo
On Thu, 21 Jul 2016 11:05:05 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 21/07/2016 10:36, Markus Armbruster wrote: > > ... have the obvious error_propagate() + return here. Matter of taste, > > between you and the maintainer. Except there is none. Inexcusable for > > a file created in 2014. Suggest you appoint yourself. > > For now I've queued the patch thanks >, but I suggest that memory backends go > together with numa.c. Do you mean via Eduardo tree since here is numa maintainer? > > Paolo >
On Thu, 21 Jul 2016 10:36:40 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Igor Mammedov <imammedo@redhat.com> writes: [...] > > @@ -286,8 +291,7 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp) > > if (bc->alloc) { > > bc->alloc(backend, &local_err); > > if (local_err) { > > - error_propagate(errp, local_err); > > - return; > > + goto out; > > I'd leave the tail merging optimization to the compiler, i.e. don't > touch the code here, and ... > > > } > > > > ptr = memory_region_get_ram_ptr(&backend->mr); > > @@ -343,9 +347,15 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp) > > * specified NUMA policy in place. > > */ > > if (backend->prealloc) { > > - os_mem_prealloc(memory_region_get_fd(&backend->mr), ptr, sz); > > + os_mem_prealloc(memory_region_get_fd(&backend->mr), ptr, sz, > > + &local_err); > > + if (local_err) { > > + goto out; > > ... have the obvious error_propagate() + return here. Matter of taste, > between you and the maintainer. Except there is none. Inexcusable for > a file created in 2014. Suggest you appoint yourself. > > > + } I might if Eduardo declines or I could be and additional one. wrt consolidating error_propagate(), it's what we were doing in target-i386/cpu.c any time we touched similar code pathes to reduce code duplication. > > } > > } > > +out: > > + error_propagate(errp, local_err); > > } [...] > > @@ -352,15 +352,14 @@ void os_mem_prealloc(int fd, char *area, size_t memory) > > for (i = 0; i < numpages; i++) { > > memset(area + (hpagesize * i), 0, 1); > > } > > + } > > > > - ret = sigaction(SIGBUS, &oldact, NULL); > > - if (ret) { > > - perror("os_mem_prealloc: failed to reinstall signal handler"); > > - exit(1); > > - } > > - > > - pthread_sigmask(SIG_SETMASK, &oldset, NULL); > > + ret = sigaction(SIGBUS, &oldact, NULL); > > + if (ret) { > > + perror("os_mem_prealloc: failed to reinstall signal handler"); > > + exit(1); > > I guess you're keeping the exit() here, because you can't recover > cleanly from this error. I should never happen anyway. Worth a > comment, I think. I didn't added comment because it's obvious that it's not possible to recover and also because it's just the same old code that haven't had a comment to begin with (probably also due to its obviousness) > > } > > + pthread_sigmask(SIG_SETMASK, &oldset, NULL); > > } > > > > > > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > > index 6debc2b..4c1dcf1 100644 > > --- a/util/oslib-win32.c > > +++ b/util/oslib-win32.c > > @@ -539,7 +539,7 @@ int getpagesize(void) > > return system_info.dwPageSize; > > } > > > > -void os_mem_prealloc(int fd, char *area, size_t memory) > > +void os_mem_prealloc(int fd, char *area, size_t memory, Error **errp) > > { > > int i; > > size_t pagesize = getpagesize(); > > Reviewed-by: Markus Armbruster <armbru@redhat.com> Thanks
> > On 21/07/2016 10:36, Markus Armbruster wrote: > > > ... have the obvious error_propagate() + return here. Matter of taste, > > > between you and the maintainer. Except there is none. Inexcusable for > > > a file created in 2014. Suggest you appoint yourself. > > > > For now I've queued the patch > thanks > > >, but I suggest that memory backends go > > together with numa.c. > Do you mean via Eduardo tree since here is numa maintainer? Yes, for the future. Paolo
Igor Mammedov <imammedo@redhat.com> writes: > On Thu, 21 Jul 2016 10:36:40 +0200 > Markus Armbruster <armbru@redhat.com> wrote: > >> Igor Mammedov <imammedo@redhat.com> writes: [...] >> > @@ -352,15 +352,14 @@ void os_mem_prealloc(int fd, char *area, size_t memory) >> > for (i = 0; i < numpages; i++) { >> > memset(area + (hpagesize * i), 0, 1); >> > } >> > + } >> > >> > - ret = sigaction(SIGBUS, &oldact, NULL); >> > - if (ret) { >> > - perror("os_mem_prealloc: failed to reinstall signal handler"); >> > - exit(1); >> > - } >> > - >> > - pthread_sigmask(SIG_SETMASK, &oldset, NULL); >> > + ret = sigaction(SIGBUS, &oldact, NULL); >> > + if (ret) { >> > + perror("os_mem_prealloc: failed to reinstall signal handler"); >> > + exit(1); >> >> I guess you're keeping the exit() here, because you can't recover >> cleanly from this error. I should never happen anyway. Worth a >> comment, I think. > I didn't added comment because it's obvious that it's not possible > to recover and also because it's just the same old code that haven't > had a comment to begin with (probably also due to its obviousness) It's obvious enough once you read closely enough to see what the failing code is doing. The old code uses perror() + exit() consistently. The new code mixes Error with them, which is an anti-pattern. That's okay, the exit() is perfectly fine here. The issue for me is that the anti-pattern triggers the WTF-detector at a glance, but then I have to read closely to see #1 it's intentional, and #2 it's okay. That wasn't the case before. [...]
On Fri, 22 Jul 2016 18:19:44 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Igor Mammedov <imammedo@redhat.com> writes: > > > On Thu, 21 Jul 2016 10:36:40 +0200 > > Markus Armbruster <armbru@redhat.com> wrote: > > > >> Igor Mammedov <imammedo@redhat.com> writes: > [...] > >> > @@ -352,15 +352,14 @@ void os_mem_prealloc(int fd, char *area, size_t memory) > >> > for (i = 0; i < numpages; i++) { > >> > memset(area + (hpagesize * i), 0, 1); > >> > } > >> > + } > >> > > >> > - ret = sigaction(SIGBUS, &oldact, NULL); > >> > - if (ret) { > >> > - perror("os_mem_prealloc: failed to reinstall signal handler"); > >> > - exit(1); > >> > - } > >> > - > >> > - pthread_sigmask(SIG_SETMASK, &oldset, NULL); > >> > + ret = sigaction(SIGBUS, &oldact, NULL); > >> > + if (ret) { > >> > + perror("os_mem_prealloc: failed to reinstall signal handler"); > >> > + exit(1); > >> > >> I guess you're keeping the exit() here, because you can't recover > >> cleanly from this error. I should never happen anyway. Worth a > >> comment, I think. > > I didn't added comment because it's obvious that it's not possible > > to recover and also because it's just the same old code that haven't > > had a comment to begin with (probably also due to its obviousness) > > It's obvious enough once you read closely enough to see what the failing > code is doing. > > The old code uses perror() + exit() consistently. The new code mixes > Error with them, which is an anti-pattern. That's okay, the exit() is > perfectly fine here. The issue for me is that the anti-pattern triggers > the WTF-detector at a glance, but then I have to read closely to see #1 > it's intentional, and #2 it's okay. That wasn't the case before. > > [...] I'll post a patch to add comment on top of this one.
On Thu, Jul 21, 2016 at 11:00:46AM -0400, Paolo Bonzini wrote: > > > > On 21/07/2016 10:36, Markus Armbruster wrote: > > > > ... have the obvious error_propagate() + return here. Matter of taste, > > > > between you and the maintainer. Except there is none. Inexcusable for > > > > a file created in 2014. Suggest you appoint yourself. > > > > > > For now I've queued the patch > > thanks > > > > >, but I suggest that memory backends go > > > together with numa.c. > > Do you mean via Eduardo tree since here is numa maintainer? > > Yes, for the future. Works for me. I will submit a MAINTAINERS patch later, making hostmem*.c part of the NUMA section.
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index fbb8759..d7c111d 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -379,7 +379,7 @@ unsigned long qemu_getauxval(unsigned long type); void qemu_set_tty_echo(int fd, bool echo); -void os_mem_prealloc(int fd, char *area, size_t sz); +void os_mem_prealloc(int fd, char *area, size_t sz, Error **errp); int qemu_read_password(char *buf, int buf_size); diff --git a/backends/hostmem.c b/backends/hostmem.c index ac80257..b7a208d 100644 --- a/backends/hostmem.c +++ b/backends/hostmem.c @@ -203,6 +203,7 @@ static bool host_memory_backend_get_prealloc(Object *obj, Error **errp) static void host_memory_backend_set_prealloc(Object *obj, bool value, Error **errp) { + Error *local_err = NULL; HostMemoryBackend *backend = MEMORY_BACKEND(obj); if (backend->force_prealloc) { @@ -223,7 +224,11 @@ static void host_memory_backend_set_prealloc(Object *obj, bool value, void *ptr = memory_region_get_ram_ptr(&backend->mr); uint64_t sz = memory_region_size(&backend->mr); - os_mem_prealloc(fd, ptr, sz); + os_mem_prealloc(fd, ptr, sz, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } backend->prealloc = true; } } @@ -286,8 +291,7 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp) if (bc->alloc) { bc->alloc(backend, &local_err); if (local_err) { - error_propagate(errp, local_err); - return; + goto out; } ptr = memory_region_get_ram_ptr(&backend->mr); @@ -343,9 +347,15 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp) * specified NUMA policy in place. */ if (backend->prealloc) { - os_mem_prealloc(memory_region_get_fd(&backend->mr), ptr, sz); + os_mem_prealloc(memory_region_get_fd(&backend->mr), ptr, sz, + &local_err); + if (local_err) { + goto out; + } } } +out: + error_propagate(errp, local_err); } static bool diff --git a/exec.c b/exec.c index 60cf46a..bf1446c 100644 --- a/exec.c +++ b/exec.c @@ -1268,7 +1268,7 @@ static void *file_ram_alloc(RAMBlock *block, char *filename; char *sanitized_name; char *c; - void *area; + void *area = MAP_FAILED; int fd = -1; int64_t page_size; @@ -1356,13 +1356,19 @@ static void *file_ram_alloc(RAMBlock *block, } if (mem_prealloc) { - os_mem_prealloc(fd, area, memory); + os_mem_prealloc(fd, area, memory, errp); + if (errp && *errp) { + goto error; + } } block->fd = fd; return area; error: + if (area != MAP_FAILED) { + qemu_ram_munmap(area, memory); + } if (unlink_on_error) { unlink(path); } diff --git a/util/oslib-posix.c b/util/oslib-posix.c index 6d70d9a..a60ac97 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -318,7 +318,7 @@ static void sigbus_handler(int signal) siglongjmp(sigjump, 1); } -void os_mem_prealloc(int fd, char *area, size_t memory) +void os_mem_prealloc(int fd, char *area, size_t memory, Error **errp) { int ret; struct sigaction act, oldact; @@ -330,8 +330,9 @@ void os_mem_prealloc(int fd, char *area, size_t memory) ret = sigaction(SIGBUS, &act, &oldact); if (ret) { - perror("os_mem_prealloc: failed to install signal handler"); - exit(1); + error_setg_errno(errp, errno, + "os_mem_prealloc: failed to install signal handler"); + return; } /* unblock SIGBUS */ @@ -340,9 +341,8 @@ void os_mem_prealloc(int fd, char *area, size_t memory) pthread_sigmask(SIG_UNBLOCK, &set, &oldset); if (sigsetjmp(sigjump, 1)) { - fprintf(stderr, "os_mem_prealloc: Insufficient free host memory " - "pages available to allocate guest RAM\n"); - exit(1); + error_setg(errp, "os_mem_prealloc: Insufficient free host memory " + "pages available to allocate guest RAM\n"); } else { int i; size_t hpagesize = qemu_fd_getpagesize(fd); @@ -352,15 +352,14 @@ void os_mem_prealloc(int fd, char *area, size_t memory) for (i = 0; i < numpages; i++) { memset(area + (hpagesize * i), 0, 1); } + } - ret = sigaction(SIGBUS, &oldact, NULL); - if (ret) { - perror("os_mem_prealloc: failed to reinstall signal handler"); - exit(1); - } - - pthread_sigmask(SIG_SETMASK, &oldset, NULL); + ret = sigaction(SIGBUS, &oldact, NULL); + if (ret) { + perror("os_mem_prealloc: failed to reinstall signal handler"); + exit(1); } + pthread_sigmask(SIG_SETMASK, &oldset, NULL); } diff --git a/util/oslib-win32.c b/util/oslib-win32.c index 6debc2b..4c1dcf1 100644 --- a/util/oslib-win32.c +++ b/util/oslib-win32.c @@ -539,7 +539,7 @@ int getpagesize(void) return system_info.dwPageSize; } -void os_mem_prealloc(int fd, char *area, size_t memory) +void os_mem_prealloc(int fd, char *area, size_t memory, Error **errp) { int i; size_t pagesize = getpagesize();
When adding hostmem backend at runtime, QEMU might exit with error: "os_mem_prealloc: Insufficient free host memory pages available to allocate guest RAM" It happens due to os_mem_prealloc() not handling errors gracefully. Fix it by passing errp argument so that os_mem_prealloc() could report error to callers and undo performed allocation when os_mem_prealloc() fails. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- include/qemu/osdep.h | 2 +- backends/hostmem.c | 18 ++++++++++++++---- exec.c | 10 ++++++++-- util/oslib-posix.c | 25 ++++++++++++------------- util/oslib-win32.c | 2 +- 5 files changed, 36 insertions(+), 21 deletions(-)