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

[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 <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?

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);
>>  	__unregister_pernet_operations(ops);
>>  	rcu_barrier();
>>  	if (ops->id)
>>