pie: Optimize send_fds() and recv_fds() with opts

Submitted by Kirill Tkhai on Oct. 27, 2016, 5:59 p.m.

Details

Message ID 147759114878.30090.12107482473438957995.stgit@localhost.localdomain
State Superseded
Series "pie: Optimize send_fds() and recv_fds() with opts"
Headers show

Commit Message

Kirill Tkhai Oct. 27, 2016, 5:59 p.m.
Do not ask kernel to transfer more opts than we really need.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 criu/pie/util-fd.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/pie/util-fd.c b/criu/pie/util-fd.c
index 51696ee..f3f9c2e 100644
--- a/criu/pie/util-fd.c
+++ b/criu/pie/util-fd.c
@@ -21,7 +21,7 @@ 
 
 #include "common/bug.h"
 
-static void scm_fdset_init_chunk(struct scm_fdset *fdset, int nr_fds)
+static void scm_fdset_init_chunk(struct scm_fdset *fdset, int nr_fds, bool with_flags)
 {
 	struct cmsghdr *cmsg;
 
@@ -29,6 +29,8 @@  static void scm_fdset_init_chunk(struct scm_fdset *fdset, int nr_fds)
 
 	cmsg		= CMSG_FIRSTHDR(&fdset->hdr);
 	cmsg->cmsg_len	= fdset->hdr.msg_controllen;
+
+	fdset->iov.iov_len = with_flags ? (sizeof(struct fd_opts) * nr_fds) : 1;
 }
 
 static int *scm_fdset_init(struct scm_fdset *fdset, struct sockaddr_un *saddr,
@@ -39,7 +41,6 @@  static int *scm_fdset_init(struct scm_fdset *fdset, struct sockaddr_un *saddr,
 	BUILD_BUG_ON(sizeof(fdset->msg_buf) < (CMSG_SPACE(sizeof(int) * CR_SCM_MAX_FD)));
 
 	fdset->iov.iov_base		= fdset->opts;
-	fdset->iov.iov_len		= with_flags ? sizeof(fdset->opts) : 1;
 
 	fdset->hdr.msg_iov		= &fdset->iov;
 	fdset->hdr.msg_iovlen		= 1;
@@ -67,7 +68,7 @@  int send_fds(int sock, struct sockaddr_un *saddr, int len,
 	cmsg_data = scm_fdset_init(&fdset, saddr, len, with_flags);
 	for (i = 0; i < nr_fds; i += min_fd) {
 		min_fd = min(CR_SCM_MAX_FD, nr_fds - i);
-		scm_fdset_init_chunk(&fdset, min_fd);
+		scm_fdset_init_chunk(&fdset, min_fd, with_flags);
 		builtin_memcpy(cmsg_data, &fds[i], sizeof(int) * min_fd);
 
 		if (with_flags) {
@@ -133,7 +134,7 @@  int recv_fds(int sock, int *fds, int nr_fds, struct fd_opts *opts)
 	cmsg_data = scm_fdset_init(&fdset, NULL, 0, opts != NULL);
 	for (i = 0; i < nr_fds; i += min_fd) {
 		min_fd = min(CR_SCM_MAX_FD, nr_fds - i);
-		scm_fdset_init_chunk(&fdset, min_fd);
+		scm_fdset_init_chunk(&fdset, min_fd, opts != NULL);
 
 		ret = __sys(recvmsg)(sock, &fdset.hdr, 0);
 		if (ret <= 0)

Comments

Pavel Emelianov Nov. 1, 2016, 11:36 p.m.
On 10/27/2016 08:59 PM, Kirill Tkhai wrote:
> Do not ask kernel to transfer more opts than we really need.

Can you write some more details here, please.

> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  criu/pie/util-fd.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/criu/pie/util-fd.c b/criu/pie/util-fd.c
> index 51696ee..f3f9c2e 100644
> --- a/criu/pie/util-fd.c
> +++ b/criu/pie/util-fd.c
> @@ -21,7 +21,7 @@
>  
>  #include "common/bug.h"
>  
> -static void scm_fdset_init_chunk(struct scm_fdset *fdset, int nr_fds)
> +static void scm_fdset_init_chunk(struct scm_fdset *fdset, int nr_fds, bool with_flags)
>  {
>  	struct cmsghdr *cmsg;
>  
> @@ -29,6 +29,8 @@ static void scm_fdset_init_chunk(struct scm_fdset *fdset, int nr_fds)
>  
>  	cmsg		= CMSG_FIRSTHDR(&fdset->hdr);
>  	cmsg->cmsg_len	= fdset->hdr.msg_controllen;
> +
> +	fdset->iov.iov_len = with_flags ? (sizeof(struct fd_opts) * nr_fds) : 1;
>  }
>  
>  static int *scm_fdset_init(struct scm_fdset *fdset, struct sockaddr_un *saddr,
> @@ -39,7 +41,6 @@ static int *scm_fdset_init(struct scm_fdset *fdset, struct sockaddr_un *saddr,
>  	BUILD_BUG_ON(sizeof(fdset->msg_buf) < (CMSG_SPACE(sizeof(int) * CR_SCM_MAX_FD)));
>  
>  	fdset->iov.iov_base		= fdset->opts;
> -	fdset->iov.iov_len		= with_flags ? sizeof(fdset->opts) : 1;
>  
>  	fdset->hdr.msg_iov		= &fdset->iov;
>  	fdset->hdr.msg_iovlen		= 1;
> @@ -67,7 +68,7 @@ int send_fds(int sock, struct sockaddr_un *saddr, int len,
>  	cmsg_data = scm_fdset_init(&fdset, saddr, len, with_flags);
>  	for (i = 0; i < nr_fds; i += min_fd) {
>  		min_fd = min(CR_SCM_MAX_FD, nr_fds - i);
> -		scm_fdset_init_chunk(&fdset, min_fd);
> +		scm_fdset_init_chunk(&fdset, min_fd, with_flags);
>  		builtin_memcpy(cmsg_data, &fds[i], sizeof(int) * min_fd);
>  
>  		if (with_flags) {
> @@ -133,7 +134,7 @@ int recv_fds(int sock, int *fds, int nr_fds, struct fd_opts *opts)
>  	cmsg_data = scm_fdset_init(&fdset, NULL, 0, opts != NULL);
>  	for (i = 0; i < nr_fds; i += min_fd) {
>  		min_fd = min(CR_SCM_MAX_FD, nr_fds - i);
> -		scm_fdset_init_chunk(&fdset, min_fd);
> +		scm_fdset_init_chunk(&fdset, min_fd, opts != NULL);
>  
>  		ret = __sys(recvmsg)(sock, &fdset.hdr, 0);
>  		if (ret <= 0)
> 
> .
>
Kirill Tkhai Nov. 2, 2016, 9:33 a.m.
On 02.11.2016 02:36, Pavel Emelyanov wrote:
> On 10/27/2016 08:59 PM, Kirill Tkhai wrote:
>> Do not ask kernel to transfer more opts than we really need.
> 
> Can you write some more details here, please.

When we're sending fds with flags, we ask kernel to copy the whole
struct scm_fdset::opts array, like we'd send CR_SCM_MAX_FD fds,
even if really we're transmitting only one fd.
send_fds() does not initializes the rest of array memory, but kernel
transmits this garbage. Also, recv_msg() does not return it to userspace.

This patch makes kernel do not transmit uninitialized garbage.
 
>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> ---
>>  criu/pie/util-fd.c |    9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/criu/pie/util-fd.c b/criu/pie/util-fd.c
>> index 51696ee..f3f9c2e 100644
>> --- a/criu/pie/util-fd.c
>> +++ b/criu/pie/util-fd.c
>> @@ -21,7 +21,7 @@
>>  
>>  #include "common/bug.h"
>>  
>> -static void scm_fdset_init_chunk(struct scm_fdset *fdset, int nr_fds)
>> +static void scm_fdset_init_chunk(struct scm_fdset *fdset, int nr_fds, bool with_flags)
>>  {
>>  	struct cmsghdr *cmsg;
>>  
>> @@ -29,6 +29,8 @@ static void scm_fdset_init_chunk(struct scm_fdset *fdset, int nr_fds)
>>  
>>  	cmsg		= CMSG_FIRSTHDR(&fdset->hdr);
>>  	cmsg->cmsg_len	= fdset->hdr.msg_controllen;
>> +
>> +	fdset->iov.iov_len = with_flags ? (sizeof(struct fd_opts) * nr_fds) : 1;
>>  }
>>  
>>  static int *scm_fdset_init(struct scm_fdset *fdset, struct sockaddr_un *saddr,
>> @@ -39,7 +41,6 @@ static int *scm_fdset_init(struct scm_fdset *fdset, struct sockaddr_un *saddr,
>>  	BUILD_BUG_ON(sizeof(fdset->msg_buf) < (CMSG_SPACE(sizeof(int) * CR_SCM_MAX_FD)));
>>  
>>  	fdset->iov.iov_base		= fdset->opts;
>> -	fdset->iov.iov_len		= with_flags ? sizeof(fdset->opts) : 1;
>>  
>>  	fdset->hdr.msg_iov		= &fdset->iov;
>>  	fdset->hdr.msg_iovlen		= 1;
>> @@ -67,7 +68,7 @@ int send_fds(int sock, struct sockaddr_un *saddr, int len,
>>  	cmsg_data = scm_fdset_init(&fdset, saddr, len, with_flags);
>>  	for (i = 0; i < nr_fds; i += min_fd) {
>>  		min_fd = min(CR_SCM_MAX_FD, nr_fds - i);
>> -		scm_fdset_init_chunk(&fdset, min_fd);
>> +		scm_fdset_init_chunk(&fdset, min_fd, with_flags);
>>  		builtin_memcpy(cmsg_data, &fds[i], sizeof(int) * min_fd);
>>  
>>  		if (with_flags) {
>> @@ -133,7 +134,7 @@ int recv_fds(int sock, int *fds, int nr_fds, struct fd_opts *opts)
>>  	cmsg_data = scm_fdset_init(&fdset, NULL, 0, opts != NULL);
>>  	for (i = 0; i < nr_fds; i += min_fd) {
>>  		min_fd = min(CR_SCM_MAX_FD, nr_fds - i);
>> -		scm_fdset_init_chunk(&fdset, min_fd);
>> +		scm_fdset_init_chunk(&fdset, min_fd, opts != NULL);
>>  
>>  		ret = __sys(recvmsg)(sock, &fdset.hdr, 0);
>>  		if (ret <= 0)
>>
>> .
>>
>
Pavel Emelianov Nov. 3, 2016, 4:53 a.m.
Applied