linux-kernel - Re: [RFC PATCH] fs: use a sequence counter instead of file_lock in fd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1429206438.7346.204.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Thu, 16 Apr 2015 10:47:18 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Mateusz Guzik <mguzik@...hat.com>
Cc:	Alexander Viro <viro@...iv.linux.org.uk>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Yann Droneaud <ydroneaud@...eya.com>,
	Konstantin Khlebnikov <khlebnikov@...dex-team.ru>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] fs: use a sequence counter instead of file_lock in
 fd_install

On Thu, 2015-04-16 at 14:16 +0200, Mateusz Guzik wrote:
> Hi,
> 
> Currently obtaining a new file descriptor results in locking fdtable
> twice - once in order to reserve a slot and second time to fill it.
> 
> Hack below gets rid of the second lock usage.
> 
> It gives me a ~30% speedup (~300k ops -> ~400k ops) in a microbenchmark
> where 16 threads create a pipe (2 fds) and call 2 * close.
> 
> Results are fluctuating and even with the patch sometimes drop to around
> ~300k ops. However, without the patch they never get higher.
> 
> I can come up with a better benchmark if that's necessary.

Please push a patch with this test program alone, it will serve as a
baseline.

I discussed with Al about this problem in LKS 2014 in Chicago.

I am pleased to see you are working on this !

Please find one comment enclosed.

> 
> Comments?
> 
> ==============================================
> 
> Subject: [PATCH] fs: use a sequence counter instead of file_lock in fd_install
> 
> Because the lock is not held, it is possible that fdtable will be
> reallocated as we fill it.
> 
> RCU is used to guarantee the old table is not freed just in case we
> happen to write to it (which is harmless).
> 
> sequence counter is used to ensure we updated the right table.
> 
> Signed-off-by: Mateusz Guzik <mguzik@...hat.com>
> ---
>  fs/file.c               | 24 +++++++++++++++++++-----
>  include/linux/fdtable.h |  5 +++++
>  2 files changed, 24 insertions(+), 5 deletions(-)


>  void fd_install(unsigned int fd, struct file *file)
> diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
> index 230f87b..9e41765 100644
> --- a/include/linux/fdtable.h
> +++ b/include/linux/fdtable.h
> @@ -12,6 +12,7 @@
>  #include <linux/types.h>
>  #include <linux/init.h>
>  #include <linux/fs.h>
> +#include <linux/seqlock.h>
>  
>  #include <linux/atomic.h>
>  
> @@ -47,6 +48,7 @@ struct files_struct {
>     * read mostly part
>     */
>  	atomic_t count;
> +	seqcount_t fdt_seqcount;


You put fdt_seqcount in the 'read mostly part' of 'struct files_struct',
please move it in the 'written part'


>  	struct fdtable __rcu *fdt;
>  	struct fdtable fdtab;
>    /*
> @@ -69,6 +71,9 @@ struct dentry;
>  #define files_fdtable(files) \
>  	rcu_dereference_check_fdtable((files), (files)->fdt)
>  
> +#define files_fdtable_seq(files) \
> +	rcu_dereference((files)->fdt)
> +
>  /*
>   * The caller must ensure that fd table isn't shared or hold rcu or file lock
>   */


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/