reducing overheads in fget/fput
PATCH 1/1 reducing overheads in fget/fput
Sometime in the recent past there were complaints about smp overheads doubling system time taken for a 1MB "dd" from /dev/zero. This
path is very sensitive to fget()/fput() costs predominantly from acquiring the rwlock in files_struct and atomic increment/decrement of file->f_count (I experimented to determine this). It seems that this can be avoided in some of the most common cases resulting in significant benefits.
The idea is that if we are not sharing the file descriptor table, there is no need to increase the struct file refcnt or even take the files_struct reader-writer lock while lookup up an fd. open/close gurantees that atleast one refcount for file struct is held and while forking (but not sharing fd table), an additional refcount is added for the child for each open fd. By avoiding both the rwlock and atomic operation on file->f_count, we shave off about 0.35 seconds of system time from the following simple test -
time dd if=/dev/zero of=foo bs=1 count=1M
Results on a 4-CPU P3 Xeon box with 512MB memory 1MB L2 cache - (sys time in seconds)
kernel sys time std-dev ------------ -------- ------- UP - vanilla 2.032 0.018 SMP - vanilla 2.996 0.013 UP - file 1.991 0.034 SMP - file 2.646 0.042
I have included my experimental patch here (not meant for inclusion). I added fget_light() and fput_light() which avoid the rwlock and ->f_count if the fd table is not shared. It seems safe provided you are using fget_light() and fput_light() within the same system call and are not forking after an fget_light(). To test it I modified some common syscalls in fs/read_write.c.
It of course doesn't optimize the case when we share fd tables using CLONE_FILES. That case can probably be optimized using an old RCU-based patch that we have.
Does this approach make sense ? Or am I missing some big gotcha somewhere ? Oh, btw the patch survives LTP.
Thanks
Dipankar
fs/file_table.c +32 -0
@@ -147,6 +147,15 @@__fput (file); } +void fput_light (struct file * file) +{ +struct files_struct *files = current->files; + +if (unlikely (atomic_read (&files->count) !=1 )) +if (atomic_dec_and_test (&file->f_count)) +__fput (file); +} +/* __fput is called from task context when aio completion releases the last * last use of a struct file *. Do not use otherwise. */
@@ -190,6 +199,29 @@return file; } +/* +* Lightweight file lookup - no refcnt increment if fd table isn't shared. +* You can use this only if it is guranteed that the current task already +* holds a refcnt to that file +*/ +struct file *fget_light (unsigned int fd) +{ +struct file *file; +struct files_struct *files = current->files; + +if (likely ((atomic_read (&files->count) ==1 ))) { + file =fcheck (fd); + }else { +read_lock (&files->file_lock); + file =fcheck (fd); +if (file) +get_file (file); +read_unlock (&files->file_lock); + } +return file; +} + +void put_filp (struct file *file) {if (atomic_dec_and_test (&file->f_count)) {
fs/read_write.c +20 -20
@@ -117,7 +117,7 @@struct file * file; retval = -EBADF ; - file =fget (fd); + file =fget_light (fd);if (!file) goto bad;
@@ -128,7 +128,7 @@if (res != (loff_t )retval) retval = -EOVERFLOW ;/* LFS: should only happen on 32 bit platforms */ } -fput (file); +fput_light (file); bad:return retval; }
@@ -143,7 +143,7 @@loff_t offset; retval = -EBADF ; - file =fget (fd); + file =fget_light (fd);if (!file) goto bad;
@@ -161,7 +161,7 @@ retval =0 ; } out_putf: -fput (file); +fput_light (file); bad:return retval; }
@@ -252,10 +252,10 @@struct file *file;ssize_t ret = -EBADF ; - file =fget (fd); + file =fget_light (fd);if (file) { ret =vfs_read (file, buf, count, &file->f_pos); -fput (file); +fput_light (file); }return ret;
@@ -266,10 +266,10 @@struct file *file;ssize_t ret = -EBADF ; - file =fget (fd); + file =fget_light (fd);if (file) { ret =vfs_write (file, buf, count, &file->f_pos); -fput (file); +fput_light (file); }return ret;
@@ -284,10 +284,10 @@if (pos <0 )return -EINVAL ; - file =fget (fd); + file =fget_light (fd);if (file) { ret =vfs_read (file, buf, count, &pos); -fput (file); +fput_light (file); }return ret;
@@ -302,10 +302,10 @@if (pos <0 )return -EINVAL ; - file =fget (fd); + file =fget_light (fd);if (file) { ret =vfs_write (file, buf, count, &pos); -fput (file); +fput_light (file); }return ret;
@@ -480,10 +480,10 @@struct file *file;ssize_t ret = -EBADF ; - file =fget (fd); + file =fget_light (fd);if (file) { ret =vfs_readv (file, vec, vlen, &file->f_pos); -fput (file); +fput_light (file); }return ret;
@@ -495,10 +495,10 @@struct file *file;ssize_t ret = -EBADF ; - file =fget (fd); + file =fget_light (fd);if (file) { ret =vfs_writev (file, vec, vlen, &file->f_pos); -fput (file); +fput_light (file); }return ret;
@@ -516,7 +516,7 @@* Get input file, and verify that it is ok.. */ retval = -EBADF ; - in_file =fget (in_fd); + in_file =fget_light (in_fd);if (!in_file) goto out;if (!(in_file->f_mode &FMODE_READ ))
@@ -539,7 +539,7 @@* Get output file, and verify that it is ok.. */ retval = -EBADF ; - out_file =fget (out_fd); + out_file =fget_light (out_fd);if (!out_file) goto fput_in;if (!(out_file->f_mode &FMODE_WRITE ))
@@ -579,9 +579,9 @@ retval = -EOVERFLOW ; fput_out: -fput (out_file); +fput_light (out_file); fput_in: -fput (in_file); +fput_light (in_file); out:return retval; }
include/linux/file.h +2 -0
@@ -35,7 +35,9 @@extern void FASTCALL (__fput (struct file *));extern void FASTCALL (fput (struct file *)); +extern void FASTCALL (fput_light (struct file *));extern struct file *FASTCALL (fget (unsigned int fd)); +extern struct file *FASTCALL (fget_light (unsigned int fd));extern void FASTCALL (set_close_on_exec (unsigned int fd,int flag));extern void put_filp (struct file *);extern int get_unused_fd (void );
Quoted 2 lines · Dipankar Sarma
Does this approach make sense ? Or am I missing some big gotcha somewhere ? Oh, btw the patch survives LTP.
You are. Have a process share file table at the time of call and have its sibling die in the middle. Oops - condition that had been true at time of fget_light() (->files->count > 1) is false at the time we fput_light(). Have fun - we had just leaked a reference to struct file.
Quoted 5 lines · On Mon, Apr 28, 2003 at 08:32:28PM +0100…
You are. Have a process share file table at the time of call and have its sibling die in the middle. Oops - condition that had been true at time of
fget_light()(->files->count > 1) is false at the time wefput_light(). Have fun - we had just leaked a reference to struct file.
That shouldn't be very difficult to fix. For the fget_light/fput_light pair in a syscall, we make the files->count == 1 check only once at the beginning. Do you see a problem with that ?
Thanks
Dipankar
Quoted 10 lines · Dipankar Sarma
Quoted 5 lines · On Mon, Apr 28, 2003 at 08:32:28PM +0100…
You are. Have a process share file table at the time of call and have its sibling die in the middle. Oops - condition that had been true at time of
fget_light()(->files->count > 1) is false at the time wefput_light(). Have fun - we had just leaked a reference to struct file.That shouldn't be very difficult to fix. For the fget_light/fput_light pair in a syscall, we make the
files->count== 1 check only once at the beginning. Do you see a problem with that ?
Here is a patch that fixes that. I re-did the measurements with Andrew's experiment (dd if=/dev/zero of=foo bs=1 count=1M). [4CPU P3 xeon with 1MB L2 cache and 512MB ram]
kernel sys time std-dev ------------ -------- ------- UP - vanilla 2.104 0.028 SMP - vanilla 2.976 0.023 UP - file 1.867 0.019 SMP - file 2.719 0.026
Thanks
Dipankar
fs/file_table.c +35 -0
@@ -147,6 +147,13 @@__fput (file); } +void fput_light (struct file * file,int flag) +{ +if (unlikely (flag)) +if (atomic_dec_and_test (&file->f_count)) +__fput (file); +} +/* __fput is called from task context when aio completion releases the last * last use of a struct file *. Do not use otherwise. */
@@ -190,6 +197,34 @@return file; } +/* +* Lightweight file lookup - no refcnt increment if fd table isn't shared. +* You can use this only if it is guranteed that the current task already +* holds a refcnt to that file. That check has to be done at fget() only +* and a flag is returned to be passed to the corresponding fput_light(). +* There must not be a cloning between an fget_light/fput_light pair. +*/ +struct file *fget_light (unsigned int fd,int *flag) +{ +struct file *file; +struct files_struct *files = current->files; + + *flag =0 ; +if (likely ((atomic_read (&files->count) ==1 ))) { + file =fcheck (fd); + }else { +read_lock (&files->file_lock); + file =fcheck (fd); +if (file) { +get_file (file); + *flag =1 ; + } +read_unlock (&files->file_lock); + } +return file; +} + +void put_filp (struct file *file) {if (atomic_dec_and_test (&file->f_count)) {
fs/read_write.c +29 -20
161 lines not on this page · raw message
include/linux/file.h +2 -0
@@ -35,7 +35,9 @@extern void FASTCALL (__fput (struct file *));extern void FASTCALL (fput (struct file *)); +extern void FASTCALL (fput_light (struct file *,int ));extern struct file *FASTCALL (fget (unsigned int fd)); +extern struct file *FASTCALL (fget_light (unsigned int fd,int *flag));extern void FASTCALL (set_close_on_exec (unsigned int fd,int flag));extern void put_filp (struct file *);extern int get_unused_fd (void );
Quoted 6 lines · Dipankar Sarma
Quoted 4 lines
That shouldn't be very difficult to fix. For the fget_light/fput_light pair in a syscall, we make the
files->count== 1 check only once at the beginning. Do you see a problem with that ?Here is a patch that fixes that.
This patch is fairly foul.
Quoted 6 lines
kernel sys time std-dev ------------ -------- ------- UP - vanilla 2.104 0.028 SMP - vanilla 2.976 0.023 UP - file 1.867 0.019 SMP - file 2.719 0.026
But it is localised, and makes a substantial difference.
I inlined fput_light:
fs/file_table.c renamed from fs/file_table.c~fget-speedup-inline-fput_light +1 -8
@@ -141,19 +141,12 @@ void close_private_file(struct file *filsecurity_file_free (file); } -void fput (struct file * file) +void fput (struct file *file) {if (atomic_dec_and_test (&file->f_count))__fput (file); } -void fput_light (struct file * file,int flag) -{ -if (unlikely (flag)) -if (atomic_dec_and_test (&file->f_count)) -__fput (file); -} -/* __fput is called from task context when aio completion releases the last * last use of a struct file *. Do not use otherwise. */
include/linux/file.h renamed from include/linux/file.h~fget-speedup-inline-fput_light +7 -1
@@ -35,7 +35,13 @@ struct files_struct {extern void FASTCALL (__fput (struct file *));extern void FASTCALL (fput (struct file *)); -extern void FASTCALL (fput_light (struct file *,int )); + +static inline void fput_light (struct file *file,int flag) +{ +if (unlikely (flag)) +fput (file); +} +extern struct file *FASTCALL (fget (unsigned int fd));extern struct file *FASTCALL (fget_light (unsigned int fd,int *flag));extern void FASTCALL (set_close_on_exec (unsigned int fd,int flag));
_
Quoted 4 lines · Andrew Morton
Quoted 1 line · Dipankar Sarma
Here is a patch that fixes that.
This patch is fairly foul.
Not sure what your grouse is, but I don't like the fget_ligth()/fput_light semantics myself. They don't seem natural, but I can't think of better way to do this.
Quoted 9 lines
Quoted 7 lines
kernel sys time std-dev ------------ -------- ------- UP - vanilla 2.104 0.028 SMP - vanilla 2.976 0.023 UP - file 1.867 0.019 SMP - file 2.719 0.026But it is localised, and makes a substantial difference.
If I haven't broken too many things, then I will try to get some performance results from large machines.
Quoted 2 lines
I inlined
fput_light:
Looks good.
Thanks
Dipankar
Quoted 10 lines · Dipankar Sarma
Quoted 4 lines · Andrew Morton
Quoted 1 line · Dipankar Sarma
Here is a patch that fixes that.
This patch is fairly foul.
Not sure what your grouse is, but I don't like the fget_ligth()/fput_light semantics myself. They don't seem natural, but I can't think of better way to do this.
Precisely.
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
kernel sys time std-dev ------------ -------- ------- UP - vanilla 2.104 0.028 SMP - vanilla 2.976 0.023 UP - file 1.867 0.019 SMP - file 2.719 0.026
Quoted 4 lines
Quoted 1 line
But it is localised, and makes a substantial difference.
If I haven't broken too many things, then I will try to get some performance results from large machines.
P4's will like it more.
Quoted 4 lines
Quoted 2 lines
I inlined
fput_light:Looks good.
I also renamed flag' to fput_needed' everywhere.
Quoted 6 lines · Andrew Morton
Quoted 3 lines · Dipankar Sarma
Not sure what your grouse is, but I don't like the fget_ligth()/fput_light semantics myself. They don't seem natural, but I can't think of better way to do this.
Precisely.
I thought of doing something like -
static inline is_fds_shared(void)
{
struct files_struct *files = current->files;
return atomic_read(&files->count) != 1;
}sys_blah(..)
{
int fds_shared = is_fds_shared(); file = fget_light(fd, fds_shared); ... ... fput_light(file, fds_shared); }
It still didn't look very natural. We leave open the possibility of users doing is_fds_shared() for both fget_light and fput_light. With fput_needed flag, atleast we force them to use what is returned by fget_light.
Thanks
Dipankar