From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46202) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZvkIf-0008B3-Hm for qemu-devel@nongnu.org; Mon, 09 Nov 2015 06:07:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZvkIc-0003sA-5y for qemu-devel@nongnu.org; Mon, 09 Nov 2015 06:07:49 -0500 Received: from mga09.intel.com ([134.134.136.24]:60176) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZvkIb-0003s0-Sf for qemu-devel@nongnu.org; Mon, 09 Nov 2015 06:07:46 -0500 References: <1446184587-142784-1-git-send-email-guangrong.xiao@linux.intel.com> <1446184587-142784-9-git-send-email-guangrong.xiao@linux.intel.com> <20151109103931.GA3324@redhat.com> From: Xiao Guangrong Message-ID: <56407D05.7080201@linux.intel.com> Date: Mon, 9 Nov 2015 19:01:25 +0800 MIME-Version: 1.0 In-Reply-To: <20151109103931.GA3324@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 08/33] exec: allow memory to be allocated from any kind of path List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: ehabkost@redhat.com, kvm@vger.kernel.org, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, imammedo@redhat.com, pbonzini@redhat.com, dan.j.williams@intel.com, rth@twiddle.net On 11/09/2015 06:39 PM, Michael S. Tsirkin wrote: > On Fri, Oct 30, 2015 at 01:56:02PM +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 > > So this allows regular memory to be specified directly. > This needs to be split out and merged separately > from acpi/nvdimm bits. Yup, that is in my plan. > > Alternatively, if it's possible to use nvdimm with DAX fs > (similar to hugetlbfs), leave these patches off for now. > DAX is a filesystem mount options, it is not easy to get this info from a file. > >> --- >> exec.c | 56 +++++++++++++++++--------------------------------------- >> 1 file changed, 17 insertions(+), 39 deletions(-) >> >> diff --git a/exec.c b/exec.c >> index 8af2570..3ca7e50 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -1174,32 +1174,6 @@ void qemu_mutex_unlock_ramlist(void) >> } >> >> #ifdef __linux__ >> - >> -#include >> - >> -#define HUGETLBFS_MAGIC 0x958458f6 >> - >> -static long gethugepagesize(const char *path, Error **errp) >> -{ >> - struct statfs fs; >> - int ret; >> - >> - do { >> - ret = statfs(path, &fs); >> - } while (ret != 0 && errno == EINTR); >> - >> - if (ret != 0) { >> - error_setg_errno(errp, errno, "failed to get page size of file %s", >> - path); >> - return 0; >> - } >> - >> - if (fs.f_type != HUGETLBFS_MAGIC) >> - fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path); >> - >> - return fs.f_bsize; >> -} >> - >> static void *file_ram_alloc(RAMBlock *block, >> ram_addr_t memory, >> const char *path, >> @@ -1210,20 +1184,24 @@ static void *file_ram_alloc(RAMBlock *block, >> char *c; >> void *area; >> int fd; >> - uint64_t hpagesize; >> - Error *local_err = NULL; >> + uint64_t pagesize; >> >> - hpagesize = gethugepagesize(path, &local_err); >> - if (local_err) { >> - error_propagate(errp, local_err); >> + pagesize = qemu_file_get_page_size(path); >> + if (!pagesize) { >> + error_setg(errp, "can't get page size for %s", path); >> goto error; >> } >> - block->mr->align = hpagesize; >> >> - if (memory < hpagesize) { >> + if (pagesize == getpagesize()) { >> + fprintf(stderr, "Memory is not allocated from HugeTlbfs.\n"); >> + } >> + >> + block->mr->align = pagesize; >> + >> + 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; >> } >> >> @@ -1247,14 +1225,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); > > Looks like we are still calling mkstemp/unlink here. > How does this work? Hmm? We have got the fd so the file can be safely unlinked (kernel does not actually unlink the file since mkstemp() holds a refcount.). And this patch just renames the variables, no logic changed. Will drop it from the serials for now on.