rcu.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ipc: use a work queue to free_ipc
@ 2020-02-11 16:24 Giuseppe Scrivano
  2020-02-16 14:11 ` Paul E. McKenney
  0 siblings, 1 reply; 4+ messages in thread
From: Giuseppe Scrivano @ 2020-02-11 16:24 UTC (permalink / raw)
  To: linux-kernel; +Cc: rcu, ebiederm, paulmck

it avoids blocking on synchronize_rcu() in kern_umount().

the code:

\#define _GNU_SOURCE
\#include <sched.h>
\#include <error.h>
\#include <errno.h>
\#include <stdlib.h>
int main()
{
  int i;
  for (i  = 0; i < 1000; i++)
    if (unshare (CLONE_NEWIPC) < 0)
      error (EXIT_FAILURE, errno, "unshare");
}

gets from:

	Command being timed: "./ipc-namespace"
	User time (seconds): 0.00
	System time (seconds): 0.06
	Percent of CPU this job got: 0%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:08.05

to:

	Command being timed: "./ipc-namespace"
	User time (seconds): 0.00
	System time (seconds): 0.02
	Percent of CPU this job got: 96%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.03

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
---
 include/linux/ipc_namespace.h |  2 ++
 ipc/namespace.c               | 17 +++++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h
index c309f43bde45..a06a78c67f19 100644
--- a/include/linux/ipc_namespace.h
+++ b/include/linux/ipc_namespace.h
@@ -68,6 +68,8 @@ struct ipc_namespace {
 	struct user_namespace *user_ns;
 	struct ucounts *ucounts;
 
+	struct llist_node mnt_llist;
+
 	struct ns_common ns;
 } __randomize_layout;
 
diff --git a/ipc/namespace.c b/ipc/namespace.c
index b3ca1476ca51..37d27e1b807a 100644
--- a/ipc/namespace.c
+++ b/ipc/namespace.c
@@ -117,6 +117,7 @@ void free_ipcs(struct ipc_namespace *ns, struct ipc_ids *ids,
 
 static void free_ipc_ns(struct ipc_namespace *ns)
 {
+	mq_put_mnt(ns);
 	sem_exit_ns(ns);
 	msg_exit_ns(ns);
 	shm_exit_ns(ns);
@@ -127,6 +128,17 @@ static void free_ipc_ns(struct ipc_namespace *ns)
 	kfree(ns);
 }
 
+static LLIST_HEAD(free_ipc_list);
+static void free_ipc(struct work_struct *unused)
+{
+	struct llist_node *node = llist_del_all(&free_ipc_list);
+	struct ipc_namespace *n, *t;
+
+	llist_for_each_entry_safe(n, t, node, mnt_llist)
+		free_ipc_ns(n);
+}
+static DECLARE_WORK(free_ipc_work, free_ipc);
+
 /*
  * put_ipc_ns - drop a reference to an ipc namespace.
  * @ns: the namespace to put
@@ -148,8 +160,9 @@ void put_ipc_ns(struct ipc_namespace *ns)
 	if (refcount_dec_and_lock(&ns->count, &mq_lock)) {
 		mq_clear_sbinfo(ns);
 		spin_unlock(&mq_lock);
-		mq_put_mnt(ns);
-		free_ipc_ns(ns);
+
+		if (llist_add(&ns->mnt_llist, &free_ipc_list))
+			schedule_work(&free_ipc_work);
 	}
 }
 
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] ipc: use a work queue to free_ipc
  2020-02-11 16:24 [PATCH] ipc: use a work queue to free_ipc Giuseppe Scrivano
@ 2020-02-16 14:11 ` Paul E. McKenney
  2020-02-17  9:23   ` Giuseppe Scrivano
  0 siblings, 1 reply; 4+ messages in thread
From: Paul E. McKenney @ 2020-02-16 14:11 UTC (permalink / raw)
  To: Giuseppe Scrivano; +Cc: linux-kernel, rcu, ebiederm

On Tue, Feb 11, 2020 at 05:24:08PM +0100, Giuseppe Scrivano wrote:
> it avoids blocking on synchronize_rcu() in kern_umount().
> 
> the code:
> 
> \#define _GNU_SOURCE
> \#include <sched.h>
> \#include <error.h>
> \#include <errno.h>
> \#include <stdlib.h>
> int main()
> {
>   int i;
>   for (i  = 0; i < 1000; i++)
>     if (unshare (CLONE_NEWIPC) < 0)
>       error (EXIT_FAILURE, errno, "unshare");
> }
> 
> gets from:
> 
> 	Command being timed: "./ipc-namespace"
> 	User time (seconds): 0.00
> 	System time (seconds): 0.06
> 	Percent of CPU this job got: 0%
> 	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:08.05
> 
> to:
> 
> 	Command being timed: "./ipc-namespace"
> 	User time (seconds): 0.00
> 	System time (seconds): 0.02
> 	Percent of CPU this job got: 96%
> 	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.03

Nice speedup!

However, I am not convinced that the code shown below is safe.

I believe that you need either a synchronize_rcu() in your free_ipc()
function or that you need to pass free_ipc() to queue_rcu_work() instead
of directly schedule_work().  As things are, I would expect you to see
free_ipc_ns() being invoke too soon on heavily loaded CONFIG_PREEMPT=y
kernels.  Which can be quite a pain to debug!

Or am I missing something?

							Thanx, Paul

> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
> ---
>  include/linux/ipc_namespace.h |  2 ++
>  ipc/namespace.c               | 17 +++++++++++++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h
> index c309f43bde45..a06a78c67f19 100644
> --- a/include/linux/ipc_namespace.h
> +++ b/include/linux/ipc_namespace.h
> @@ -68,6 +68,8 @@ struct ipc_namespace {
>  	struct user_namespace *user_ns;
>  	struct ucounts *ucounts;
>  
> +	struct llist_node mnt_llist;
> +
>  	struct ns_common ns;
>  } __randomize_layout;
>  
> diff --git a/ipc/namespace.c b/ipc/namespace.c
> index b3ca1476ca51..37d27e1b807a 100644
> --- a/ipc/namespace.c
> +++ b/ipc/namespace.c
> @@ -117,6 +117,7 @@ void free_ipcs(struct ipc_namespace *ns, struct ipc_ids *ids,
>  
>  static void free_ipc_ns(struct ipc_namespace *ns)
>  {
> +	mq_put_mnt(ns);
>  	sem_exit_ns(ns);
>  	msg_exit_ns(ns);
>  	shm_exit_ns(ns);
> @@ -127,6 +128,17 @@ static void free_ipc_ns(struct ipc_namespace *ns)
>  	kfree(ns);
>  }
>  
> +static LLIST_HEAD(free_ipc_list);
> +static void free_ipc(struct work_struct *unused)
> +{
> +	struct llist_node *node = llist_del_all(&free_ipc_list);
> +	struct ipc_namespace *n, *t;
> +
> +	llist_for_each_entry_safe(n, t, node, mnt_llist)
> +		free_ipc_ns(n);
> +}
> +static DECLARE_WORK(free_ipc_work, free_ipc);
> +
>  /*
>   * put_ipc_ns - drop a reference to an ipc namespace.
>   * @ns: the namespace to put
> @@ -148,8 +160,9 @@ void put_ipc_ns(struct ipc_namespace *ns)
>  	if (refcount_dec_and_lock(&ns->count, &mq_lock)) {
>  		mq_clear_sbinfo(ns);
>  		spin_unlock(&mq_lock);
> -		mq_put_mnt(ns);
> -		free_ipc_ns(ns);
> +
> +		if (llist_add(&ns->mnt_llist, &free_ipc_list))
> +			schedule_work(&free_ipc_work);
>  	}
>  }
>  
> -- 
> 2.24.1
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] ipc: use a work queue to free_ipc
  2020-02-16 14:11 ` Paul E. McKenney
@ 2020-02-17  9:23   ` Giuseppe Scrivano
  2020-02-17 18:08     ` Paul E. McKenney
  0 siblings, 1 reply; 4+ messages in thread
From: Giuseppe Scrivano @ 2020-02-17  9:23 UTC (permalink / raw)
  To: Paul E. McKenney; +Cc: linux-kernel, rcu, ebiederm

Hi Paul,

"Paul E. McKenney" <paulmck@kernel.org> writes:

> Nice speedup!
>
> However, I am not convinced that the code shown below is safe.
>
> I believe that you need either a synchronize_rcu() in your free_ipc()
> function or that you need to pass free_ipc() to queue_rcu_work() instead
> of directly schedule_work().  As things are, I would expect you to see
> free_ipc_ns() being invoke too soon on heavily loaded CONFIG_PREEMPT=y
> kernels.  Which can be quite a pain to debug!
>
> Or am I missing something?

thanks for the review!

Would being called too soon be a problem?  The current version calls
free_ipc_ns() as part of the put_ipc_ns cleanup.

free_ipc() calls immediately synchronize_rcu() through free_ipc_ns():

free_ipc_ns() -> mq_put_mnt() -> kern_unmount() -> synchronize_rcu()

Do you think we should make it more explicit and add a synchronize_rcu()
as part of the free_ipc_ns() function itself?

Regards,
Giuseppe


>
> 							Thanx, Paul
>
>> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
>> ---
>>  include/linux/ipc_namespace.h |  2 ++
>>  ipc/namespace.c               | 17 +++++++++++++++--
>>  2 files changed, 17 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h
>> index c309f43bde45..a06a78c67f19 100644
>> --- a/include/linux/ipc_namespace.h
>> +++ b/include/linux/ipc_namespace.h
>> @@ -68,6 +68,8 @@ struct ipc_namespace {
>>  	struct user_namespace *user_ns;
>>  	struct ucounts *ucounts;
>>  
>> +	struct llist_node mnt_llist;
>> +
>>  	struct ns_common ns;
>>  } __randomize_layout;
>>  
>> diff --git a/ipc/namespace.c b/ipc/namespace.c
>> index b3ca1476ca51..37d27e1b807a 100644
>> --- a/ipc/namespace.c
>> +++ b/ipc/namespace.c
>> @@ -117,6 +117,7 @@ void free_ipcs(struct ipc_namespace *ns, struct ipc_ids *ids,
>>  
>>  static void free_ipc_ns(struct ipc_namespace *ns)
>>  {
>> +	mq_put_mnt(ns);
>>  	sem_exit_ns(ns);
>>  	msg_exit_ns(ns);
>>  	shm_exit_ns(ns);
>> @@ -127,6 +128,17 @@ static void free_ipc_ns(struct ipc_namespace *ns)
>>  	kfree(ns);
>>  }
>>  
>> +static LLIST_HEAD(free_ipc_list);
>> +static void free_ipc(struct work_struct *unused)
>> +{
>> +	struct llist_node *node = llist_del_all(&free_ipc_list);
>> +	struct ipc_namespace *n, *t;
>> +
>> +	llist_for_each_entry_safe(n, t, node, mnt_llist)
>> +		free_ipc_ns(n);
>> +}
>> +static DECLARE_WORK(free_ipc_work, free_ipc);
>> +
>>  /*
>>   * put_ipc_ns - drop a reference to an ipc namespace.
>>   * @ns: the namespace to put
>> @@ -148,8 +160,9 @@ void put_ipc_ns(struct ipc_namespace *ns)
>>  	if (refcount_dec_and_lock(&ns->count, &mq_lock)) {
>>  		mq_clear_sbinfo(ns);
>>  		spin_unlock(&mq_lock);
>> -		mq_put_mnt(ns);
>> -		free_ipc_ns(ns);
>> +
>> +		if (llist_add(&ns->mnt_llist, &free_ipc_list))
>> +			schedule_work(&free_ipc_work);
>>  	}
>>  }
>>  
>> -- 
>> 2.24.1
>> 


^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] ipc: use a work queue to free_ipc
  2020-02-17  9:23   ` Giuseppe Scrivano
@ 2020-02-17 18:08     ` Paul E. McKenney
  0 siblings, 0 replies; 4+ messages in thread
From: Paul E. McKenney @ 2020-02-17 18:08 UTC (permalink / raw)
  To: Giuseppe Scrivano; +Cc: linux-kernel, rcu, ebiederm

On Mon, Feb 17, 2020 at 10:23:18AM +0100, Giuseppe Scrivano wrote:
> Hi Paul,
> 
> "Paul E. McKenney" <paulmck@kernel.org> writes:
> 
> > Nice speedup!
> >
> > However, I am not convinced that the code shown below is safe.
> >
> > I believe that you need either a synchronize_rcu() in your free_ipc()
> > function or that you need to pass free_ipc() to queue_rcu_work() instead
> > of directly schedule_work().  As things are, I would expect you to see
> > free_ipc_ns() being invoke too soon on heavily loaded CONFIG_PREEMPT=y
> > kernels.  Which can be quite a pain to debug!
> >
> > Or am I missing something?
> 
> thanks for the review!
> 
> Would being called too soon be a problem?  The current version calls
> free_ipc_ns() as part of the put_ipc_ns cleanup.
> 
> free_ipc() calls immediately synchronize_rcu() through free_ipc_ns():
> 
> free_ipc_ns() -> mq_put_mnt() -> kern_unmount() -> synchronize_rcu()

Ah, I did not look deeply enough.

> Do you think we should make it more explicit and add a synchronize_rcu()
> as part of the free_ipc_ns() function itself?

Maybe just a comment on the free_ipc_ns() saying that it waits for
a grace period.

							Thanx, Paul

> Regards,
> Giuseppe
> 
> 
> >
> > 							Thanx, Paul
> >
> >> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
> >> ---
> >>  include/linux/ipc_namespace.h |  2 ++
> >>  ipc/namespace.c               | 17 +++++++++++++++--
> >>  2 files changed, 17 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h
> >> index c309f43bde45..a06a78c67f19 100644
> >> --- a/include/linux/ipc_namespace.h
> >> +++ b/include/linux/ipc_namespace.h
> >> @@ -68,6 +68,8 @@ struct ipc_namespace {
> >>  	struct user_namespace *user_ns;
> >>  	struct ucounts *ucounts;
> >>  
> >> +	struct llist_node mnt_llist;
> >> +
> >>  	struct ns_common ns;
> >>  } __randomize_layout;
> >>  
> >> diff --git a/ipc/namespace.c b/ipc/namespace.c
> >> index b3ca1476ca51..37d27e1b807a 100644
> >> --- a/ipc/namespace.c
> >> +++ b/ipc/namespace.c
> >> @@ -117,6 +117,7 @@ void free_ipcs(struct ipc_namespace *ns, struct ipc_ids *ids,
> >>  
> >>  static void free_ipc_ns(struct ipc_namespace *ns)
> >>  {
> >> +	mq_put_mnt(ns);
> >>  	sem_exit_ns(ns);
> >>  	msg_exit_ns(ns);
> >>  	shm_exit_ns(ns);
> >> @@ -127,6 +128,17 @@ static void free_ipc_ns(struct ipc_namespace *ns)
> >>  	kfree(ns);
> >>  }
> >>  
> >> +static LLIST_HEAD(free_ipc_list);
> >> +static void free_ipc(struct work_struct *unused)
> >> +{
> >> +	struct llist_node *node = llist_del_all(&free_ipc_list);
> >> +	struct ipc_namespace *n, *t;
> >> +
> >> +	llist_for_each_entry_safe(n, t, node, mnt_llist)
> >> +		free_ipc_ns(n);
> >> +}
> >> +static DECLARE_WORK(free_ipc_work, free_ipc);
> >> +
> >>  /*
> >>   * put_ipc_ns - drop a reference to an ipc namespace.
> >>   * @ns: the namespace to put
> >> @@ -148,8 +160,9 @@ void put_ipc_ns(struct ipc_namespace *ns)
> >>  	if (refcount_dec_and_lock(&ns->count, &mq_lock)) {
> >>  		mq_clear_sbinfo(ns);
> >>  		spin_unlock(&mq_lock);
> >> -		mq_put_mnt(ns);
> >> -		free_ipc_ns(ns);
> >> +
> >> +		if (llist_add(&ns->mnt_llist, &free_ipc_list))
> >> +			schedule_work(&free_ipc_work);
> >>  	}
> >>  }
> >>  
> >> -- 
> >> 2.24.1
> >> 
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-02-17 18:08 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-11 16:24 [PATCH] ipc: use a work queue to free_ipc Giuseppe Scrivano
2020-02-16 14:11 ` Paul E. McKenney
2020-02-17  9:23   ` Giuseppe Scrivano
2020-02-17 18:08     ` Paul E. McKenney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).