[PATCH v2] iproute2: build nsid-name cache only for commands that need it

[PATCH v2] iproute2: build nsid-name cache only for commands that need it

[Anton Aksola]

The calling of netns_map_init() before command parsing introduced
a performance issue with large number of namespaces.

As commands such as add, del and exec do not need to iterate through
/var/run/netns it would be good not no build the cache before executing
these commands.

Example:
unpatched:
time seq 1 1000 | xargs -n 1 ip netns add

real    0m16.832s
user    0m1.350s
sys    0m15.029s

patched:
time seq 1 1000 | xargs -n 1 ip netns add

real    0m3.859s
user    0m0.132s
sys    0m3.205s

Signed-off-by: Anton Aksola <aakso@iki.fi>
---
 ip/ipnetns.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index af87065..564d034 100644
--- a/ip/ipnetns.c
+++ b/ip/ipnetns.c
@@ -775,17 +775,21 @@ static int netns_monitor(int argc, char **argv)
 
 int do_netns(int argc, char **argv)
 {
-	netns_map_init();
-
-	if (argc < 1)
+	if (argc < 1) {
+		netns_map_init();
 		return netns_list(0, NULL);
+	}
 
 	if ((matches(*argv, "list") == 0) || (matches(*argv, "show") == 0) ||
-	    (matches(*argv, "lst") == 0))
+	    (matches(*argv, "lst") == 0)) {
+		netns_map_init();
 		return netns_list(argc-1, argv+1);
+	}
 
-	if ((matches(*argv, "list-id") == 0))
+	if ((matches(*argv, "list-id") == 0)) {
+		netns_map_init();
 		return netns_list_id(argc-1, argv+1);
+	}
 
 	if (matches(*argv, "help") == 0)
 		return usage();
-- 
1.8.3.1

Re: [PATCH v2] iproute2: build nsid-name cache only for commands that need it

[Nicolas Dichtel]

Le 16/09/2016 à 09:22, Anton Aksola a écrit :
> The calling of netns_map_init() before command parsing introduced
> a performance issue with large number of namespaces.
> 
> As commands such as add, del and exec do not need to iterate through
> /var/run/netns it would be good not no build the cache before executing
> these commands.
> 
> Example:
> unpatched:
> time seq 1 1000 | xargs -n 1 ip netns add
> 
> real    0m16.832s
> user    0m1.350s
> sys    0m15.029s
> 
> patched:
> time seq 1 1000 | xargs -n 1 ip netns add
> 
> real    0m3.859s
> user    0m0.132s
> sys    0m3.205s
> 
> Signed-off-by: Anton Aksola <aakso@iki.fi>
> ---
There is still some differences:
$ cat test.batch
netns add foo
netns set foo 1234
netns list-id

Before your patch:
$ ip -b test.batch
nsid 1234 (iproute2 netns name: foo)

After your patch:
$ ip -b test.batch
nsid 1234

Re: [PATCH v2] iproute2: build nsid-name cache only for commands that need it

[Vadim Kochan]

On Fri, Sep 16, 2016 at 12:13 PM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> Le 16/09/2016 à 09:22, Anton Aksola a écrit :
>> The calling of netns_map_init() before command parsing introduced
>> a performance issue with large number of namespaces.
>>
>> As commands such as add, del and exec do not need to iterate through
>> /var/run/netns it would be good not no build the cache before executing
>> these commands.
>>
>> Example:
>> unpatched:
>> time seq 1 1000 | xargs -n 1 ip netns add
>>
>> real    0m16.832s
>> user    0m1.350s
>> sys    0m15.029s
>>
>> patched:
>> time seq 1 1000 | xargs -n 1 ip netns add
>>
>> real    0m3.859s
>> user    0m0.132s
>> sys    0m3.205s
>>
>> Signed-off-by: Anton Aksola <aakso@iki.fi>
>> ---
> There is still some differences:
> $ cat test.batch
> netns add foo
> netns set foo 1234
> netns list-id
>
> Before your patch:
> $ ip -b test.batch
> nsid 1234 (iproute2 netns name: foo)
>
> After your patch:
> $ ip -b test.batch
> nsid 1234

Would it be useful to add test for this case into testsuite/ ?

Regards,
Vadim Kochan

Re: [PATCH v2] iproute2: build nsid-name cache only for commands that need it

[Nicolas Dichtel]

Le 16/09/2016 à 11:23, Vadim Kochan a écrit :
[snip]
> Would it be useful to add test for this case into testsuite/ ?
Yes, it's a good idea.

Regards,
Nicolas

Re: [PATCH v2] iproute2: build nsid-name cache only for commands that need it

[Vadim Kochan]

On Fri, Sep 16, 2016 at 2:21 PM, Anton Aksola <aakso@iki.fi> wrote:
> Ok, I will post a new patch version. Should tests be posted in a separate
> patch?
>
> 2016-09-16 12:44 GMT+03:00 Nicolas Dichtel <nicolas.dichtel@6wind.com>:
>>
>> Le 16/09/2016 à 11:23, Vadim Kochan a écrit :
>> [snip]
>> > Would it be useful to add test for this case into testsuite/ ?
>> Yes, it's a good idea.
>>
>> Regards,
>> Nicolas
>
>

Anton, I just looked into tests after when I did post here. I am not
sure it will be trivial,
currently tests are running within separated network namespace by
default (which I did) via
'unshare' tool, and now I see that it is better to call it explicitly
from the each test case. So
I am not sure netns related  tests might be valid if they will be ran
after 'unshare -n', if yes - then there is no problem, otherwise
it needs to be fixed somehow - I will try to do this.

Regards,
Vadim Kochan

Re: [PATCH v2] iproute2: build nsid-name cache only for commands that need it

[Anton Aksola]

On Fri, Sep 16, 2016 at 02:25:40PM +0300, Vadim Kochan wrote:
> Anton, I just looked into tests after when I did post here. I am not
> sure it will be trivial,
> currently tests are running within separated network namespace by
> default (which I did) via
> 'unshare' tool, and now I see that it is better to call it explicitly
> from the each test case. So
> I am not sure netns related  tests might be valid if they will be ran
> after 'unshare -n', if yes - then there is no problem, otherwise
> it needs to be fixed somehow - I will try to do this.
>
(excuse my duplicate emails if any received)

It seems that mounts made after 'unshare -n' will propagate back:

[root@toys iproute2]# ip netns
[root@toys iproute2]# unshare -n
[root@toys iproute2]# ip netns add foo
[root@toys iproute2]# ip netns exec foo ip link add type dummy
[root@toys iproute2]# exit
logout
[root@toys iproute2]# ip netns
foo
[root@toys iproute2]# ip netns exec foo ip link show dev dummy0
6: dummy0: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT
    link/ether ba:cc:6c:ca:2d:41 brd ff:ff:ff:ff:ff:ff

This doesn't seem to work with 'ip netns exec' as it makes sure mounts
do not propagate.

BR, Anton

Re: [PATCH v2] iproute2: build nsid-name cache only for commands that need it

[Anton Aksola]

On Fri, Sep 16, 2016 at 11:13:11AM +0200, Nicolas Dichtel wrote:
> There is still some differences:
> $ cat test.batch
> netns add foo
> netns set foo 1234
> netns list-id
>
> Before your patch:
> $ ip -b test.batch
> nsid 1234 (iproute2 netns name: foo)
>
> After your patch:
> $ ip -b test.batch
> nsid 1234

Nicolas,
This seems to be caused by netns_add calling unshare(CLONE_NEWNET).
If we initialize the socket for nsid after that it doesn't seem to work.

Unfortunately I'm not an expert in these details. Should we separate the
socket and cache initialization to different functions and call the
socket init in the beginning of do_netns() as before? What do you think?
I made a quick patch and it seems to work in batch mode now.

BR,
Anton

Re: [PATCH v2] iproute2: build nsid-name cache only for commands that need it

[Nicolas Dichtel]

Le 16/09/2016 à 15:18, Anton Aksola a écrit :
[snip]
> Nicolas,
> This seems to be caused by netns_add calling unshare(CLONE_NEWNET).
> If we initialize the socket for nsid after that it doesn't seem to work.
> 
> Unfortunately I'm not an expert in these details. Should we separate the
> socket and cache initialization to different functions and call the
> socket init in the beginning of do_netns() as before? What do you think?
Seems good.

> I made a quick patch and it seems to work in batch mode now.
What is the result of that sequence?

$ ip netns add bar
$ ip netns set bar 5678
$ ip -b test.batch
nsid 1234 (iproute2 netns name: foo)
nsid 5678 (iproute2 netns name: bar)
$