Messages in this thread | | | Date | Fri, 10 Jan 2014 16:34:59 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 0/2] rcu_dereference_check_fdtable fix/cleanups |
| |
On 01/08, Paul E. McKenney wrote: > > On Wed, Jan 08, 2014 at 04:19:18PM +0100, Oleg Nesterov wrote: > > On 01/08, Paul E. McKenney wrote: > > > > > > 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?
But only one user, close_files(), needs files_fdtable(files, true). Why complicate the patch and the code? I think it would be better to simply change close_files() to use rcu_dereference_raw().
And note that rcu_dereference_check_fdtable() needs the new argument too.
And we should also take care of fcheck_files(),
> > 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); > > }
doesn't this look much simpler than adding the "bool unshared" argument and changing the callers?
Oleg.
| |