[PATCH 0/1] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()

[PATCH 0/1] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()

[Oleg Nesterov]

Completely untested and I know nothing about aio, thus needs an ack
from maintainers or should be ignored.

But exit_aio() looks "obviously unnecessarily overcomplicated", I was
really puzzled when I looked at this code by accident.

Kent, could you also explain kioctx->dead is atomic_t? This looks
pointless? afaics atomic_ buys nothing in this case.

Oleg.

Re: [PATCH 0/1] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()

[Oleg Nesterov]

1. We can read ->ioctx_table only once and we do not read rcu_read_lock()
   or even rcu_dereference().

   This mm has no users, nobody else can play with ->ioctx_table. Otherwise
   the code is buggy anyway, if we need rcu_read_lock() in a loop because
   ->ioctx_table can be updated then kfree(table) is obviously wrong.

2. Update the comment. "exit_mmap(mm) is coming" is the good reason to avoid
   munmap(), but another reason is that we simply can't do vm_munmap() unless
   current->mm == mm and this is not true in general, the caller is mmput().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/aio.c |   47 ++++++++++++++++++-----------------------------
 1 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 12a3de0..5fd1fe7 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -777,40 +777,29 @@ EXPORT_SYMBOL(wait_on_sync_kiocb);
  */
 void exit_aio(struct mm_struct *mm)
 {
-	struct kioctx_table *table;
-	struct kioctx *ctx;
-	unsigned i = 0;
-
-	while (1) {
-		rcu_read_lock();
-		table = rcu_dereference(mm->ioctx_table);
-
-		do {
-			if (!table || i >= table->nr) {
-				rcu_read_unlock();
-				rcu_assign_pointer(mm->ioctx_table, NULL);
-				if (table)
-					kfree(table);
-				return;
-			}
-
-			ctx = table->table[i++];
-		} while (!ctx);
+	struct kioctx_table *table = rcu_dereference_raw(mm->ioctx_table);
+	int i;
 
-		rcu_read_unlock();
+	if (!table)
+		return;
 
+	for (i = 0; i < table->nr; ++i) {
+		struct kioctx *ctx = table->table[i];
 		/*
-		 * We don't need to bother with munmap() here -
-		 * exit_mmap(mm) is coming and it'll unmap everything.
-		 * Since aio_free_ring() uses non-zero ->mmap_size
-		 * as indicator that it needs to unmap the area,
-		 * just set it to 0; aio_free_ring() is the only
-		 * place that uses ->mmap_size, so it's safe.
+		 * We don't need to bother with munmap() here - exit_mmap(mm)
+		 * is coming and it'll unmap everything. And we simply can't,
+		 * this is not necessarily our ->mm.
+		 * Since kill_ioctx() uses non-zero ->mmap_size as indicator
+		 * that it needs to unmap the area, just set it to 0.
 		 */
-		ctx->mmap_size = 0;
-
-		kill_ioctx(mm, ctx);
+		if (ctx) {
+			ctx->mmap_size = 0;
+			kill_ioctx(mm, ctx);
+		}
 	}
+
+	rcu_assign_pointer(mm->ioctx_table, NULL);
+	kfree(table);
 }
 
 static void put_reqs_available(struct kioctx *ctx, unsigned nr)
-- 
1.5.5.1

Re: [PATCH 0/1] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()

[Kent Overstreet]

On Tue, Apr 29, 2014 at 08:39:30PM +0200, Oleg Nesterov wrote:
> 1. We can read ->ioctx_table only once and we do not read rcu_read_lock()
>    or even rcu_dereference().
> 
>    This mm has no users, nobody else can play with ->ioctx_table. Otherwise
>    the code is buggy anyway, if we need rcu_read_lock() in a loop because
>    ->ioctx_table can be updated then kfree(table) is obviously wrong.
> 
> 2. Update the comment. "exit_mmap(mm) is coming" is the good reason to avoid
>    munmap(), but another reason is that we simply can't do vm_munmap() unless
>    current->mm == mm and this is not true in general, the caller is mmput().

I'm pretty sure you're analysis is all correct. IIRC there's other things in the
shutdown path we still have to be carefull with synchronization wise, but unless
I've forgotten something you're right about ioctx_table.

If I wrote that code (I'd have to check git blame), I'd say I just wrote it that
way because I prefer to be consistent about how things like RCU protected
pointers are accessed, even if it's not strictly necessary. Feels less tricky to
me. But I don't have a strong preference one way or the other.

w.r.t. ioctx->dead, yes, no need for it to be atomic. a regular int and xchg()
is probably more correct anyways since xchg() implies memory barriers and
atomic_xchg() does not.

[PATCH 1/1] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()

[Oleg Nesterov]

1. We can read ->ioctx_table only once and we do not read rcu_read_lock()
   or even rcu_dereference().

   This mm has no users, nobody else can play with ->ioctx_table. Otherwise
   the code is buggy anyway, if we need rcu_read_lock() in a loop because
   ->ioctx_table can be updated then kfree(table) is obviously wrong.

2. Update the comment. "exit_mmap(mm) is coming" is the good reason to avoid
   munmap(), but another reason is that we simply can't do vm_munmap() unless
   current->mm == mm and this is not true in general, the caller is mmput().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/aio.c |   47 ++++++++++++++++++-----------------------------
 1 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 12a3de0..5fd1fe7 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -777,40 +777,29 @@ EXPORT_SYMBOL(wait_on_sync_kiocb);
  */
 void exit_aio(struct mm_struct *mm)
 {
-	struct kioctx_table *table;
-	struct kioctx *ctx;
-	unsigned i = 0;
-
-	while (1) {
-		rcu_read_lock();
-		table = rcu_dereference(mm->ioctx_table);
-
-		do {
-			if (!table || i >= table->nr) {
-				rcu_read_unlock();
-				rcu_assign_pointer(mm->ioctx_table, NULL);
-				if (table)
-					kfree(table);
-				return;
-			}
-
-			ctx = table->table[i++];
-		} while (!ctx);
+	struct kioctx_table *table = rcu_dereference_raw(mm->ioctx_table);
+	int i;
 
-		rcu_read_unlock();
+	if (!table)
+		return;
 
+	for (i = 0; i < table->nr; ++i) {
+		struct kioctx *ctx = table->table[i];
 		/*
-		 * We don't need to bother with munmap() here -
-		 * exit_mmap(mm) is coming and it'll unmap everything.
-		 * Since aio_free_ring() uses non-zero ->mmap_size
-		 * as indicator that it needs to unmap the area,
-		 * just set it to 0; aio_free_ring() is the only
-		 * place that uses ->mmap_size, so it's safe.
+		 * We don't need to bother with munmap() here - exit_mmap(mm)
+		 * is coming and it'll unmap everything. And we simply can't,
+		 * this is not necessarily our ->mm.
+		 * Since kill_ioctx() uses non-zero ->mmap_size as indicator
+		 * that it needs to unmap the area, just set it to 0.
 		 */
-		ctx->mmap_size = 0;
-
-		kill_ioctx(mm, ctx);
+		if (ctx) {
+			ctx->mmap_size = 0;
+			kill_ioctx(mm, ctx);
+		}
 	}
+
+	rcu_assign_pointer(mm->ioctx_table, NULL);
+	kfree(table);
 }
 
 static void put_reqs_available(struct kioctx *ctx, unsigned nr)
-- 
1.5.5.1

Re: [PATCH 1/1] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()

[Benjamin LaHaise]

On Tue, Apr 29, 2014 at 08:40:04PM +0200, Oleg Nesterov wrote:
> 1. We can read ->ioctx_table only once and we do not read rcu_read_lock()
>    or even rcu_dereference().
> 
>    This mm has no users, nobody else can play with ->ioctx_table. Otherwise
>    the code is buggy anyway, if we need rcu_read_lock() in a loop because
>    ->ioctx_table can be updated then kfree(table) is obviously wrong.
> 
> 2. Update the comment. "exit_mmap(mm) is coming" is the good reason to avoid
>    munmap(), but another reason is that we simply can't do vm_munmap() unless
>    current->mm == mm and this is not true in general, the caller is mmput().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Your patch does not apply because it is whitespace damaged.  Please resend 
and verify that it applies with 'git am'.

		-ben

> ---
>  fs/aio.c |   47 ++++++++++++++++++-----------------------------
>  1 files changed, 18 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/aio.c b/fs/aio.c
> index 12a3de0..5fd1fe7 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -777,40 +777,29 @@ EXPORT_SYMBOL(wait_on_sync_kiocb);
>   */
>  void exit_aio(struct mm_struct *mm)
>  {
> -	struct kioctx_table *table;
> -	struct kioctx *ctx;
> -	unsigned i = 0;
> -
> -	while (1) {
> -		rcu_read_lock();
> -		table = rcu_dereference(mm->ioctx_table);
> -
> -		do {
> -			if (!table || i >= table->nr) {
> -				rcu_read_unlock();
> -				rcu_assign_pointer(mm->ioctx_table, NULL);
> -				if (table)
> -					kfree(table);
> -				return;
> -			}
> -
> -			ctx = table->table[i++];
> -		} while (!ctx);
> +	struct kioctx_table *table = rcu_dereference_raw(mm->ioctx_table);
> +	int i;
>  
> -		rcu_read_unlock();
> +	if (!table)
> +		return;
>  
> +	for (i = 0; i < table->nr; ++i) {
> +		struct kioctx *ctx = table->table[i];
>  		/*
> -		 * We don't need to bother with munmap() here -
> -		 * exit_mmap(mm) is coming and it'll unmap everything.
> -		 * Since aio_free_ring() uses non-zero ->mmap_size
> -		 * as indicator that it needs to unmap the area,
> -		 * just set it to 0; aio_free_ring() is the only
> -		 * place that uses ->mmap_size, so it's safe.
> +		 * We don't need to bother with munmap() here - exit_mmap(mm)
> +		 * is coming and it'll unmap everything. And we simply can't,
> +		 * this is not necessarily our ->mm.
> +		 * Since kill_ioctx() uses non-zero ->mmap_size as indicator
> +		 * that it needs to unmap the area, just set it to 0.
>  		 */
> -		ctx->mmap_size = 0;
> -
> -		kill_ioctx(mm, ctx);
> +		if (ctx) {
> +			ctx->mmap_size = 0;
> +			kill_ioctx(mm, ctx);
> +		}
>  	}
> +
> +	rcu_assign_pointer(mm->ioctx_table, NULL);
> +	kfree(table);
>  }
>  
>  static void put_reqs_available(struct kioctx *ctx, unsigned nr)
> -- 
> 1.5.5.1
> 

-- 
"Thought is the essence of where you are now."

Re: [PATCH 1/1] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()

[Benjamin LaHaise]

On Tue, Apr 29, 2014 at 04:42:17PM -0400, Benjamin LaHaise wrote:
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> 
> Your patch does not apply because it is whitespace damaged.  Please resend 
> and verify that it applies with 'git am'.

Whoops, it's not whitespace damange, but rather that it doesn't apply with 
the other changes that are queued up in the aio-next tree.  You can find a 
copy of that tree at git://git.kvack.org/~bcrl/aio-next.git .  The change 
that conflicts is an additional parameter to kill_ioctx().

		-ben
-- 
"Thought is the essence of where you are now."

Re: [PATCH 1/1] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()

[Mateusz Guzik]

On Tue, Apr 29, 2014 at 05:22:22PM -0400, Benjamin LaHaise wrote:
> On Tue, Apr 29, 2014 at 04:42:17PM -0400, Benjamin LaHaise wrote:
> > > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > 
> > Your patch does not apply because it is whitespace damaged.  Please resend 
> > and verify that it applies with 'git am'.
> 
> Whoops, it's not whitespace damange, but rather that it doesn't apply with 
> the other changes that are queued up in the aio-next tree.  You can find a 
> copy of that tree at git://git.kvack.org/~bcrl/aio-next.git .  The change 
> that conflicts is an additional parameter to kill_ioctx().

While here is there any reason for:
rcu_assign_pointer(mm->ioctx_table, NULL);

Nothing looks at this pointer afterwards and mm is about to be freed.

I thought it would be used to sanity check that everything was cleared
before freeing, but that is nod one and not every pointer is nullified
anyway.

-- 
Mateusz Guzik

Re: [PATCH 1/1] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()

[Oleg Nesterov]

On 04/30, Mateusz Guzik wrote:
>
> On Tue, Apr 29, 2014 at 05:22:22PM -0400, Benjamin LaHaise wrote:
> > On Tue, Apr 29, 2014 at 04:42:17PM -0400, Benjamin LaHaise wrote:
> > > > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > >
> > > Your patch does not apply because it is whitespace damaged.  Please resend
> > > and verify that it applies with 'git am'.
> >
> > Whoops, it's not whitespace damange, but rather that it doesn't apply with
> > the other changes that are queued up in the aio-next tree.  You can find a
> > copy of that tree at git://git.kvack.org/~bcrl/aio-next.git .  The change
> > that conflicts is an additional parameter to kill_ioctx().
>
> While here is there any reason for:
> rcu_assign_pointer(mm->ioctx_table, NULL);
>
> Nothing looks at this pointer afterwards and mm is about to be freed.

Yees, and initially I was going to remove it, but this needs "3" in the
changelog and I am lazy ;)

> I thought it would be used to sanity check that everything was cleared
> before freeing,

Actually, ->mm_count can be > 1, we do not know if we are going to free
this mm_struct or not.

We do not really need to nullify this ptr anyway, but perhaps it makes
sense to keep this initialization anyway, I dunno. But I'll change it to
use RCU_INIT_POINTER(NULL) in v2.

Oleg.

[PATCH v2 0/2] aio: ioctx_table/rcu_read_lock cleanups

[Oleg Nesterov]

On 04/29, Benjamin LaHaise wrote:
>
> Whoops, it's not whitespace damange, but rather that it doesn't apply with
> the other changes that are queued up in the aio-next tree.  You can find a
> copy of that tree at git://git.kvack.org/~bcrl/aio-next.git .  The change
> that conflicts is an additional parameter to kill_ioctx().

Please see v2. Rebased, added another untested cleanup.

Kent, it seems that you agree at least with 1/2, I'll appreciate your ack ;)

And it seems that we can kill mm->ioctx_lock, with the minimal complications
we can move it into kioctx_table, but this is minor.

Oleg.

[PATCH v2 1/2] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()

[Oleg Nesterov]

1. We can read ->ioctx_table only once and we do not read rcu_read_lock()
   or even rcu_dereference().

   This mm has no users, nobody else can play with ->ioctx_table. Otherwise
   the code is buggy anyway, if we need rcu_read_lock() in a loop because
   ->ioctx_table can be updated then kfree(table) is obviously wrong.

2. Update the comment. "exit_mmap(mm) is coming" is the good reason to avoid
   munmap(), but another reason is that we simply can't do vm_munmap() unless
   current->mm == mm and this is not true in general, the caller is mmput().

3. We do not really need to nullify mm->ioctx_table before return, probably
   the current code does this to catch the potential problems. But in this
   case RCU_INIT_POINTER(NULL) looks better.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/aio.c |   47 ++++++++++++++++++-----------------------------
 1 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 79b7e69..3526c2b 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -791,40 +791,29 @@ EXPORT_SYMBOL(wait_on_sync_kiocb);
  */
 void exit_aio(struct mm_struct *mm)
 {
-	struct kioctx_table *table;
-	struct kioctx *ctx;
-	unsigned i = 0;
-
-	while (1) {
-		rcu_read_lock();
-		table = rcu_dereference(mm->ioctx_table);
-
-		do {
-			if (!table || i >= table->nr) {
-				rcu_read_unlock();
-				rcu_assign_pointer(mm->ioctx_table, NULL);
-				if (table)
-					kfree(table);
-				return;
-			}
-
-			ctx = table->table[i++];
-		} while (!ctx);
+	struct kioctx_table *table = rcu_dereference_raw(mm->ioctx_table);
+	int i;
 
-		rcu_read_unlock();
+	if (!table)
+		return;
 
+	for (i = 0; i < table->nr; ++i) {
+		struct kioctx *ctx = table->table[i];
 		/*
-		 * We don't need to bother with munmap() here -
-		 * exit_mmap(mm) is coming and it'll unmap everything.
-		 * Since aio_free_ring() uses non-zero ->mmap_size
-		 * as indicator that it needs to unmap the area,
-		 * just set it to 0; aio_free_ring() is the only
-		 * place that uses ->mmap_size, so it's safe.
+		 * We don't need to bother with munmap() here - exit_mmap(mm)
+		 * is coming and it'll unmap everything. And we simply can't,
+		 * this is not necessarily our ->mm.
+		 * Since kill_ioctx() uses non-zero ->mmap_size as indicator
+		 * that it needs to unmap the area, just set it to 0.
 		 */
-		ctx->mmap_size = 0;
-
-		kill_ioctx(mm, ctx, NULL);
+		if (ctx) {
+			ctx->mmap_size = 0;
+			kill_ioctx(mm, ctx, NULL);
+		}
 	}
+
+	RCU_INIT_POINTER(mm->ioctx_table, NULL);
+	kfree(table);
 }
 
 static void put_reqs_available(struct kioctx *ctx, unsigned nr)
-- 
1.5.5.1

Re: [PATCH v2 1/2] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()

[Benjamin LaHaise]

Hi Oleg,

On Wed, Apr 30, 2014 at 04:16:16PM +0200, Oleg Nesterov wrote:
> 1. We can read ->ioctx_table only once and we do not read rcu_read_lock()
>    or even rcu_dereference().
> 
>    This mm has no users, nobody else can play with ->ioctx_table. Otherwise
>    the code is buggy anyway, if we need rcu_read_lock() in a loop because
>    ->ioctx_table can be updated then kfree(table) is obviously wrong.
> 
> 2. Update the comment. "exit_mmap(mm) is coming" is the good reason to avoid
>    munmap(), but another reason is that we simply can't do vm_munmap() unless
>    current->mm == mm and this is not true in general, the caller is mmput().
> 
> 3. We do not really need to nullify mm->ioctx_table before return, probably
>    the current code does this to catch the potential problems. But in this
>    case RCU_INIT_POINTER(NULL) looks better.

Looks pretty good.  One minor style comment below.

> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  fs/aio.c |   47 ++++++++++++++++++-----------------------------
>  1 files changed, 18 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/aio.c b/fs/aio.c
> index 79b7e69..3526c2b 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -791,40 +791,29 @@ EXPORT_SYMBOL(wait_on_sync_kiocb);
>   */
>  void exit_aio(struct mm_struct *mm)
>  {
> -	struct kioctx_table *table;
> -	struct kioctx *ctx;
> -	unsigned i = 0;
> -
> -	while (1) {
> -		rcu_read_lock();
> -		table = rcu_dereference(mm->ioctx_table);
> -
> -		do {
> -			if (!table || i >= table->nr) {
> -				rcu_read_unlock();
> -				rcu_assign_pointer(mm->ioctx_table, NULL);
> -				if (table)
> -					kfree(table);
> -				return;
> -			}
> -
> -			ctx = table->table[i++];
> -		} while (!ctx);
> +	struct kioctx_table *table = rcu_dereference_raw(mm->ioctx_table);
> +	int i;
>  
> -		rcu_read_unlock();
> +	if (!table)
> +		return;
>  
> +	for (i = 0; i < table->nr; ++i) {
> +		struct kioctx *ctx = table->table[i];
>  		/*
> -		 * We don't need to bother with munmap() here -
> -		 * exit_mmap(mm) is coming and it'll unmap everything.
> -		 * Since aio_free_ring() uses non-zero ->mmap_size
> -		 * as indicator that it needs to unmap the area,
> -		 * just set it to 0; aio_free_ring() is the only
> -		 * place that uses ->mmap_size, so it's safe.
> +		 * We don't need to bother with munmap() here - exit_mmap(mm)
> +		 * is coming and it'll unmap everything. And we simply can't,
> +		 * this is not necessarily our ->mm.
> +		 * Since kill_ioctx() uses non-zero ->mmap_size as indicator
> +		 * that it needs to unmap the area, just set it to 0.
>  		 */
> -		ctx->mmap_size = 0;
> -
> -		kill_ioctx(mm, ctx, NULL);
> +		if (ctx) {
> +			ctx->mmap_size = 0;
> +			kill_ioctx(mm, ctx, NULL);
> +		}

Rather than indenting and moving the two lines changing mmap_size and the 
kill_ioctx() call, why not just do "if (!ctx) ... continue;"?  That reduces 
the number of lines changed and avoid excessive indentation.

		-ben

>  	}
> +
> +	RCU_INIT_POINTER(mm->ioctx_table, NULL);
> +	kfree(table);
>  }
>  
>  static void put_reqs_available(struct kioctx *ctx, unsigned nr)
> -- 
> 1.5.5.1
> 

-- 
"Thought is the essence of where you are now."

[PATCH v3 1/2] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()

[Oleg Nesterov]

On 04/30, Benjamin LaHaise wrote:
>
> > -		ctx->mmap_size = 0;
> > -
> > -		kill_ioctx(mm, ctx, NULL);
> > +		if (ctx) {
> > +			ctx->mmap_size = 0;
> > +			kill_ioctx(mm, ctx, NULL);
> > +		}
>
> Rather than indenting and moving the two lines changing mmap_size and the
> kill_ioctx() call, why not just do "if (!ctx) ... continue;"?  That reduces
> the number of lines changed and avoid excessive indentation.

OK. To me the code looks better/simpler with "if (ctx)", but this is subjective
of course, I won't argue.

The patch still removes the empty line between mmap_size = 0 and kill_ioctx(),
we reset mmap_size only for kill_ioctx(). But feel free to remove this change.

-------------------------------------------------------------------------------
Subject: [PATCH v3 1/2] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()

1. We can read ->ioctx_table only once and we do not read rcu_read_lock()
   or even rcu_dereference().

   This mm has no users, nobody else can play with ->ioctx_table. Otherwise
   the code is buggy anyway, if we need rcu_read_lock() in a loop because
   ->ioctx_table can be updated then kfree(table) is obviously wrong.

2. Update the comment. "exit_mmap(mm) is coming" is the good reason to avoid
   munmap(), but another reason is that we simply can't do vm_munmap() unless
   current->mm == mm and this is not true in general, the caller is mmput().

3. We do not really need to nullify mm->ioctx_table before return, probably
   the current code does this to catch the potential problems. But in this
   case RCU_INIT_POINTER(NULL) looks better.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/aio.c |   42 ++++++++++++++++--------------------------
 1 files changed, 16 insertions(+), 26 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 79b7e69..b67f3e2 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -791,40 +791,30 @@ EXPORT_SYMBOL(wait_on_sync_kiocb);
  */
 void exit_aio(struct mm_struct *mm)
 {
-	struct kioctx_table *table;
-	struct kioctx *ctx;
-	unsigned i = 0;
-
-	while (1) {
-		rcu_read_lock();
-		table = rcu_dereference(mm->ioctx_table);
-
-		do {
-			if (!table || i >= table->nr) {
-				rcu_read_unlock();
-				rcu_assign_pointer(mm->ioctx_table, NULL);
-				if (table)
-					kfree(table);
-				return;
-			}
+	struct kioctx_table *table = rcu_dereference_raw(mm->ioctx_table);
+	int i;
 
-			ctx = table->table[i++];
-		} while (!ctx);
+	if (!table)
+		return;
 
-		rcu_read_unlock();
+	for (i = 0; i < table->nr; ++i) {
+		struct kioctx *ctx = table->table[i];
 
+		if (!ctx)
+			continue;
 		/*
-		 * We don't need to bother with munmap() here -
-		 * exit_mmap(mm) is coming and it'll unmap everything.
-		 * Since aio_free_ring() uses non-zero ->mmap_size
-		 * as indicator that it needs to unmap the area,
-		 * just set it to 0; aio_free_ring() is the only
-		 * place that uses ->mmap_size, so it's safe.
+		 * We don't need to bother with munmap() here - exit_mmap(mm)
+		 * is coming and it'll unmap everything. And we simply can't,
+		 * this is not necessarily our ->mm.
+		 * Since kill_ioctx() uses non-zero ->mmap_size as indicator
+		 * that it needs to unmap the area, just set it to 0.
 		 */
 		ctx->mmap_size = 0;
-
 		kill_ioctx(mm, ctx, NULL);
 	}
+
+	RCU_INIT_POINTER(mm->ioctx_table, NULL);
+	kfree(table);
 }
 
 static void put_reqs_available(struct kioctx *ctx, unsigned nr)
-- 
1.5.5.1

[PATCH v2 2/2] aio: kill the misleading rcu read locks in ioctx_add_table() and kill_ioctx()

[Oleg Nesterov]

ioctx_add_table() is the writer, it does not need rcu_read_lock() to
protect ->ioctx_table. It relies on mm->ioctx_lock and rcu locks just
add the confusion.

And it doesn't need rcu_dereference() by the same reason, it must see
any updates previously done under the same ->ioctx_lock. We could use
rcu_dereference_protected() but the patch uses rcu_dereference_raw(),
the function is simple enough.

The same for kill_ioctx(), although it does not update the pointer.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/aio.c |   14 +++-----------
 1 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 3526c2b..a415870 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -554,8 +554,7 @@ static int ioctx_add_table(struct kioctx *ctx, struct mm_struct *mm)
 	struct aio_ring *ring;
 
 	spin_lock(&mm->ioctx_lock);
-	rcu_read_lock();
-	table = rcu_dereference(mm->ioctx_table);
+	table = rcu_dereference_raw(mm->ioctx_table);
 
 	while (1) {
 		if (table)
@@ -563,7 +562,6 @@ static int ioctx_add_table(struct kioctx *ctx, struct mm_struct *mm)
 				if (!table->table[i]) {
 					ctx->id = i;
 					table->table[i] = ctx;
-					rcu_read_unlock();
 					spin_unlock(&mm->ioctx_lock);
 
 					/* While kioctx setup is in progress,
@@ -577,8 +575,6 @@ static int ioctx_add_table(struct kioctx *ctx, struct mm_struct *mm)
 				}
 
 		new_nr = (table ? table->nr : 1) * 4;
-
-		rcu_read_unlock();
 		spin_unlock(&mm->ioctx_lock);
 
 		table = kzalloc(sizeof(*table) + sizeof(struct kioctx *) *
@@ -589,8 +585,7 @@ static int ioctx_add_table(struct kioctx *ctx, struct mm_struct *mm)
 		table->nr = new_nr;
 
 		spin_lock(&mm->ioctx_lock);
-		rcu_read_lock();
-		old = rcu_dereference(mm->ioctx_table);
+		old = rcu_dereference_raw(mm->ioctx_table);
 
 		if (!old) {
 			rcu_assign_pointer(mm->ioctx_table, table);
@@ -737,12 +732,9 @@ static int kill_ioctx(struct mm_struct *mm, struct kioctx *ctx,
 
 
 	spin_lock(&mm->ioctx_lock);
-	rcu_read_lock();
-	table = rcu_dereference(mm->ioctx_table);
-
+	table = rcu_dereference_raw(mm->ioctx_table);
 	WARN_ON(ctx != table->table[ctx->id]);
 	table->table[ctx->id] = NULL;
-	rcu_read_unlock();
 	spin_unlock(&mm->ioctx_lock);
 
 	/* percpu_ref_kill() will do the necessary call_rcu() */
-- 
1.5.5.1

Re: [PATCH 0/1] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()

[Benjamin LaHaise]

On Tue, Apr 29, 2014 at 08:39:15PM +0200, Oleg Nesterov wrote:
> Completely untested and I know nothing about aio, thus needs an ack
> from maintainers or should be ignored.
> 
> But exit_aio() looks "obviously unnecessarily overcomplicated", I was
> really puzzled when I looked at this code by accident.
> 
> Kent, could you also explain kioctx->dead is atomic_t? This looks
> pointless? afaics atomic_ buys nothing in this case.

If it wasn't atomic, it would need to be volatile.

		-ben
-- 
"Thought is the essence of where you are now."

Re: [PATCH 0/1] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()

[Oleg Nesterov]

On 04/29, Benjamin LaHaise wrote:
>
> On Tue, Apr 29, 2014 at 08:39:15PM +0200, Oleg Nesterov wrote:
> >
> > Kent, could you also explain kioctx->dead is atomic_t? This looks
> > pointless? afaics atomic_ buys nothing in this case.
>
> If it wasn't atomic, it would need to be volatile.

Then ACCESS_ONCE() in aio_read_events() makes more sense. Although
I can't say I understand why it is needed.

Oleg.