Hi
On Tue, Jul 28, 2015 at 9:33 AM, Andrew Jones <drjo...@redhat.com> 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
