* [RFC] iproute2, ifindex option
@ 2011-07-18 8:38 Denys Fedoryshchenko
2011-07-20 15:37 ` Stephen Hemminger
0 siblings, 1 reply; 5+ messages in thread
From: Denys Fedoryshchenko @ 2011-07-18 8:38 UTC (permalink / raw)
To: Stephen Hemminger, netdev
[-- Attachment #1: Type: text/plain, Size: 1176 bytes --]
After battling with iproute2 interface name caching, i decided to try
to introduce ifindex option, where i can specify manually device index,
and avoid expensive device index lookups, especially during massive
changes for qdisc/class.
In batch mode ll_map cache will cause problems on servers with ppp
interfaces (same name after while can have another index), and also if i
run command too often, each start it will retrieve list of all
interfaces, on 3k+ interfaces it will be CPU hog.
This is sample of patch, just for qdisc/class/filter modify and show
operation.
Here is some changes in logic, because before qdisc code during _list
operation was not checking duplicate "dev" argument, as it done in
_modify code and class/filter list code.
Also maybe i need to change duparg to something else? Because:
centaur iproute2-newifindex # tc/tc -s -d filter show ifindex 23 dev
sdf
Error: duplicate "ifindex": "sdf" is the second value.
Or it is ok like this?
I'm sorry that patch is not inline, seems my webmail can't do it now, i
will try to install normal mail client.
---
System administrator
Denys Fedoryshchenko
Virtual ISP S.A.L.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ifindex-option.diff --]
[-- Type: text/x-diff; name=ifindex-option.diff, Size: 4455 bytes --]
diff -Naur iproute2/tc/tc_class.c iproute2-newifindex/tc/tc_class.c
--- iproute2/tc/tc_class.c 2011-07-18 11:10:41.390973397 +0300
+++ iproute2-newifindex/tc/tc_class.c 2011-07-18 11:29:50.177611576 +0300
@@ -67,7 +67,16 @@
NEXT_ARG();
if (d[0])
duparg("dev", *argv);
+ if (req.t.tcm_ifindex)
+ duparg("ifindex", *argv);
strncpy(d, *argv, sizeof(d)-1);
+ } else if (strcmp(*argv, "ifindex") == 0) {
+ NEXT_ARG();
+ if (d[0])
+ duparg("dev", *argv);
+ if (req.t.tcm_ifindex)
+ duparg("ifindex", *argv);
+ req.t.tcm_ifindex = atoi(*argv);
} else if (strcmp(*argv, "classid") == 0) {
__u32 handle;
NEXT_ARG();
@@ -246,7 +255,16 @@
NEXT_ARG();
if (d[0])
duparg("dev", *argv);
+ if (t.tcm_ifindex)
+ duparg("ifindex", *argv);
strncpy(d, *argv, sizeof(d)-1);
+ } else if (strcmp(*argv, "ifindex") == 0) {
+ NEXT_ARG();
+ if (d[0])
+ duparg("dev", *argv);
+ if (t.tcm_ifindex)
+ duparg("ifindex", *argv);
+ t.tcm_ifindex = atoi(*argv);
} else if (strcmp(*argv, "qdisc") == 0) {
NEXT_ARG();
if (filter_qdisc)
@@ -290,9 +308,10 @@
fprintf(stderr, "Cannot find device \"%s\"\n", d);
return 1;
}
- filter_ifindex = t.tcm_ifindex;
}
+ filter_ifindex = t.tcm_ifindex;
+
if (rtnl_dump_request(&rth, RTM_GETTCLASS, &t, sizeof(t)) < 0) {
perror("Cannot send dump request");
return 1;
diff -Naur iproute2/tc/tc_filter.c iproute2-newifindex/tc/tc_filter.c
--- iproute2/tc/tc_filter.c 2011-07-18 11:10:41.390973397 +0300
+++ iproute2-newifindex/tc/tc_filter.c 2011-07-18 11:28:18.097762745 +0300
@@ -80,7 +80,16 @@
NEXT_ARG();
if (d[0])
duparg("dev", *argv);
+ if (req.t.tcm_ifindex)
+ duparg("ifindex", *argv);
strncpy(d, *argv, sizeof(d)-1);
+ } else if (strcmp(*argv, "ifindex") == 0) {
+ NEXT_ARG();
+ if (d[0])
+ duparg("dev", *argv);
+ if (req.t.tcm_ifindex)
+ duparg("ifindex", *argv);
+ req.t.tcm_ifindex = atoi(*argv);
} else if (strcmp(*argv, "root") == 0) {
if (req.t.tcm_parent) {
fprintf(stderr, "Error: \"root\" is duplicate parent ID\n");
@@ -277,7 +286,16 @@
NEXT_ARG();
if (d[0])
duparg("dev", *argv);
+ if (t.tcm_ifindex)
+ duparg("ifindex", *argv);
strncpy(d, *argv, sizeof(d)-1);
+ } else if (strcmp(*argv, "ifindex") == 0) {
+ NEXT_ARG();
+ if (d[0])
+ duparg("dev", *argv);
+ if (t.tcm_ifindex)
+ duparg("ifindex", *argv);
+ t.tcm_ifindex = atoi(*argv);
} else if (strcmp(*argv, "root") == 0) {
if (t.tcm_parent) {
fprintf(stderr, "Error: \"root\" is duplicate parent ID\n");
@@ -332,10 +350,11 @@
if ((t.tcm_ifindex = ll_name_to_index(d)) == 0) {
fprintf(stderr, "Cannot find device \"%s\"\n", d);
return 1;
- }
- filter_ifindex = t.tcm_ifindex;
+ }
}
+ filter_ifindex = t.tcm_ifindex;
+
if (rtnl_dump_request(&rth, RTM_GETTFILTER, &t, sizeof(t)) < 0) {
perror("Cannot send dump request");
return 1;
diff -Naur iproute2/tc/tc_qdisc.c iproute2-newifindex/tc/tc_qdisc.c
--- iproute2/tc/tc_qdisc.c 2011-07-18 11:10:41.390973397 +0300
+++ iproute2-newifindex/tc/tc_qdisc.c 2011-07-18 11:29:09.322122349 +0300
@@ -76,7 +76,16 @@
NEXT_ARG();
if (d[0])
duparg("dev", *argv);
+ if (req.t.tcm_ifindex)
+ duparg("ifindex", *argv);
strncpy(d, *argv, sizeof(d)-1);
+ } else if (strcmp(*argv, "ifindex") == 0) {
+ NEXT_ARG();
+ if (d[0])
+ duparg("dev", *argv);
+ if (req.t.tcm_ifindex)
+ duparg("ifindex", *argv);
+ req.t.tcm_ifindex = atoi(*argv);
} else if (strcmp(*argv, "handle") == 0) {
__u32 handle;
if (req.t.tcm_handle)
@@ -289,7 +298,18 @@
while (argc > 0) {
if (strcmp(*argv, "dev") == 0) {
NEXT_ARG();
+ if (d[0])
+ duparg("dev", *argv);
+ if (t.tcm_ifindex)
+ duparg("ifindex", *argv);
strncpy(d, *argv, sizeof(d)-1);
+ } else if (strcmp(*argv, "ifindex") == 0) {
+ NEXT_ARG();
+ if (d[0])
+ duparg("dev", *argv);
+ if (t.tcm_ifindex)
+ duparg("ifindex", *argv);
+ t.tcm_ifindex = atoi(*argv);
#ifdef TC_H_INGRESS
} else if (strcmp(*argv, "ingress") == 0) {
if (t.tcm_parent) {
@@ -315,9 +335,10 @@
fprintf(stderr, "Cannot find device \"%s\"\n", d);
return 1;
}
- filter_ifindex = t.tcm_ifindex;
}
+ filter_ifindex = t.tcm_ifindex;
+
if (rtnl_dump_request(&rth, RTM_GETQDISC, &t, sizeof(t)) < 0) {
perror("Cannot send dump request");
return 1;
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC] iproute2, ifindex option
2011-07-18 8:38 [RFC] iproute2, ifindex option Denys Fedoryshchenko
@ 2011-07-20 15:37 ` Stephen Hemminger
2011-07-20 15:54 ` Denys Fedoryshchenko
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Hemminger @ 2011-07-20 15:37 UTC (permalink / raw)
To: Denys Fedoryshchenko; +Cc: netdev
On Mon, 18 Jul 2011 11:38:33 +0300
Denys Fedoryshchenko <denys@visp.net.lb> wrote:
> After battling with iproute2 interface name caching, i decided to try
> to introduce ifindex option, where i can specify manually device index,
> and avoid expensive device index lookups, especially during massive
> changes for qdisc/class.
> In batch mode ll_map cache will cause problems on servers with ppp
> interfaces (same name after while can have another index), and also if i
> run command too often, each start it will retrieve list of all
> interfaces, on 3k+ interfaces it will be CPU hog.
>
> This is sample of patch, just for qdisc/class/filter modify and show
> operation.
>
> Here is some changes in logic, because before qdisc code during _list
> operation was not checking duplicate "dev" argument, as it done in
> _modify code and class/filter list code.
>
> Also maybe i need to change duparg to something else? Because:
> centaur iproute2-newifindex # tc/tc -s -d filter show ifindex 23 dev
> sdf
> Error: duplicate "ifindex": "sdf" is the second value.
> Or it is ok like this?
>
> I'm sorry that patch is not inline, seems my webmail can't do it now, i
> will try to install normal mail client.
If caching is a problem, I would rather just get rid of the cache altogether.
There are a number of other places where it is a problem as well.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] iproute2, ifindex option
2011-07-20 15:37 ` Stephen Hemminger
@ 2011-07-20 15:54 ` Denys Fedoryshchenko
2011-07-20 15:58 ` Stephen Hemminger
0 siblings, 1 reply; 5+ messages in thread
From: Denys Fedoryshchenko @ 2011-07-20 15:54 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
On Wed, 20 Jul 2011 08:37:53 -0700, Stephen Hemminger wrote:
>
> If caching is a problem, I would rather just get rid of the cache
> altogether.
> There are a number of other places where it is a problem as well.
It is good, but it must be controlled. Also people put some efforts to
improve it...
Before i did custom patch to "trash" the cache, but after cache
improved i should rewrite it. If required, i can make cache behavior
changeable (on,off,flush ?).
Will it help for you too?
I think ifindex is good anyway, because sometimes it is better to refer
to specific ppp interface by ifindex.
---
System administrator
Denys Fedoryshchenko
Virtual ISP S.A.L.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] iproute2, ifindex option
2011-07-20 15:54 ` Denys Fedoryshchenko
@ 2011-07-20 15:58 ` Stephen Hemminger
2011-07-20 16:12 ` Denys Fedoryshchenko
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Hemminger @ 2011-07-20 15:58 UTC (permalink / raw)
To: Denys Fedoryshchenko; +Cc: netdev
On Wed, 20 Jul 2011 18:54:44 +0300
Denys Fedoryshchenko <denys@visp.net.lb> wrote:
> On Wed, 20 Jul 2011 08:37:53 -0700, Stephen Hemminger wrote:
> >
> > If caching is a problem, I would rather just get rid of the cache
> > altogether.
> > There are a number of other places where it is a problem as well.
> It is good, but it must be controlled. Also people put some efforts to
> improve it...
> Before i did custom patch to "trash" the cache, but after cache
> improved i should rewrite it. If required, i can make cache behavior
> changeable (on,off,flush ?).
> Will it help for you too?
>
> I think ifindex is good anyway, because sometimes it is better to refer
> to specific ppp interface by ifindex.
The problem is that it is not possible to make a "safe" cache since
it is possible for network device names to be changed at any time.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] iproute2, ifindex option
2011-07-20 15:58 ` Stephen Hemminger
@ 2011-07-20 16:12 ` Denys Fedoryshchenko
0 siblings, 0 replies; 5+ messages in thread
From: Denys Fedoryshchenko @ 2011-07-20 16:12 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
On Wed, 20 Jul 2011 08:58:42 -0700, Stephen Hemminger wrote:
> On Wed, 20 Jul 2011 18:54:44 +0300
> Denys Fedoryshchenko <denys@visp.net.lb> wrote:
>
>> On Wed, 20 Jul 2011 08:37:53 -0700, Stephen Hemminger wrote:
>> >
>> > If caching is a problem, I would rather just get rid of the cache
>> > altogether.
>> > There are a number of other places where it is a problem as well.
>> It is good, but it must be controlled. Also people put some efforts
>> to
>> improve it...
>> Before i did custom patch to "trash" the cache, but after cache
>> improved i should rewrite it. If required, i can make cache
>> behavior
>> changeable (on,off,flush ?).
>> Will it help for you too?
>>
>> I think ifindex is good anyway, because sometimes it is better to
>> refer
>> to specific ppp interface by ifindex.
>
> The problem is that it is not possible to make a "safe" cache since
> it is possible for network device names to be changed at any time.
Maybe then by removing cache, but using ifindex option, and maybe even
adding in output additional ifindex field (for show commands), we can
leave user to implement his own caching?
Another thing, on tc -s -d qdisc show, if there is many interfaces,
without cache on each line ll_index_to_name() it will be CPU and netlink
hog to retrieve all interface names again and again, especially if there
is too many intefaces.
---
System administrator
Denys Fedoryshchenko
Virtual ISP S.A.L.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-07-20 16:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-18 8:38 [RFC] iproute2, ifindex option Denys Fedoryshchenko
2011-07-20 15:37 ` Stephen Hemminger
2011-07-20 15:54 ` Denys Fedoryshchenko
2011-07-20 15:58 ` Stephen Hemminger
2011-07-20 16:12 ` Denys Fedoryshchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox