Re: [Qemu-devel] [PATCH v2 33/45] ivshmem-server: fix hugetlbfs support

[Marc-André Lureau]

Hi

On Tue, Jul 28, 2015 at 9:33 AM, Andrew Jones <address@hidden> wrote:
> On Tue, Jul 28, 2015 at 02:32:45AM +0200, Marc-André Lureau wrote:
>> +
>> +    return fs.f_bsize;
>> +}
>> +
>> +
>> +
> few extra lines here

cut, thanks

>>  /* open shm, create and bind to the unix socket */
>>  int
>>  ivshmem_server_start(IvshmemServer *server)
>>  {
>>      struct sockaddr_un sun;
>>      int shm_fd, sock_fd, ret;
>> +    long hpagesize;
>> +    gchar *filename;
>>
>>      /* open shm file */
>> -    shm_fd = shm_open(server->shm_path, O_CREAT|O_RDWR, S_IRWXU);
>> +    hpagesize = gethugepagesize(server->shm_path);
>> +    if (hpagesize > 0) {
>> +        if (server->shm_size < hpagesize) {
> should be >, but isn't this forcing the shared memory to be less than
> equal to the size of a single hugepage? I think we should allow up to
> nr-pages * page-size.
>

oops, I clearly didn't test correctly this patch, or it's an outdated version.

This check is quite useless since ftruncate will check if it can be sized after.

> Also, I'm not sure we want the dependency, but what about libhugetlbfs?
> It has hugetlbfs_test_path

I don't know that library, it's not on my system either :)
After installing it, it doesn't look like an API but rather a preload library

Imho, it's not necessary at this point.

>
>> +            fprintf(stderr, "hugepage must be at least of size: %ld\n",
>> +                    hpagesize);
>> +            return -1;
>> +        }
>> +        filename = g_strdup_printf("%s/ivshmem.XXXXXX", server->shm_path);
>> +        shm_fd = mkstemp(filename);
>
> Shouldn't we change the perms for shm_fd to match the non-hugetlbfs
> case? Or change the non-hugetlbfs case to match mkstemp? Actually,
> probably the later, because I don't think we want the region to have

Good question. I wonder what the exec mode actually means for shm, as
the execution in memory is rather defined by mmap PROT_EXEC.
It doesn't make much sense to create executable files/shm.

I'll make a followup rfc patch to fix shm_open() args in both server & qemu

> execute perms, or do we? Also, I guess the plan is to pass the hugetlbfs
> file descriptor around if other host processes need to know where the
> memory is, as we never allow a full path. Should we do the same for shm?
> i.e. keep them anonymous too and always pass file descriptors?

it's already only passing fd, in any case

>> +        unlink(filename);
>> +        g_free(filename);
>> +    } else {
>> +        shm_fd = shm_open(server->shm_path, O_CREAT|O_RDWR, S_IRWXU);
>> +    }
>> +
>>      if (shm_fd < 0) {
>>          fprintf(stderr, "cannot open shm file %s: %s\n", server->shm_path,
>>                  strerror(errno));
>> diff --git a/contrib/ivshmem-server/ivshmem-server.h 
>> b/contrib/ivshmem-server/ivshmem-server.h
>> index 2176d5e..e9b0e7a 100644
>> --- a/contrib/ivshmem-server/ivshmem-server.h
>> +++ b/contrib/ivshmem-server/ivshmem-server.h
>> @@ -81,8 +81,7 @@ typedef struct IvshmemServer {
>>   * @server:         A pointer to an uninitialized IvshmemServer structure
>>   * @unix_sock_path: The pointer to the unix socket file name
>>   * @shm_path:       Path to the shared memory. The path corresponds to a 
>> POSIX
>> - *                  shm name. To use a real file, for instance in a 
>> hugetlbfs,
>> - *                  it is possible to use /../../abspath/to/file.
>> + *                  shm name or a hugetlbfs mount point.
>>   * @shm_size:       Size of shared memory
>>   * @n_vectors:      Number of interrupt vectors per client
>>   * @verbose:        True to enable verbose mode
>> diff --git a/contrib/ivshmem-server/main.c b/contrib/ivshmem-server/main.c
>> index 84ffc4d..cd8d9ed 100644
>> --- a/contrib/ivshmem-server/main.c
>> +++ b/contrib/ivshmem-server/main.c
>> @@ -47,9 +47,8 @@ ivshmem_server_usage(const char *name, int code)
>>                      "     to listen to.\n"
>>                      "     Default=%s\n", 
>> IVSHMEM_SERVER_DEFAULT_UNIX_SOCK_PATH);
>>      fprintf(stderr, "  -m <shm_path>: path to the shared memory.\n"
>> -                    "     The path corresponds to a POSIX shm name. To use 
>> a\n"
>> -                    "     real file, for instance in a hugetlbfs, use\n"
>> -                    "     /../../abspath/to/file.\n"
>> +                    "     The path corresponds to a POSIX shm name or a\n"
>> +                    "     hugetlbfs mount point.\n"
>>                      "     default=%s\n", IVSHMEM_SERVER_DEFAULT_SHM_PATH);
>>      fprintf(stderr, "  -l <size>: size of shared memory in bytes. The 
>> suffix\n"
>>                      "     K, M and G can be used (ex: 1K means 1024).\n"
>> --
>> 2.4.3
>>
>>

thanks


-- 
Marc-André Lureau