[PATCH 2/2] change close_files() to use rcu_lock_acquire() to shut up RCU-lockdep

[Oleg Nesterov]

put_files_struct() and close_files() do rcu_read_lock() to make
rcu_dereference_check_fdtable() happy.

We can use rcu_lock_acquire(&rcu_lock_map) instead, this looks
self-explanatory and this is nop without CONFIG_DEBUG_LOCK_ALLOC().

While at it, change close_files() to return fdt, this avoids another
rcu_read_lock() + files_fdtable() in put_files_struct().

Signed-off-by: Oleg Nesterov <ol...@redhat.com>
---
fs/file.c | 20 +++++++++-----------
1 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 957cbc0..716d747 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -348,7 +348,7 @@ out:
return NULL;
}

-static void close_files(struct files_struct * files)
+static struct fdtable *close_files(struct files_struct * files)
{
int i, j;
struct fdtable *fdt;
@@ -358,11 +358,12 @@ static void close_files(struct files_struct * files)
/*
* It is safe to dereference the fd table without RCU or
* ->file_lock because this is the last reference to the
- * files structure. But use RCU to shut RCU-lockdep up.
+ * files structure. But we need to shut RCU-lockdep up.
*/
- rcu_read_lock();
+ rcu_lock_acquire(&rcu_lock_map);
fdt = files_fdtable(files);
- rcu_read_unlock();
+ rcu_lock_release(&rcu_lock_map);
+
for (;;) {
unsigned long set;
i = j * BITS_PER_LONG;
@@ -381,6 +382,8 @@ static void close_files(struct files_struct * files)
set >>= 1;
}
}
+
+ return fdt;
}

struct files_struct *get_files_struct(struct task_struct *task)
@@ -398,14 +401,9 @@ struct files_struct *get_files_struct(struct task_struct *task)

void put_files_struct(struct files_struct *files)
{
- struct fdtable *fdt;
-
if (atomic_dec_and_test(&files->count)) {
- close_files(files);
- /* not really needed, since nobody can see us */
- rcu_read_lock();
- fdt = files_fdtable(files);
- rcu_read_unlock();
+ struct fdtable *fdt = close_files(files);
+
/* free the arrays if they are not embedded */
if (fdt != &files->fdtab)
__free_fdtable(fdt);
--
1.5.5.1

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

[Oleg Nesterov]

Hello.

I tried to audit the users of thread_group_empty() (we need
to change it) and found rcu_my_thread_group_empty() which
looks wrong.

The patches look simple, but I am not sure it is fine to use
rcu_lock_acquire() directly. Perhaps it makes sense to add a
new helper? Note that we have more users which take rcu lock
only to shut up lockdep. Please review.

And I am a bit confused. Perhaps rcu_lock_acquire() should
depend on CONFIG_PROVE_RCU, not on CONFIG_DEBUG_LOCK_ALLOC?
We only need rcu_lock_map/etc for rcu_lockdep_assert().

Oleg.

fs/file.c | 24 +++++++++++-------------
include/linux/fdtable.h | 19 +++++++++++++------
include/linux/rcupdate.h | 2 --
kernel/rcu/update.c | 11 -----------
4 files changed, 24 insertions(+), 32 deletions(-)

[Oleg Nesterov]

rcu_dereference_check_fdtable() looks very wrong,

1. rcu_my_thread_group_empty() was added by 844b9a8707f1 "vfs: fix
RCU-lockdep false positive due to /proc" but it doesn't really
fix the problem. A CLONE_THREAD (without CLONE_FILES) task can
hit the same race with get_files_struct().

And otoh rcu_my_thread_group_empty() can suppress the correct
warning if the caller is the CLONE_FILES (without CLONE_THREAD)
task.

2. files->count == 1 check is not really right too. Even if this
files_struct is not shared it is not safe to access it lockless
unless the caller is the owner.

Otoh, this check is sub-optimal. files->count == 0 always means
it is safe to use it lockless even if files != current->files,
but put_files_struct() has to take rcu_read_lock(). See the next
patch.

This patch removes the buggy checks and adds the new helper which
simply does rcu_lock_acquire/release around fcheck_files(). The
files->count == 1 callers, fget_light() and fget_raw_light(), can
use this helper to avoid the warning from RCU-lockdep.

Signed-off-by: Oleg Nesterov <ol...@redhat.com>
---

fs/file.c | 4 ++--

include/linux/fdtable.h | 19 +++++++++++++------
include/linux/rcupdate.h | 2 --
kernel/rcu/update.c | 11 -----------

4 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 4a78f98..957cbc0 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -707,7 +707,7 @@ struct file *fget_light(unsigned int fd, int *fput_needed)

*fput_needed = 0;
if (atomic_read(&files->count) == 1) {
- file = fcheck_files(files, fd);
+ file = __fcheck_files(files, fd);
if (file && (file->f_mode & FMODE_PATH))
file = NULL;
} else {
@@ -735,7 +735,7 @@ struct file *fget_raw_light(unsigned int fd, int *fput_needed)

*fput_needed = 0;
if (atomic_read(&files->count) == 1) {
- file = fcheck_files(files, fd);
+ file = __fcheck_files(files, fd);
} else {
rcu_read_lock();
file = fcheck_files(files, fd);
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 085197b..f39d50a 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -60,10 +60,7 @@ struct files_struct {
};

#define rcu_dereference_check_fdtable(files, fdtfd) \
- (rcu_dereference_check((fdtfd), \
- lockdep_is_held(&(files)->file_lock) || \
- atomic_read(&(files)->count) == 1 || \
- rcu_my_thread_group_empty()))
+ rcu_dereference_check((fdtfd), lockdep_is_held(&(files)->file_lock))

#define files_fdtable(files) \
(rcu_dereference_check_fdtable((files), (files)->fdt))
@@ -74,9 +71,9 @@ struct dentry;

extern void __init files_defer_init(void);

-static inline struct file * fcheck_files(struct files_struct *files, unsigned int fd)
+static inline struct file *fcheck_files(struct files_struct *files, unsigned int fd)
{
- struct file * file = NULL;
+ struct file *file = NULL;
struct fdtable *fdt = files_fdtable(files);

if (fd < fdt->max_fds)
@@ -84,6 +81,16 @@ static inline struct file * fcheck_files(struct files_struct *files, unsigned in
return file;
}

+/* The caller must ensure that fd table isn't shared */
+static inline struct file *__fcheck_files(struct files_struct *files, unsigned int fd)
+{
+ struct file *file;
+ /* make rcu_dereference_check_fdtable() happy */
+ rcu_lock_acquire(&rcu_lock_map);
+ file = fcheck_files(files, fd);
+ rcu_lock_release(&rcu_lock_map);
+ return file;
+}
/*
* Check whether the specified fd has an open file.
*/
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 39cbb88..a2482cf 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -448,8 +448,6 @@ static inline int rcu_read_lock_sched_held(void)

#ifdef CONFIG_PROVE_RCU

-extern int rcu_my_thread_group_empty(void);
-
/**
* rcu_lockdep_assert - emit lockdep splat if specified condition not met
* @c: condition to check
diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index 6cb3dff..a3596c8 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -195,17 +195,6 @@ void wait_rcu_gp(call_rcu_func_t crf)
}
EXPORT_SYMBOL_GPL(wait_rcu_gp);

-#ifdef CONFIG_PROVE_RCU
-/*
- * wrapper function to avoid #include problems.
- */
-int rcu_my_thread_group_empty(void)
-{
- return thread_group_empty(current);
-}
-EXPORT_SYMBOL_GPL(rcu_my_thread_group_empty);
-#endif /* #ifdef CONFIG_PROVE_RCU */
-
#ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD
static inline void debug_init_rcu_head(struct rcu_head *head)
{
--
1.5.5.1

[Paul E. McKenney]

On Tue, Jan 07, 2014 at 07:12:58PM +0100, Oleg Nesterov wrote:
> Hello.
>
> I tried to audit the users of thread_group_empty() (we need
> to change it) and found rcu_my_thread_group_empty() which
> looks wrong.
>
> The patches look simple, but I am not sure it is fine to use
> rcu_lock_acquire() directly. Perhaps it makes sense to add a
> new helper? Note that we have more users which take rcu lock
> only to shut up lockdep. Please review.
>
> And I am a bit confused. Perhaps rcu_lock_acquire() should
> depend on CONFIG_PROVE_RCU, not on CONFIG_DEBUG_LOCK_ALLOC?
> We only need rcu_lock_map/etc for rcu_lockdep_assert().

I am not all that excited about invoking rcu_lock_acquire() outside
of RCU...

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?

Thanx, Paul

[Oleg Nesterov]

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

#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.

[Paul E. McKenney]

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

[Oleg Nesterov]

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?

[Paul E. McKenney]

I might be being too paranoid, but my concern with using rcu_lock_acquire()
and rcu_lock_release() is the possibility of code needing rcu_read_lock()
appearing somewhere in the function-call graph between rcu_lock_acquire()
and rcu_lock_release(). In that case, lockdep would be happy, but the
required RCU protection would not be present.

Sort of like my experience with people using RCU from idle.

Thanx, Paul

[Oleg Nesterov]

On 01/10, Paul E. McKenney wrote:
>
> On Fri, Jan 10, 2014 at 04:34:59PM +0100, Oleg Nesterov wrote:
> >
> > doesn't this look much simpler than adding the "bool unshared" argument
> > and changing the callers?
>
> I might be being too paranoid, but my concern with using rcu_lock_acquire()
> and rcu_lock_release()

And I agree, I no longer suggest to use rcu_lock_acquire/release.

It seems that you misunderstood me, let me send v2 for review.

Oleg.

fs/file.c | 30 +++++++++++-------------------
include/linux/fdtable.h | 35 +++++++++++++++++++++--------------

include/linux/rcupdate.h | 2 --
kernel/rcu/update.c | 11 -----------

4 files changed, 32 insertions(+), 46 deletions(-)

[Oleg Nesterov]

put_files_struct() and close_files() do rcu_read_lock() to make
rcu_dereference_check_fdtable() happy.

This looks a bit ugly, files_fdtable() just reads the pointer,
we can simply use rcu_dereference_raw() to avoid the warning.

The patch also changes close_files() to return fdt, this avoids
another rcu_read_lock()/files_fdtable() in put_files_struct().

I think close_files() needs more cleanups:

- we do not need xchg() exactly because we are the last
user of this files_struct

- "if (file)" should be turned into WARN_ON(!file)

Signed-off-by: Oleg Nesterov <ol...@redhat.com>
---

fs/file.c | 26 +++++++++-----------------
1 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 957cbc0..d34e59e 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -348,21 +348,16 @@ out:

return NULL;
}

-static void close_files(struct files_struct * files)
+static struct fdtable *close_files(struct files_struct * files)
{

- int i, j;

- struct fdtable *fdt;
-

- j = 0;
-

/*
* It is safe to dereference the fd table without RCU or
* ->file_lock because this is the last reference to the

- * files structure. But use RCU to shut RCU-lockdep up.
+ * files structure.

*/
- rcu_read_lock();
- fdt = files_fdtable(files);
- rcu_read_unlock();

+ struct fdtable *fdt = rcu_dereference_raw(files->fdt);
+ int i, j = 0;

+
for (;;) {
unsigned long set;
i = j * BITS_PER_LONG;

@@ -381,6 +376,8 @@ static void close_files(struct files_struct * files)

set >>= 1;
}
}
+
+ return fdt;
}

struct files_struct *get_files_struct(struct task_struct *task)

@@ -398,14 +395,9 @@ struct files_struct *get_files_struct(struct task_struct *task)

void put_files_struct(struct files_struct *files)
{
- struct fdtable *fdt;
-
if (atomic_dec_and_test(&files->count)) {
- close_files(files);
- /* not really needed, since nobody can see us */
- rcu_read_lock();
- fdt = files_fdtable(files);
- rcu_read_unlock();
+ struct fdtable *fdt = close_files(files);
+
/* free the arrays if they are not embedded */
if (fdt != &files->fdtab)
__free_fdtable(fdt);
--
1.5.5.1

[Oleg Nesterov]

rcu_dereference_check_fdtable() looks very wrong,

1. rcu_my_thread_group_empty() was added by 844b9a8707f1 "vfs: fix
RCU-lockdep false positive due to /proc" but it doesn't really
fix the problem. A CLONE_THREAD (without CLONE_FILES) task can
hit the same race with get_files_struct().

And otoh rcu_my_thread_group_empty() can suppress the correct
warning if the caller is the CLONE_FILES (without CLONE_THREAD)
task.

2. files->count == 1 check is not really right too. Even if this
files_struct is not shared it is not safe to access it lockless
unless the caller is the owner.

Otoh, this check is sub-optimal. files->count == 0 always means
it is safe to use it lockless even if files != current->files,
but put_files_struct() has to take rcu_read_lock(). See the next
patch.

This patch removes the buggy checks and turns fcheck_files() into
__fcheck_files() which uses rcu_dereference_raw(), the "unshared"
callers, fget_light() and fget_raw_light(), can use it to avoid
the warning from RCU-lockdep.

fcheck_files() is trivially reimplemented as rcu_lockdep_assert()
plus __fcheck_files().

Signed-off-by: Oleg Nesterov <ol...@redhat.com>
---

fs/file.c | 4 ++--

include/linux/fdtable.h | 35 +++++++++++++++++++++--------------
include/linux/rcupdate.h | 2 --
kernel/rcu/update.c | 11 -----------

4 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 4a78f98..957cbc0 100644
--- a/fs/file.c
+++ b/fs/file.c

@@ -707,7 +707,7 @@ struct file *fget_light(unsigned int fd, int *fput_needed)

*fput_needed = 0;
if (atomic_read(&files->count) == 1) {
- file = fcheck_files(files, fd);
+ file = __fcheck_files(files, fd);
if (file && (file->f_mode & FMODE_PATH))
file = NULL;
} else {
@@ -735,7 +735,7 @@ struct file *fget_raw_light(unsigned int fd, int *fput_needed)

*fput_needed = 0;
if (atomic_read(&files->count) == 1) {
- file = fcheck_files(files, fd);
+ file = __fcheck_files(files, fd);
} else {
rcu_read_lock();
file = fcheck_files(files, fd);
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h

index 085197b..70e8e21 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -59,29 +59,36 @@ struct files_struct {
struct file __rcu * fd_array[NR_OPEN_DEFAULT];
};

-#define rcu_dereference_check_fdtable(files, fdtfd) \

- (rcu_dereference_check((fdtfd), \
- lockdep_is_held(&(files)->file_lock) || \
- atomic_read(&(files)->count) == 1 || \
- rcu_my_thread_group_empty()))

-
-#define files_fdtable(files) \
- (rcu_dereference_check_fdtable((files), (files)->fdt))
-
struct file_operations;
struct vfsmount;

struct dentry;

extern void __init files_defer_init(void);

-static inline struct file * fcheck_files(struct files_struct *files, unsigned int fd)
+#define rcu_dereference_check_fdtable(files, fdtfd) \
+ rcu_dereference_check((fdtfd), lockdep_is_held(&(files)->file_lock))
+
+#define files_fdtable(files) \
+ rcu_dereference_check_fdtable((files), (files)->fdt)
+
+/*
+ * The caller must ensure that fd table isn't shared or hold rcu or file lock
+ */
+static inline struct file *__fcheck_files(struct files_struct *files, unsigned int fd)

{
- struct file * file = NULL;

- struct fdtable *fdt = files_fdtable(files);
+ struct fdtable *fdt = rcu_dereference_raw(files->fdt);

if (fd < fdt->max_fds)
- file = rcu_dereference_check_fdtable(files, fdt->fd[fd]);
- return file;
+ return rcu_dereference_raw(fdt->fd[fd]);
+ return NULL;
+}
+
+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),
+ "suspicious rcu_dereference_check() usage");
+ return __fcheck_files(files, fd);
}

/*

[Paul E. McKenney]

On Sat, Jan 11, 2014 at 07:19:08PM +0100, Oleg Nesterov wrote:
> On 01/10, Paul E. McKenney wrote:
> >
> > On Fri, Jan 10, 2014 at 04:34:59PM +0100, Oleg Nesterov wrote:
> > >
> > > doesn't this look much simpler than adding the "bool unshared" argument
> > > and changing the callers?
> >
> > I might be being too paranoid, but my concern with using rcu_lock_acquire()
> > and rcu_lock_release()
>
> And I agree, I no longer suggest to use rcu_lock_acquire/release.
>
> It seems that you misunderstood me, let me send v2 for review.

Ah, I did misunderstand you. For both:

Acked-by: Paul E. McKenney <pau...@linux.vnet.ibm.com>

Thanx, Paul

[Oleg Nesterov]

The slow path in __fget_light() can use __fget() to avoid the
code duplication. Saves 232 bytes.

Signed-off-by: Oleg Nesterov <ol...@redhat.com>
---

fs/file.c | 14 +++-----------
1 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 50c1208..771578b 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -694,17 +694,9 @@ struct file *__fget_light(unsigned int fd, fmode_t mask, int *fput_needed)
if (file && (file->f_mode & mask))
file = NULL;
} else {
- rcu_read_lock();

- file = fcheck_files(files, fd);

- if (file) {
- if (!(file->f_mode & mask) &&
- atomic_long_inc_not_zero(&file->f_count))
- *fput_needed = 1;
- else
- /* Didn't get the reference, someone's freed */
- file = NULL;
- }
- rcu_read_unlock();
+ file = __fget(fd, mask);
+ if (file)
+ *fput_needed = 1;
}

return file;
--
1.5.5.1

[Oleg Nesterov]

Apart from FMODE_PATH check fget() and fget_raw() are identical,
shift the code into the new simple helper, __fget(fd, mask). Saves
160 bytes.

Signed-off-by: Oleg Nesterov <ol...@redhat.com>
---

fs/file.c | 25 ++++++++-----------------
1 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index d34e59e..4ed58a3 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -637,16 +637,16 @@ void do_close_on_exec(struct files_struct *files)
spin_unlock(&files->file_lock);
}

-struct file *fget(unsigned int fd)
+static struct file *__fget(unsigned int fd, fmode_t mask)
{
- struct file *file;
struct files_struct *files = current->files;
+ struct file *file;

rcu_read_lock();
file = fcheck_files(files, fd);

if (file) {
/* File object ref couldn't be taken */
- if (file->f_mode & FMODE_PATH ||
+ if ((file->f_mode & mask) ||
!atomic_long_inc_not_zero(&file->f_count))
file = NULL;
}
@@ -655,25 +655,16 @@ struct file *fget(unsigned int fd)
return file;
}

+struct file *fget(unsigned int fd)
+{
+ return __fget(fd, FMODE_PATH);
+}
EXPORT_SYMBOL(fget);

struct file *fget_raw(unsigned int fd)
{
- struct file *file;
- struct files_struct *files = current->files;
-

- rcu_read_lock();
- file = fcheck_files(files, fd);
- if (file) {

- /* File object ref couldn't be taken */
- if (!atomic_long_inc_not_zero(&file->f_count))

- file = NULL;
- }
- rcu_read_unlock();

-
- return file;
+ return __fget(fd, 0);
}
-
EXPORT_SYMBOL(fget_raw);

/*
--
1.5.5.1

[Oleg Nesterov]

On 01/11, Paul E. McKenney wrote:
>
> On Sat, Jan 11, 2014 at 07:19:08PM +0100, Oleg Nesterov wrote:
> >
> > It seems that you misunderstood me, let me send v2 for review.
>
> Ah, I did misunderstand you. For both:
>
> Acked-by: Paul E. McKenney <pau...@linux.vnet.ibm.com>

Thanks!

While at it, can't we factor out the code in fget*() ?

Probably I dislike the code duplications too much, but imho fget*()
abuse the copy-and-paste.

On top of this series, saves 600 bytes.

Oleg.

fs/file.c | 70 ++++++++++++++++--------------------------------------------
1 files changed, 19 insertions(+), 51 deletions(-)

[Oleg Nesterov]

Apart from FMODE_PATH check fget_light() and fget_raw_light() are
identical, shift the code into the new helper, __fget_light(fd, mask).
Saves 208 bytes.

Signed-off-by: Oleg Nesterov <ol...@redhat.com>
---

fs/file.c | 33 +++++++++------------------------
1 files changed, 9 insertions(+), 24 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 4ed58a3..50c1208 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -683,21 +683,21 @@ EXPORT_SYMBOL(fget_raw);
* The fput_needed flag returned by fget_light should be passed to the
* corresponding fput_light.
*/
-struct file *fget_light(unsigned int fd, int *fput_needed)
+struct file *__fget_light(unsigned int fd, fmode_t mask, int *fput_needed)

{
- struct file *file;
struct files_struct *files = current->files;
+ struct file *file;

*fput_needed = 0;
if (atomic_read(&files->count) == 1) {

file = __fcheck_files(files, fd);
- if (file && (file->f_mode & FMODE_PATH))
+ if (file && (file->f_mode & mask))
file = NULL;
} else {

rcu_read_lock();
file = fcheck_files(files, fd);
if (file) {

- if (!(file->f_mode & FMODE_PATH) &&
+ if (!(file->f_mode & mask) &&
atomic_long_inc_not_zero(&file->f_count))
*fput_needed = 1;
else
@@ -709,30 +709,15 @@ struct file *fget_light(unsigned int fd, int *fput_needed)

return file;
}
+struct file *fget_light(unsigned int fd, int *fput_needed)
+{
+ return __fget_light(fd, FMODE_PATH, fput_needed);
+}
EXPORT_SYMBOL(fget_light);

struct file *fget_raw_light(unsigned int fd, int *fput_needed)

{
- struct file *file;
- struct files_struct *files = current->files;
-

- *fput_needed = 0;
- if (atomic_read(&files->count) == 1) {
- file = __fcheck_files(files, fd);
- } else {

- rcu_read_lock();
- file = fcheck_files(files, fd);
- if (file) {

- if (atomic_long_inc_not_zero(&file->f_count))

- *fput_needed = 1;
- else
- /* Didn't get the reference, someone's freed */

- file = NULL;
- }
- rcu_read_unlock();
- }
-
- return file;

+ return __fget_light(fd, 0, fput_needed);
}

void set_close_on_exec(unsigned int fd, int flag)
--
1.5.5.1

[Paul E. McKenney]

On Mon, Jan 13, 2014 at 04:48:40PM +0100, Oleg Nesterov wrote:
> Apart from FMODE_PATH check fget_light() and fget_raw_light() are
> identical, shift the code into the new helper, __fget_light(fd, mask).
> Saves 208 bytes.
>
> Signed-off-by: Oleg Nesterov <ol...@redhat.com>

Reviewed-by: Paul E. McKenney <pau...@linux.vnet.ibm.com>

[Paul E. McKenney]

On Mon, Jan 13, 2014 at 04:49:06PM +0100, Oleg Nesterov wrote:
> The slow path in __fget_light() can use __fget() to avoid the
> code duplication. Saves 232 bytes.
>
> Signed-off-by: Oleg Nesterov <ol...@redhat.com>

Reviewed-by: Paul E. McKenney <pau...@linux.vnet.ibm.com>

[Paul E. McKenney]

On Mon, Jan 13, 2014 at 04:48:19PM +0100, Oleg Nesterov wrote:
> Apart from FMODE_PATH check fget() and fget_raw() are identical,
> shift the code into the new simple helper, __fget(fd, mask). Saves
> 160 bytes.
>
> Signed-off-by: Oleg Nesterov <ol...@redhat.com>

Not sure why local variable "file" was moved, but regardless:

Reviewed-by: Paul E. McKenney <pau...@linux.vnet.ibm.com>

[Al Viro]

On Mon, Jan 13, 2014 at 04:47:48PM +0100, Oleg Nesterov wrote:
> On 01/11, Paul E. McKenney wrote:
> >
> > On Sat, Jan 11, 2014 at 07:19:08PM +0100, Oleg Nesterov wrote:
> > >
> > > It seems that you misunderstood me, let me send v2 for review.
> >
> > Ah, I did misunderstand you. For both:
> >
> > Acked-by: Paul E. McKenney <pau...@linux.vnet.ibm.com>
>
> Thanks!
>
> While at it, can't we factor out the code in fget*() ?
>
> Probably I dislike the code duplications too much, but imho fget*()
> abuse the copy-and-paste.
>
> On top of this series, saves 600 bytes.

Applied, along with fcheck_files() stuff. Sorry about late reply, I'm
digging through huge piles of mail and putting a queue for -next right
now...