Performance improvements for socket/close and accept/close.
Subsystems (5)
PATCH 1/1 Performance improvements for socket/close and accept/close.
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
From: Maciej Żenczykowski <[email protected]>
functional no-op, but easier to read
--- net/socket.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
net/socket.c +5 -5
@@ -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)
Signature · 1 line
2.2.0.rc0.207.ga3a616c
PATCH 2/8 net/socket: sock_map_fd - call sock_release in error path
From: Maciej Żenczykowski <[email protected]>
(sock_map_fd only called from sys_socket)
--- net/socket.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
net/socket.c +4 -7
@@ -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; }/*
Signature · 1 line
2.2.0.rc0.207.ga3a616c
PATCH 3/8 net/socket: sock_map_fd - reverse allocation order
In reply to Maciej ŻenczykowskiFrom: Maciej Żenczykowski <[email protected]>
--- net/socket.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
net/socket.c +7 -6
@@ -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; }
Signature · 1 line
2.2.0.rc0.207.ga3a616c
David Miller
maintainerRe: [PATCH 3/8] net/socket: sock_map_fd - reverse allocation order
From: Maciej Żenczykowski <[email protected]>
Date: Mon, 9 Mar 2015 14:19:58 -0700
Quoted 3 lines
From: Maciej Żenczykowski <[email protected]>
Signed-off-by Maciej Żenczykowski <[email protected]>
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
Quoted 21 lines · David Miller
From: Maciej Żenczykowski <[email protected]>
Date: Mon, 9 Mar 2015 14:19:58 -0700Quoted 3 lines
From: Maciej Żenczykowski <[email protected]>
Signed-off-by Maciej Żenczykowski <[email protected]>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.
Quoted 5 lines
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.
PATCH 4/8 net/socket: sys_accept4 - reverse allocation order
In reply to Maciej ŻenczykowskiFrom: Maciej Żenczykowski <[email protected]>
--- net/socket.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)
net/socket.c +11 -13
@@ -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; }
Signature · 1 line
2.2.0.rc0.207.ga3a616c
Re: [PATCH 4/8] net/socket: sys_accept4 - reverse allocation order
Quoted 4 lines · Maciej Żenczykowski
From: Maciej Żenczykowski <[email protected]>
Signed-off-by Maciej Żenczykowski <[email protected]>---
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
Quoted 8 lines
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?
Quoted 2 lines
Also, this patch series should be CCed to lkml and Al Viro, as it is a mix of fs & network changes.
Ok.
PATCH 5/8 fs/file: __alloc_fd takes extra argument: newfile
In reply to Maciej ŻenczykowskiFrom: Maciej Żenczykowski <[email protected]>
--- drivers/android/binder.c | 2 +- fs/file.c | 10 ++++++---- include/linux/fdtable.h | 3 ++- 3 files changed, 9 insertions(+), 6 deletions(-)
drivers/android/binder.c +1 -1
@@ -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 ); }/*
fs/file.c +6 -4
@@ -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);
include/linux/fdtable.h +2 -1
@@ -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,
Signature · 1 line
2.2.0.rc0.207.ga3a616c
PATCH 6/8 fs/file: add get_unused_fd_and_install_flags(flags, newfile) api
In reply to Maciej ŻenczykowskiFrom: Maciej Żenczykowski <[email protected]>
--- fs/file.c | 7 +++++++ include/linux/file.h | 2 ++ 2 files changed, 9 insertions(+)
fs/file.c +7 -0
@@ -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);
include/linux/file.h +2 -0
@@ -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);
Signature · 1 line
2.2.0.rc0.207.ga3a616c
PATCH 7/8 net/socket: sock_map_fd - use faster fd allocation api
In reply to Maciej ŻenczykowskiFrom: Maciej Żenczykowski <[email protected]>
--- net/socket.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
net/socket.c +1 -2
@@ -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; }
Signature · 1 line
2.2.0.rc0.207.ga3a616c
PATCH 8/8 net/socket: sys_accept4 - use faster fd allocation api
In reply to Maciej ŻenczykowskiFrom: Maciej Żenczykowski <[email protected]>
--- net/socket.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
net/socket.c +4 -9
@@ -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);
Signature · 1 line
2.2.0.rc0.207.ga3a616c