[PATCH net-next] Performance improvements for socket/close and accept/close.

[PATCH net-next] Performance improvements for socket/close and accept/close.

[Maciej Żenczykowski]

Currently socket/close and accept/close require the acquisition of the
files lock 3 times
(twice in socket/accept, once in close), this patch series cuts this down to 2.

net-next performance on dual socket, 8 cores per socket, 2 HTs per
core, i.e. 32 hyperthreaded intel x86_64 machine:

prior to patch series:

843780 socket/close per sec (single thread)
629763 socket/close per sec (32 threaded process)

after patch series:

856149 socket/close per sec (single thread)
763282 socket/close per sec (32 threaded process)

- Maciej

[PATCH 1/8] net/socket: sock_map_fd - reverse error handling logic

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

functional no-op, but easier to read

Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 net/socket.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 95d3085cb477..323ebd530b88 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -397,13 +397,13 @@ static int sock_map_fd(struct socket *sock, int flags)
 		return fd;
 
 	newfile = sock_alloc_file(sock, flags, NULL);
-	if (likely(!IS_ERR(newfile))) {
-		fd_install(fd, newfile);
-		return fd;
+	if (unlikely(IS_ERR(newfile))) {
+		put_unused_fd(fd);
+		return PTR_ERR(newfile);
 	}
 
-	put_unused_fd(fd);
-	return PTR_ERR(newfile);
+	fd_install(fd, newfile);
+	return fd;
 }
 
 struct socket *sock_from_file(struct file *file, int *err)
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 2/8] net/socket: sock_map_fd - call sock_release in error path

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

(sock_map_fd only called from sys_socket)

Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 net/socket.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 323ebd530b88..8e282bae488d 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -393,12 +393,15 @@ static int sock_map_fd(struct socket *sock, int flags)
 {
 	struct file *newfile;
 	int fd = get_unused_fd_flags(flags);
-	if (unlikely(fd < 0))
+	if (unlikely(fd < 0)) {
+		sock_release(sock);
 		return fd;
+	}
 
 	newfile = sock_alloc_file(sock, flags, NULL);
 	if (unlikely(IS_ERR(newfile))) {
 		put_unused_fd(fd);
+		sock_release(sock);
 		return PTR_ERR(newfile);
 	}
 
@@ -1256,16 +1259,10 @@ SYSCALL_DEFINE3(socket, int, family, int, type, int, protocol)
 		goto out;
 
 	retval = sock_map_fd(sock, flags & (O_CLOEXEC | O_NONBLOCK));
-	if (retval < 0)
-		goto out_release;
 
 out:
 	/* It may be already another descriptor 8) Not kernel problem. */
 	return retval;
-
-out_release:
-	sock_release(sock);
-	return retval;
 }
 
 /*
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 3/8] net/socket: sock_map_fd - reverse allocation order

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 net/socket.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 8e282bae488d..a62c5cda5720 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -392,19 +392,20 @@ EXPORT_SYMBOL(sock_alloc_file);
 static int sock_map_fd(struct socket *sock, int flags)
 {
 	struct file *newfile;
-	int fd = get_unused_fd_flags(flags);
-	if (unlikely(fd < 0)) {
-		sock_release(sock);
-		return fd;
-	}
+	int fd;
 
 	newfile = sock_alloc_file(sock, flags, NULL);
 	if (unlikely(IS_ERR(newfile))) {
-		put_unused_fd(fd);
 		sock_release(sock);
 		return PTR_ERR(newfile);
 	}
 
+	fd = get_unused_fd_flags(flags);
+	if (unlikely(fd < 0)) {
+		fput(newfile);
+		return fd;
+	}
+
 	fd_install(fd, newfile);
 	return fd;
 }
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 4/8] net/socket: sys_accept4 - reverse allocation order

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 net/socket.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index a62c5cda5720..87fd36939778 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1473,38 +1473,37 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr,
 	 */
 	__module_get(newsock->ops->owner);
 
-	newfd = get_unused_fd_flags(flags);
-	if (unlikely(newfd < 0)) {
-		err = newfd;
-		sock_release(newsock);
-		goto out_put;
-	}
 	newfile = sock_alloc_file(newsock, flags, sock->sk->sk_prot_creator->name);
 	if (unlikely(IS_ERR(newfile))) {
 		err = PTR_ERR(newfile);
-		put_unused_fd(newfd);
 		sock_release(newsock);
 		goto out_put;
 	}
 
 	err = security_socket_accept(sock, newsock);
 	if (err)
-		goto out_fd;
+		goto out_fput;
 
 	err = sock->ops->accept(sock, newsock, sock->file->f_flags);
 	if (err < 0)
-		goto out_fd;
+		goto out_fput;
 
 	if (upeer_sockaddr) {
 		if (newsock->ops->getname(newsock, (struct sockaddr *)&address,
 					  &len, 2) < 0) {
 			err = -ECONNABORTED;
-			goto out_fd;
+			goto out_fput;
 		}
 		err = move_addr_to_user(&address,
 					len, upeer_sockaddr, upeer_addrlen);
 		if (err < 0)
-			goto out_fd;
+			goto out_fput;
+	}
+
+	newfd = get_unused_fd_flags(flags);
+	if (unlikely(newfd < 0)) {
+		err = newfd;
+		goto out_fput;
 	}
 
 	/* File flags are not inherited via accept() unlike another OSes. */
@@ -1516,9 +1515,8 @@ out_put:
 	fput_light(sock->file, fput_needed);
 out:
 	return err;
-out_fd:
+out_fput:
 	fput(newfile);
-	put_unused_fd(newfd);
 	goto out_put;
 }
 
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 5/8] fs/file: __alloc_fd takes extra argument: newfile

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 drivers/android/binder.c |  2 +-
 fs/file.c                | 10 ++++++----
 include/linux/fdtable.h  |  3 ++-
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 33b09b6568a4..75ad6b604f2a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -389,7 +389,7 @@ static int task_get_unused_fd_flags(struct binder_proc *proc, int flags)
 	rlim_cur = task_rlimit(proc->tsk, RLIMIT_NOFILE);
 	unlock_task_sighand(proc->tsk, &irqs);
 
-	return __alloc_fd(files, 0, rlim_cur, flags);
+	return __alloc_fd(files, 0, rlim_cur, flags, NULL);
 }
 
 /*
diff --git a/fs/file.c b/fs/file.c
index ee738ea028fa..01b1e171ce0a 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -443,7 +443,8 @@ struct files_struct init_files = {
  * allocate a file descriptor, mark it busy.
  */
 int __alloc_fd(struct files_struct *files,
-	       unsigned start, unsigned end, unsigned flags)
+	       unsigned start, unsigned end, unsigned flags,
+	       struct file *newfile)
 {
 	unsigned int fd;
 	int error;
@@ -491,9 +492,9 @@ repeat:
 	/* Sanity check */
 	if (rcu_access_pointer(fdt->fd[fd]) != NULL) {
 		printk(KERN_WARNING "alloc_fd: slot %d not NULL!\n", fd);
-		rcu_assign_pointer(fdt->fd[fd], NULL);
 	}
 #endif
+	rcu_assign_pointer(fdt->fd[fd], newfile);
 
 out:
 	spin_unlock(&files->file_lock);
@@ -502,12 +503,13 @@ out:
 
 static int alloc_fd(unsigned start, unsigned flags)
 {
-	return __alloc_fd(current->files, start, rlimit(RLIMIT_NOFILE), flags);
+	return __alloc_fd(current->files, start, rlimit(RLIMIT_NOFILE), flags,
+			  NULL);
 }
 
 int get_unused_fd_flags(unsigned flags)
 {
-	return __alloc_fd(current->files, 0, rlimit(RLIMIT_NOFILE), flags);
+	return __alloc_fd(current->files, 0, rlimit(RLIMIT_NOFILE), flags, NULL);
 }
 EXPORT_SYMBOL(get_unused_fd_flags);
 
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 230f87bdf5ad..a4e0a98750c3 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -107,7 +107,8 @@ int iterate_fd(struct files_struct *, unsigned,
 		const void *);
 
 extern int __alloc_fd(struct files_struct *files,
-		      unsigned start, unsigned end, unsigned flags);
+		      unsigned start, unsigned end, unsigned flags,
+		      struct file *newfile);
 extern void __fd_install(struct files_struct *files,
 		      unsigned int fd, struct file *file);
 extern int __close_fd(struct files_struct *files,
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 6/8] fs/file: add get_unused_fd_and_install_flags(flags, newfile) api

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 fs/file.c            | 7 +++++++
 include/linux/file.h | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/fs/file.c b/fs/file.c
index 01b1e171ce0a..d3e692e99ec5 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -513,6 +513,13 @@ int get_unused_fd_flags(unsigned flags)
 }
 EXPORT_SYMBOL(get_unused_fd_flags);
 
+int get_unused_fd_and_install_flags(unsigned flags, struct file *newfile)
+{
+	return __alloc_fd(current->files, 0, rlimit(RLIMIT_NOFILE), flags,
+			  newfile);
+}
+EXPORT_SYMBOL(get_unused_fd_and_install_flags);
+
 static void __put_unused_fd(struct files_struct *files, unsigned int fd)
 {
 	struct fdtable *fdt = files_fdtable(files);
diff --git a/include/linux/file.h b/include/linux/file.h
index f87d30882a24..92b43187aa39 100644
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -66,6 +66,8 @@ extern void set_close_on_exec(unsigned int fd, int flag);
 extern bool get_close_on_exec(unsigned int fd);
 extern void put_filp(struct file *);
 extern int get_unused_fd_flags(unsigned flags);
+extern int get_unused_fd_and_install_flags(unsigned flags,
+	struct file *newfile);
 extern void put_unused_fd(unsigned int fd);
 
 extern void fd_install(unsigned int fd, struct file *file);
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 7/8] net/socket: sock_map_fd - use faster fd allocation api

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 net/socket.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 87fd36939778..10f5daf0e625 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -400,13 +400,12 @@ static int sock_map_fd(struct socket *sock, int flags)
 		return PTR_ERR(newfile);
 	}
 
-	fd = get_unused_fd_flags(flags);
+	fd = get_unused_fd_and_install_flags(flags, newfile);
 	if (unlikely(fd < 0)) {
 		fput(newfile);
 		return fd;
 	}
 
-	fd_install(fd, newfile);
 	return fd;
 }
 
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 8/8] net/socket: sys_accept4 - use faster fd allocation api

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 net/socket.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 10f5daf0e625..f947c98831b2 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1445,7 +1445,7 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr,
 {
 	struct socket *sock, *newsock;
 	struct file *newfile;
-	int err, len, newfd, fput_needed;
+	int err, len, fput_needed;
 	struct sockaddr_storage address;
 
 	if (flags & ~(SOCK_CLOEXEC | SOCK_NONBLOCK))
@@ -1499,16 +1499,11 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr,
 			goto out_fput;
 	}
 
-	newfd = get_unused_fd_flags(flags);
-	if (unlikely(newfd < 0)) {
-		err = newfd;
-		goto out_fput;
-	}
-
 	/* File flags are not inherited via accept() unlike another OSes. */
 
-	fd_install(newfd, newfile);
-	err = newfd;
+	err = get_unused_fd_and_install_flags(flags, newfile);
+	if (unlikely(err < 0))
+		goto out_fput;
 
 out_put:
 	fput_light(sock->file, fput_needed);
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH 4/8] net/socket: sys_accept4 - reverse allocation order

[Eric Dumazet]

On Mon, 2015-03-09 at 14:19 -0700, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@google.com>
> 
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
> ---

Unfortunately this changes accept() behavior when/if ENFILE/EMFILE is
reported.

Some applications might depend on this behavior to eventually start a
garbage collection of a cache using file descriptors to regular files.

After your patch, a failed accept() call would silently 'consume' a
session in the listener queue.

Also, this patch series should be CCed to lkml and Al Viro, as it is a
mix of fs & network changes.

Re: [PATCH 4/8] net/socket: sys_accept4 - reverse allocation order

[Maciej Żenczykowski]

> Unfortunately this changes accept() behavior when/if ENFILE/EMFILE is
> reported.
>
> Some applications might depend on this behavior to eventually start a
> garbage collection of a cache using file descriptors to regular files.
>
> After your patch, a failed accept() call would silently 'consume' a
> session in the listener queue.

We could in the case of the first failure, attempt to grab a file
descriptor anyway, and if that fails return that failure instead, and
if it succeeds, release it and still return the first failure.

Would that be ok?

> Also, this patch series should be CCed to lkml and Al Viro, as it is a
> mix of fs & network changes.

Ok.

Re: [PATCH 3/8] net/socket: sock_map_fd - reverse allocation order

[David Miller]

From: Maciej Żenczykowski <zenczykowski@gmail.com>
Date: Mon,  9 Mar 2015 14:19:58 -0700

> From: Maciej Żenczykowski <maze@google.com>
> 
> Signed-off-by: Maciej Żenczykowski <maze@google.com>

The ordering of these operations was carefully choosen over the years
such that we prioritize "easy" things to allocate and recover from,
before harder things.

And allocating and releasing an 'fd' is much cheaper to deal with than
a socket file object.

And as has been shown in the accept() case, you're even changing user
visible semantics.

If you really want to reduce the amount of locking, create __*()
interfaces that assume the files lock is held already, and take
them around a batch of related operations.

By adding the file argument to the fs interfaces you making the caller
become a contortionist in order to fit into the interfaces
requirements.  Just let the callers do what they already do, which we
already know is correct and properly prioritized ordering wise, and
control the locking at a higher level.

Re: [PATCH 3/8] net/socket: sock_map_fd - reverse allocation order

[Eric Dumazet]

On Mon, 2015-03-09 at 23:56 -0400, David Miller wrote:
> From: Maciej Żenczykowski <zenczykowski@gmail.com>
> Date: Mon,  9 Mar 2015 14:19:58 -0700
> 
> > From: Maciej Żenczykowski <maze@google.com>
> > 
> > Signed-off-by: Maciej Żenczykowski <maze@google.com>
> 
> The ordering of these operations was carefully choosen over the years
> such that we prioritize "easy" things to allocate and recover from,
> before harder things.
> 
> And allocating and releasing an 'fd' is much cheaper to deal with than
> a socket file object.
> 
> And as has been shown in the accept() case, you're even changing user
> visible semantics.
> 
> If you really want to reduce the amount of locking, create __*()
> interfaces that assume the files lock is held already, and take
> them around a batch of related operations.
> 
> By adding the file argument to the fs interfaces you making the caller
> become a contortionist in order to fit into the interfaces
> requirements.  Just let the callers do what they already do, which we
> already know is correct and properly prioritized ordering wise, and
> control the locking at a higher level.

In accept() case, the socket operation can block, so we cannot hold the
lock.

Ideally, as discussed with Al Viro in last Kernel Summit, we would need
to convert the file descriptor management to use RCU, and atomic
bit ops. That would be a very tricky change.