On Tue, Jul 28, 2015 at 08:02:54PM +0200, Marc-André Lureau wrote:
> 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
$ dnf info libhugetlbfs-devel | grep Desc
Description : Contains header files for building with libhugetlbfs.
>
> Imho, it's not necessary at this point.
OK
>
> >
> >> + 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
Sounds good.
Thanks,
drew
>
> >> + 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
>
