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

[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 <ebiederm@xxxxxxxxxxxx>
> Signed-off-by: Kirill Tkhai <ktkhai@xxxxxxxxxxxxx>
> ---
>  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(&net_mutex));
>  	BUG_ON(id < MIN_PERNET_OPS_ID);
>  
>  	old_ng = rcu_dereference_protected(net->gen,
> -					   lockdep_is_held(&net_mutex));
> +					   lockdep_is_held(&net_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(&net_sem);
>  	if (rv < 0)
>  		goto put_userns;
> -	rv = mutex_lock_killable(&net_mutex);
> -	if (rv < 0)
> -		goto up_read;
> +	if (nr_sync_pernet_ops) {
> +		rv = mutex_lock_killable(&net_mutex);
> +		if (rv < 0)
> +			goto up_read;
> +	}
>  	rv = setup_net(net, user_ns);
> -	mutex_unlock(&net_mutex);
> +	if (nr_sync_pernet_ops)
> +		mutex_unlock(&net_mutex);
>  up_read:
>  	up_read(&net_sem);
>  	if (rv < 0) {
> @@ -453,7 +456,8 @@ static void cleanup_net(struct work_struct *work)
>  	spin_unlock_irq(&cleanup_list_lock);
>  
>  	down_read(&net_sem);
> -	mutex_lock(&net_mutex);
> +	if (nr_sync_pernet_ops)
> +		mutex_lock(&net_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, &pernet_list, list)
>  		ops_exit_list(ops, &net_exit_list);
>  
> -	mutex_unlock(&net_mutex);
> +	if (nr_sync_pernet_ops)
> +		mutex_unlock(&net_mutex);
>  
>  	/* Free the net generic variables */
>  	list_for_each_entry_reverse(ops, &pernet_list, list)
> @@ -961,6 +966,9 @@ static int register_pernet_operations(struct list_head *list,
>  		rcu_barrier();
>  		if (ops->id)
>  			ida_remove(&net_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)
>