lkml.org 
[lkml]   [2014]   [Jan]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 0/2] rcu_dereference_check_fdtable fix/cleanups
On Wed, Jan 08, 2014 at 04:19:18PM +0100, Oleg Nesterov wrote:
> On 01/08, Paul E. McKenney wrote:
> >
> > I am not all that excited about invoking rcu_lock_acquire() outside
> > of RCU...
>
> Yes, me too. That is why I thought about the helper with a good name,
> see below.
>
> > Another approach would be to add an argument to files_fdtable()
> > that is zero normally and one for "we know we don't need RCU
> > protection." Then rcu_dereference_check() could be something
> > like the following:
> >
> > #define files_fdtable(files, c) \
> > (rcu_dereference_check_fdtable((files), (files)->fdt) || c)
> >
> > Would that work?
>
> Yes, I considered this optiion, but this needs much more uglifications^W
> changes.
>
> Either we need to change all users of files_fdtable(), or we need something
> like

There are only about 20 uses of files_fdtable() in 3.12, with almost all
of them in fs/file.c. So is changing all the users really all that
problematic?

Thanx, Paul

> #define __rcu_dereference_check_fdtable(files, unshared, fdtfd) \
> rcu_dereference_check((fdtfd), unshared || lockdep_is_held(&(files)->file_lock))
>
> #define rcu_dereference_check_fdtable(files, fdtfd)
> __rcu_dereference_check_fdtable(files, false, fdtfd)
>
> #define __files_fdtable(files)
> __rcu_dereference_check_fdtable((files), true, (files)->fdt)
>
> #define files_fdtable(files)
> __rcu_dereference_check_fdtable((files), false, (files)->fdt)
>
> Plus we need
>
> static inline struct file *__fcheck_files(struct files_struct *files,
> bool unshared, unsigned int fd)
> {
> struct file *file = NULL;
> struct fdtable *fdt = __rcu_dereference_check_fdtable(files, unshared, files->fdt);
>
> if (fd < fdt->max_fds)
> file = __rcu_dereference_check_fdtable(files, unshared, fdt->fd[fd]);
> return file;
> }
>
> doesn't look very nice...
>
> As for 2/2, probably close_files() can simply do
>
> /*
> * It is safe to dereference the fd table without RCU or
> * ->file_lock because this is the last reference to the
> * files structure.
> */
> fdt = rcu_dereference_raw(files->fdt);
>
> Or we can add
>
> #define __files_fdtable(files) \
> rcu_dereference_raw((files)->fdt)
>
> but it is not clear to me what 1/1 should do. Perhaps
>
> static inline struct file *__fcheck_files(struct files_struct *files, unsigned int fd)
> {
> struct fdtable *fdt = rcu_dereference_raw(files->fdt);
> struct file *file = NULL;
>
> if (fd < fdt->max_fds)
> file = rcu_dereference_raw(fdt->fd[fd]);
>
> return file;
> }
>
> static inline struct file *fcheck_files(struct files_struct *files, unsigned int fd)
> {
> rcu_lockdep_assert(rcu_read_lock_held() ||
> lockdep_is_held(files->file_lock),
> "message");
> return __fcheck_files(files, fd);
> }
>
> ?
>
> Oleg.
>



\
 
 \ /
  Last update: 2014-01-10 00:21    [W:0.084 / U:0.260 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site