Re: [PATCH v2 05/31] net: Allow pernet_operations to be executed in parallel

2018-01-18 Thread Kirill Tkhai 
On 17.01.2018 21:34, Andrei Vagin wrote:
> On Mon, Nov 20, 2017 at 09:32:55PM +0300, Kirill Tkhai wrote:
>> This adds new pernet_operations::async flag to indicate operations,
>> which ->init(), ->exit() and ->exit_batch() methods are allowed
>> to be executed in parallel with the methods of any other pernet_operations.
>>
>> When there are only asynchronous pernet_operations in the system,
>> net_mutex won't be taken for a net construction and destruction.
>>
>> Also, remove BUG_ON(mutex_is_locked()) from net_assign_generic()
>> without replacing with the equivalent net_sem check, as there is
>> one more lockdep assert below.
>>
>> Suggested-by: Eric W. Biederman 
>> Signed-off-by: Kirill Tkhai 
>> ---
>>  include/net/net_namespace.h |6 ++
>>  net/core/net_namespace.c|   29 +++--
>>  2 files changed, 25 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
>> index 10f99dafd5ac..db978c4755f7 100644
>> --- a/include/net/net_namespace.h
>> +++ b/include/net/net_namespace.h
>> @@ -303,6 +303,12 @@ struct pernet_operations {
>>  void (*exit_batch)(struct list_head *net_exit_list);
>>  unsigned int *id;
>>  size_t size;
>> +/*
>> + * Indicates above methods are allowe to be executed in parallel
>> + * with methods of any other pernet_operations, i.e. they are not
>> + * need synchronization via net_mutex.
>> + */
>> +bool async;
>>  };
>>  
>>  /*
>> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
>> index c4f7452906bb..550c766f73aa 100644
>> --- a/net/core/net_namespace.c
>> +++ b/net/core/net_namespace.c
>> @@ -41,8 +41,9 @@ struct net init_net = {
>>  EXPORT_SYMBOL(init_net);
>>  
>>  static bool init_net_initialized;
>> +static unsigned nr_sync_pernet_ops;
>>  /*
>> - * net_sem: protects: pernet_list, net_generic_ids,
>> + * net_sem: protects: pernet_list, net_generic_ids, nr_sync_pernet_ops,
>>   * init_net_initialized and first_device pointer.
>>   */
>>  DECLARE_RWSEM(net_sem);
>> @@ -70,11 +71,10 @@ static int net_assign_generic(struct net *net, unsigned 
>> int id, void *data)
>>  {
>>  struct net_generic *ng, *old_ng;
>>  
>> -BUG_ON(!mutex_is_locked(_mutex));
>>  BUG_ON(id < MIN_PERNET_OPS_ID);
>>  
>>  old_ng = rcu_dereference_protected(net->gen,
>> -   lockdep_is_held(_mutex));
>> +   lockdep_is_held(_sem));
>>  if (old_ng->s.len > id) {
>>  old_ng->ptr[id] = data;
>>  return 0;
>> @@ -419,11 +419,14 @@ struct net *copy_net_ns(unsigned long flags,
>>  rv = down_read_killable(_sem);
>>  if (rv < 0)
>>  goto put_userns;
>> -rv = mutex_lock_killable(_mutex);
>> -if (rv < 0)
>> -goto up_read;
>> +if (nr_sync_pernet_ops) {
>> +rv = mutex_lock_killable(_mutex);
>> +if (rv < 0)
>> +goto up_read;
>> +}
>>  rv = setup_net(net, user_ns);
>> -mutex_unlock(_mutex);
>> +if (nr_sync_pernet_ops)
>> +mutex_unlock(_mutex);
>>  up_read:
>>  up_read(_sem);
>>  if (rv < 0) {
>> @@ -453,7 +456,8 @@ static void cleanup_net(struct work_struct *work)
>>  spin_unlock_irq(_list_lock);
>>  
>>  down_read(_sem);
>> -mutex_lock(_mutex);
>> +if (nr_sync_pernet_ops)
>> +mutex_lock(_mutex);
>>  
>>  /* Don't let anyone else find us. */
>>  rtnl_lock();
>> @@ -489,7 +493,8 @@ static void cleanup_net(struct work_struct *work)
>>  list_for_each_entry_reverse(ops, _list, list)
>>  ops_exit_list(ops, _exit_list);
>>  
>> -mutex_unlock(_mutex);
>> +if (nr_sync_pernet_ops)
>> +mutex_unlock(_mutex);
>>  
>>  /* Free the net generic variables */
>>  list_for_each_entry_reverse(ops, _list, list)
>> @@ -961,6 +966,9 @@ static int register_pernet_operations(struct list_head 
>> *list,
>>  rcu_barrier();
>>  if (ops->id)
>>  ida_remove(_generic_ids, *ops->id);
>> +} else if (!ops->async) {
>> +pr_info_once("Pernet operations %ps are sync.\n", ops);
> 
> As far as I understand, we have this sync mode for backward
> compatibility with non-upstream modules, don't we? If the answer is yes,
> it may be better to add WARN_ONCE here?

There are 200+ more pernet operations requiring the review and making them 
async.
This pr_info_once() is to help people find unconverted pernet_operations they 
use
and start the work on converting them.

Thanks,
Kirill
 
>> +nr_sync_pernet_ops++;
>>  }
>>  
>>  return error;
>> @@ -968,7 +976,8 @@ static int register_pernet_operations(struct list_head 
>> *list,
>>  
>>  static void unregister_pernet_operations(struct pernet_operations *ops)
>>  {
>> -
>> +if (!ops->async)
>> +BUG_ON(nr_sync_pernet_ops-- == 0);
>>

Re: [PATCH v2 05/31] net: Allow pernet_operations to be executed in parallel

2018-01-17 Thread Andrei Vagin 
On Mon, Nov 20, 2017 at 09:32:55PM +0300, Kirill Tkhai wrote:
> This adds new pernet_operations::async flag to indicate operations,
> which ->init(), ->exit() and ->exit_batch() methods are allowed
> to be executed in parallel with the methods of any other pernet_operations.
> 
> When there are only asynchronous pernet_operations in the system,
> net_mutex won't be taken for a net construction and destruction.
> 
> Also, remove BUG_ON(mutex_is_locked()) from net_assign_generic()
> without replacing with the equivalent net_sem check, as there is
> one more lockdep assert below.
> 
> Suggested-by: Eric W. Biederman 
> Signed-off-by: Kirill Tkhai 
> ---
>  include/net/net_namespace.h |6 ++
>  net/core/net_namespace.c|   29 +++--
>  2 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
> index 10f99dafd5ac..db978c4755f7 100644
> --- a/include/net/net_namespace.h
> +++ b/include/net/net_namespace.h
> @@ -303,6 +303,12 @@ struct pernet_operations {
>   void (*exit_batch)(struct list_head *net_exit_list);
>   unsigned int *id;
>   size_t size;
> + /*
> +  * Indicates above methods are allowe to be executed in parallel
> +  * with methods of any other pernet_operations, i.e. they are not
> +  * need synchronization via net_mutex.
> +  */
> + bool async;
>  };
>  
>  /*
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index c4f7452906bb..550c766f73aa 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -41,8 +41,9 @@ struct net init_net = {
>  EXPORT_SYMBOL(init_net);
>  
>  static bool init_net_initialized;
> +static unsigned nr_sync_pernet_ops;
>  /*
> - * net_sem: protects: pernet_list, net_generic_ids,
> + * net_sem: protects: pernet_list, net_generic_ids, nr_sync_pernet_ops,
>   * init_net_initialized and first_device pointer.
>   */
>  DECLARE_RWSEM(net_sem);
> @@ -70,11 +71,10 @@ static int net_assign_generic(struct net *net, unsigned 
> int id, void *data)
>  {
>   struct net_generic *ng, *old_ng;
>  
> - BUG_ON(!mutex_is_locked(_mutex));
>   BUG_ON(id < MIN_PERNET_OPS_ID);
>  
>   old_ng = rcu_dereference_protected(net->gen,
> -lockdep_is_held(_mutex));
> +lockdep_is_held(_sem));
>   if (old_ng->s.len > id) {
>   old_ng->ptr[id] = data;
>   return 0;
> @@ -419,11 +419,14 @@ struct net *copy_net_ns(unsigned long flags,
>   rv = down_read_killable(_sem);
>   if (rv < 0)
>   goto put_userns;
> - rv = mutex_lock_killable(_mutex);
> - if (rv < 0)
> - goto up_read;
> + if (nr_sync_pernet_ops) {
> + rv = mutex_lock_killable(_mutex);
> + if (rv < 0)
> + goto up_read;
> + }
>   rv = setup_net(net, user_ns);
> - mutex_unlock(_mutex);
> + if (nr_sync_pernet_ops)
> + mutex_unlock(_mutex);
>  up_read:
>   up_read(_sem);
>   if (rv < 0) {
> @@ -453,7 +456,8 @@ static void cleanup_net(struct work_struct *work)
>   spin_unlock_irq(_list_lock);
>  
>   down_read(_sem);
> - mutex_lock(_mutex);
> + if (nr_sync_pernet_ops)
> + mutex_lock(_mutex);
>  
>   /* Don't let anyone else find us. */
>   rtnl_lock();
> @@ -489,7 +493,8 @@ static void cleanup_net(struct work_struct *work)
>   list_for_each_entry_reverse(ops, _list, list)
>   ops_exit_list(ops, _exit_list);
>  
> - mutex_unlock(_mutex);
> + if (nr_sync_pernet_ops)
> + mutex_unlock(_mutex);
>  
>   /* Free the net generic variables */
>   list_for_each_entry_reverse(ops, _list, list)
> @@ -961,6 +966,9 @@ static int register_pernet_operations(struct list_head 
> *list,
>   rcu_barrier();
>   if (ops->id)
>   ida_remove(_generic_ids, *ops->id);
> + } else if (!ops->async) {
> + pr_info_once("Pernet operations %ps are sync.\n", ops);

As far as I understand, we have this sync mode for backward
compatibility with non-upstream modules, don't we? If the answer is yes,
it may be better to add WARN_ONCE here?

> + nr_sync_pernet_ops++;
>   }
>  
>   return error;
> @@ -968,7 +976,8 @@ static int register_pernet_operations(struct list_head 
> *list,
>  
>  static void unregister_pernet_operations(struct pernet_operations *ops)
>  {
> - 
> + if (!ops->async)
> + BUG_ON(nr_sync_pernet_ops-- == 0);
>   __unregister_pernet_operations(ops);
>   rcu_barrier();
>   if (ops->id)
>

[PATCH v2 05/31] net: Allow pernet_operations to be executed in parallel

2017-11-20 Thread Kirill Tkhai 
This adds new pernet_operations::async flag to indicate operations,
which ->init(), ->exit() and ->exit_batch() methods are allowed
to be executed in parallel with the methods of any other pernet_operations.

When there are only asynchronous pernet_operations in the system,
net_mutex won't be taken for a net construction and destruction.

Also, remove BUG_ON(mutex_is_locked()) from net_assign_generic()
without replacing with the equivalent net_sem check, as there is
one more lockdep assert below.

Suggested-by: Eric W. Biederman 
Signed-off-by: Kirill Tkhai 
---
 include/net/net_namespace.h |6 ++
 net/core/net_namespace.c|   29 +++--
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 10f99dafd5ac..db978c4755f7 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -303,6 +303,12 @@ struct pernet_operations {
void (*exit_batch)(struct list_head *net_exit_list);
unsigned int *id;
size_t size;
+   /*
+* Indicates above methods are allowe to be executed in parallel
+* with methods of any other pernet_operations, i.e. they are not
+* need synchronization via net_mutex.
+*/
+   bool async;
 };
 
 /*
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index c4f7452906bb..550c766f73aa 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -41,8 +41,9 @@ struct net init_net = {
 EXPORT_SYMBOL(init_net);
 
 static bool init_net_initialized;
+static unsigned nr_sync_pernet_ops;
 /*
- * net_sem: protects: pernet_list, net_generic_ids,
+ * net_sem: protects: pernet_list, net_generic_ids, nr_sync_pernet_ops,
  * init_net_initialized and first_device pointer.
  */
 DECLARE_RWSEM(net_sem);
@@ -70,11 +71,10 @@ static int net_assign_generic(struct net *net, unsigned int 
id, void *data)
 {
struct net_generic *ng, *old_ng;
 
-   BUG_ON(!mutex_is_locked(_mutex));
BUG_ON(id < MIN_PERNET_OPS_ID);
 
old_ng = rcu_dereference_protected(net->gen,
-  lockdep_is_held(_mutex));
+  lockdep_is_held(_sem));
if (old_ng->s.len > id) {
old_ng->ptr[id] = data;
return 0;
@@ -419,11 +419,14 @@ struct net *copy_net_ns(unsigned long flags,
rv = down_read_killable(_sem);
if (rv < 0)
goto put_userns;
-   rv = mutex_lock_killable(_mutex);
-   if (rv < 0)
-   goto up_read;
+   if (nr_sync_pernet_ops) {
+   rv = mutex_lock_killable(_mutex);
+   if (rv < 0)
+   goto up_read;
+   }
rv = setup_net(net, user_ns);
-   mutex_unlock(_mutex);
+   if (nr_sync_pernet_ops)
+   mutex_unlock(_mutex);
 up_read:
up_read(_sem);
if (rv < 0) {
@@ -453,7 +456,8 @@ static void cleanup_net(struct work_struct *work)
spin_unlock_irq(_list_lock);
 
down_read(_sem);
-   mutex_lock(_mutex);
+   if (nr_sync_pernet_ops)
+   mutex_lock(_mutex);
 
/* Don't let anyone else find us. */
rtnl_lock();
@@ -489,7 +493,8 @@ static void cleanup_net(struct work_struct *work)
list_for_each_entry_reverse(ops, _list, list)
ops_exit_list(ops, _exit_list);
 
-   mutex_unlock(_mutex);
+   if (nr_sync_pernet_ops)
+   mutex_unlock(_mutex);
 
/* Free the net generic variables */
list_for_each_entry_reverse(ops, _list, list)
@@ -961,6 +966,9 @@ static int register_pernet_operations(struct list_head 
*list,
rcu_barrier();
if (ops->id)
ida_remove(_generic_ids, *ops->id);
+   } else if (!ops->async) {
+   pr_info_once("Pernet operations %ps are sync.\n", ops);
+   nr_sync_pernet_ops++;
}
 
return error;
@@ -968,7 +976,8 @@ static int register_pernet_operations(struct list_head 
*list,
 
 static void unregister_pernet_operations(struct pernet_operations *ops)
 {
-   
+   if (!ops->async)
+   BUG_ON(nr_sync_pernet_ops-- == 0);
__unregister_pernet_operations(ops);
rcu_barrier();
if (ops->id)