From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35193) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adISA-0002RG-Nk for qemu-devel@nongnu.org; Tue, 08 Mar 2016 09:17:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adIS6-0003Fw-LY for qemu-devel@nongnu.org; Tue, 08 Mar 2016 09:17:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36702) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adIS6-0003Fj-D3 for qemu-devel@nongnu.org; Tue, 08 Mar 2016 09:17:34 -0500 References: <1457378754-21649-1-git-send-email-armbru@redhat.com> <1457378754-21649-3-git-send-email-armbru@redhat.com> From: Paolo Bonzini Message-ID: <56DEDEF9.6030506@redhat.com> Date: Tue, 8 Mar 2016 15:17:29 +0100 MIME-Version: 1.0 In-Reply-To: <1457378754-21649-3-git-send-email-armbru@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 02/42] exec: Fix memory allocation when memory path isn't on hugetlbfs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: claudio.fontana@huawei.com, cam@cs.ualberta.ca, mlureau@redhat.com, david.marchand@6wind.com On 07/03/2016 20:25, Markus Armbruster wrote: > gethugepagesize() works reliably only when its argument is on > hugetlbfs. When it's not, it returns the filesystem's "optimal > transfer block size", which may or may not be the actual page size > you'll get when you mmap(). > > If the value is too small or not a power of two, we fail > qemu_ram_mmap()'s assertions. These were added in commit 794e8f3 > (v2.5.0). The bug's impact before that is currently unknown. Seems > fairly unlikely at least when the normal page size is 4KiB. > > Else, if the value is too large, we align more strictly than > necessary. > > gethugepagesize() goes back to commit c902760 (v0.13). That commit > clearly intended gethugepagesize() to be used on hugetlbfs only. Not > only was it named accordingly, it also printed a warning when used on > anything else. However, the commit neglected to spell out the > restriction in user documentation of -mem-path. > > Commit bfc2a1a (v2.5.0) dropped the warning as bogus "because QEMU > functions perfectly well with the path on a regular tmpfs filesystem". > It sure does when you're sufficiently lucky. In my testing, I was > lucky, too. > > Fix by switching to qemu_fd_getpagesize(). Rename the variable > holding its result from hpagesize to page_size. > > Cc: Paolo Bonzini > Signed-off-by: Markus Armbruster > --- > exec.c | 40 +++++++--------------------------------- > 1 file changed, 7 insertions(+), 33 deletions(-) > > diff --git a/exec.c b/exec.c > index 5275ff4..d41194e 100644 > --- a/exec.c > +++ b/exec.c > @@ -1207,27 +1207,6 @@ void qemu_mutex_unlock_ramlist(void) > } > > #ifdef __linux__ > - > -#include > - > -#define HUGETLBFS_MAGIC 0x958458f6 > - > -static long gethugepagesize(int fd) > -{ > - struct statfs fs; > - int ret; > - > - do { > - ret = fstatfs(fd, &fs); > - } while (ret != 0 && errno == EINTR); > - > - if (ret != 0) { > - return -1; > - } > - > - return fs.f_bsize; > -} > - > static void *file_ram_alloc(RAMBlock *block, > ram_addr_t memory, > const char *path, > @@ -1239,7 +1218,7 @@ static void *file_ram_alloc(RAMBlock *block, > char *c; > void *area; > int fd; > - int64_t hpagesize; > + int64_t page_size; > > if (kvm_enabled() && !kvm_has_sync_mmu()) { > error_setg(errp, > @@ -1294,22 +1273,17 @@ static void *file_ram_alloc(RAMBlock *block, > */ > } > > - hpagesize = gethugepagesize(fd); > - if (hpagesize < 0) { > - error_setg_errno(errp, errno, "can't get page size for %s", > - path); > - goto error; > - } > - block->mr->align = hpagesize; > + page_size = qemu_fd_getpagesize(fd); > + block->mr->align = page_size; > > - if (memory < hpagesize) { > + if (memory < page_size) { > error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to " > "or larger than page size 0x%" PRIx64, > - memory, hpagesize); > + memory, page_size); > goto error; > } > > - memory = ROUND_UP(memory, hpagesize); > + memory = ROUND_UP(memory, page_size); > > /* > * ftruncate is not supported by hugetlbfs in older > @@ -1321,7 +1295,7 @@ 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, page_size, block->flags & RAM_SHARED); > if (area == MAP_FAILED) { > error_setg_errno(errp, errno, > "unable to map backing store for guest RAM"); > Queued, thanks. Paolo