iproute2 proposal

iproute2 proposal

[Denys Fedoryshchenko]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

While using iproute2 in batch mode i notice that adding 11362 rules, on system 
with 500+ interfaces takes up to 1minute on Xeon 3.0 Ghz.
After "oprofiling" i got result:

CPU: P4 / Xeon with 2 hyper-threads, speed 2992.84 MHz (estimated)
Counted GLOBAL_POWER_EVENTS events (time during which processor is not 
stopped) with a unit mask of 0x01 (mandatory) count 100000
samples  %        image name               symbol name
86166    57.3473  tc                       ll_remember_index
48103    32.0147  tc                       parse_rtattr
8497      5.6551  tc                       rtnl_dump_filter
1751      1.1654  [vdso] (tgid:22801 range:0xb7f66000-0xb7f67000) (no symbols)
1085      0.7221  tc                       tc_calc_xmittime
672       0.4472  tc                       .plt

I did small patch, and voila! batch finished in 2 seconds successfully!

Proof of concept patch is attached. Not sure if it is not breaking any other 
things. Waiting for any comments.

[-- Attachment #2: iproute2-proposal.patch --]
[-- Type: text/x-diff, Size: 594 bytes --]

diff -Naur iproute2-original/lib/ll_map.c iproute2-patched/lib/ll_map.c
--- iproute2-original/lib/ll_map.c	2008-08-05 19:15:56.000000000 +0000
+++ iproute2-patched/lib/ll_map.c	2008-08-05 22:22:36.000000000 +0000
@@ -23,6 +23,7 @@
 #include "ll_map.h"
 
 extern unsigned int if_nametoindex (const char *);
+static int ifacecache=0;
 
 struct idxmap
 {
@@ -160,6 +161,10 @@
 
 int ll_init_map(struct rtnl_handle *rth)
 {
+	if (ifacecache == 1)
+	    return 0;
+	ifacecache = 1;
+
 	if (rtnl_wilddump_request(rth, AF_UNSPEC, RTM_GETLINK) < 0) {
 		perror("Cannot send dump request");
 		exit(1);

Re: iproute2 proposal

[Patrick McHardy]

Denys Fedoryshchenko wrote:
> While using iproute2 in batch mode i notice that adding 11362 rules, on system 
> with 500+ interfaces takes up to 1minute on Xeon 3.0 Ghz.
> After "oprofiling" i got result:
>
> CPU: P4 / Xeon with 2 hyper-threads, speed 2992.84 MHz (estimated)
> Counted GLOBAL_POWER_EVENTS events (time during which processor is not 
> stopped) with a unit mask of 0x01 (mandatory) count 100000
> samples  %        image name               symbol name
> 86166    57.3473  tc                       ll_remember_index
> 48103    32.0147  tc                       parse_rtattr
> 8497      5.6551  tc                       rtnl_dump_filter
> 1751      1.1654  [vdso] (tgid:22801 range:0xb7f66000-0xb7f67000) (no symbols)
> 1085      0.7221  tc                       tc_calc_xmittime
> 672       0.4472  tc                       .plt
>
> I did small patch, and voila! batch finished in 2 seconds successfully!
>
> Proof of concept patch is attached. Not sure if it is not breaking any other 
> things. Waiting for any comments.

It breaks batches that add links and then configure them. You need
to invalidate the link map on "ip link add/del" and "ip tunnel add/del".

Re: iproute2 proposal

[Denys Fedoryshchenko]

On Wednesday 06 August 2008, Patrick McHardy wrote:
> It breaks batches that add links and then configure them. You need
> to invalidate the link map on "ip link add/del" and "ip tunnel add/del".
Probably for tc batch it doesn't matter, so this optimization can be done only 
for tc (i exclude also monitor functions, they must renew list of 
interfaces).

Is new patch looks better? (i test it, it compiles and doesn't break tc, and 
makes still things much faster on systems with many interfaces).

Re: iproute2 proposal

[Patrick McHardy]

Denys Fedoryshchenko wrote:
> On Wednesday 06 August 2008, Patrick McHardy wrote:
>> It breaks batches that add links and then configure them. You need
>> to invalidate the link map on "ip link add/del" and "ip tunnel add/del".
 >
> Probably for tc batch it doesn't matter, so this optimization can be done only 
> for tc (i exclude also monitor functions, they must renew list of 
> interfaces).

I'm not sure I understand you correctly. It matters *only* for
tc batch since otherwise the ll map needs to be generated anyways.


> Is new patch looks better? (i test it, it compiles and doesn't break tc, and 
> makes still things much faster on systems with many interfaces).

No patch attached ..

Re: iproute2 proposal

[Patrick McHardy]

Patrick McHardy wrote:
> Denys Fedoryshchenko wrote:
>> On Wednesday 06 August 2008, Patrick McHardy wrote:
>>> It breaks batches that add links and then configure them. You need
>>> to invalidate the link map on "ip link add/del" and "ip tunnel add/del".
>  >
>> Probably for tc batch it doesn't matter, so this optimization can be 
>> done only for tc (i exclude also monitor functions, they must renew 
>> list of interfaces).
> 
> I'm not sure I understand you correctly. It matters *only* for
> tc batch since otherwise the ll map needs to be generated anyways.

Ok I noticed the misunderstanding. Yes, it doesn't matter for
tc, only for ip.

Re: iproute2 proposal

[Denys Fedoryshchenko]

[-- Attachment #1: Type: text/plain, Size: 108 bytes --]

On Wednesday 06 August 2008, Patrick McHardy wrote:

> No patch attached ..

Sorry :-) Now must be attached

[-- Attachment #2: iproute2-proposal2.patch --]
[-- Type: text/x-diff, Size: 3961 bytes --]

diff -Naur iproute2-original/tc/f_route.c iproute2-patched/tc/f_route.c
--- iproute2-original/tc/f_route.c	2008-08-05 19:15:56.000000000 +0000
+++ iproute2-patched/tc/f_route.c	2008-08-06 09:50:48.000000000 +0000
@@ -25,6 +25,8 @@
 #include "tc_common.h"
 #include "tc_util.h"
 
+extern int link_cache;
+
 static void explain(void)
 {
 	fprintf(stderr, "Usage: ... route [ from REALM | fromif TAG ] [ to REALM ]\n");
@@ -83,7 +85,10 @@
 		} else if (matches(*argv, "fromif") == 0) {
 			__u32 id;
 			NEXT_ARG();
-			ll_init_map(&rth);
+			if (!link_cache) {
+ 			    ll_init_map(&rth);
+			    link_cache = 1;
+			}
 			if ((id=ll_name_to_index(*argv)) <= 0) {
 				fprintf(stderr, "Illegal \"fromif\"\n");
 				return -1;
diff -Naur iproute2-original/tc/m_mirred.c iproute2-patched/tc/m_mirred.c
--- iproute2-original/tc/m_mirred.c	2008-08-05 19:15:56.000000000 +0000
+++ iproute2-patched/tc/m_mirred.c	2008-08-06 09:49:39.000000000 +0000
@@ -27,6 +27,7 @@
 #include <linux/tc_act/tc_mirred.h>
 
 int mirred_d = 1;
+extern int link_cache;
 
 static void
 explain(void)
@@ -146,7 +147,10 @@
 
 	if (d[0])  {
 		int idx;
-		ll_init_map(&rth);
+		if (!link_cache) {
+ 		    ll_init_map(&rth);
+		    link_cache = 1;
+		}
 
 		if ((idx = ll_name_to_index(d)) == 0) {
 			fprintf(stderr, "Cannot find device \"%s\"\n", d);
diff -Naur iproute2-original/tc/tc.c iproute2-patched/tc/tc.c
--- iproute2-original/tc/tc.c	2008-08-05 19:15:56.000000000 +0000
+++ iproute2-patched/tc/tc.c	2008-08-06 09:47:18.000000000 +0000
@@ -38,6 +38,7 @@
 int resolve_hosts = 0;
 int use_iec = 0;
 int force = 0;
+int link_cache = 0;
 struct rtnl_handle rth;
 
 static void *BODY = NULL;	/* cached handle dlopen(NULL) */
diff -Naur iproute2-original/tc/tc_class.c iproute2-patched/tc/tc_class.c
--- iproute2-original/tc/tc_class.c	2008-08-05 19:15:56.000000000 +0000
+++ iproute2-patched/tc/tc_class.c	2008-08-06 09:51:18.000000000 +0000
@@ -25,6 +25,8 @@
 #include "tc_util.h"
 #include "tc_common.h"
 
+extern int link_cache;
+
 static void usage(void);
 
 static void usage(void)
@@ -130,7 +132,10 @@
 	}
 
 	if (d[0])  {
-		ll_init_map(&rth);
+		if (!link_cache) {
+ 		    ll_init_map(&rth);
+		    link_cache = 1;
+		}
 
 		if ((req.t.tcm_ifindex = ll_name_to_index(d)) == 0) {
 			fprintf(stderr, "Cannot find device \"%s\"\n", d);
@@ -273,7 +278,10 @@
 		argc--; argv++;
 	}
 
- 	ll_init_map(&rth);
+	if (!link_cache) {
+	    ll_init_map(&rth);
+	    link_cache = 1;
+	}
 
 	if (d[0]) {
 		if ((t.tcm_ifindex = ll_name_to_index(d)) == 0) {
diff -Naur iproute2-original/tc/tc_filter.c iproute2-patched/tc/tc_filter.c
--- iproute2-original/tc/tc_filter.c	2008-08-05 19:15:56.000000000 +0000
+++ iproute2-patched/tc/tc_filter.c	2008-08-06 09:48:54.000000000 +0000
@@ -26,6 +26,7 @@
 #include "tc_util.h"
 #include "tc_common.h"
 
+extern int link_cache;
 static void usage(void);
 
 static void usage(void)
@@ -158,7 +159,10 @@
 
 
 	if (d[0])  {
- 		ll_init_map(&rth);
+		if (!link_cache) {
+ 		    ll_init_map(&rth);
+		    link_cache = 1;
+		}
 
 		if ((req.t.tcm_ifindex = ll_name_to_index(d)) == 0) {
 			fprintf(stderr, "Cannot find device \"%s\"\n", d);
diff -Naur iproute2-original/tc/tc_qdisc.c iproute2-patched/tc/tc_qdisc.c
--- iproute2-original/tc/tc_qdisc.c	2008-08-05 19:15:56.000000000 +0000
+++ iproute2-patched/tc/tc_qdisc.c	2008-08-06 09:53:11.000000000 +0000
@@ -25,6 +25,8 @@
 #include "tc_util.h"
 #include "tc_common.h"
 
+extern int link_cache;
+
 static int usage(void);
 
 static int usage(void)
@@ -145,7 +147,10 @@
 	if (d[0])  {
 		int idx;
 
- 		ll_init_map(&rth);
+		if (!link_cache) {
+ 		    ll_init_map(&rth);
+		    link_cache = 1;
+		}
 
 		if ((idx = ll_name_to_index(d)) == 0) {
 			fprintf(stderr, "Cannot find device \"%s\"\n", d);
@@ -272,7 +277,10 @@
 		argc--; argv++;
 	}
 
- 	ll_init_map(&rth);
+	if (!link_cache) {
+ 	    ll_init_map(&rth);
+	    link_cache = 1;
+	}
 
 	if (d[0]) {
 		if ((t.tcm_ifindex = ll_name_to_index(d)) == 0) {

Re: iproute2 proposal

[Patrick McHardy]

Denys Fedoryshchenko wrote:
> On Wednesday 06 August 2008, Patrick McHardy wrote:
> 
>> No patch attached ..
> 
> Sorry :-) Now must be attached

> +++ iproute2-patched/tc/m_mirred.c	2008-08-06 09:49:39.000000000 +0000
> @@ -27,6 +27,7 @@
>  #include <linux/tc_act/tc_mirred.h>
>  
>  int mirred_d = 1;
> +extern int link_cache;
>  
>  static void
>  explain(void)
> @@ -146,7 +147,10 @@
>  
>  	if (d[0])  {
>  		int idx;
> -		ll_init_map(&rth);
> +		if (!link_cache) {
> + 		    ll_init_map(&rth);
> +		    link_cache = 1;
> +		}

It would be nicer to avoid the initialization in ll_init_map() as
you did in your first patch and add an explicit ll_invalidate_map()
and call it when necessary (ip link add/del/change(?), ip tunnel
add/del/change(?)).

Re: iproute2 proposal

[Denys Fedoryshchenko]

On Wednesday 06 August 2008, Patrick McHardy wrote:
> It would be nicer to avoid the initialization in ll_init_map() as
> you did in your first patch and add an explicit ll_invalidate_map()
> and call it when necessary (ip link add/del/change(?), ip tunnel
> add/del/change(?)).

It is more than that. I must also invalidate in monitor mode, since this is 
very volatile variable. In monitor mode very possible that interfaces can be 
changed. Also i guess it is not required and better to invalidate 
in "printing" functions?

If it is ok, i will try to do patch in this way now.