From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45404) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKKso-0000aH-Pw for qemu-devel@nongnu.org; Wed, 29 Jul 2015 02:30:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKKsh-0001Mf-B3 for qemu-devel@nongnu.org; Wed, 29 Jul 2015 02:30:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41039) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKKsg-0001Lx-VW for qemu-devel@nongnu.org; Wed, 29 Jul 2015 02:30:23 -0400 Date: Wed, 29 Jul 2015 08:30:18 +0200 From: Andrew Jones Message-ID: <20150729063018.GA3456@hawk.localdomain> References: <1438043577-28636-1-git-send-email-marcandre.lureau@redhat.com> <1438043577-28636-34-git-send-email-marcandre.lureau@redhat.com> <20150728073347.GA3629@hawk.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 33/45] ivshmem-server: fix hugetlbfs support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Marc-Andr=E9?= Lureau Cc: cam , QEMU , stefanha@redhat.com On Tue, Jul 28, 2015 at 08:02:54PM +0200, Marc-Andr=E9 Lureau wrote: > Hi >=20 > On Tue, Jul 28, 2015 at 9:33 AM, Andrew Jones wrot= e: > > On Tue, Jul 28, 2015 at 02:32:45AM +0200, Marc-Andr=E9 Lureau wrote: > >> + > >> + return fs.f_bsize; > >> +} > >> + > >> + > >> + > > few extra lines here >=20 > cut, thanks >=20 > >> /* 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 =3D shm_open(server->shm_path, O_CREAT|O_RDWR, S_IRWXU); > >> + hpagesize =3D 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. > > >=20 > oops, I clearly didn't test correctly this patch, or it's an outdated v= ersion. >=20 > This check is quite useless since ftruncate will check if it can be siz= ed after. >=20 > > Also, I'm not sure we want the dependency, but what about libhugetlbf= s? > > It has hugetlbfs_test_path >=20 > 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 l= ibrary $ dnf info libhugetlbfs-devel | grep Desc Description : Contains header files for building with libhugetlbfs. >=20 > Imho, it's not necessary at this point. OK >=20 > > > >> + fprintf(stderr, "hugepage must be at least of size: %ld= \n", > >> + hpagesize); > >> + return -1; > >> + } > >> + filename =3D g_strdup_printf("%s/ivshmem.XXXXXX", server->s= hm_path); > >> + shm_fd =3D 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 >=20 > 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. >=20 > I'll make a followup rfc patch to fix shm_open() args in both server & = qemu >=20 > > execute perms, or do we? Also, I guess the plan is to pass the hugetl= bfs > > 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 s= hm? > > i.e. keep them anonymous too and always pass file descriptors? >=20 > it's already only passing fd, in any case Sounds good. Thanks, drew >=20 > >> + unlink(filename); > >> + g_free(filename); > >> + } else { > >> + shm_fd =3D shm_open(server->shm_path, O_CREAT|O_RDWR, S_IRW= XU); > >> + } > >> + > >> if (shm_fd < 0) { > >> fprintf(stderr, "cannot open shm file %s: %s\n", server->sh= m_path, > >> strerror(errno)); > >> diff --git a/contrib/ivshmem-server/ivshmem-server.h b/contrib/ivshm= em-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 str= ucture > >> * @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=3D%s\n", IVSHMEM_SERVER_DEFAULT_U= NIX_SOCK_PATH); > >> fprintf(stderr, " -m : 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, u= se\n" > >> - " /../../abspath/to/file.\n" > >> + " The path corresponds to a POSIX shm name = or a\n" > >> + " hugetlbfs mount point.\n" > >> " default=3D%s\n", IVSHMEM_SERVER_DEFAULT_S= HM_PATH); > >> fprintf(stderr, " -l : size of shared memory in bytes. T= he suffix\n" > >> " K, M and G can be used (ex: 1K means 1024= ).\n" > >> -- > >> 2.4.3 > >> > >> >=20 > thanks >=20 >=20 > --=20 > Marc-Andr=E9 Lureau >=20