Re: [PATCH 1/3] net: Kill net_mutex

[Kirill Tkhai]

Hi, Stephen,

On 21.02.2018 02:18, Stephen Hemminger wrote:
> On Mon, 19 Feb 2018 12:58:38 +0300
> Kirill Tkhai  wrote:
> 
>> +struct list_headexit_list;  /* To linked to call pernet exit
>> + * methods on dead net (net_sem
>> + * read locked), or to 
>> unregister
>> + * pernet ops (net_sem wr 
>> locked).
>> + */
> 
> Sorry, that comment is completely unparseable.
> Either you know what it does, and therefore comment is unnecessary
> Or change comment to a valid explanation of the semantics of the list.
> 
> Maybe comments about locking model are best left to where
> it is used in the code.

Let's improve it :) It's used to call pernet exit methods, and net ns logic
guarantees, we never call exit methods for the same net in parallel. How
about writing this directly without mention of net_sem? Something like this:

/* To link net to call pernet exit methods */

Or maybe you have better variant?

Thanks,
Kirill

Re: [PATCH 1/3] net: Kill net_mutex

[Stephen Hemminger]

On Mon, 19 Feb 2018 12:58:38 +0300
Kirill Tkhai  wrote:

> + struct list_headexit_list;  /* To linked to call pernet exit
> +  * methods on dead net (net_sem
> +  * read locked), or to 
> unregister
> +  * pernet ops (net_sem wr 
> locked).
> +  */

Sorry, that comment is completely unparseable.
Either you know what it does, and therefore comment is unnecessary
Or change comment to a valid explanation of the semantics of the list.

Maybe comments about locking model are best left to where
it is used in the code.

[PATCH 1/3] net: Kill net_mutex

[Kirill Tkhai]

We take net_mutex, when there are !async pernet_operations
registered, and read locking of net_sem is not enough. But
we may get rid of taking the mutex, and just change the logic
to write lock net_sem in such cases. This obviously reduces
the number of lock operations, we do.

Signed-off-by: Kirill Tkhai 
---
 include/linux/rtnetlink.h   |1 -
 include/net/net_namespace.h |   11 ++---
 net/core/net_namespace.c|   53 ++-
 3 files changed, 39 insertions(+), 26 deletions(-)

diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index e9ee9ad0a681..3573b4bf2fdf 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -35,7 +35,6 @@ extern int rtnl_trylock(void);
 extern int rtnl_is_locked(void);
 
 extern wait_queue_head_t netdev_unregistering_wq;
-extern struct mutex net_mutex;
 extern struct rw_semaphore net_sem;
 
 #ifdef CONFIG_PROVE_LOCKING
diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 9158ec1ad06f..115b01b92f4d 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -60,8 +60,11 @@ struct net {
 
struct list_headlist;   /* list of network namespaces */
struct list_headcleanup_list;   /* namespaces on death row */
-   struct list_headexit_list;  /* Use only net_mutex */
-
+   struct list_headexit_list;  /* To linked to call pernet exit
+* methods on dead net (net_sem
+* read locked), or to 
unregister
+* pernet ops (net_sem wr 
locked).
+*/
struct user_namespace   *user_ns;   /* Owning user namespace */
struct ucounts  *ucounts;
spinlock_t  nsid_lock;
@@ -89,7 +92,7 @@ struct net {
/* core fib_rules */
struct list_headrules_ops;
 
-   struct list_headfib_notifier_ops;  /* protected by net_mutex */
+   struct list_headfib_notifier_ops;  /* protected by net_sem */
 
struct net_device   *loopback_dev;  /* The loopback */
struct netns_core   core;
@@ -316,7 +319,7 @@ struct pernet_operations {
/*
 * Indicates above methods are allowed to be executed in parallel
 * with methods of any other pernet_operations, i.e. they are not
-* need synchronization via net_mutex.
+* need write locked net_sem.
 */
bool async;
 };
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index bcab9a938d6f..e89a516620dd 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -29,8 +29,6 @@
 
 static LIST_HEAD(pernet_list);
 static struct list_head *first_device = _list;
-/* Used only if there are !async pernet_operations registered */
-DEFINE_MUTEX(net_mutex);
 
 LIST_HEAD(net_namespace_list);
 EXPORT_SYMBOL_GPL(net_namespace_list);
@@ -407,6 +405,7 @@ struct net *copy_net_ns(unsigned long flags,
 {
struct ucounts *ucounts;
struct net *net;
+   unsigned write;
int rv;
 
if (!(flags & CLONE_NEWNET))
@@ -424,20 +423,26 @@ struct net *copy_net_ns(unsigned long flags,
refcount_set(>passive, 1);
net->ucounts = ucounts;
get_user_ns(user_ns);
-
-   rv = down_read_killable(_sem);
+again:
+   write = READ_ONCE(nr_sync_pernet_ops);
+   if (write)
+   rv = down_write_killable(_sem);
+   else
+   rv = down_read_killable(_sem);
if (rv < 0)
goto put_userns;
-   if (nr_sync_pernet_ops) {
-   rv = mutex_lock_killable(_mutex);
-   if (rv < 0)
-   goto up_read;
+
+   if (!write && unlikely(READ_ONCE(nr_sync_pernet_ops))) {
+   up_read(_sem);
+   goto again;
}
rv = setup_net(net, user_ns);
-   if (nr_sync_pernet_ops)
-   mutex_unlock(_mutex);
-up_read:
-   up_read(_sem);
+
+   if (write)
+   up_write(_sem);
+   else
+   up_read(_sem);
+
if (rv < 0) {
 put_userns:
put_user_ns(user_ns);
@@ -485,15 +490,23 @@ static void cleanup_net(struct work_struct *work)
struct net *net, *tmp, *last;
struct list_head net_kill_list;
LIST_HEAD(net_exit_list);
+   unsigned write;
 
/* Atomically snapshot the list of namespaces to cleanup */
spin_lock_irq(_list_lock);
list_replace_init(_list, _kill_list);
spin_unlock_irq(_list_lock);
+again:
+   write = READ_ONCE(nr_sync_pernet_ops);
+   if (write)
+   down_write(_sem);
+   else
+   down_read(_sem);
 
-   down_read(_sem);
-   if (nr_sync_pernet_ops)
-   mutex_lock(_mutex);
+   if (!write &&