| Message ID | 151120277590.3159.12461615068657469111.stgit@localhost.localdomain |
|---|---|
| State | Deferred, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | Replacing net_mutex with rw_semaphore | expand |
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@xmission.com> > Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> > --- > 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) >
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@xmission.com> >> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> >> --- >> 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) >>
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); + 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)
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@xmission.com> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com> --- include/net/net_namespace.h | 6 ++++++ net/core/net_namespace.c | 29 +++++++++++++++++++---------- 2 files changed, 25 insertions(+), 10 deletions(-)