net: Convert net_mutex into rw_semaphore and down read it on net->init/->exit

Curently mutex is used to protect pernet operations list. It makes
cleanup_net() to execute ->exit methods of the same operations set,
which was used on the time of ->init, even after net namespace is
unlinked from net_namespace_list.

But the problem is it's need to synchronize_rcu() after net is removed
from net_namespace_list():

Destroy net_ns:
cleanup_net()
  mutex_lock(&net_mutex)
  list_del_rcu(&net->list)
  synchronize_rcu()                                  <--- Sleep there for ages
  list_for_each_entry_reverse(ops, &pernet_list, list)
    ops_exit_list(ops, &net_exit_list)
  list_for_each_entry_reverse(ops, &pernet_list, list)
    ops_free_list(ops, &net_exit_list)
  mutex_unlock(&net_mutex)

This primitive is not fast, especially on the systems with many processors
and/or when preemptible RCU is enabled in config. So, all the time, while
cleanup_net() is waiting for RCU grace period, creation of new net namespaces
is not possible, the tasks, who makes it, are sleeping on the same mutex:

Create net_ns:
copy_net_ns()
  mutex_lock_killable(&net_mutex)                    <--- Sleep there for ages

The solution is to convert net_mutex to the rw_semaphore. Then,
pernet_operations::init/::exit methods, modifying the net-related data,
will require down_read() locking only, while down_write() will be used
for changing pernet_list.

This gives signify performance increase, like you may see below. There
is measured sequential net namespace creation in a cycle, in single
thread, without other tasks (single user mode):

1)int main(int argc, char *argv[])
{
        unsigned nr;
        if (argc < 2) {
                fprintf(stderr, "Provide nr iterations arg\n");
                return 1;
        }
        nr = atoi(argv[1]);
        while (nr-- > 0) {
                if (unshare(CLONE_NEWNET)) {
                        perror("Can't unshare");
                        return 1;
                }
        }
        return 0;
}

Origin, 100000 unshare():
0.03user 23.14system 1:39.85elapsed 23%CPU

Patched, 100000 unshare():
0.03user 67.49system 1:08.34elapsed 98%CPU

2)for i in {1..10000}; do unshare -n bash -c exit; done

Origin:
real 1m24,190s
user 0m6,225s
sys 0m15,132s

Patched:
real 0m18,235s   (4.6 times faster)
user 0m4,544s
sys 0m13,796s

This patch requires commit 76f8507f7a64 "locking/rwsem: Add down_read_killable()"
from Linus tree (not in net-next yet).

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 include/linux/rtnetlink.h   |    2 +-
 include/net/net_namespace.h |    9 +++++++--
 net/core/net_namespace.c    |   40 ++++++++++++++++++++--------------------
 net/core/rtnetlink.c        |    4 ++--
 4 files changed, 30 insertions(+), 25 deletions(-)

On Tue, 2017-11-14 at 16:53 +0300, Kirill Tkhai wrote:
> Curently mutex is used to protect pernet operations list. It makes
> cleanup_net() to execute ->exit methods of the same operations set,
> which was used on the time of ->init, even after net namespace is
> unlinked from net_namespace_list.
> 
...

> The solution is to convert net_mutex to the rw_semaphore. Then,
> pernet_operations::init/::exit methods, modifying the net-related data,
> will require down_read() locking only, while down_write() will be used
> for changing pernet_list.
> 
...

> This patch requires commit 76f8507f7a64 "locking/rwsem: Add down_read_killable()"
> from Linus tree (not in net-next yet).


Looks great, thanks for doing this.

I wonder if the down_read_killable() is really needed, maybe a
down_read() would not be blocking the thread too long after this change.

On 14.11.2017 20:07, Eric Dumazet wrote:
> On Tue, 2017-11-14 at 16:53 +0300, Kirill Tkhai wrote:
>> Curently mutex is used to protect pernet operations list. It makes
>> cleanup_net() to execute ->exit methods of the same operations set,
>> which was used on the time of ->init, even after net namespace is
>> unlinked from net_namespace_list.
>>
> ...
> 
>> The solution is to convert net_mutex to the rw_semaphore. Then,
>> pernet_operations::init/::exit methods, modifying the net-related data,
>> will require down_read() locking only, while down_write() will be used
>> for changing pernet_list.
>>
> ...
> 
>> This patch requires commit 76f8507f7a64 "locking/rwsem: Add down_read_killable()"
>> from Linus tree (not in net-next yet).
> 
> 
> Looks great, thanks for doing this.
> 
> I wonder if the down_read_killable() is really needed, maybe a
> down_read() would not be blocking the thread too long after this change.

I'm afraid there are spin_unlock() or similar actions leading to preempt_enable(),
which are made under the mutex/new rw_sem. They may be preempted and leave a cpu
for some time. So the possibility to break the wait may be useful in some way...

On Tue, Nov 14, 2017 at 04:53:33PM +0300, Kirill Tkhai wrote:
> Curently mutex is used to protect pernet operations list. It makes
> cleanup_net() to execute ->exit methods of the same operations set,
> which was used on the time of ->init, even after net namespace is
> unlinked from net_namespace_list.
> 
> But the problem is it's need to synchronize_rcu() after net is removed
> from net_namespace_list():
> 
> Destroy net_ns:
> cleanup_net()
>   mutex_lock(&net_mutex)
>   list_del_rcu(&net->list)
>   synchronize_rcu()                                  <--- Sleep there for ages
>   list_for_each_entry_reverse(ops, &pernet_list, list)
>     ops_exit_list(ops, &net_exit_list)
>   list_for_each_entry_reverse(ops, &pernet_list, list)
>     ops_free_list(ops, &net_exit_list)
>   mutex_unlock(&net_mutex)
> 
> This primitive is not fast, especially on the systems with many processors
> and/or when preemptible RCU is enabled in config. So, all the time, while
> cleanup_net() is waiting for RCU grace period, creation of new net namespaces
> is not possible, the tasks, who makes it, are sleeping on the same mutex:
> 
> Create net_ns:
> copy_net_ns()
>   mutex_lock_killable(&net_mutex)                    <--- Sleep there for ages
> 
> The solution is to convert net_mutex to the rw_semaphore. Then,
> pernet_operations::init/::exit methods, modifying the net-related data,
> will require down_read() locking only, while down_write() will be used
> for changing pernet_list.
> 
> This gives signify performance increase, like you may see below. There
> is measured sequential net namespace creation in a cycle, in single
> thread, without other tasks (single user mode):
> 
> 1)int main(int argc, char *argv[])
> {
>         unsigned nr;
>         if (argc < 2) {
>                 fprintf(stderr, "Provide nr iterations arg\n");
>                 return 1;
>         }
>         nr = atoi(argv[1]);
>         while (nr-- > 0) {
>                 if (unshare(CLONE_NEWNET)) {
>                         perror("Can't unshare");
>                         return 1;
>                 }
>         }
>         return 0;
> }
> 
> Origin, 100000 unshare():
> 0.03user 23.14system 1:39.85elapsed 23%CPU
> 
> Patched, 100000 unshare():
> 0.03user 67.49system 1:08.34elapsed 98%CPU
> 
> 2)for i in {1..10000}; do unshare -n bash -c exit; done

Hi Kirill,

This mutex has another role. You know that net namespaces are destroyed
asynchronously, and the net mutex gurantees that a backlog will be not
big. If we have something in backlog, we know that it will be handled
before creating a new net ns.

As far as I remember net namespaces are created much faster than
they are destroyed, so with this changes we can create a really big
backlog, can't we?

There was a discussion a few month ago:
https://lists.onap.org/pipermail/containers/2016-October/037509.html


> 
> Origin:
> real 1m24,190s
> user 0m6,225s
> sys 0m15,132s

Here you measure time of creating and destroying net namespaces.

> 
> Patched:
> real 0m18,235s   (4.6 times faster)
> user 0m4,544s
> sys 0m13,796s

But here you measure time of crearing namespaces and you know nothing
when they will be destroyed.

Thanks,
Andrei

On Tue, 2017-11-14 at 09:44 -0800, Andrei Vagin wrote:
> On Tue, Nov 14, 2017 at 04:53:33PM +0300, Kirill Tkhai wrote:
> > Curently mutex is used to protect pernet operations list. It makes
> > cleanup_net() to execute ->exit methods of the same operations set,
> > which was used on the time of ->init, even after net namespace is
> > unlinked from net_namespace_list.
> > 
> > But the problem is it's need to synchronize_rcu() after net is removed
> > from net_namespace_list():
> > 
> > Destroy net_ns:
> > cleanup_net()
> >   mutex_lock(&net_mutex)
> >   list_del_rcu(&net->list)
> >   synchronize_rcu()                                  <--- Sleep there for ages
> >   list_for_each_entry_reverse(ops, &pernet_list, list)
> >     ops_exit_list(ops, &net_exit_list)
> >   list_for_each_entry_reverse(ops, &pernet_list, list)
> >     ops_free_list(ops, &net_exit_list)
> >   mutex_unlock(&net_mutex)
> > 
> > This primitive is not fast, especially on the systems with many processors
> > and/or when preemptible RCU is enabled in config. So, all the time, while
> > cleanup_net() is waiting for RCU grace period, creation of new net namespaces
> > is not possible, the tasks, who makes it, are sleeping on the same mutex:
> > 
> > Create net_ns:
> > copy_net_ns()
> >   mutex_lock_killable(&net_mutex)                    <--- Sleep there for ages
> > 
> > The solution is to convert net_mutex to the rw_semaphore. Then,
> > pernet_operations::init/::exit methods, modifying the net-related data,
> > will require down_read() locking only, while down_write() will be used
> > for changing pernet_list.
> > 
> > This gives signify performance increase, like you may see below. There
> > is measured sequential net namespace creation in a cycle, in single
> > thread, without other tasks (single user mode):
> > 
> > 1)int main(int argc, char *argv[])
> > {
> >         unsigned nr;
> >         if (argc < 2) {
> >                 fprintf(stderr, "Provide nr iterations arg\n");
> >                 return 1;
> >         }
> >         nr = atoi(argv[1]);
> >         while (nr-- > 0) {
> >                 if (unshare(CLONE_NEWNET)) {
> >                         perror("Can't unshare");
> >                         return 1;
> >                 }
> >         }
> >         return 0;
> > }
> > 
> > Origin, 100000 unshare():
> > 0.03user 23.14system 1:39.85elapsed 23%CPU
> > 
> > Patched, 100000 unshare():
> > 0.03user 67.49system 1:08.34elapsed 98%CPU
> > 
> > 2)for i in {1..10000}; do unshare -n bash -c exit; done
> 
> Hi Kirill,
> 
> This mutex has another role. You know that net namespaces are destroyed
> asynchronously, and the net mutex gurantees that a backlog will be not
> big. If we have something in backlog, we know that it will be handled
> before creating a new net ns.
> 
> As far as I remember net namespaces are created much faster than
> they are destroyed, so with this changes we can create a really big
> backlog, can't we?

Please take a look at the recent patches I did :

8ca712c373a462cfa1b62272870b6c2c74aa83f9 Merge branch 'net-speedup-netns-create-delete-time'
64bc17811b72758753e2b64cd8f2a63812c61fe1 ipv4: speedup ipv6 tunnels dismantle
bb401caefe9d2c65e0c0fa23b21deecfbfa473fe ipv6: speedup ipv6 tunnels dismantle
789e6ddb0b2fb5d5024b760b178a47876e4de7a6 tcp: batch tcp_net_metrics_exit
a90c9347e90ed1e9323d71402ed18023bc910cd8 ipv6: addrlabel: per netns list
d464e84eed02993d40ad55fdc19f4523e4deee5b kobject: factorize skb setup in kobject_uevent_net_broadcast()
4a336a23d619e96aef37d4d054cfadcdd1b581ba kobject: copy env blob in one go
16dff336b33d87c15d9cbe933cfd275aae2a8251 kobject: add kobject_uevent_net_broadcast()

On 14.11.2017 20:44, Andrei Vagin wrote:
> On Tue, Nov 14, 2017 at 04:53:33PM +0300, Kirill Tkhai wrote:
>> Curently mutex is used to protect pernet operations list. It makes
>> cleanup_net() to execute ->exit methods of the same operations set,
>> which was used on the time of ->init, even after net namespace is
>> unlinked from net_namespace_list.
>>
>> But the problem is it's need to synchronize_rcu() after net is removed
>> from net_namespace_list():
>>
>> Destroy net_ns:
>> cleanup_net()
>>   mutex_lock(&net_mutex)
>>   list_del_rcu(&net->list)
>>   synchronize_rcu()                                  <--- Sleep there for ages
>>   list_for_each_entry_reverse(ops, &pernet_list, list)
>>     ops_exit_list(ops, &net_exit_list)
>>   list_for_each_entry_reverse(ops, &pernet_list, list)
>>     ops_free_list(ops, &net_exit_list)
>>   mutex_unlock(&net_mutex)
>>
>> This primitive is not fast, especially on the systems with many processors
>> and/or when preemptible RCU is enabled in config. So, all the time, while
>> cleanup_net() is waiting for RCU grace period, creation of new net namespaces
>> is not possible, the tasks, who makes it, are sleeping on the same mutex:
>>
>> Create net_ns:
>> copy_net_ns()
>>   mutex_lock_killable(&net_mutex)                    <--- Sleep there for ages
>>
>> The solution is to convert net_mutex to the rw_semaphore. Then,
>> pernet_operations::init/::exit methods, modifying the net-related data,
>> will require down_read() locking only, while down_write() will be used
>> for changing pernet_list.
>>
>> This gives signify performance increase, like you may see below. There
>> is measured sequential net namespace creation in a cycle, in single
>> thread, without other tasks (single user mode):
>>
>> 1)int main(int argc, char *argv[])
>> {
>>         unsigned nr;
>>         if (argc < 2) {
>>                 fprintf(stderr, "Provide nr iterations arg\n");
>>                 return 1;
>>         }
>>         nr = atoi(argv[1]);
>>         while (nr-- > 0) {
>>                 if (unshare(CLONE_NEWNET)) {
>>                         perror("Can't unshare");
>>                         return 1;
>>                 }
>>         }
>>         return 0;
>> }
>>
>> Origin, 100000 unshare():
>> 0.03user 23.14system 1:39.85elapsed 23%CPU
>>
>> Patched, 100000 unshare():
>> 0.03user 67.49system 1:08.34elapsed 98%CPU
>>
>> 2)for i in {1..10000}; do unshare -n bash -c exit; done
> 
> Hi Kirill,
> 
> This mutex has another role. You know that net namespaces are destroyed
> asynchronously, and the net mutex gurantees that a backlog will be not
> big. If we have something in backlog, we know that it will be handled
> before creating a new net ns.
> 
> As far as I remember net namespaces are created much faster than
> they are destroyed, so with this changes we can create a really big
> backlog, can't we?

I don't think limitation is a good goal or a gool for the mutex,
because it's very easy to create many net namespaces in case of
the mutex exists. You may open /proc/[pid]/ns/net like a file,
and net_ns counter will increment. Then, do unshare(), and
the mutex has no a way to protect against that. Anyway, mutex
can't limit a number of something in general, I've never seen
a (good) example in kernel.

As I see, the real limitation happen in inc_net_namespaces(),
which is decremented after RCU grace period in cleanup_net(),
and it has not changed.

> There was a discussion a few month ago:
> https://lists.onap.org/pipermail/containers/2016-October/037509.html
> 
> 
>>
>> Origin:
>> real 1m24,190s
>> user 0m6,225s
>> sys 0m15,132s
> 
> Here you measure time of creating and destroying net namespaces.
> 
>>
>> Patched:
>> real 0m18,235s   (4.6 times faster)
>> user 0m4,544s
>> sys 0m13,796s
> 
> But here you measure time of crearing namespaces and you know nothing
> when they will be destroyed.

You're right, and I predict, the sum time, spent on cpu, will remain the same,
but the think is that now creation and destroying may be executed in parallel.

On Tue, 14 Nov 2017 16:53:33 +0300
Kirill Tkhai <ktkhai@virtuozzo.com> wrote:

> +	/*
> +	 * RCU-protected list, modifiable by pernet-init and -exit methods.
> +	 * When net namespace is alive (net::count > 0), all the changes
> +	 * are made under rw_sem held on write.
> +	 */
> +	struct list_head	fib_notifier_ops;
>  

If you use __rcu annotation then it could be checked (and the comment is not needed).

On Tue, Nov 14, 2017 at 09:04:06PM +0300, Kirill Tkhai wrote:
> On 14.11.2017 20:44, Andrei Vagin wrote:
> > On Tue, Nov 14, 2017 at 04:53:33PM +0300, Kirill Tkhai wrote:
> >> Curently mutex is used to protect pernet operations list. It makes
> >> cleanup_net() to execute ->exit methods of the same operations set,
> >> which was used on the time of ->init, even after net namespace is
> >> unlinked from net_namespace_list.
> >>
> >> But the problem is it's need to synchronize_rcu() after net is removed
> >> from net_namespace_list():
> >>
> >> Destroy net_ns:
> >> cleanup_net()
> >>   mutex_lock(&net_mutex)
> >>   list_del_rcu(&net->list)
> >>   synchronize_rcu()                                  <--- Sleep there for ages
> >>   list_for_each_entry_reverse(ops, &pernet_list, list)
> >>     ops_exit_list(ops, &net_exit_list)
> >>   list_for_each_entry_reverse(ops, &pernet_list, list)
> >>     ops_free_list(ops, &net_exit_list)
> >>   mutex_unlock(&net_mutex)
> >>
> >> This primitive is not fast, especially on the systems with many processors
> >> and/or when preemptible RCU is enabled in config. So, all the time, while
> >> cleanup_net() is waiting for RCU grace period, creation of new net namespaces
> >> is not possible, the tasks, who makes it, are sleeping on the same mutex:
> >>
> >> Create net_ns:
> >> copy_net_ns()
> >>   mutex_lock_killable(&net_mutex)                    <--- Sleep there for ages
> >>
> >> The solution is to convert net_mutex to the rw_semaphore. Then,
> >> pernet_operations::init/::exit methods, modifying the net-related data,
> >> will require down_read() locking only, while down_write() will be used
> >> for changing pernet_list.
> >>
> >> This gives signify performance increase, like you may see below. There
> >> is measured sequential net namespace creation in a cycle, in single
> >> thread, without other tasks (single user mode):
> >>
> >> 1)int main(int argc, char *argv[])
> >> {
> >>         unsigned nr;
> >>         if (argc < 2) {
> >>                 fprintf(stderr, "Provide nr iterations arg\n");
> >>                 return 1;
> >>         }
> >>         nr = atoi(argv[1]);
> >>         while (nr-- > 0) {
> >>                 if (unshare(CLONE_NEWNET)) {
> >>                         perror("Can't unshare");
> >>                         return 1;
> >>                 }
> >>         }
> >>         return 0;
> >> }
> >>
> >> Origin, 100000 unshare():
> >> 0.03user 23.14system 1:39.85elapsed 23%CPU
> >>
> >> Patched, 100000 unshare():
> >> 0.03user 67.49system 1:08.34elapsed 98%CPU
> >>
> >> 2)for i in {1..10000}; do unshare -n bash -c exit; done
> > 
> > Hi Kirill,
> > 
> > This mutex has another role. You know that net namespaces are destroyed
> > asynchronously, and the net mutex gurantees that a backlog will be not
> > big. If we have something in backlog, we know that it will be handled
> > before creating a new net ns.
> > 
> > As far as I remember net namespaces are created much faster than
> > they are destroyed, so with this changes we can create a really big
> > backlog, can't we?
> 
> I don't think limitation is a good goal or a gool for the mutex,
> because it's very easy to create many net namespaces in case of
> the mutex exists. You may open /proc/[pid]/ns/net like a file,
> and net_ns counter will increment. Then, do unshare(), and
> the mutex has no a way to protect against that.

You are right, but with the mutex a user can not support a big backlog
for a long time, it is shrunk to zero periodically. With these changes
he can support a big backlog for a long time.

A big backlog affects other users. If someone creates namespaces, he
probably expects that they will be destroyed for a reasonable time.

But currently someone else can increase a destroy time to a really big
values. This problem was before your patches, but they may do this
problem worse. The question here is: Should we think about this problem
in the context of these patches?


> Anyway, mutex
> can't limit a number of something in general, I've never seen
> a (good) example in kernel.

I'm agree with you here.


> 
> As I see, the real limitation happen in inc_net_namespaces(),
> which is decremented after RCU grace period in cleanup_net(),
> and it has not changed.

ucount limits are to big to handle this problem.


> 
> > There was a discussion a few month ago:
> > https://lists.onap.org/pipermail/containers/2016-October/037509.html
> > 
> > 
> >>
> >> Origin:
> >> real 1m24,190s
> >> user 0m6,225s
> >> sys 0m15,132s
> > 
> > Here you measure time of creating and destroying net namespaces.
> > 
> >>
> >> Patched:
> >> real 0m18,235s   (4.6 times faster)
> >> user 0m4,544s
> >> sys 0m13,796s
> > 
> > But here you measure time of crearing namespaces and you know nothing
> > when they will be destroyed.
> 
> You're right, and I predict, the sum time, spent on cpu, will remain the same,
> but the think is that now creation and destroying may be executed in parallel.

On Tue, Nov 14, 2017 at 5:53 AM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
> @@ -406,7 +406,7 @@ struct net *copy_net_ns(unsigned long flags,
>
>         get_user_ns(user_ns);
>
> -       rv = mutex_lock_killable(&net_mutex);
> +       rv = down_read_killable(&net_sem);
>         if (rv < 0) {
>                 net_free(net);
>                 dec_net_namespaces(ucounts);
> @@ -421,7 +421,7 @@ struct net *copy_net_ns(unsigned long flags,
>                 list_add_tail_rcu(&net->list, &net_namespace_list);
>                 rtnl_unlock();
>         }
> -       mutex_unlock(&net_mutex);
> +       up_read(&net_sem);
>         if (rv < 0) {
>                 dec_net_namespaces(ucounts);
>                 put_user_ns(user_ns);
> @@ -446,7 +446,7 @@ static void cleanup_net(struct work_struct *work)
>         list_replace_init(&cleanup_list, &net_kill_list);
>         spin_unlock_irq(&cleanup_list_lock);
>
> -       mutex_lock(&net_mutex);
> +       down_read(&net_sem);
>
>         /* Don't let anyone else find us. */
>         rtnl_lock();
> @@ -486,7 +486,7 @@ static void cleanup_net(struct work_struct *work)
>         list_for_each_entry_reverse(ops, &pernet_list, list)
>                 ops_free_list(ops, &net_exit_list);
>
> -       mutex_unlock(&net_mutex);
> +       up_read(&net_sem);

After your patch setup_net() could run concurrently with cleanup_net(),
given that ops_exit_list() is called on error path of setup_net() too,
it means ops->exit() now could run concurrently if it doesn't have its
own lock. Not sure if this breaks any existing user.

On Tue, 2017-11-14 at 10:11 -0800, Stephen Hemminger wrote:
> On Tue, 14 Nov 2017 16:53:33 +0300
> Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
> 
> > +	/*
> > +	 * RCU-protected list, modifiable by pernet-init and -exit methods.
> > +	 * When net namespace is alive (net::count > 0), all the changes
> > +	 * are made under rw_sem held on write.
> > +	 */
> > +	struct list_head	fib_notifier_ops;
> >  
> 
> If you use __rcu annotation then it could be checked (and the comment is not needed).

I do not think we can use __rcu annotation yet on a struct list_head ?

(The annotation would be needed on the members)

On 14.11.2017 21:39, Cong Wang wrote:
> On Tue, Nov 14, 2017 at 5:53 AM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>> @@ -406,7 +406,7 @@ struct net *copy_net_ns(unsigned long flags,
>>
>>         get_user_ns(user_ns);
>>
>> -       rv = mutex_lock_killable(&net_mutex);
>> +       rv = down_read_killable(&net_sem);
>>         if (rv < 0) {
>>                 net_free(net);
>>                 dec_net_namespaces(ucounts);
>> @@ -421,7 +421,7 @@ struct net *copy_net_ns(unsigned long flags,
>>                 list_add_tail_rcu(&net->list, &net_namespace_list);
>>                 rtnl_unlock();
>>         }
>> -       mutex_unlock(&net_mutex);
>> +       up_read(&net_sem);
>>         if (rv < 0) {
>>                 dec_net_namespaces(ucounts);
>>                 put_user_ns(user_ns);
>> @@ -446,7 +446,7 @@ static void cleanup_net(struct work_struct *work)
>>         list_replace_init(&cleanup_list, &net_kill_list);
>>         spin_unlock_irq(&cleanup_list_lock);
>>
>> -       mutex_lock(&net_mutex);
>> +       down_read(&net_sem);
>>
>>         /* Don't let anyone else find us. */
>>         rtnl_lock();
>> @@ -486,7 +486,7 @@ static void cleanup_net(struct work_struct *work)
>>         list_for_each_entry_reverse(ops, &pernet_list, list)
>>                 ops_free_list(ops, &net_exit_list);
>>
>> -       mutex_unlock(&net_mutex);
>> +       up_read(&net_sem);
> 
> After your patch setup_net() could run concurrently with cleanup_net(),
> given that ops_exit_list() is called on error path of setup_net() too,
> it means ops->exit() now could run concurrently if it doesn't have its
> own lock. Not sure if this breaks any existing user.

Yes, there will be possible concurrent ops->init() for a net namespace,
and ops->exit() for another one. I hadn't found pernet operations, which
have a problem with that. If they exist, they are hidden and not clear seen.
The pernet operations in general do not touch someone else's memory.
If suddenly there is one, KASAN should show it after a while.

On 14.11.2017 21:38, Andrei Vagin wrote:
> On Tue, Nov 14, 2017 at 09:04:06PM +0300, Kirill Tkhai wrote:
>> On 14.11.2017 20:44, Andrei Vagin wrote:
>>> On Tue, Nov 14, 2017 at 04:53:33PM +0300, Kirill Tkhai wrote:
>>>> Curently mutex is used to protect pernet operations list. It makes
>>>> cleanup_net() to execute ->exit methods of the same operations set,
>>>> which was used on the time of ->init, even after net namespace is
>>>> unlinked from net_namespace_list.
>>>>
>>>> But the problem is it's need to synchronize_rcu() after net is removed
>>>> from net_namespace_list():
>>>>
>>>> Destroy net_ns:
>>>> cleanup_net()
>>>>   mutex_lock(&net_mutex)
>>>>   list_del_rcu(&net->list)
>>>>   synchronize_rcu()                                  <--- Sleep there for ages
>>>>   list_for_each_entry_reverse(ops, &pernet_list, list)
>>>>     ops_exit_list(ops, &net_exit_list)
>>>>   list_for_each_entry_reverse(ops, &pernet_list, list)
>>>>     ops_free_list(ops, &net_exit_list)
>>>>   mutex_unlock(&net_mutex)
>>>>
>>>> This primitive is not fast, especially on the systems with many processors
>>>> and/or when preemptible RCU is enabled in config. So, all the time, while
>>>> cleanup_net() is waiting for RCU grace period, creation of new net namespaces
>>>> is not possible, the tasks, who makes it, are sleeping on the same mutex:
>>>>
>>>> Create net_ns:
>>>> copy_net_ns()
>>>>   mutex_lock_killable(&net_mutex)                    <--- Sleep there for ages
>>>>
>>>> The solution is to convert net_mutex to the rw_semaphore. Then,
>>>> pernet_operations::init/::exit methods, modifying the net-related data,
>>>> will require down_read() locking only, while down_write() will be used
>>>> for changing pernet_list.
>>>>
>>>> This gives signify performance increase, like you may see below. There
>>>> is measured sequential net namespace creation in a cycle, in single
>>>> thread, without other tasks (single user mode):
>>>>
>>>> 1)int main(int argc, char *argv[])
>>>> {
>>>>         unsigned nr;
>>>>         if (argc < 2) {
>>>>                 fprintf(stderr, "Provide nr iterations arg\n");
>>>>                 return 1;
>>>>         }
>>>>         nr = atoi(argv[1]);
>>>>         while (nr-- > 0) {
>>>>                 if (unshare(CLONE_NEWNET)) {
>>>>                         perror("Can't unshare");
>>>>                         return 1;
>>>>                 }
>>>>         }
>>>>         return 0;
>>>> }
>>>>
>>>> Origin, 100000 unshare():
>>>> 0.03user 23.14system 1:39.85elapsed 23%CPU
>>>>
>>>> Patched, 100000 unshare():
>>>> 0.03user 67.49system 1:08.34elapsed 98%CPU
>>>>
>>>> 2)for i in {1..10000}; do unshare -n bash -c exit; done
>>>
>>> Hi Kirill,
>>>
>>> This mutex has another role. You know that net namespaces are destroyed
>>> asynchronously, and the net mutex gurantees that a backlog will be not
>>> big. If we have something in backlog, we know that it will be handled
>>> before creating a new net ns.
>>>
>>> As far as I remember net namespaces are created much faster than
>>> they are destroyed, so with this changes we can create a really big
>>> backlog, can't we?
>>
>> I don't think limitation is a good goal or a gool for the mutex,
>> because it's very easy to create many net namespaces in case of
>> the mutex exists. You may open /proc/[pid]/ns/net like a file,
>> and net_ns counter will increment. Then, do unshare(), and
>> the mutex has no a way to protect against that.
> 
> You are right, but with the mutex a user can not support a big backlog
> for a long time, it is shrunk to zero periodically. With these changes
> he can support a big backlog for a long time.
> 
> A big backlog affects other users. If someone creates namespaces, he
> probably expects that they will be destroyed for a reasonable time.

Here 2 different topics:
1)Limitation.
This problem exists in current code, and the patch does not make it worse.
If someone wants to limit number of net namespaces per user_ns/cgroup,
he should solve it in another way. Mutex does not help there.

2)Speed-up struct net destruction.
Many structures in kernel are released via rcu. The only difference
of struct net is it has more delayed actions, then task_struct for example.
User is not interested in the time, when task_struct is completely destroyed.
But I may imagine it's more important for struct net, if (for example) some
delayed connections are destroyed from ops->exit(). Are they?!
But like in the first paragraph, I think this is not related to the patch,
because in the current code you may generate dozen thousands of net namespaces
and then destroy them at once. And they will be destroyed for the same long time,
and there is additional worse thing, that it won't be possible to create a new
one for a long period. So, it's completely unrelated to the patch.
 
> But currently someone else can increase a destroy time to a really big
> values. This problem was before your patches, but they may do this
> problem worse. The question here is: Should we think about this problem
> in the context of these patches?

I think we shouldn't, because the main thing in this patch is parallel
execution of ops->init() and ->exit() and their safety. I hope people
will look at this and nothing will remain hidden.

The problem of long time destruction of net namespaces may be solved as next step.
As we will be able to destroy them in parallel, we'll implement percpu destroy
works and will destroy them much time faster then now.
 
>> Anyway, mutex
>> can't limit a number of something in general, I've never seen
>> a (good) example in kernel.
> 
> I'm agree with you here.
> 
> 
>>
>> As I see, the real limitation happen in inc_net_namespaces(),
>> which is decremented after RCU grace period in cleanup_net(),
>> and it has not changed.
> 
> ucount limits are to big to handle this problem.
> 
> 
>>
>>> There was a discussion a few month ago:
>>> https://lists.onap.org/pipermail/containers/2016-October/037509.html
>>>
>>>
>>>>
>>>> Origin:
>>>> real 1m24,190s
>>>> user 0m6,225s
>>>> sys 0m15,132s
>>>
>>> Here you measure time of creating and destroying net namespaces.
>>>
>>>>
>>>> Patched:
>>>> real 0m18,235s   (4.6 times faster)
>>>> user 0m4,544s
>>>> sys 0m13,796s
>>>
>>> But here you measure time of crearing namespaces and you know nothing
>>> when they will be destroyed.
>>
>> You're right, and I predict, the sum time, spent on cpu, will remain the same,
>> but the think is that now creation and destroying may be executed in parallel.

On Tue, Nov 14, 2017 at 10:00:59AM -0800, Eric Dumazet wrote:
> On Tue, 2017-11-14 at 09:44 -0800, Andrei Vagin wrote:
> > On Tue, Nov 14, 2017 at 04:53:33PM +0300, Kirill Tkhai wrote:
> > > Curently mutex is used to protect pernet operations list. It makes
> > > cleanup_net() to execute ->exit methods of the same operations set,
> > > which was used on the time of ->init, even after net namespace is
> > > unlinked from net_namespace_list.
> > > 
> > > But the problem is it's need to synchronize_rcu() after net is removed
> > > from net_namespace_list():
> > > 
> > > Destroy net_ns:
> > > cleanup_net()
> > >   mutex_lock(&net_mutex)
> > >   list_del_rcu(&net->list)
> > >   synchronize_rcu()                                  <--- Sleep there for ages
> > >   list_for_each_entry_reverse(ops, &pernet_list, list)
> > >     ops_exit_list(ops, &net_exit_list)
> > >   list_for_each_entry_reverse(ops, &pernet_list, list)
> > >     ops_free_list(ops, &net_exit_list)
> > >   mutex_unlock(&net_mutex)
> > > 
> > > This primitive is not fast, especially on the systems with many processors
> > > and/or when preemptible RCU is enabled in config. So, all the time, while
> > > cleanup_net() is waiting for RCU grace period, creation of new net namespaces
> > > is not possible, the tasks, who makes it, are sleeping on the same mutex:
> > > 
> > > Create net_ns:
> > > copy_net_ns()
> > >   mutex_lock_killable(&net_mutex)                    <--- Sleep there for ages
> > > 
> > > The solution is to convert net_mutex to the rw_semaphore. Then,
> > > pernet_operations::init/::exit methods, modifying the net-related data,
> > > will require down_read() locking only, while down_write() will be used
> > > for changing pernet_list.
> > > 
> > > This gives signify performance increase, like you may see below. There
> > > is measured sequential net namespace creation in a cycle, in single
> > > thread, without other tasks (single user mode):
> > > 
> > > 1)int main(int argc, char *argv[])
> > > {
> > >         unsigned nr;
> > >         if (argc < 2) {
> > >                 fprintf(stderr, "Provide nr iterations arg\n");
> > >                 return 1;
> > >         }
> > >         nr = atoi(argv[1]);
> > >         while (nr-- > 0) {
> > >                 if (unshare(CLONE_NEWNET)) {
> > >                         perror("Can't unshare");
> > >                         return 1;
> > >                 }
> > >         }
> > >         return 0;
> > > }
> > > 
> > > Origin, 100000 unshare():
> > > 0.03user 23.14system 1:39.85elapsed 23%CPU
> > > 
> > > Patched, 100000 unshare():
> > > 0.03user 67.49system 1:08.34elapsed 98%CPU
> > > 
> > > 2)for i in {1..10000}; do unshare -n bash -c exit; done
> > 
> > Hi Kirill,
> > 
> > This mutex has another role. You know that net namespaces are destroyed
> > asynchronously, and the net mutex gurantees that a backlog will be not
> > big. If we have something in backlog, we know that it will be handled
> > before creating a new net ns.
> > 
> > As far as I remember net namespaces are created much faster than
> > they are destroyed, so with this changes we can create a really big
> > backlog, can't we?
> 
> Please take a look at the recent patches I did :
> 
> 8ca712c373a462cfa1b62272870b6c2c74aa83f9 Merge branch 'net-speedup-netns-create-delete-time'
> 64bc17811b72758753e2b64cd8f2a63812c61fe1 ipv4: speedup ipv6 tunnels dismantle
> bb401caefe9d2c65e0c0fa23b21deecfbfa473fe ipv6: speedup ipv6 tunnels dismantle
> 789e6ddb0b2fb5d5024b760b178a47876e4de7a6 tcp: batch tcp_net_metrics_exit
> a90c9347e90ed1e9323d71402ed18023bc910cd8 ipv6: addrlabel: per netns list
> d464e84eed02993d40ad55fdc19f4523e4deee5b kobject: factorize skb setup in kobject_uevent_net_broadcast()
> 4a336a23d619e96aef37d4d054cfadcdd1b581ba kobject: copy env blob in one go
> 16dff336b33d87c15d9cbe933cfd275aae2a8251 kobject: add kobject_uevent_net_broadcast()
> 

Good job! Now it really works much faster. I tested these patches with
Kirill's one and everithing works good. I could not reproduce a
situation, when a backlog starts growing.

Thanks Kirill and Eric.

Kirill Tkhai <ktkhai@virtuozzo.com> writes:

> On 14.11.2017 21:39, Cong Wang wrote:
>> On Tue, Nov 14, 2017 at 5:53 AM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>>> @@ -406,7 +406,7 @@ struct net *copy_net_ns(unsigned long flags,
>>>
>>>         get_user_ns(user_ns);
>>>
>>> -       rv = mutex_lock_killable(&net_mutex);
>>> +       rv = down_read_killable(&net_sem);
>>>         if (rv < 0) {
>>>                 net_free(net);
>>>                 dec_net_namespaces(ucounts);
>>> @@ -421,7 +421,7 @@ struct net *copy_net_ns(unsigned long flags,
>>>                 list_add_tail_rcu(&net->list, &net_namespace_list);
>>>                 rtnl_unlock();
>>>         }
>>> -       mutex_unlock(&net_mutex);
>>> +       up_read(&net_sem);
>>>         if (rv < 0) {
>>>                 dec_net_namespaces(ucounts);
>>>                 put_user_ns(user_ns);
>>> @@ -446,7 +446,7 @@ static void cleanup_net(struct work_struct *work)
>>>         list_replace_init(&cleanup_list, &net_kill_list);
>>>         spin_unlock_irq(&cleanup_list_lock);
>>>
>>> -       mutex_lock(&net_mutex);
>>> +       down_read(&net_sem);
>>>
>>>         /* Don't let anyone else find us. */
>>>         rtnl_lock();
>>> @@ -486,7 +486,7 @@ static void cleanup_net(struct work_struct *work)
>>>         list_for_each_entry_reverse(ops, &pernet_list, list)
>>>                 ops_free_list(ops, &net_exit_list);
>>>
>>> -       mutex_unlock(&net_mutex);
>>> +       up_read(&net_sem);
>> 
>> After your patch setup_net() could run concurrently with cleanup_net(),
>> given that ops_exit_list() is called on error path of setup_net() too,
>> it means ops->exit() now could run concurrently if it doesn't have its
>> own lock. Not sure if this breaks any existing user.
>
> Yes, there will be possible concurrent ops->init() for a net namespace,
> and ops->exit() for another one. I hadn't found pernet operations, which
> have a problem with that. If they exist, they are hidden and not clear seen.
> The pernet operations in general do not touch someone else's memory.
> If suddenly there is one, KASAN should show it after a while.

Certainly the use of hash tables shared between multiple network
namespaces would count.  I don't rembmer how many of these we have but
there used to be quite a few.

Eric

Kirill Tkhai <ktkhai@virtuozzo.com> writes:

> Curently mutex is used to protect pernet operations list. It makes
> cleanup_net() to execute ->exit methods of the same operations set,
> which was used on the time of ->init, even after net namespace is
> unlinked from net_namespace_list.
>
> But the problem is it's need to synchronize_rcu() after net is removed
> from net_namespace_list():
>
> Destroy net_ns:
> cleanup_net()
>   mutex_lock(&net_mutex)
>   list_del_rcu(&net->list)
>   synchronize_rcu()                                  <--- Sleep there for ages
>   list_for_each_entry_reverse(ops, &pernet_list, list)
>     ops_exit_list(ops, &net_exit_list)
>   list_for_each_entry_reverse(ops, &pernet_list, list)
>     ops_free_list(ops, &net_exit_list)
>   mutex_unlock(&net_mutex)
>
> This primitive is not fast, especially on the systems with many processors
> and/or when preemptible RCU is enabled in config. So, all the time, while
> cleanup_net() is waiting for RCU grace period, creation of new net namespaces
> is not possible, the tasks, who makes it, are sleeping on the same mutex:
>
> Create net_ns:
> copy_net_ns()
>   mutex_lock_killable(&net_mutex)                    <--- Sleep there for ages
>
> The solution is to convert net_mutex to the rw_semaphore. Then,
> pernet_operations::init/::exit methods, modifying the net-related data,
> will require down_read() locking only, while down_write() will be used
> for changing pernet_list.
>
> This gives signify performance increase, like you may see below. There
> is measured sequential net namespace creation in a cycle, in single
> thread, without other tasks (single user mode):
>
> 1)int main(int argc, char *argv[])
> {
>         unsigned nr;
>         if (argc < 2) {
>                 fprintf(stderr, "Provide nr iterations arg\n");
>                 return 1;
>         }
>         nr = atoi(argv[1]);
>         while (nr-- > 0) {
>                 if (unshare(CLONE_NEWNET)) {
>                         perror("Can't unshare");
>                         return 1;
>                 }
>         }
>         return 0;
> }
>
> Origin, 100000 unshare():
> 0.03user 23.14system 1:39.85elapsed 23%CPU
>
> Patched, 100000 unshare():
> 0.03user 67.49system 1:08.34elapsed 98%CPU
>
> 2)for i in {1..10000}; do unshare -n bash -c exit; done
>
> Origin:
> real 1m24,190s
> user 0m6,225s
> sys 0m15,132s
>
> Patched:
> real 0m18,235s   (4.6 times faster)
> user 0m4,544s
> sys 0m13,796s
>
> This patch requires commit 76f8507f7a64 "locking/rwsem: Add down_read_killable()"
> from Linus tree (not in net-next yet).

Using a rwsem to protect the list of operations makes sense.

That should allow removing the sing

I am not wild about taking a the rwsem down_write in
rtnl_link_unregister, and net_ns_barrier.  I think that works but it
goes from being a mild hack to being a pretty bad hack and something
else that can kill the parallelism you are seeking it add.

There are about 204 instances of struct pernet_operations.  That is a
lot of code to have carefully audited to ensure it can in parallel all
at once.  The existence of the exit_batch method, net_ns_barrier,
for_each_net and taking of net_mutex in rtnl_link_unregister all testify
to the fact that there are data structures accessed by multiple network
namespaces.

My preference would be to:

- Add the net_sem in addition to net_mutex with down_write only held in
  register and unregister, and maybe net_ns_barrier and
  rtnl_link_unregister.

- Factor out struct pernet_ops out of struct pernet_operations.  With
  struct pernet_ops not having the exit_batch method.  With pernet_ops
  being embedded an anonymous member of the old struct pernet_operations.

- Add [un]register_pernet_{sys,dev} functions that take a struct
  pernet_ops, that don't take net_mutex.  Have them order the
  pernet_list as:

  pernet_sys
  pernet_subsys
  pernet_device
  pernet_dev

  With the chunk in the middle taking the net_mutex.

  I think I would enforce the ordering with a failure to register
  if a subsystem or a device tried to register out of order.  

- Disable use of the single threaded workqueue if nothing needs the
  net_mutex.

- Add a test mode that deliberartely spawns threads on multiple
  processors and deliberately creates multiple network namespaces
  at the same time.

- Add a test mode that deliberately spawns threads on multiple
  processors and delibearate destrosy multiple network namespaces
  at the same time.
  
- Convert the code to unlocked operation one pernet_operations to at a
  time.  Being careful with the loopback device it's order in the list
  strongly matters.

- Finally remove the unnecessary code.


At the end of the day because all of the operations for one network
namespace will run in parallel with all of the operations for another
network namespace all of the sophistication that goes into batching the
cleanup of multiple network namespaces can be removed.  As different
tasks (not sharing a lock) can wait in syncrhonize_rcu at the same time
without slowing each other down.

I think we can remove the batching but I am afraid that will lead us into
rtnl_lock contention.

I am definitely not comfortable with changing the locking on so much
code without an explanation at all why it is safe in the commit comments
in all 204 instances.  Which combined equal most of the well maintained
and regularly used part of the networking stack.

Eric

> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  include/linux/rtnetlink.h   |    2 +-
>  include/net/net_namespace.h |    9 +++++++--
>  net/core/net_namespace.c    |   40 ++++++++++++++++++++--------------------
>  net/core/rtnetlink.c        |    4 ++--
>  4 files changed, 30 insertions(+), 25 deletions(-)
>
> diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
> index 54bcd970bfd3..36cc009f4580 100644
> --- a/include/linux/rtnetlink.h
> +++ b/include/linux/rtnetlink.h
> @@ -32,7 +32,7 @@ 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
>  extern bool lockdep_rtnl_is_held(void);
> diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
> index 10f99dafd5ac..aaed826ccbe7 100644
> --- a/include/net/net_namespace.h
> +++ b/include/net/net_namespace.h
> @@ -60,7 +60,7 @@ struct net {
>  
>  	struct list_head	list;		/* list of network namespaces */
>  	struct list_head	cleanup_list;	/* namespaces on death row */
> -	struct list_head	exit_list;	/* Use only net_mutex */
> +	struct list_head	exit_list;	/* Use only net_sem */
>  
>  	struct user_namespace   *user_ns;	/* Owning user namespace */
>  	struct ucounts		*ucounts;
> @@ -89,7 +89,12 @@ struct net {
>  	/* core fib_rules */
>  	struct list_head	rules_ops;
>  
> -	struct list_head	fib_notifier_ops;  /* protected by net_mutex */
> +	/*
> +	 * RCU-protected list, modifiable by pernet-init and -exit methods.
> +	 * When net namespace is alive (net::count > 0), all the changes
> +	 * are made under rw_sem held on write.
> +	 */
> +	struct list_head	fib_notifier_ops;
>  
>  	struct net_device       *loopback_dev;          /* The loopback */
>  	struct netns_core	core;
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index 6cfdc7c84c48..f502b11b507e 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -29,7 +29,7 @@
>  
>  static LIST_HEAD(pernet_list);
>  static struct list_head *first_device = &pernet_list;
> -DEFINE_MUTEX(net_mutex);
> +DECLARE_RWSEM(net_sem);
>  
>  LIST_HEAD(net_namespace_list);
>  EXPORT_SYMBOL_GPL(net_namespace_list);
> @@ -65,11 +65,11 @@ 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(!rwsem_is_locked(&net_sem));
>  	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;
> @@ -278,7 +278,7 @@ struct net *get_net_ns_by_id(struct net *net, int id)
>   */
>  static __net_init int setup_net(struct net *net, struct user_namespace *user_ns)
>  {
> -	/* Must be called with net_mutex held */
> +	/* Must be called with net_sem held */
>  	const struct pernet_operations *ops, *saved_ops;
>  	int error = 0;
>  	LIST_HEAD(net_exit_list);
> @@ -406,7 +406,7 @@ struct net *copy_net_ns(unsigned long flags,
>  
>  	get_user_ns(user_ns);
>  
> -	rv = mutex_lock_killable(&net_mutex);
> +	rv = down_read_killable(&net_sem);
>  	if (rv < 0) {
>  		net_free(net);
>  		dec_net_namespaces(ucounts);
> @@ -421,7 +421,7 @@ struct net *copy_net_ns(unsigned long flags,
>  		list_add_tail_rcu(&net->list, &net_namespace_list);
>  		rtnl_unlock();
>  	}
> -	mutex_unlock(&net_mutex);
> +	up_read(&net_sem);
>  	if (rv < 0) {
>  		dec_net_namespaces(ucounts);
>  		put_user_ns(user_ns);
> @@ -446,7 +446,7 @@ static void cleanup_net(struct work_struct *work)
>  	list_replace_init(&cleanup_list, &net_kill_list);
>  	spin_unlock_irq(&cleanup_list_lock);
>  
> -	mutex_lock(&net_mutex);
> +	down_read(&net_sem);
>  
>  	/* Don't let anyone else find us. */
>  	rtnl_lock();
> @@ -486,7 +486,7 @@ static void cleanup_net(struct work_struct *work)
>  	list_for_each_entry_reverse(ops, &pernet_list, list)
>  		ops_free_list(ops, &net_exit_list);
>  
> -	mutex_unlock(&net_mutex);
> +	up_read(&net_sem);
>  
>  	/* Ensure there are no outstanding rcu callbacks using this
>  	 * network namespace.
> @@ -513,8 +513,8 @@ static void cleanup_net(struct work_struct *work)
>   */
>  void net_ns_barrier(void)
>  {
> -	mutex_lock(&net_mutex);
> -	mutex_unlock(&net_mutex);
> +	down_write(&net_sem);
> +	up_write(&net_sem);
>  }
>  EXPORT_SYMBOL(net_ns_barrier);
>  
> @@ -841,7 +841,7 @@ static int __init net_ns_init(void)
>  
>  	rcu_assign_pointer(init_net.gen, ng);
>  
> -	mutex_lock(&net_mutex);
> +	down_read(&net_sem);
>  	if (setup_net(&init_net, &init_user_ns))
>  		panic("Could not setup the initial network namespace");
>  
> @@ -851,7 +851,7 @@ static int __init net_ns_init(void)
>  	list_add_tail_rcu(&init_net.list, &net_namespace_list);
>  	rtnl_unlock();
>  
> -	mutex_unlock(&net_mutex);
> +	up_read(&net_sem);
>  
>  	register_pernet_subsys(&net_ns_ops);
>  
> @@ -991,9 +991,9 @@ static void unregister_pernet_operations(struct pernet_operations *ops)
>  int register_pernet_subsys(struct pernet_operations *ops)
>  {
>  	int error;
> -	mutex_lock(&net_mutex);
> +	down_write(&net_sem);
>  	error =  register_pernet_operations(first_device, ops);
> -	mutex_unlock(&net_mutex);
> +	up_write(&net_sem);
>  	return error;
>  }
>  EXPORT_SYMBOL_GPL(register_pernet_subsys);
> @@ -1009,9 +1009,9 @@ EXPORT_SYMBOL_GPL(register_pernet_subsys);
>   */
>  void unregister_pernet_subsys(struct pernet_operations *ops)
>  {
> -	mutex_lock(&net_mutex);
> +	down_write(&net_sem);
>  	unregister_pernet_operations(ops);
> -	mutex_unlock(&net_mutex);
> +	up_write(&net_sem);
>  }
>  EXPORT_SYMBOL_GPL(unregister_pernet_subsys);
>  
> @@ -1037,11 +1037,11 @@ EXPORT_SYMBOL_GPL(unregister_pernet_subsys);
>  int register_pernet_device(struct pernet_operations *ops)
>  {
>  	int error;
> -	mutex_lock(&net_mutex);
> +	down_write(&net_sem);
>  	error = register_pernet_operations(&pernet_list, ops);
>  	if (!error && (first_device == &pernet_list))
>  		first_device = &ops->list;
> -	mutex_unlock(&net_mutex);
> +	up_write(&net_sem);
>  	return error;
>  }
>  EXPORT_SYMBOL_GPL(register_pernet_device);
> @@ -1057,11 +1057,11 @@ EXPORT_SYMBOL_GPL(register_pernet_device);
>   */
>  void unregister_pernet_device(struct pernet_operations *ops)
>  {
> -	mutex_lock(&net_mutex);
> +	down_write(&net_sem);
>  	if (&ops->list == first_device)
>  		first_device = first_device->next;
>  	unregister_pernet_operations(ops);
> -	mutex_unlock(&net_mutex);
> +	up_write(&net_sem);
>  }
>  EXPORT_SYMBOL_GPL(unregister_pernet_device);
>  
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 5ace48926b19..caa215fd170b 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -390,11 +390,11 @@ static void rtnl_lock_unregistering_all(void)
>  void rtnl_link_unregister(struct rtnl_link_ops *ops)
>  {
>  	/* Close the race with cleanup_net() */
> -	mutex_lock(&net_mutex);
> +	down_write(&net_sem);
>  	rtnl_lock_unregistering_all();
>  	__rtnl_link_unregister(ops);
>  	rtnl_unlock();
> -	mutex_unlock(&net_mutex);
> +	up_write(&net_sem);
>  }
>  EXPORT_SYMBOL_GPL(rtnl_link_unregister);
>

On 15.11.2017 09:25, Eric W. Biederman wrote:
> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
> 
>> Curently mutex is used to protect pernet operations list. It makes
>> cleanup_net() to execute ->exit methods of the same operations set,
>> which was used on the time of ->init, even after net namespace is
>> unlinked from net_namespace_list.
>>
>> But the problem is it's need to synchronize_rcu() after net is removed
>> from net_namespace_list():
>>
>> Destroy net_ns:
>> cleanup_net()
>>   mutex_lock(&net_mutex)
>>   list_del_rcu(&net->list)
>>   synchronize_rcu()                                  <--- Sleep there for ages
>>   list_for_each_entry_reverse(ops, &pernet_list, list)
>>     ops_exit_list(ops, &net_exit_list)
>>   list_for_each_entry_reverse(ops, &pernet_list, list)
>>     ops_free_list(ops, &net_exit_list)
>>   mutex_unlock(&net_mutex)
>>
>> This primitive is not fast, especially on the systems with many processors
>> and/or when preemptible RCU is enabled in config. So, all the time, while
>> cleanup_net() is waiting for RCU grace period, creation of new net namespaces
>> is not possible, the tasks, who makes it, are sleeping on the same mutex:
>>
>> Create net_ns:
>> copy_net_ns()
>>   mutex_lock_killable(&net_mutex)                    <--- Sleep there for ages
>>
>> The solution is to convert net_mutex to the rw_semaphore. Then,
>> pernet_operations::init/::exit methods, modifying the net-related data,
>> will require down_read() locking only, while down_write() will be used
>> for changing pernet_list.
>>
>> This gives signify performance increase, like you may see below. There
>> is measured sequential net namespace creation in a cycle, in single
>> thread, without other tasks (single user mode):
>>
>> 1)int main(int argc, char *argv[])
>> {
>>         unsigned nr;
>>         if (argc < 2) {
>>                 fprintf(stderr, "Provide nr iterations arg\n");
>>                 return 1;
>>         }
>>         nr = atoi(argv[1]);
>>         while (nr-- > 0) {
>>                 if (unshare(CLONE_NEWNET)) {
>>                         perror("Can't unshare");
>>                         return 1;
>>                 }
>>         }
>>         return 0;
>> }
>>
>> Origin, 100000 unshare():
>> 0.03user 23.14system 1:39.85elapsed 23%CPU
>>
>> Patched, 100000 unshare():
>> 0.03user 67.49system 1:08.34elapsed 98%CPU
>>
>> 2)for i in {1..10000}; do unshare -n bash -c exit; done
>>
>> Origin:
>> real 1m24,190s
>> user 0m6,225s
>> sys 0m15,132s
>>
>> Patched:
>> real 0m18,235s   (4.6 times faster)
>> user 0m4,544s
>> sys 0m13,796s
>>
>> This patch requires commit 76f8507f7a64 "locking/rwsem: Add down_read_killable()"
>> from Linus tree (not in net-next yet).
> 
> Using a rwsem to protect the list of operations makes sense.
> 
> That should allow removing the sing
> 
> I am not wild about taking a the rwsem down_write in
> rtnl_link_unregister, and net_ns_barrier.  I think that works but it
> goes from being a mild hack to being a pretty bad hack and something
> else that can kill the parallelism you are seeking it add.
> 
> There are about 204 instances of struct pernet_operations.  That is a
> lot of code to have carefully audited to ensure it can in parallel all
> at once.  The existence of the exit_batch method, net_ns_barrier,
> for_each_net and taking of net_mutex in rtnl_link_unregister all testify
> to the fact that there are data structures accessed by multiple network
> namespaces.
> 
> My preference would be to:
> 
> - Add the net_sem in addition to net_mutex with down_write only held in
>   register and unregister, and maybe net_ns_barrier and
>   rtnl_link_unregister.
> 
> - Factor out struct pernet_ops out of struct pernet_operations.  With
>   struct pernet_ops not having the exit_batch method.  With pernet_ops
>   being embedded an anonymous member of the old struct pernet_operations.
> 
> - Add [un]register_pernet_{sys,dev} functions that take a struct
>   pernet_ops, that don't take net_mutex.  Have them order the
>   pernet_list as:
> 
>   pernet_sys
>   pernet_subsys
>   pernet_device
>   pernet_dev
> 
>   With the chunk in the middle taking the net_mutex.

I think this approach will work. Thanks for the suggestion. Some more
thoughts to the plan below.

The only difficult thing there will be to choose the right order
to move ops from pernet_subsys to pernet_sys and from pernet_device
to pernet_dev one by one.

This is rather easy in case of tristate drivers, as modules may be loaded
at any time, and the only important order is dependences between them.
So, it's possible to start from a module, who has no dependences,
and move it to pernet_sys, and then continue with modules,
who have no more dependences in pernet_subsys. For pernet_device
it's vise versa.

In case of bool drivers, the situation may be worse, because
the order is not so clear there. The same priority initcalls
(for example, initcalls, registered via core_initcall()) may require
the certain order, driven by linking order. I know one example from
device mapper code, which lives here: drivers/md/Makefile.
This problem is also solvable, even if such places do not contain
comments about linking order. It's just need to respect Makefile
order, when choosing a new candidate to move.
 
>   I think I would enforce the ordering with a failure to register
>   if a subsystem or a device tried to register out of order.  
> 
> - Disable use of the single threaded workqueue if nothing needs the
>   net_mutex.

We may use per-cpu worqueues in the future. The idea to refuse using
worqueue doesn't seem good for me, because asynchronous net destroying
looks very useful.

> - Add a test mode that deliberartely spawns threads on multiple
>   processors and deliberately creates multiple network namespaces
>   at the same time.
> 
> - Add a test mode that deliberately spawns threads on multiple
>   processors and delibearate destrosy multiple network namespaces
>   at the same time.>
> - Convert the code to unlocked operation one pernet_operations to at a
>   time.  Being careful with the loopback device it's order in the list
>   strongly matters.
> 
> - Finally remove the unnecessary code.
> 
> 
> At the end of the day because all of the operations for one network
> namespace will run in parallel with all of the operations for another
> network namespace all of the sophistication that goes into batching the
> cleanup of multiple network namespaces can be removed.  As different
> tasks (not sharing a lock) can wait in syncrhonize_rcu at the same time
> without slowing each other down.
> 
> I think we can remove the batching but I am afraid that will lead us into
> rtnl_lock contention.

I've looked into this lock. It used in many places for many reasons.
It's a little strange, nobody tried to break it up in several small locks..
 
> I am definitely not comfortable with changing the locking on so much
> code without an explanation at all why it is safe in the commit comments
> in all 204 instances.  Which combined equal most of the well maintained
> and regularly used part of the networking stack.

David,

could you please check the plan above and say whether 1)it's OK for you,
and 2)if so, will you expect all the changes are made in one big 200+ patch set
or we may go sequentially(50 patches a time, for example)?

>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> ---
>>  include/linux/rtnetlink.h   |    2 +-
>>  include/net/net_namespace.h |    9 +++++++--
>>  net/core/net_namespace.c    |   40 ++++++++++++++++++++--------------------
>>  net/core/rtnetlink.c        |    4 ++--
>>  4 files changed, 30 insertions(+), 25 deletions(-)
>>
>> diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
>> index 54bcd970bfd3..36cc009f4580 100644
>> --- a/include/linux/rtnetlink.h
>> +++ b/include/linux/rtnetlink.h
>> @@ -32,7 +32,7 @@ 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
>>  extern bool lockdep_rtnl_is_held(void);
>> diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
>> index 10f99dafd5ac..aaed826ccbe7 100644
>> --- a/include/net/net_namespace.h
>> +++ b/include/net/net_namespace.h
>> @@ -60,7 +60,7 @@ struct net {
>>  
>>  	struct list_head	list;		/* list of network namespaces */
>>  	struct list_head	cleanup_list;	/* namespaces on death row */
>> -	struct list_head	exit_list;	/* Use only net_mutex */
>> +	struct list_head	exit_list;	/* Use only net_sem */
>>  
>>  	struct user_namespace   *user_ns;	/* Owning user namespace */
>>  	struct ucounts		*ucounts;
>> @@ -89,7 +89,12 @@ struct net {
>>  	/* core fib_rules */
>>  	struct list_head	rules_ops;
>>  
>> -	struct list_head	fib_notifier_ops;  /* protected by net_mutex */
>> +	/*
>> +	 * RCU-protected list, modifiable by pernet-init and -exit methods.
>> +	 * When net namespace is alive (net::count > 0), all the changes
>> +	 * are made under rw_sem held on write.
>> +	 */
>> +	struct list_head	fib_notifier_ops;
>>  
>>  	struct net_device       *loopback_dev;          /* The loopback */
>>  	struct netns_core	core;
>> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
>> index 6cfdc7c84c48..f502b11b507e 100644
>> --- a/net/core/net_namespace.c
>> +++ b/net/core/net_namespace.c
>> @@ -29,7 +29,7 @@
>>  
>>  static LIST_HEAD(pernet_list);
>>  static struct list_head *first_device = &pernet_list;
>> -DEFINE_MUTEX(net_mutex);
>> +DECLARE_RWSEM(net_sem);
>>  
>>  LIST_HEAD(net_namespace_list);
>>  EXPORT_SYMBOL_GPL(net_namespace_list);
>> @@ -65,11 +65,11 @@ 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(!rwsem_is_locked(&net_sem));
>>  	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;
>> @@ -278,7 +278,7 @@ struct net *get_net_ns_by_id(struct net *net, int id)
>>   */
>>  static __net_init int setup_net(struct net *net, struct user_namespace *user_ns)
>>  {
>> -	/* Must be called with net_mutex held */
>> +	/* Must be called with net_sem held */
>>  	const struct pernet_operations *ops, *saved_ops;
>>  	int error = 0;
>>  	LIST_HEAD(net_exit_list);
>> @@ -406,7 +406,7 @@ struct net *copy_net_ns(unsigned long flags,
>>  
>>  	get_user_ns(user_ns);
>>  
>> -	rv = mutex_lock_killable(&net_mutex);
>> +	rv = down_read_killable(&net_sem);
>>  	if (rv < 0) {
>>  		net_free(net);
>>  		dec_net_namespaces(ucounts);
>> @@ -421,7 +421,7 @@ struct net *copy_net_ns(unsigned long flags,
>>  		list_add_tail_rcu(&net->list, &net_namespace_list);
>>  		rtnl_unlock();
>>  	}
>> -	mutex_unlock(&net_mutex);
>> +	up_read(&net_sem);
>>  	if (rv < 0) {
>>  		dec_net_namespaces(ucounts);
>>  		put_user_ns(user_ns);
>> @@ -446,7 +446,7 @@ static void cleanup_net(struct work_struct *work)
>>  	list_replace_init(&cleanup_list, &net_kill_list);
>>  	spin_unlock_irq(&cleanup_list_lock);
>>  
>> -	mutex_lock(&net_mutex);
>> +	down_read(&net_sem);
>>  
>>  	/* Don't let anyone else find us. */
>>  	rtnl_lock();
>> @@ -486,7 +486,7 @@ static void cleanup_net(struct work_struct *work)
>>  	list_for_each_entry_reverse(ops, &pernet_list, list)
>>  		ops_free_list(ops, &net_exit_list);
>>  
>> -	mutex_unlock(&net_mutex);
>> +	up_read(&net_sem);
>>  
>>  	/* Ensure there are no outstanding rcu callbacks using this
>>  	 * network namespace.
>> @@ -513,8 +513,8 @@ static void cleanup_net(struct work_struct *work)
>>   */
>>  void net_ns_barrier(void)
>>  {
>> -	mutex_lock(&net_mutex);
>> -	mutex_unlock(&net_mutex);
>> +	down_write(&net_sem);
>> +	up_write(&net_sem);
>>  }
>>  EXPORT_SYMBOL(net_ns_barrier);
>>  
>> @@ -841,7 +841,7 @@ static int __init net_ns_init(void)
>>  
>>  	rcu_assign_pointer(init_net.gen, ng);
>>  
>> -	mutex_lock(&net_mutex);
>> +	down_read(&net_sem);
>>  	if (setup_net(&init_net, &init_user_ns))
>>  		panic("Could not setup the initial network namespace");
>>  
>> @@ -851,7 +851,7 @@ static int __init net_ns_init(void)
>>  	list_add_tail_rcu(&init_net.list, &net_namespace_list);
>>  	rtnl_unlock();
>>  
>> -	mutex_unlock(&net_mutex);
>> +	up_read(&net_sem);
>>  
>>  	register_pernet_subsys(&net_ns_ops);
>>  
>> @@ -991,9 +991,9 @@ static void unregister_pernet_operations(struct pernet_operations *ops)
>>  int register_pernet_subsys(struct pernet_operations *ops)
>>  {
>>  	int error;
>> -	mutex_lock(&net_mutex);
>> +	down_write(&net_sem);
>>  	error =  register_pernet_operations(first_device, ops);
>> -	mutex_unlock(&net_mutex);
>> +	up_write(&net_sem);
>>  	return error;
>>  }
>>  EXPORT_SYMBOL_GPL(register_pernet_subsys);
>> @@ -1009,9 +1009,9 @@ EXPORT_SYMBOL_GPL(register_pernet_subsys);
>>   */
>>  void unregister_pernet_subsys(struct pernet_operations *ops)
>>  {
>> -	mutex_lock(&net_mutex);
>> +	down_write(&net_sem);
>>  	unregister_pernet_operations(ops);
>> -	mutex_unlock(&net_mutex);
>> +	up_write(&net_sem);
>>  }
>>  EXPORT_SYMBOL_GPL(unregister_pernet_subsys);
>>  
>> @@ -1037,11 +1037,11 @@ EXPORT_SYMBOL_GPL(unregister_pernet_subsys);
>>  int register_pernet_device(struct pernet_operations *ops)
>>  {
>>  	int error;
>> -	mutex_lock(&net_mutex);
>> +	down_write(&net_sem);
>>  	error = register_pernet_operations(&pernet_list, ops);
>>  	if (!error && (first_device == &pernet_list))
>>  		first_device = &ops->list;
>> -	mutex_unlock(&net_mutex);
>> +	up_write(&net_sem);
>>  	return error;
>>  }
>>  EXPORT_SYMBOL_GPL(register_pernet_device);
>> @@ -1057,11 +1057,11 @@ EXPORT_SYMBOL_GPL(register_pernet_device);
>>   */
>>  void unregister_pernet_device(struct pernet_operations *ops)
>>  {
>> -	mutex_lock(&net_mutex);
>> +	down_write(&net_sem);
>>  	if (&ops->list == first_device)
>>  		first_device = first_device->next;
>>  	unregister_pernet_operations(ops);
>> -	mutex_unlock(&net_mutex);
>> +	up_write(&net_sem);
>>  }
>>  EXPORT_SYMBOL_GPL(unregister_pernet_device);
>>  
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index 5ace48926b19..caa215fd170b 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -390,11 +390,11 @@ static void rtnl_lock_unregistering_all(void)
>>  void rtnl_link_unregister(struct rtnl_link_ops *ops)
>>  {
>>  	/* Close the race with cleanup_net() */
>> -	mutex_lock(&net_mutex);
>> +	down_write(&net_sem);
>>  	rtnl_lock_unregistering_all();
>>  	__rtnl_link_unregister(ops);
>>  	rtnl_unlock();
>> -	mutex_unlock(&net_mutex);
>> +	up_write(&net_sem);
>>  }
>>  EXPORT_SYMBOL_GPL(rtnl_link_unregister);
>>

On 15.11.2017 06:19, Eric W. Biederman wrote:
> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
> 
>> On 14.11.2017 21:39, Cong Wang wrote:
>>> On Tue, Nov 14, 2017 at 5:53 AM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>>>> @@ -406,7 +406,7 @@ struct net *copy_net_ns(unsigned long flags,
>>>>
>>>>         get_user_ns(user_ns);
>>>>
>>>> -       rv = mutex_lock_killable(&net_mutex);
>>>> +       rv = down_read_killable(&net_sem);
>>>>         if (rv < 0) {
>>>>                 net_free(net);
>>>>                 dec_net_namespaces(ucounts);
>>>> @@ -421,7 +421,7 @@ struct net *copy_net_ns(unsigned long flags,
>>>>                 list_add_tail_rcu(&net->list, &net_namespace_list);
>>>>                 rtnl_unlock();
>>>>         }
>>>> -       mutex_unlock(&net_mutex);
>>>> +       up_read(&net_sem);
>>>>         if (rv < 0) {
>>>>                 dec_net_namespaces(ucounts);
>>>>                 put_user_ns(user_ns);
>>>> @@ -446,7 +446,7 @@ static void cleanup_net(struct work_struct *work)
>>>>         list_replace_init(&cleanup_list, &net_kill_list);
>>>>         spin_unlock_irq(&cleanup_list_lock);
>>>>
>>>> -       mutex_lock(&net_mutex);
>>>> +       down_read(&net_sem);
>>>>
>>>>         /* Don't let anyone else find us. */
>>>>         rtnl_lock();
>>>> @@ -486,7 +486,7 @@ static void cleanup_net(struct work_struct *work)
>>>>         list_for_each_entry_reverse(ops, &pernet_list, list)
>>>>                 ops_free_list(ops, &net_exit_list);
>>>>
>>>> -       mutex_unlock(&net_mutex);
>>>> +       up_read(&net_sem);
>>>
>>> After your patch setup_net() could run concurrently with cleanup_net(),
>>> given that ops_exit_list() is called on error path of setup_net() too,
>>> it means ops->exit() now could run concurrently if it doesn't have its
>>> own lock. Not sure if this breaks any existing user.
>>
>> Yes, there will be possible concurrent ops->init() for a net namespace,
>> and ops->exit() for another one. I hadn't found pernet operations, which
>> have a problem with that. If they exist, they are hidden and not clear seen.
>> The pernet operations in general do not touch someone else's memory.
>> If suddenly there is one, KASAN should show it after a while.
> 
> Certainly the use of hash tables shared between multiple network
> namespaces would count.  I don't rembmer how many of these we have but
> there used to be quite a few.

Could you please provide an example of hash tables, you mean?

On 15.11.2017 12:51, Kirill Tkhai wrote:
> On 15.11.2017 06:19, Eric W. Biederman wrote:
>> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>>
>>> On 14.11.2017 21:39, Cong Wang wrote:
>>>> On Tue, Nov 14, 2017 at 5:53 AM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>>>>> @@ -406,7 +406,7 @@ struct net *copy_net_ns(unsigned long flags,
>>>>>
>>>>>         get_user_ns(user_ns);
>>>>>
>>>>> -       rv = mutex_lock_killable(&net_mutex);
>>>>> +       rv = down_read_killable(&net_sem);
>>>>>         if (rv < 0) {
>>>>>                 net_free(net);
>>>>>                 dec_net_namespaces(ucounts);
>>>>> @@ -421,7 +421,7 @@ struct net *copy_net_ns(unsigned long flags,
>>>>>                 list_add_tail_rcu(&net->list, &net_namespace_list);
>>>>>                 rtnl_unlock();
>>>>>         }
>>>>> -       mutex_unlock(&net_mutex);
>>>>> +       up_read(&net_sem);
>>>>>         if (rv < 0) {
>>>>>                 dec_net_namespaces(ucounts);
>>>>>                 put_user_ns(user_ns);
>>>>> @@ -446,7 +446,7 @@ static void cleanup_net(struct work_struct *work)
>>>>>         list_replace_init(&cleanup_list, &net_kill_list);
>>>>>         spin_unlock_irq(&cleanup_list_lock);
>>>>>
>>>>> -       mutex_lock(&net_mutex);
>>>>> +       down_read(&net_sem);
>>>>>
>>>>>         /* Don't let anyone else find us. */
>>>>>         rtnl_lock();
>>>>> @@ -486,7 +486,7 @@ static void cleanup_net(struct work_struct *work)
>>>>>         list_for_each_entry_reverse(ops, &pernet_list, list)
>>>>>                 ops_free_list(ops, &net_exit_list);
>>>>>
>>>>> -       mutex_unlock(&net_mutex);
>>>>> +       up_read(&net_sem);
>>>>
>>>> After your patch setup_net() could run concurrently with cleanup_net(),
>>>> given that ops_exit_list() is called on error path of setup_net() too,
>>>> it means ops->exit() now could run concurrently if it doesn't have its
>>>> own lock. Not sure if this breaks any existing user.
>>>
>>> Yes, there will be possible concurrent ops->init() for a net namespace,
>>> and ops->exit() for another one. I hadn't found pernet operations, which
>>> have a problem with that. If they exist, they are hidden and not clear seen.
>>> The pernet operations in general do not touch someone else's memory.
>>> If suddenly there is one, KASAN should show it after a while.
>>
>> Certainly the use of hash tables shared between multiple network
>> namespaces would count.  I don't rembmer how many of these we have but
>> there used to be quite a few.
> 
> Could you please provide an example of hash tables, you mean?

Ah, I see, it's dccp_hashinfo etc.

Kirill Tkhai <ktkhai@virtuozzo.com> writes:

> On 15.11.2017 09:25, Eric W. Biederman wrote:
>> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>> 
>>> Curently mutex is used to protect pernet operations list. It makes
>>> cleanup_net() to execute ->exit methods of the same operations set,
>>> which was used on the time of ->init, even after net namespace is
>>> unlinked from net_namespace_list.
>>>
>>> But the problem is it's need to synchronize_rcu() after net is removed
>>> from net_namespace_list():
>>>
>>> Destroy net_ns:
>>> cleanup_net()
>>>   mutex_lock(&net_mutex)
>>>   list_del_rcu(&net->list)
>>>   synchronize_rcu()                                  <--- Sleep there for ages
>>>   list_for_each_entry_reverse(ops, &pernet_list, list)
>>>     ops_exit_list(ops, &net_exit_list)
>>>   list_for_each_entry_reverse(ops, &pernet_list, list)
>>>     ops_free_list(ops, &net_exit_list)
>>>   mutex_unlock(&net_mutex)
>>>
>>> This primitive is not fast, especially on the systems with many processors
>>> and/or when preemptible RCU is enabled in config. So, all the time, while
>>> cleanup_net() is waiting for RCU grace period, creation of new net namespaces
>>> is not possible, the tasks, who makes it, are sleeping on the same mutex:
>>>
>>> Create net_ns:
>>> copy_net_ns()
>>>   mutex_lock_killable(&net_mutex)                    <--- Sleep there for ages
>>>
>>> The solution is to convert net_mutex to the rw_semaphore. Then,
>>> pernet_operations::init/::exit methods, modifying the net-related data,
>>> will require down_read() locking only, while down_write() will be used
>>> for changing pernet_list.
>>>
>>> This gives signify performance increase, like you may see below. There
>>> is measured sequential net namespace creation in a cycle, in single
>>> thread, without other tasks (single user mode):
>>>
>>> 1)int main(int argc, char *argv[])
>>> {
>>>         unsigned nr;
>>>         if (argc < 2) {
>>>                 fprintf(stderr, "Provide nr iterations arg\n");
>>>                 return 1;
>>>         }
>>>         nr = atoi(argv[1]);
>>>         while (nr-- > 0) {
>>>                 if (unshare(CLONE_NEWNET)) {
>>>                         perror("Can't unshare");
>>>                         return 1;
>>>                 }
>>>         }
>>>         return 0;
>>> }
>>>
>>> Origin, 100000 unshare():
>>> 0.03user 23.14system 1:39.85elapsed 23%CPU
>>>
>>> Patched, 100000 unshare():
>>> 0.03user 67.49system 1:08.34elapsed 98%CPU
>>>
>>> 2)for i in {1..10000}; do unshare -n bash -c exit; done
>>>
>>> Origin:
>>> real 1m24,190s
>>> user 0m6,225s
>>> sys 0m15,132s
>>>
>>> Patched:
>>> real 0m18,235s   (4.6 times faster)
>>> user 0m4,544s
>>> sys 0m13,796s
>>>
>>> This patch requires commit 76f8507f7a64 "locking/rwsem: Add down_read_killable()"
>>> from Linus tree (not in net-next yet).
>> 
>> Using a rwsem to protect the list of operations makes sense.
>> 
>> That should allow removing the sing
>> 
>> I am not wild about taking a the rwsem down_write in
>> rtnl_link_unregister, and net_ns_barrier.  I think that works but it
>> goes from being a mild hack to being a pretty bad hack and something
>> else that can kill the parallelism you are seeking it add.
>> 
>> There are about 204 instances of struct pernet_operations.  That is a
>> lot of code to have carefully audited to ensure it can in parallel all
>> at once.  The existence of the exit_batch method, net_ns_barrier,
>> for_each_net and taking of net_mutex in rtnl_link_unregister all testify
>> to the fact that there are data structures accessed by multiple network
>> namespaces.
>> 
>> My preference would be to:
>> 
>> - Add the net_sem in addition to net_mutex with down_write only held in
>>   register and unregister, and maybe net_ns_barrier and
>>   rtnl_link_unregister.
>> 
>> - Factor out struct pernet_ops out of struct pernet_operations.  With
>>   struct pernet_ops not having the exit_batch method.  With pernet_ops
>>   being embedded an anonymous member of the old struct pernet_operations.
>> 
>> - Add [un]register_pernet_{sys,dev} functions that take a struct
>>   pernet_ops, that don't take net_mutex.  Have them order the
>>   pernet_list as:
>> 
>>   pernet_sys
>>   pernet_subsys
>>   pernet_device
>>   pernet_dev
>> 
>>   With the chunk in the middle taking the net_mutex.
>
> I think this approach will work. Thanks for the suggestion. Some more
> thoughts to the plan below.
>
> The only difficult thing there will be to choose the right order
> to move ops from pernet_subsys to pernet_sys and from pernet_device
> to pernet_dev one by one.
>
> This is rather easy in case of tristate drivers, as modules may be loaded
> at any time, and the only important order is dependences between them.
> So, it's possible to start from a module, who has no dependences,
> and move it to pernet_sys, and then continue with modules,
> who have no more dependences in pernet_subsys. For pernet_device
> it's vise versa.
>
> In case of bool drivers, the situation may be worse, because
> the order is not so clear there. The same priority initcalls
> (for example, initcalls, registered via core_initcall()) may require
> the certain order, driven by linking order. I know one example from
> device mapper code, which lives here: drivers/md/Makefile.
> This problem is also solvable, even if such places do not contain
> comments about linking order. It's just need to respect Makefile
> order, when choosing a new candidate to move.
>  
>>   I think I would enforce the ordering with a failure to register
>>   if a subsystem or a device tried to register out of order.  
>> 
>> - Disable use of the single threaded workqueue if nothing needs the
>>   net_mutex.
>
> We may use per-cpu worqueues in the future. The idea to refuse using
> worqueue doesn't seem good for me, because asynchronous net destroying
> looks very useful.

per-cpu workqueues are fine, and definitely what I am expecting.  If we
are doing this I want to get us off the single threaded workqueue that
serializes all of the cleanup.  That has a huge potential for
simplifying things and reducing maintenance if running everything in
parallel is actually safe.

I forget how the modern per-cpu workqueues work with respect to sleeps
and locking (I don't remember if when piece of work sleeps for a long
time we spin up another thread per-cpu workqueue thread, and thus avoid
priority inversion problems).

If locks between workqueues are not a problem we could start the
transition off of the single-threaded serializing workqueue sooner
rather than later.

>> - Add a test mode that deliberartely spawns threads on multiple
>>   processors and deliberately creates multiple network namespaces
>>   at the same time.
>> 
>> - Add a test mode that deliberately spawns threads on multiple
>>   processors and delibearate destrosy multiple network namespaces
>>   at the same time.>
>> - Convert the code to unlocked operation one pernet_operations to at a
>>   time.  Being careful with the loopback device it's order in the list
>>   strongly matters.
>> 
>> - Finally remove the unnecessary code.
>> 
>> 
>> At the end of the day because all of the operations for one network
>> namespace will run in parallel with all of the operations for another
>> network namespace all of the sophistication that goes into batching the
>> cleanup of multiple network namespaces can be removed.  As different
>> tasks (not sharing a lock) can wait in syncrhonize_rcu at the same time
>> without slowing each other down.
>> 
>> I think we can remove the batching but I am afraid that will lead us into
>> rtnl_lock contention.
>
> I've looked into this lock. It used in many places for many reasons.
> It's a little strange, nobody tried to break it up in several small locks..

It gets tricky.  At this point getting net_mutex is enough to start
with. Mostly the rtnl_lock covers the slow path which tends to keep it
from rising to the top of the priority list.

>> I am definitely not comfortable with changing the locking on so much
>> code without an explanation at all why it is safe in the commit comments
>> in all 204 instances.  Which combined equal most of the well maintained
>> and regularly used part of the networking stack.
>
> David,
>
> could you please check the plan above and say whether 1)it's OK for you,
> and 2)if so, will you expect all the changes are made in one big 200+ patch set
> or we may go sequentially(50 patches a time, for example)?

I think I would start with the something covering the core networking
pieces so the improvements can be seen.


Eric

Kirill Tkhai <ktkhai@virtuozzo.com> writes:

> On 15.11.2017 12:51, Kirill Tkhai wrote:
>> On 15.11.2017 06:19, Eric W. Biederman wrote:
>>> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>>>
>>>> On 14.11.2017 21:39, Cong Wang wrote:
>>>>> On Tue, Nov 14, 2017 at 5:53 AM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>>>>>> @@ -406,7 +406,7 @@ struct net *copy_net_ns(unsigned long flags,
>>>>>>
>>>>>>         get_user_ns(user_ns);
>>>>>>
>>>>>> -       rv = mutex_lock_killable(&net_mutex);
>>>>>> +       rv = down_read_killable(&net_sem);
>>>>>>         if (rv < 0) {
>>>>>>                 net_free(net);
>>>>>>                 dec_net_namespaces(ucounts);
>>>>>> @@ -421,7 +421,7 @@ struct net *copy_net_ns(unsigned long flags,
>>>>>>                 list_add_tail_rcu(&net->list, &net_namespace_list);
>>>>>>                 rtnl_unlock();
>>>>>>         }
>>>>>> -       mutex_unlock(&net_mutex);
>>>>>> +       up_read(&net_sem);
>>>>>>         if (rv < 0) {
>>>>>>                 dec_net_namespaces(ucounts);
>>>>>>                 put_user_ns(user_ns);
>>>>>> @@ -446,7 +446,7 @@ static void cleanup_net(struct work_struct *work)
>>>>>>         list_replace_init(&cleanup_list, &net_kill_list);
>>>>>>         spin_unlock_irq(&cleanup_list_lock);
>>>>>>
>>>>>> -       mutex_lock(&net_mutex);
>>>>>> +       down_read(&net_sem);
>>>>>>
>>>>>>         /* Don't let anyone else find us. */
>>>>>>         rtnl_lock();
>>>>>> @@ -486,7 +486,7 @@ static void cleanup_net(struct work_struct *work)
>>>>>>         list_for_each_entry_reverse(ops, &pernet_list, list)
>>>>>>                 ops_free_list(ops, &net_exit_list);
>>>>>>
>>>>>> -       mutex_unlock(&net_mutex);
>>>>>> +       up_read(&net_sem);
>>>>>
>>>>> After your patch setup_net() could run concurrently with cleanup_net(),
>>>>> given that ops_exit_list() is called on error path of setup_net() too,
>>>>> it means ops->exit() now could run concurrently if it doesn't have its
>>>>> own lock. Not sure if this breaks any existing user.
>>>>
>>>> Yes, there will be possible concurrent ops->init() for a net namespace,
>>>> and ops->exit() for another one. I hadn't found pernet operations, which
>>>> have a problem with that. If they exist, they are hidden and not clear seen.
>>>> The pernet operations in general do not touch someone else's memory.
>>>> If suddenly there is one, KASAN should show it after a while.
>>>
>>> Certainly the use of hash tables shared between multiple network
>>> namespaces would count.  I don't rembmer how many of these we have but
>>> there used to be quite a few.
>> 
>> Could you please provide an example of hash tables, you mean?
>
> Ah, I see, it's dccp_hashinfo etc.

The big one used to be the route cache.  With resizable hash tables
things may be getting better in that regard.

Eric

On 15.11.2017 19:29, Eric W. Biederman wrote:
> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
> 
>> On 15.11.2017 09:25, Eric W. Biederman wrote:
>>> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>>>
>>>> Curently mutex is used to protect pernet operations list. It makes
>>>> cleanup_net() to execute ->exit methods of the same operations set,
>>>> which was used on the time of ->init, even after net namespace is
>>>> unlinked from net_namespace_list.
>>>>
>>>> But the problem is it's need to synchronize_rcu() after net is removed
>>>> from net_namespace_list():
>>>>
>>>> Destroy net_ns:
>>>> cleanup_net()
>>>>   mutex_lock(&net_mutex)
>>>>   list_del_rcu(&net->list)
>>>>   synchronize_rcu()                                  <--- Sleep there for ages
>>>>   list_for_each_entry_reverse(ops, &pernet_list, list)
>>>>     ops_exit_list(ops, &net_exit_list)
>>>>   list_for_each_entry_reverse(ops, &pernet_list, list)
>>>>     ops_free_list(ops, &net_exit_list)
>>>>   mutex_unlock(&net_mutex)
>>>>
>>>> This primitive is not fast, especially on the systems with many processors
>>>> and/or when preemptible RCU is enabled in config. So, all the time, while
>>>> cleanup_net() is waiting for RCU grace period, creation of new net namespaces
>>>> is not possible, the tasks, who makes it, are sleeping on the same mutex:
>>>>
>>>> Create net_ns:
>>>> copy_net_ns()
>>>>   mutex_lock_killable(&net_mutex)                    <--- Sleep there for ages
>>>>
>>>> The solution is to convert net_mutex to the rw_semaphore. Then,
>>>> pernet_operations::init/::exit methods, modifying the net-related data,
>>>> will require down_read() locking only, while down_write() will be used
>>>> for changing pernet_list.
>>>>
>>>> This gives signify performance increase, like you may see below. There
>>>> is measured sequential net namespace creation in a cycle, in single
>>>> thread, without other tasks (single user mode):
>>>>
>>>> 1)int main(int argc, char *argv[])
>>>> {
>>>>         unsigned nr;
>>>>         if (argc < 2) {
>>>>                 fprintf(stderr, "Provide nr iterations arg\n");
>>>>                 return 1;
>>>>         }
>>>>         nr = atoi(argv[1]);
>>>>         while (nr-- > 0) {
>>>>                 if (unshare(CLONE_NEWNET)) {
>>>>                         perror("Can't unshare");
>>>>                         return 1;
>>>>                 }
>>>>         }
>>>>         return 0;
>>>> }
>>>>
>>>> Origin, 100000 unshare():
>>>> 0.03user 23.14system 1:39.85elapsed 23%CPU
>>>>
>>>> Patched, 100000 unshare():
>>>> 0.03user 67.49system 1:08.34elapsed 98%CPU
>>>>
>>>> 2)for i in {1..10000}; do unshare -n bash -c exit; done
>>>>
>>>> Origin:
>>>> real 1m24,190s
>>>> user 0m6,225s
>>>> sys 0m15,132s
>>>>
>>>> Patched:
>>>> real 0m18,235s   (4.6 times faster)
>>>> user 0m4,544s
>>>> sys 0m13,796s
>>>>
>>>> This patch requires commit 76f8507f7a64 "locking/rwsem: Add down_read_killable()"
>>>> from Linus tree (not in net-next yet).
>>>
>>> Using a rwsem to protect the list of operations makes sense.
>>>
>>> That should allow removing the sing
>>>
>>> I am not wild about taking a the rwsem down_write in
>>> rtnl_link_unregister, and net_ns_barrier.  I think that works but it
>>> goes from being a mild hack to being a pretty bad hack and something
>>> else that can kill the parallelism you are seeking it add.
>>>
>>> There are about 204 instances of struct pernet_operations.  That is a
>>> lot of code to have carefully audited to ensure it can in parallel all
>>> at once.  The existence of the exit_batch method, net_ns_barrier,
>>> for_each_net and taking of net_mutex in rtnl_link_unregister all testify
>>> to the fact that there are data structures accessed by multiple network
>>> namespaces.
>>>
>>> My preference would be to:
>>>
>>> - Add the net_sem in addition to net_mutex with down_write only held in
>>>   register and unregister, and maybe net_ns_barrier and
>>>   rtnl_link_unregister.
>>>
>>> - Factor out struct pernet_ops out of struct pernet_operations.  With
>>>   struct pernet_ops not having the exit_batch method.  With pernet_ops
>>>   being embedded an anonymous member of the old struct pernet_operations.
>>>
>>> - Add [un]register_pernet_{sys,dev} functions that take a struct
>>>   pernet_ops, that don't take net_mutex.  Have them order the
>>>   pernet_list as:
>>>
>>>   pernet_sys
>>>   pernet_subsys
>>>   pernet_device
>>>   pernet_dev
>>>
>>>   With the chunk in the middle taking the net_mutex.
>>
>> I think this approach will work. Thanks for the suggestion. Some more
>> thoughts to the plan below.
>>
>> The only difficult thing there will be to choose the right order
>> to move ops from pernet_subsys to pernet_sys and from pernet_device
>> to pernet_dev one by one.
>>
>> This is rather easy in case of tristate drivers, as modules may be loaded
>> at any time, and the only important order is dependences between them.
>> So, it's possible to start from a module, who has no dependences,
>> and move it to pernet_sys, and then continue with modules,
>> who have no more dependences in pernet_subsys. For pernet_device
>> it's vise versa.
>>
>> In case of bool drivers, the situation may be worse, because
>> the order is not so clear there. The same priority initcalls
>> (for example, initcalls, registered via core_initcall()) may require
>> the certain order, driven by linking order. I know one example from
>> device mapper code, which lives here: drivers/md/Makefile.
>> This problem is also solvable, even if such places do not contain
>> comments about linking order. It's just need to respect Makefile
>> order, when choosing a new candidate to move.
>>  
>>>   I think I would enforce the ordering with a failure to register
>>>   if a subsystem or a device tried to register out of order.  
>>>
>>> - Disable use of the single threaded workqueue if nothing needs the
>>>   net_mutex.
>>
>> We may use per-cpu worqueues in the future. The idea to refuse using
>> worqueue doesn't seem good for me, because asynchronous net destroying
>> looks very useful.
> 
> per-cpu workqueues are fine, and definitely what I am expecting.  If we
> are doing this I want to get us off the single threaded workqueue that
> serializes all of the cleanup.  That has a huge potential for
> simplifying things and reducing maintenance if running everything in
> parallel is actually safe.
> 
> I forget how the modern per-cpu workqueues work with respect to sleeps
> and locking (I don't remember if when piece of work sleeps for a long
> time we spin up another thread per-cpu workqueue thread, and thus avoid
> priority inversion problems).

As I know, there is no a problem for worker thread to sleep, as there is
a pool. And right after one leaves cpu runqueue, another worker wakes up.

> If locks between workqueues are not a problem we could start the
> transition off of the single-threaded serializing workqueue sooner
> rather than later.
> 
>>> - Add a test mode that deliberartely spawns threads on multiple
>>>   processors and deliberately creates multiple network namespaces
>>>   at the same time.
>>>
>>> - Add a test mode that deliberately spawns threads on multiple
>>>   processors and delibearate destrosy multiple network namespaces
>>>   at the same time.>
>>> - Convert the code to unlocked operation one pernet_operations to at a
>>>   time.  Being careful with the loopback device it's order in the list
>>>   strongly matters.
>>>
>>> - Finally remove the unnecessary code.
>>>
>>>
>>> At the end of the day because all of the operations for one network
>>> namespace will run in parallel with all of the operations for another
>>> network namespace all of the sophistication that goes into batching the
>>> cleanup of multiple network namespaces can be removed.  As different
>>> tasks (not sharing a lock) can wait in syncrhonize_rcu at the same time
>>> without slowing each other down.
>>>
>>> I think we can remove the batching but I am afraid that will lead us into
>>> rtnl_lock contention.
>>
>> I've looked into this lock. It used in many places for many reasons.
>> It's a little strange, nobody tried to break it up in several small locks..
> 
> It gets tricky.  At this point getting net_mutex is enough to start
> with. Mostly the rtnl_lock covers the slow path which tends to keep it
> from rising to the top of the priority list.
> 
>>> I am definitely not comfortable with changing the locking on so much
>>> code without an explanation at all why it is safe in the commit comments
>>> in all 204 instances.  Which combined equal most of the well maintained
>>> and regularly used part of the networking stack.
>>
>> David,
>>
>> could you please check the plan above and say whether 1)it's OK for you,
>> and 2)if so, will you expect all the changes are made in one big 200+ patch set
>> or we may go sequentially(50 patches a time, for example)?
> 
> I think I would start with the something covering the core networking
> pieces so the improvements can be seen.
> 
> 
> Eric
> 
>

On 15.11.2017 19:29, Eric W. Biederman wrote:
> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
> 
>> On 15.11.2017 09:25, Eric W. Biederman wrote:
>>> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>>>
>>>> Curently mutex is used to protect pernet operations list. It makes
>>>> cleanup_net() to execute ->exit methods of the same operations set,
>>>> which was used on the time of ->init, even after net namespace is
>>>> unlinked from net_namespace_list.
>>>>
>>>> But the problem is it's need to synchronize_rcu() after net is removed
>>>> from net_namespace_list():
>>>>
>>>> Destroy net_ns:
>>>> cleanup_net()
>>>>   mutex_lock(&net_mutex)
>>>>   list_del_rcu(&net->list)
>>>>   synchronize_rcu()                                  <--- Sleep there for ages
>>>>   list_for_each_entry_reverse(ops, &pernet_list, list)
>>>>     ops_exit_list(ops, &net_exit_list)
>>>>   list_for_each_entry_reverse(ops, &pernet_list, list)
>>>>     ops_free_list(ops, &net_exit_list)
>>>>   mutex_unlock(&net_mutex)
>>>>
>>>> This primitive is not fast, especially on the systems with many processors
>>>> and/or when preemptible RCU is enabled in config. So, all the time, while
>>>> cleanup_net() is waiting for RCU grace period, creation of new net namespaces
>>>> is not possible, the tasks, who makes it, are sleeping on the same mutex:
>>>>
>>>> Create net_ns:
>>>> copy_net_ns()
>>>>   mutex_lock_killable(&net_mutex)                    <--- Sleep there for ages
>>>>
>>>> The solution is to convert net_mutex to the rw_semaphore. Then,
>>>> pernet_operations::init/::exit methods, modifying the net-related data,
>>>> will require down_read() locking only, while down_write() will be used
>>>> for changing pernet_list.
>>>>
>>>> This gives signify performance increase, like you may see below. There
>>>> is measured sequential net namespace creation in a cycle, in single
>>>> thread, without other tasks (single user mode):
>>>>
>>>> 1)int main(int argc, char *argv[])
>>>> {
>>>>         unsigned nr;
>>>>         if (argc < 2) {
>>>>                 fprintf(stderr, "Provide nr iterations arg\n");
>>>>                 return 1;
>>>>         }
>>>>         nr = atoi(argv[1]);
>>>>         while (nr-- > 0) {
>>>>                 if (unshare(CLONE_NEWNET)) {
>>>>                         perror("Can't unshare");
>>>>                         return 1;
>>>>                 }
>>>>         }
>>>>         return 0;
>>>> }
>>>>
>>>> Origin, 100000 unshare():
>>>> 0.03user 23.14system 1:39.85elapsed 23%CPU
>>>>
>>>> Patched, 100000 unshare():
>>>> 0.03user 67.49system 1:08.34elapsed 98%CPU
>>>>
>>>> 2)for i in {1..10000}; do unshare -n bash -c exit; done
>>>>
>>>> Origin:
>>>> real 1m24,190s
>>>> user 0m6,225s
>>>> sys 0m15,132s
>>>>
>>>> Patched:
>>>> real 0m18,235s   (4.6 times faster)
>>>> user 0m4,544s
>>>> sys 0m13,796s
>>>>
>>>> This patch requires commit 76f8507f7a64 "locking/rwsem: Add down_read_killable()"
>>>> from Linus tree (not in net-next yet).
>>>
>>> Using a rwsem to protect the list of operations makes sense.
>>>
>>> That should allow removing the sing
>>>
>>> I am not wild about taking a the rwsem down_write in
>>> rtnl_link_unregister, and net_ns_barrier.  I think that works but it
>>> goes from being a mild hack to being a pretty bad hack and something
>>> else that can kill the parallelism you are seeking it add.
>>>
>>> There are about 204 instances of struct pernet_operations.  That is a
>>> lot of code to have carefully audited to ensure it can in parallel all
>>> at once.  The existence of the exit_batch method, net_ns_barrier,
>>> for_each_net and taking of net_mutex in rtnl_link_unregister all testify
>>> to the fact that there are data structures accessed by multiple network
>>> namespaces.
>>>
>>> My preference would be to:
>>>
>>> - Add the net_sem in addition to net_mutex with down_write only held in
>>>   register and unregister, and maybe net_ns_barrier and
>>>   rtnl_link_unregister.
>>>
>>> - Factor out struct pernet_ops out of struct pernet_operations.  With
>>>   struct pernet_ops not having the exit_batch method.  With pernet_ops
>>>   being embedded an anonymous member of the old struct pernet_operations.
>>>
>>> - Add [un]register_pernet_{sys,dev} functions that take a struct
>>>   pernet_ops, that don't take net_mutex.  Have them order the
>>>   pernet_list as:
>>>
>>>   pernet_sys
>>>   pernet_subsys
>>>   pernet_device
>>>   pernet_dev
>>>
>>>   With the chunk in the middle taking the net_mutex.
>>
>> I think this approach will work. Thanks for the suggestion. Some more
>> thoughts to the plan below.
>>
>> The only difficult thing there will be to choose the right order
>> to move ops from pernet_subsys to pernet_sys and from pernet_device
>> to pernet_dev one by one.
>>
>> This is rather easy in case of tristate drivers, as modules may be loaded
>> at any time, and the only important order is dependences between them.
>> So, it's possible to start from a module, who has no dependences,
>> and move it to pernet_sys, and then continue with modules,
>> who have no more dependences in pernet_subsys. For pernet_device
>> it's vise versa.
>>
>> In case of bool drivers, the situation may be worse, because
>> the order is not so clear there. The same priority initcalls
>> (for example, initcalls, registered via core_initcall()) may require
>> the certain order, driven by linking order. I know one example from
>> device mapper code, which lives here: drivers/md/Makefile.
>> This problem is also solvable, even if such places do not contain
>> comments about linking order. It's just need to respect Makefile
>> order, when choosing a new candidate to move.
>>  
>>>   I think I would enforce the ordering with a failure to register
>>>   if a subsystem or a device tried to register out of order.  
>>>
>>> - Disable use of the single threaded workqueue if nothing needs the
>>>   net_mutex.
>>
>> We may use per-cpu worqueues in the future. The idea to refuse using
>> worqueue doesn't seem good for me, because asynchronous net destroying
>> looks very useful.
> 
> per-cpu workqueues are fine, and definitely what I am expecting.  If we
> are doing this I want to get us off the single threaded workqueue that
> serializes all of the cleanup.  That has a huge potential for
> simplifying things and reducing maintenance if running everything in
> parallel is actually safe.
> 
> I forget how the modern per-cpu workqueues work with respect to sleeps
> and locking (I don't remember if when piece of work sleeps for a long
> time we spin up another thread per-cpu workqueue thread, and thus avoid
> priority inversion problems).
> 
> If locks between workqueues are not a problem we could start the
> transition off of the single-threaded serializing workqueue sooner
> rather than later.
> 
>>> - Add a test mode that deliberartely spawns threads on multiple
>>>   processors and deliberately creates multiple network namespaces
>>>   at the same time.
>>>
>>> - Add a test mode that deliberately spawns threads on multiple
>>>   processors and delibearate destrosy multiple network namespaces
>>>   at the same time.>
>>> - Convert the code to unlocked operation one pernet_operations to at a
>>>   time.  Being careful with the loopback device it's order in the list
>>>   strongly matters.
>>>
>>> - Finally remove the unnecessary code.
>>>
>>>
>>> At the end of the day because all of the operations for one network
>>> namespace will run in parallel with all of the operations for another
>>> network namespace all of the sophistication that goes into batching the
>>> cleanup of multiple network namespaces can be removed.  As different
>>> tasks (not sharing a lock) can wait in syncrhonize_rcu at the same time
>>> without slowing each other down.
>>>
>>> I think we can remove the batching but I am afraid that will lead us into
>>> rtnl_lock contention.
>>
>> I've looked into this lock. It used in many places for many reasons.
>> It's a little strange, nobody tried to break it up in several small locks..
> 
> It gets tricky.  At this point getting net_mutex is enough to start
> with. Mostly the rtnl_lock covers the slow path which tends to keep it
> from rising to the top of the priority list.
> 
>>> I am definitely not comfortable with changing the locking on so much
>>> code without an explanation at all why it is safe in the commit comments
>>> in all 204 instances.  Which combined equal most of the well maintained
>>> and regularly used part of the networking stack.
>>
>> David,
>>
>> could you please check the plan above and say whether 1)it's OK for you,
>> and 2)if so, will you expect all the changes are made in one big 200+ patch set
>> or we may go sequentially(50 patches a time, for example)?
> 
> I think I would start with the something covering the core networking
> pieces so the improvements can be seen.

Since the main problem is mutex held during synchronize_rcu(), the performance
improvements may become seen only after it's completely removed.

On 15.11.2017 19:31, Eric W. Biederman wrote:
> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
> 
>> On 15.11.2017 12:51, Kirill Tkhai wrote:
>>> On 15.11.2017 06:19, Eric W. Biederman wrote:
>>>> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>>>>
>>>>> On 14.11.2017 21:39, Cong Wang wrote:
>>>>>> On Tue, Nov 14, 2017 at 5:53 AM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>>>>>>> @@ -406,7 +406,7 @@ struct net *copy_net_ns(unsigned long flags,
>>>>>>>
>>>>>>>         get_user_ns(user_ns);
>>>>>>>
>>>>>>> -       rv = mutex_lock_killable(&net_mutex);
>>>>>>> +       rv = down_read_killable(&net_sem);
>>>>>>>         if (rv < 0) {
>>>>>>>                 net_free(net);
>>>>>>>                 dec_net_namespaces(ucounts);
>>>>>>> @@ -421,7 +421,7 @@ struct net *copy_net_ns(unsigned long flags,
>>>>>>>                 list_add_tail_rcu(&net->list, &net_namespace_list);
>>>>>>>                 rtnl_unlock();
>>>>>>>         }
>>>>>>> -       mutex_unlock(&net_mutex);
>>>>>>> +       up_read(&net_sem);
>>>>>>>         if (rv < 0) {
>>>>>>>                 dec_net_namespaces(ucounts);
>>>>>>>                 put_user_ns(user_ns);
>>>>>>> @@ -446,7 +446,7 @@ static void cleanup_net(struct work_struct *work)
>>>>>>>         list_replace_init(&cleanup_list, &net_kill_list);
>>>>>>>         spin_unlock_irq(&cleanup_list_lock);
>>>>>>>
>>>>>>> -       mutex_lock(&net_mutex);
>>>>>>> +       down_read(&net_sem);
>>>>>>>
>>>>>>>         /* Don't let anyone else find us. */
>>>>>>>         rtnl_lock();
>>>>>>> @@ -486,7 +486,7 @@ static void cleanup_net(struct work_struct *work)
>>>>>>>         list_for_each_entry_reverse(ops, &pernet_list, list)
>>>>>>>                 ops_free_list(ops, &net_exit_list);
>>>>>>>
>>>>>>> -       mutex_unlock(&net_mutex);
>>>>>>> +       up_read(&net_sem);
>>>>>>
>>>>>> After your patch setup_net() could run concurrently with cleanup_net(),
>>>>>> given that ops_exit_list() is called on error path of setup_net() too,
>>>>>> it means ops->exit() now could run concurrently if it doesn't have its
>>>>>> own lock. Not sure if this breaks any existing user.
>>>>>
>>>>> Yes, there will be possible concurrent ops->init() for a net namespace,
>>>>> and ops->exit() for another one. I hadn't found pernet operations, which
>>>>> have a problem with that. If they exist, they are hidden and not clear seen.
>>>>> The pernet operations in general do not touch someone else's memory.
>>>>> If suddenly there is one, KASAN should show it after a while.
>>>>
>>>> Certainly the use of hash tables shared between multiple network
>>>> namespaces would count.  I don't rembmer how many of these we have but
>>>> there used to be quite a few.
>>>
>>> Could you please provide an example of hash tables, you mean?
>>
>> Ah, I see, it's dccp_hashinfo etc.

JFI, I've checked dccp_hashinfo, and it seems to be safe.

> 
> The big one used to be the route cache.  With resizable hash tables
> things may be getting better in that regard.

I've checked some fib-related things, and wasn't able to find that.
Excuse me, could you please clarify, if it's an assumption, or
there is exactly a problem hash table, you know? Could you please
point it me more exactly, if it's so.

Kirill Tkhai <ktkhai@virtuozzo.com> writes:

> On 15.11.2017 19:31, Eric W. Biederman wrote:
>> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>> 
>>> On 15.11.2017 12:51, Kirill Tkhai wrote:
>>>> On 15.11.2017 06:19, Eric W. Biederman wrote:
>>>>> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>>>>>
>>>>>> On 14.11.2017 21:39, Cong Wang wrote:
>>>>>>> On Tue, Nov 14, 2017 at 5:53 AM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>>>>>>>> @@ -406,7 +406,7 @@ struct net *copy_net_ns(unsigned long flags,
>>>>>>>>
>>>>>>>>         get_user_ns(user_ns);
>>>>>>>>
>>>>>>>> -       rv = mutex_lock_killable(&net_mutex);
>>>>>>>> +       rv = down_read_killable(&net_sem);
>>>>>>>>         if (rv < 0) {
>>>>>>>>                 net_free(net);
>>>>>>>>                 dec_net_namespaces(ucounts);
>>>>>>>> @@ -421,7 +421,7 @@ struct net *copy_net_ns(unsigned long flags,
>>>>>>>>                 list_add_tail_rcu(&net->list, &net_namespace_list);
>>>>>>>>                 rtnl_unlock();
>>>>>>>>         }
>>>>>>>> -       mutex_unlock(&net_mutex);
>>>>>>>> +       up_read(&net_sem);
>>>>>>>>         if (rv < 0) {
>>>>>>>>                 dec_net_namespaces(ucounts);
>>>>>>>>                 put_user_ns(user_ns);
>>>>>>>> @@ -446,7 +446,7 @@ static void cleanup_net(struct work_struct *work)
>>>>>>>>         list_replace_init(&cleanup_list, &net_kill_list);
>>>>>>>>         spin_unlock_irq(&cleanup_list_lock);
>>>>>>>>
>>>>>>>> -       mutex_lock(&net_mutex);
>>>>>>>> +       down_read(&net_sem);
>>>>>>>>
>>>>>>>>         /* Don't let anyone else find us. */
>>>>>>>>         rtnl_lock();
>>>>>>>> @@ -486,7 +486,7 @@ static void cleanup_net(struct work_struct *work)
>>>>>>>>         list_for_each_entry_reverse(ops, &pernet_list, list)
>>>>>>>>                 ops_free_list(ops, &net_exit_list);
>>>>>>>>
>>>>>>>> -       mutex_unlock(&net_mutex);
>>>>>>>> +       up_read(&net_sem);
>>>>>>>
>>>>>>> After your patch setup_net() could run concurrently with cleanup_net(),
>>>>>>> given that ops_exit_list() is called on error path of setup_net() too,
>>>>>>> it means ops->exit() now could run concurrently if it doesn't have its
>>>>>>> own lock. Not sure if this breaks any existing user.
>>>>>>
>>>>>> Yes, there will be possible concurrent ops->init() for a net namespace,
>>>>>> and ops->exit() for another one. I hadn't found pernet operations, which
>>>>>> have a problem with that. If they exist, they are hidden and not clear seen.
>>>>>> The pernet operations in general do not touch someone else's memory.
>>>>>> If suddenly there is one, KASAN should show it after a while.
>>>>>
>>>>> Certainly the use of hash tables shared between multiple network
>>>>> namespaces would count.  I don't rembmer how many of these we have but
>>>>> there used to be quite a few.
>>>>
>>>> Could you please provide an example of hash tables, you mean?
>>>
>>> Ah, I see, it's dccp_hashinfo etc.
>
> JFI, I've checked dccp_hashinfo, and it seems to be safe.
>
>> 
>> The big one used to be the route cache.  With resizable hash tables
>> things may be getting better in that regard.
>
> I've checked some fib-related things, and wasn't able to find that.
> Excuse me, could you please clarify, if it's an assumption, or
> there is exactly a problem hash table, you know? Could you please
> point it me more exactly, if it's so.

Two things.
1) Hash tables are one case I know where we access data from multiple
   network namespaces.  As such it can not be asserted that is no
   possibility for problems.

2) The responsible way to handle this is one patch for each set of
   methods explaining why those methods are safe to run in parallel.

   That ensures there is opportunity for review and people are going
   slowly enough that they actually look at these issues.

The reason I want to see this broken up is that at 200ish sets of
methods it is too much to review all at once.

I completely agree that odds are that this can be made safe and that it
is mostly likely already safe in practically every instance.    My guess
would be that if there are problems that need to be addressed they
happen in one or two places and we need to find them.  If possible I
don't want to find them after the code has shipped in a stable release.

Eric

Kirill Tkhai <ktkhai@virtuozzo.com> writes:

> On 15.11.2017 19:29, Eric W. Biederman wrote:
>> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>> 
>>> On 15.11.2017 09:25, Eric W. Biederman wrote:
>>>> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>>>>
>>>>> Curently mutex is used to protect pernet operations list. It makes
>>>>> cleanup_net() to execute ->exit methods of the same operations set,
>>>>> which was used on the time of ->init, even after net namespace is
>>>>> unlinked from net_namespace_list.
>>>>>
>>>>> But the problem is it's need to synchronize_rcu() after net is removed
>>>>> from net_namespace_list():
>>>>>
>>>>> Destroy net_ns:
>>>>> cleanup_net()
>>>>>   mutex_lock(&net_mutex)
>>>>>   list_del_rcu(&net->list)
>>>>>   synchronize_rcu()                                  <--- Sleep there for ages
>>>>>   list_for_each_entry_reverse(ops, &pernet_list, list)
>>>>>     ops_exit_list(ops, &net_exit_list)
>>>>>   list_for_each_entry_reverse(ops, &pernet_list, list)
>>>>>     ops_free_list(ops, &net_exit_list)
>>>>>   mutex_unlock(&net_mutex)
>>>>>
>>>>> This primitive is not fast, especially on the systems with many processors
>>>>> and/or when preemptible RCU is enabled in config. So, all the time, while
>>>>> cleanup_net() is waiting for RCU grace period, creation of new net namespaces
>>>>> is not possible, the tasks, who makes it, are sleeping on the same mutex:
>>>>>
>>>>> Create net_ns:
>>>>> copy_net_ns()
>>>>>   mutex_lock_killable(&net_mutex)                    <--- Sleep there for ages
>>>>>
>>>>> The solution is to convert net_mutex to the rw_semaphore. Then,
>>>>> pernet_operations::init/::exit methods, modifying the net-related data,
>>>>> will require down_read() locking only, while down_write() will be used
>>>>> for changing pernet_list.
>>>>>
>>>>> This gives signify performance increase, like you may see below. There
>>>>> is measured sequential net namespace creation in a cycle, in single
>>>>> thread, without other tasks (single user mode):
>>>>>
>>>>> 1)int main(int argc, char *argv[])
>>>>> {
>>>>>         unsigned nr;
>>>>>         if (argc < 2) {
>>>>>                 fprintf(stderr, "Provide nr iterations arg\n");
>>>>>                 return 1;
>>>>>         }
>>>>>         nr = atoi(argv[1]);
>>>>>         while (nr-- > 0) {
>>>>>                 if (unshare(CLONE_NEWNET)) {
>>>>>                         perror("Can't unshare");
>>>>>                         return 1;
>>>>>                 }
>>>>>         }
>>>>>         return 0;
>>>>> }
>>>>>
>>>>> Origin, 100000 unshare():
>>>>> 0.03user 23.14system 1:39.85elapsed 23%CPU
>>>>>
>>>>> Patched, 100000 unshare():
>>>>> 0.03user 67.49system 1:08.34elapsed 98%CPU
>>>>>
>>>>> 2)for i in {1..10000}; do unshare -n bash -c exit; done
>>>>>
>>>>> Origin:
>>>>> real 1m24,190s
>>>>> user 0m6,225s
>>>>> sys 0m15,132s
>>>>>
>>>>> Patched:
>>>>> real 0m18,235s   (4.6 times faster)
>>>>> user 0m4,544s
>>>>> sys 0m13,796s
>>>>>
>>>>> This patch requires commit 76f8507f7a64 "locking/rwsem: Add down_read_killable()"
>>>>> from Linus tree (not in net-next yet).
>>>>
>>>> Using a rwsem to protect the list of operations makes sense.
>>>>
>>>> That should allow removing the sing
>>>>
>>>> I am not wild about taking a the rwsem down_write in
>>>> rtnl_link_unregister, and net_ns_barrier.  I think that works but it
>>>> goes from being a mild hack to being a pretty bad hack and something
>>>> else that can kill the parallelism you are seeking it add.
>>>>
>>>> There are about 204 instances of struct pernet_operations.  That is a
>>>> lot of code to have carefully audited to ensure it can in parallel all
>>>> at once.  The existence of the exit_batch method, net_ns_barrier,
>>>> for_each_net and taking of net_mutex in rtnl_link_unregister all testify
>>>> to the fact that there are data structures accessed by multiple network
>>>> namespaces.
>>>>
>>>> My preference would be to:
>>>>
>>>> - Add the net_sem in addition to net_mutex with down_write only held in
>>>>   register and unregister, and maybe net_ns_barrier and
>>>>   rtnl_link_unregister.
>>>>
>>>> - Factor out struct pernet_ops out of struct pernet_operations.  With
>>>>   struct pernet_ops not having the exit_batch method.  With pernet_ops
>>>>   being embedded an anonymous member of the old struct pernet_operations.
>>>>
>>>> - Add [un]register_pernet_{sys,dev} functions that take a struct
>>>>   pernet_ops, that don't take net_mutex.  Have them order the
>>>>   pernet_list as:
>>>>
>>>>   pernet_sys
>>>>   pernet_subsys
>>>>   pernet_device
>>>>   pernet_dev
>>>>
>>>>   With the chunk in the middle taking the net_mutex.
>>>
>>> I think this approach will work. Thanks for the suggestion. Some more
>>> thoughts to the plan below.
>>>
>>> The only difficult thing there will be to choose the right order
>>> to move ops from pernet_subsys to pernet_sys and from pernet_device
>>> to pernet_dev one by one.
>>>
>>> This is rather easy in case of tristate drivers, as modules may be loaded
>>> at any time, and the only important order is dependences between them.
>>> So, it's possible to start from a module, who has no dependences,
>>> and move it to pernet_sys, and then continue with modules,
>>> who have no more dependences in pernet_subsys. For pernet_device
>>> it's vise versa.
>>>
>>> In case of bool drivers, the situation may be worse, because
>>> the order is not so clear there. The same priority initcalls
>>> (for example, initcalls, registered via core_initcall()) may require
>>> the certain order, driven by linking order. I know one example from
>>> device mapper code, which lives here: drivers/md/Makefile.
>>> This problem is also solvable, even if such places do not contain
>>> comments about linking order. It's just need to respect Makefile
>>> order, when choosing a new candidate to move.
>>>  
>>>>   I think I would enforce the ordering with a failure to register
>>>>   if a subsystem or a device tried to register out of order.  
>>>>
>>>> - Disable use of the single threaded workqueue if nothing needs the
>>>>   net_mutex.
>>>
>>> We may use per-cpu worqueues in the future. The idea to refuse using
>>> worqueue doesn't seem good for me, because asynchronous net destroying
>>> looks very useful.
>> 
>> per-cpu workqueues are fine, and definitely what I am expecting.  If we
>> are doing this I want to get us off the single threaded workqueue that
>> serializes all of the cleanup.  That has a huge potential for
>> simplifying things and reducing maintenance if running everything in
>> parallel is actually safe.
>> 
>> I forget how the modern per-cpu workqueues work with respect to sleeps
>> and locking (I don't remember if when piece of work sleeps for a long
>> time we spin up another thread per-cpu workqueue thread, and thus avoid
>> priority inversion problems).
>> 
>> If locks between workqueues are not a problem we could start the
>> transition off of the single-threaded serializing workqueue sooner
>> rather than later.
>> 
>>>> - Add a test mode that deliberartely spawns threads on multiple
>>>>   processors and deliberately creates multiple network namespaces
>>>>   at the same time.
>>>>
>>>> - Add a test mode that deliberately spawns threads on multiple
>>>>   processors and delibearate destrosy multiple network namespaces
>>>>   at the same time.>
>>>> - Convert the code to unlocked operation one pernet_operations to at a
>>>>   time.  Being careful with the loopback device it's order in the list
>>>>   strongly matters.
>>>>
>>>> - Finally remove the unnecessary code.
>>>>
>>>>
>>>> At the end of the day because all of the operations for one network
>>>> namespace will run in parallel with all of the operations for another
>>>> network namespace all of the sophistication that goes into batching the
>>>> cleanup of multiple network namespaces can be removed.  As different
>>>> tasks (not sharing a lock) can wait in syncrhonize_rcu at the same time
>>>> without slowing each other down.
>>>>
>>>> I think we can remove the batching but I am afraid that will lead us into
>>>> rtnl_lock contention.
>>>
>>> I've looked into this lock. It used in many places for many reasons.
>>> It's a little strange, nobody tried to break it up in several small locks..
>> 
>> It gets tricky.  At this point getting net_mutex is enough to start
>> with. Mostly the rtnl_lock covers the slow path which tends to keep it
>> from rising to the top of the priority list.
>> 
>>>> I am definitely not comfortable with changing the locking on so much
>>>> code without an explanation at all why it is safe in the commit comments
>>>> in all 204 instances.  Which combined equal most of the well maintained
>>>> and regularly used part of the networking stack.
>>>
>>> David,
>>>
>>> could you please check the plan above and say whether 1)it's OK for you,
>>> and 2)if so, will you expect all the changes are made in one big 200+ patch set
>>> or we may go sequentially(50 patches a time, for example)?
>> 
>> I think I would start with the something covering the core networking
>> pieces so the improvements can be seen.
>
> Since the main problem is mutex held during synchronize_rcu(), the performance
> improvements may become seen only after it's completely removed.

In a distro kernel build sure.  But I think with just a few conversions
some built in network devices the core networking stack and a few ipv4
pieces you should be able to have a minimal but usualbe configuration
that does not need the lock at all.

Further dropping the lock earlier means the code can be actively
exercised running in parallel in case something is missed in review.

Eric

On 17.11.2017 21:54, Eric W. Biederman wrote:
> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
> 
>> On 15.11.2017 19:29, Eric W. Biederman wrote:
>>> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>>>
>>>> On 15.11.2017 09:25, Eric W. Biederman wrote:
>>>>> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>>>>>
>>>>>> Curently mutex is used to protect pernet operations list. It makes
>>>>>> cleanup_net() to execute ->exit methods of the same operations set,
>>>>>> which was used on the time of ->init, even after net namespace is
>>>>>> unlinked from net_namespace_list.
>>>>>>
>>>>>> But the problem is it's need to synchronize_rcu() after net is removed
>>>>>> from net_namespace_list():
>>>>>>
>>>>>> Destroy net_ns:
>>>>>> cleanup_net()
>>>>>>   mutex_lock(&net_mutex)
>>>>>>   list_del_rcu(&net->list)
>>>>>>   synchronize_rcu()                                  <--- Sleep there for ages
>>>>>>   list_for_each_entry_reverse(ops, &pernet_list, list)
>>>>>>     ops_exit_list(ops, &net_exit_list)
>>>>>>   list_for_each_entry_reverse(ops, &pernet_list, list)
>>>>>>     ops_free_list(ops, &net_exit_list)
>>>>>>   mutex_unlock(&net_mutex)
>>>>>>
>>>>>> This primitive is not fast, especially on the systems with many processors
>>>>>> and/or when preemptible RCU is enabled in config. So, all the time, while
>>>>>> cleanup_net() is waiting for RCU grace period, creation of new net namespaces
>>>>>> is not possible, the tasks, who makes it, are sleeping on the same mutex:
>>>>>>
>>>>>> Create net_ns:
>>>>>> copy_net_ns()
>>>>>>   mutex_lock_killable(&net_mutex)                    <--- Sleep there for ages
>>>>>>
>>>>>> The solution is to convert net_mutex to the rw_semaphore. Then,
>>>>>> pernet_operations::init/::exit methods, modifying the net-related data,
>>>>>> will require down_read() locking only, while down_write() will be used
>>>>>> for changing pernet_list.
>>>>>>
>>>>>> This gives signify performance increase, like you may see below. There
>>>>>> is measured sequential net namespace creation in a cycle, in single
>>>>>> thread, without other tasks (single user mode):
>>>>>>
>>>>>> 1)int main(int argc, char *argv[])
>>>>>> {
>>>>>>         unsigned nr;
>>>>>>         if (argc < 2) {
>>>>>>                 fprintf(stderr, "Provide nr iterations arg\n");
>>>>>>                 return 1;
>>>>>>         }
>>>>>>         nr = atoi(argv[1]);
>>>>>>         while (nr-- > 0) {
>>>>>>                 if (unshare(CLONE_NEWNET)) {
>>>>>>                         perror("Can't unshare");
>>>>>>                         return 1;
>>>>>>                 }
>>>>>>         }
>>>>>>         return 0;
>>>>>> }
>>>>>>
>>>>>> Origin, 100000 unshare():
>>>>>> 0.03user 23.14system 1:39.85elapsed 23%CPU
>>>>>>
>>>>>> Patched, 100000 unshare():
>>>>>> 0.03user 67.49system 1:08.34elapsed 98%CPU
>>>>>>
>>>>>> 2)for i in {1..10000}; do unshare -n bash -c exit; done
>>>>>>
>>>>>> Origin:
>>>>>> real 1m24,190s
>>>>>> user 0m6,225s
>>>>>> sys 0m15,132s
>>>>>>
>>>>>> Patched:
>>>>>> real 0m18,235s   (4.6 times faster)
>>>>>> user 0m4,544s
>>>>>> sys 0m13,796s
>>>>>>
>>>>>> This patch requires commit 76f8507f7a64 "locking/rwsem: Add down_read_killable()"
>>>>>> from Linus tree (not in net-next yet).
>>>>>
>>>>> Using a rwsem to protect the list of operations makes sense.
>>>>>
>>>>> That should allow removing the sing
>>>>>
>>>>> I am not wild about taking a the rwsem down_write in
>>>>> rtnl_link_unregister, and net_ns_barrier.  I think that works but it
>>>>> goes from being a mild hack to being a pretty bad hack and something
>>>>> else that can kill the parallelism you are seeking it add.
>>>>>
>>>>> There are about 204 instances of struct pernet_operations.  That is a
>>>>> lot of code to have carefully audited to ensure it can in parallel all
>>>>> at once.  The existence of the exit_batch method, net_ns_barrier,
>>>>> for_each_net and taking of net_mutex in rtnl_link_unregister all testify
>>>>> to the fact that there are data structures accessed by multiple network
>>>>> namespaces.
>>>>>
>>>>> My preference would be to:
>>>>>
>>>>> - Add the net_sem in addition to net_mutex with down_write only held in
>>>>>   register and unregister, and maybe net_ns_barrier and
>>>>>   rtnl_link_unregister.
>>>>>
>>>>> - Factor out struct pernet_ops out of struct pernet_operations.  With
>>>>>   struct pernet_ops not having the exit_batch method.  With pernet_ops
>>>>>   being embedded an anonymous member of the old struct pernet_operations.
>>>>>
>>>>> - Add [un]register_pernet_{sys,dev} functions that take a struct
>>>>>   pernet_ops, that don't take net_mutex.  Have them order the
>>>>>   pernet_list as:
>>>>>
>>>>>   pernet_sys
>>>>>   pernet_subsys
>>>>>   pernet_device
>>>>>   pernet_dev
>>>>>
>>>>>   With the chunk in the middle taking the net_mutex.
>>>>
>>>> I think this approach will work. Thanks for the suggestion. Some more
>>>> thoughts to the plan below.
>>>>
>>>> The only difficult thing there will be to choose the right order
>>>> to move ops from pernet_subsys to pernet_sys and from pernet_device
>>>> to pernet_dev one by one.
>>>>
>>>> This is rather easy in case of tristate drivers, as modules may be loaded
>>>> at any time, and the only important order is dependences between them.
>>>> So, it's possible to start from a module, who has no dependences,
>>>> and move it to pernet_sys, and then continue with modules,
>>>> who have no more dependences in pernet_subsys. For pernet_device
>>>> it's vise versa.
>>>>
>>>> In case of bool drivers, the situation may be worse, because
>>>> the order is not so clear there. The same priority initcalls
>>>> (for example, initcalls, registered via core_initcall()) may require
>>>> the certain order, driven by linking order. I know one example from
>>>> device mapper code, which lives here: drivers/md/Makefile.
>>>> This problem is also solvable, even if such places do not contain
>>>> comments about linking order. It's just need to respect Makefile
>>>> order, when choosing a new candidate to move.
>>>>  
>>>>>   I think I would enforce the ordering with a failure to register
>>>>>   if a subsystem or a device tried to register out of order.  
>>>>>
>>>>> - Disable use of the single threaded workqueue if nothing needs the
>>>>>   net_mutex.
>>>>
>>>> We may use per-cpu worqueues in the future. The idea to refuse using
>>>> worqueue doesn't seem good for me, because asynchronous net destroying
>>>> looks very useful.
>>>
>>> per-cpu workqueues are fine, and definitely what I am expecting.  If we
>>> are doing this I want to get us off the single threaded workqueue that
>>> serializes all of the cleanup.  That has a huge potential for
>>> simplifying things and reducing maintenance if running everything in
>>> parallel is actually safe.
>>>
>>> I forget how the modern per-cpu workqueues work with respect to sleeps
>>> and locking (I don't remember if when piece of work sleeps for a long
>>> time we spin up another thread per-cpu workqueue thread, and thus avoid
>>> priority inversion problems).
>>>
>>> If locks between workqueues are not a problem we could start the
>>> transition off of the single-threaded serializing workqueue sooner
>>> rather than later.
>>>
>>>>> - Add a test mode that deliberartely spawns threads on multiple
>>>>>   processors and deliberately creates multiple network namespaces
>>>>>   at the same time.
>>>>>
>>>>> - Add a test mode that deliberately spawns threads on multiple
>>>>>   processors and delibearate destrosy multiple network namespaces
>>>>>   at the same time.>
>>>>> - Convert the code to unlocked operation one pernet_operations to at a
>>>>>   time.  Being careful with the loopback device it's order in the list
>>>>>   strongly matters.
>>>>>
>>>>> - Finally remove the unnecessary code.
>>>>>
>>>>>
>>>>> At the end of the day because all of the operations for one network
>>>>> namespace will run in parallel with all of the operations for another
>>>>> network namespace all of the sophistication that goes into batching the
>>>>> cleanup of multiple network namespaces can be removed.  As different
>>>>> tasks (not sharing a lock) can wait in syncrhonize_rcu at the same time
>>>>> without slowing each other down.
>>>>>
>>>>> I think we can remove the batching but I am afraid that will lead us into
>>>>> rtnl_lock contention.
>>>>
>>>> I've looked into this lock. It used in many places for many reasons.
>>>> It's a little strange, nobody tried to break it up in several small locks..
>>>
>>> It gets tricky.  At this point getting net_mutex is enough to start
>>> with. Mostly the rtnl_lock covers the slow path which tends to keep it
>>> from rising to the top of the priority list.
>>>
>>>>> I am definitely not comfortable with changing the locking on so much
>>>>> code without an explanation at all why it is safe in the commit comments
>>>>> in all 204 instances.  Which combined equal most of the well maintained
>>>>> and regularly used part of the networking stack.
>>>>
>>>> David,
>>>>
>>>> could you please check the plan above and say whether 1)it's OK for you,
>>>> and 2)if so, will you expect all the changes are made in one big 200+ patch set
>>>> or we may go sequentially(50 patches a time, for example)?
>>>
>>> I think I would start with the something covering the core networking
>>> pieces so the improvements can be seen.
>>
>> Since the main problem is mutex held during synchronize_rcu(), the performance
>> improvements may become seen only after it's completely removed.
> 
> In a distro kernel build sure.  But I think with just a few conversions
> some built in network devices the core networking stack and a few ipv4
> pieces you should be able to have a minimal but usualbe configuration
> that does not need the lock at all.
> 
> Further dropping the lock earlier means the code can be actively
> exercised running in parallel in case something is missed in review.

There is an issue. Look at the way setup_net() looks at the moment:

mutex_lock()
list_for_each_entry(ops, &pernet_list, list)
     ops_init(ops, net)
rtnl_lock
list_add_tail_rcu(&net->list, &net_namespace_list); <--- Here
rtnl_unlock();
mutex_unlock(&net_mutex);

There is linking net to net_namespace_list under net_mutex.
Can list_add_tail_rcu() be moved out of net_mutex like:

mutex_lock()
list_for_each_entry(ops, &pernet_list, list)
     ops_init(ops, net)
mutex_unlock(&net_mutex);
rtnl_lock
list_add_tail_rcu(&net->list, &net_namespace_list);
rtnl_unlock();

I assume, it can't.

This means, it's impossible to separate the lists like below,
where sys and dev don't require the mutex:

sys, subsys, device, dev,

because we can't release it until the net::list is linked to net_namespace_list.

So, the only way is to go sequentially:
1)Create sys ahead of subsys, device
2)Move all subsys operations to sys
3)Remove subsys. Now we have sys and device, where sys doesn't require mutex, while device requires.
4)Create dev ahead of device: sys, dev, device
5)Move all device operations to dev
6)Remove device. Now we have sys and dev
7)Move above list_add_tail_rcu out of new_mutex.

This keeps net_mutex till every subsys and device is removed,
and it's impossible to find a .config, the mutex won't be held.
So, there still be synchronize_rcu() with mutex held.

Or maybe, there is no problem with moving list_add_tail_rcu()
out of lock right now? What do you think about this?

On 17.11.2017 21:52, Eric W. Biederman wrote:
> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
> 
>> On 15.11.2017 19:31, Eric W. Biederman wrote:
>>> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>>>
>>>> On 15.11.2017 12:51, Kirill Tkhai wrote:
>>>>> On 15.11.2017 06:19, Eric W. Biederman wrote:
>>>>>> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>>>>>>
>>>>>>> On 14.11.2017 21:39, Cong Wang wrote:
>>>>>>>> On Tue, Nov 14, 2017 at 5:53 AM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>>>>>>>>> @@ -406,7 +406,7 @@ struct net *copy_net_ns(unsigned long flags,
>>>>>>>>>
>>>>>>>>>         get_user_ns(user_ns);
>>>>>>>>>
>>>>>>>>> -       rv = mutex_lock_killable(&net_mutex);
>>>>>>>>> +       rv = down_read_killable(&net_sem);
>>>>>>>>>         if (rv < 0) {
>>>>>>>>>                 net_free(net);
>>>>>>>>>                 dec_net_namespaces(ucounts);
>>>>>>>>> @@ -421,7 +421,7 @@ struct net *copy_net_ns(unsigned long flags,
>>>>>>>>>                 list_add_tail_rcu(&net->list, &net_namespace_list);
>>>>>>>>>                 rtnl_unlock();
>>>>>>>>>         }
>>>>>>>>> -       mutex_unlock(&net_mutex);
>>>>>>>>> +       up_read(&net_sem);
>>>>>>>>>         if (rv < 0) {
>>>>>>>>>                 dec_net_namespaces(ucounts);
>>>>>>>>>                 put_user_ns(user_ns);
>>>>>>>>> @@ -446,7 +446,7 @@ static void cleanup_net(struct work_struct *work)
>>>>>>>>>         list_replace_init(&cleanup_list, &net_kill_list);
>>>>>>>>>         spin_unlock_irq(&cleanup_list_lock);
>>>>>>>>>
>>>>>>>>> -       mutex_lock(&net_mutex);
>>>>>>>>> +       down_read(&net_sem);
>>>>>>>>>
>>>>>>>>>         /* Don't let anyone else find us. */
>>>>>>>>>         rtnl_lock();
>>>>>>>>> @@ -486,7 +486,7 @@ static void cleanup_net(struct work_struct *work)
>>>>>>>>>         list_for_each_entry_reverse(ops, &pernet_list, list)
>>>>>>>>>                 ops_free_list(ops, &net_exit_list);
>>>>>>>>>
>>>>>>>>> -       mutex_unlock(&net_mutex);
>>>>>>>>> +       up_read(&net_sem);
>>>>>>>>
>>>>>>>> After your patch setup_net() could run concurrently with cleanup_net(),
>>>>>>>> given that ops_exit_list() is called on error path of setup_net() too,
>>>>>>>> it means ops->exit() now could run concurrently if it doesn't have its
>>>>>>>> own lock. Not sure if this breaks any existing user.
>>>>>>>
>>>>>>> Yes, there will be possible concurrent ops->init() for a net namespace,
>>>>>>> and ops->exit() for another one. I hadn't found pernet operations, which
>>>>>>> have a problem with that. If they exist, they are hidden and not clear seen.
>>>>>>> The pernet operations in general do not touch someone else's memory.
>>>>>>> If suddenly there is one, KASAN should show it after a while.
>>>>>>
>>>>>> Certainly the use of hash tables shared between multiple network
>>>>>> namespaces would count.  I don't rembmer how many of these we have but
>>>>>> there used to be quite a few.
>>>>>
>>>>> Could you please provide an example of hash tables, you mean?
>>>>
>>>> Ah, I see, it's dccp_hashinfo etc.
>>
>> JFI, I've checked dccp_hashinfo, and it seems to be safe.
>>
>>>
>>> The big one used to be the route cache.  With resizable hash tables
>>> things may be getting better in that regard.
>>
>> I've checked some fib-related things, and wasn't able to find that.
>> Excuse me, could you please clarify, if it's an assumption, or
>> there is exactly a problem hash table, you know? Could you please
>> point it me more exactly, if it's so.
> 
> Two things.
> 1) Hash tables are one case I know where we access data from multiple
>    network namespaces.  As such it can not be asserted that is no
>    possibility for problems.
> 
> 2) The responsible way to handle this is one patch for each set of
>    methods explaining why those methods are safe to run in parallel.
> 
>    That ensures there is opportunity for review and people are going
>    slowly enough that they actually look at these issues.
> 
> The reason I want to see this broken up is that at 200ish sets of
> methods it is too much to review all at once.

Ok, it's possible to split the changes in 400 patches, but there is
a problem with three-state (no compile, module, built-in) drivers.
Git bisect won't work anyway. Please see the description of the problem
in cover message "[PATCH RFC 00/25] Replacing net_mutex with rw_semaphore"
I sent today.
 
> I completely agree that odds are that this can be made safe and that it
> is mostly likely already safe in practically every instance.    My guess
> would be that if there are problems that need to be addressed they
> happen in one or two places and we need to find them.  If possible I
> don't want to find them after the code has shipped in a stable release.

Kirill