Re: [Qemu-devel] [PATCH v6 08/33] exec: allow memory to be allocated from any kind of path

From: Xiao Guangrong <guangrong.xiao@linux.intel.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	pbonzini@redhat.com, imammedo@redhat.com
Cc: ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com,
	gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org,
	stefanha@redhat.com, dan.j.williams@intel.com, rth@twiddle.net
Subject: Re: [Qemu-devel] [PATCH v6 08/33] exec: allow memory to be allocated from any kind of path
Date: Sat, 31 Oct 2015 15:44:39 +0800	[thread overview]
Message-ID: <56347167.3060206@linux.intel.com> (raw)
In-Reply-To: <563378E1.5000101@virtuozzo.com>

On 10/30/2015 10:04 PM, Vladimir Sementsov-Ogievskiy wrote:
> On 30.10.2015 08:56, 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 <guangrong.xiao@linux.intel.com>
>> ---
>>   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 <sys/vfs.h>
>> -
>> -#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");
>> +    }
>
> It is strange to see this warning every time.
>
> Shouldn't the differentiation be done explicitly in command line? May be separate option mem-tlb, or
> separate flag tlbfs=on, or for new feature - new option mem-file, or prefixes for paths (tlbfs://,
> file://).. Or the other way to not mix things but split them.

This is just a reminder to users. Currently Qemu do not stop user to append a regular file
for its backend memory, particularly, hugetlbfs is not the only way to use HugePage for
the THP-enabled system.

We can implement your idea as a independent patchset in the future.