[RESEND] [NET] fib_rules: Flush route cache after rule modifications

[RESEND] [NET] fib_rules: Flush route cache after rule modifications

[Thomas Graf]

The results of FIB rules lookups are cached in the routing cache
except for IPv6 as no such cache exists. So far, it was the
responsibility of the user to flush the cache after modifying any
rules. This lead to many false bug reports due to misunderstanding
of this concept.

This patch automatically flushes the route cache after inserting
or deleting a rule.

Thanks to Muli Ben-Yehuda <muli@il.ibm.com> for catching a bug
in the previous patch.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Index: net-2.6.22/include/net/fib_rules.h
===================================================================
--- net-2.6.22.orig/include/net/fib_rules.h	2007-03-27 13:54:52.000000000 +0200
+++ net-2.6.22/include/net/fib_rules.h	2007-03-27 14:16:24.000000000 +0200
@@ -59,6 +59,10 @@ struct fib_rules_ops
 	u32			(*default_pref)(void);
 	size_t			(*nlmsg_payload)(struct fib_rule *);
 
+	/* Called after modifications to the rules set, must flush
+	 * the route cache if one exists. */
+	void			(*flush_cache)(void);
+
 	int			nlgroup;
 	struct nla_policy	*policy;
 	struct list_head	*rules_list;
Index: net-2.6.22/net/core/fib_rules.c
===================================================================
--- net-2.6.22.orig/net/core/fib_rules.c	2007-03-27 13:53:29.000000000 +0200
+++ net-2.6.22/net/core/fib_rules.c	2007-03-27 15:36:05.000000000 +0200
@@ -44,6 +44,12 @@ static void rules_ops_put(struct fib_rul
 		module_put(ops->owner);
 }
 
+static void flush_route_cache(struct fib_rules_ops *ops)
+{
+	if (ops->flush_cache)
+		ops->flush_cache();
+}
+
 int fib_rules_register(struct fib_rules_ops *ops)
 {
 	int err = -EEXIST;
@@ -314,6 +320,7 @@ static int fib_nl_newrule(struct sk_buff
 		list_add_rcu(&rule->list, ops->rules_list);
 
 	notify_rule_change(RTM_NEWRULE, rule, ops, nlh, NETLINK_CB(skb).pid);
+	flush_route_cache(ops);
 	rules_ops_put(ops);
 	return 0;
 
@@ -404,6 +411,7 @@ static int fib_nl_delrule(struct sk_buff
 		notify_rule_change(RTM_DELRULE, rule, ops, nlh,
 				   NETLINK_CB(skb).pid);
 		fib_rule_put(rule);
+		flush_route_cache(ops);
 		rules_ops_put(ops);
 		return 0;
 	}
Index: net-2.6.22/net/ipv4/fib_rules.c
===================================================================
--- net-2.6.22.orig/net/ipv4/fib_rules.c	2007-03-27 13:54:57.000000000 +0200
+++ net-2.6.22/net/ipv4/fib_rules.c	2007-03-27 15:35:43.000000000 +0200
@@ -298,6 +298,11 @@ static size_t fib4_rule_nlmsg_payload(st
 	       + nla_total_size(4); /* flow */
 }
 
+static void fib4_rule_flush_cache(void)
+{
+	rt_cache_flush(0);
+}
+
 static struct fib_rules_ops fib4_rules_ops = {
 	.family		= AF_INET,
 	.rule_size	= sizeof(struct fib4_rule),
@@ -309,6 +314,7 @@ static struct fib_rules_ops fib4_rules_o
 	.fill		= fib4_rule_fill,
 	.default_pref	= fib4_rule_default_pref,
 	.nlmsg_payload	= fib4_rule_nlmsg_payload,
+	.flush_cache	= fib4_rule_flush_cache,
 	.nlgroup	= RTNLGRP_IPV4_RULE,
 	.policy		= fib4_rule_policy,
 	.rules_list	= &fib4_rules,
Index: net-2.6.22/net/decnet/dn_rules.c
===================================================================
--- net-2.6.22.orig/net/decnet/dn_rules.c	2007-03-27 14:00:56.000000000 +0200
+++ net-2.6.22/net/decnet/dn_rules.c	2007-03-27 15:35:43.000000000 +0200
@@ -31,6 +31,7 @@
 #include <net/dn_fib.h>
 #include <net/dn_neigh.h>
 #include <net/dn_dev.h>
+#include <net/dn_route.h>
 
 static struct fib_rules_ops dn_fib_rules_ops;
 
@@ -239,6 +240,11 @@ static u32 dn_fib_rule_default_pref(void
 	return 0;
 }
 
+static void dn_fib_rule_flush_cache(void)
+{
+	dn_rt_cache_flush(0);
+}
+
 static struct fib_rules_ops dn_fib_rules_ops = {
 	.family		= AF_DECnet,
 	.rule_size	= sizeof(struct dn_fib_rule),
@@ -249,6 +255,7 @@ static struct fib_rules_ops dn_fib_rules
 	.compare	= dn_fib_rule_compare,
 	.fill		= dn_fib_rule_fill,
 	.default_pref	= dn_fib_rule_default_pref,
+	.flush_cache	= dn_fib_rule_flush_cache,
 	.nlgroup	= RTNLGRP_DECnet_RULE,
 	.policy		= dn_fib_rule_policy,
 	.rules_list	= &dn_fib_rules,

Re: [RESEND] [NET] fib_rules: Flush route cache after rule modifications

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Tue, 27 Mar 2007 15:38:45 +0200

> The results of FIB rules lookups are cached in the routing cache
> except for IPv6 as no such cache exists. So far, it was the
> responsibility of the user to flush the cache after modifying any
> rules. This lead to many false bug reports due to misunderstanding
> of this concept.
> 
> This patch automatically flushes the route cache after inserting
> or deleting a rule.
> 
> Thanks to Muli Ben-Yehuda <muli@il.ibm.com> for catching a bug
> in the previous patch.
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>

Applied, thanks.

Re: [RESEND] [NET] fib_rules: Flush route cache after rule modifications

[Jarek Poplawski]

On 27-03-2007 14:38, Thomas Graf wrote:
> The results of FIB rules lookups are cached in the routing cache
> except for IPv6 as no such cache exists. So far, it was the
> responsibility of the user to flush the cache after modifying any
> rules. This lead to many false bug reports due to misunderstanding
> of this concept.
> 
> This patch automatically flushes the route cache after inserting
> or deleting a rule.

I hope I'm wrong, but isn't this at the cost of admins
working with long rules' sets, which (probably) take extra
time now?

Regards,
Jarek P. 

Re: [RESEND] [NET] fib_rules: Flush route cache after rule modifications

[Thomas Graf]

* Jarek Poplawski <jarkao2@o2.pl> 2007-03-28 13:19
> I hope I'm wrong, but isn't this at the cost of admins
> working with long rules' sets, which (probably) take extra
> time now?

That's right, it makes the insert and delete operation more
expensive.

A compromise would be to delay the flushing and wait for
some time (default 2 seconds) whether more rules or routes
are being added before flushing.

[NET] fib_rules: delay route cache flush by ip_rt_min_delay

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Index: linux/net-2.6.22/net/decnet/dn_rules.c
===================================================================
--- linux.orig/net-2.6.22/net/decnet/dn_rules.c	2007-03-28 17:41:22.000000000 +0200
+++ linux/net-2.6.22/net/decnet/dn_rules.c	2007-03-28 17:41:39.000000000 +0200
@@ -242,7 +242,7 @@ static u32 dn_fib_rule_default_pref(void
 
 static void dn_fib_rule_flush_cache(void)
 {
-	dn_rt_cache_flush(0);
+	dn_rt_cache_flush(-1);
 }
 
 static struct fib_rules_ops dn_fib_rules_ops = {
Index: linux/net-2.6.22/net/ipv4/fib_rules.c
===================================================================
--- linux.orig/net-2.6.22/net/ipv4/fib_rules.c	2007-03-28 17:41:18.000000000 +0200
+++ linux/net-2.6.22/net/ipv4/fib_rules.c	2007-03-28 17:41:30.000000000 +0200
@@ -300,7 +300,7 @@ static size_t fib4_rule_nlmsg_payload(st
 
 static void fib4_rule_flush_cache(void)
 {
-	rt_cache_flush(0);
+	rt_cache_flush(-1);
 }
 
 static struct fib_rules_ops fib4_rules_ops = {

Re: [RESEND] [NET] fib_rules: Flush route cache after rule modifications

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Wed, 28 Mar 2007 17:49:03 +0200

> * Jarek Poplawski <jarkao2@o2.pl> 2007-03-28 13:19
> > I hope I'm wrong, but isn't this at the cost of admins
> > working with long rules' sets, which (probably) take extra
> > time now?
> 
> That's right, it makes the insert and delete operation more
> expensive.
> 
> A compromise would be to delay the flushing and wait for
> some time (default 2 seconds) whether more rules or routes
> are being added before flushing.

Another idea Thomas and I tossed around was to have some kind of way
for the rule insertion to indicate that the flush should be deferred
and I kind of prefer that explicitness.

By default it's better the flush immediately, because the old
behavior is totally unexpected.  "I insert a rule and it dosn't
show up?", nobody expects that.

Re: [RESEND] [NET] fib_rules: Flush route cache after rule modifications

[Thomas Graf]

* David Miller <davem@davemloft.net> 2007-03-28 11:24
> Another idea Thomas and I tossed around was to have some kind of way
> for the rule insertion to indicate that the flush should be deferred
> and I kind of prefer that explicitness.

Right, although I believe the flag should not only defer it
but not flush at all. This would be the optimal solution
for scripts which can do a ip ro flush cache as they know
what they're doing.

> By default it's better the flush immediately, because the old
> behavior is totally unexpected.  "I insert a rule and it dosn't
> show up?", nobody expects that.

It's a tough call, I'd favour immediate flush as well but I can
see the point in delaying by ip_rt_min_delay which can be
configured by the user. So people can choose to immediately flush
by setting it to 0. It would also be consistent to the flush
after route changes, the same delay is used there.

Re: [RESEND] [NET] fib_rules: Flush route cache after rule modifications

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Wed, 28 Mar 2007 21:34:36 +0200

> So people can choose to immediately flush by setting it to 0. It
> would also be consistent to the flush after route changes, the same
> delay is used there.

That's a good point I hadn't considered.

Therefore, I think I'll apply your patch, considering that.

Re: [RESEND] [NET] fib_rules: Flush route cache after rule modifications

[Jarek Poplawski]

On Wed, Mar 28, 2007 at 09:34:36PM +0200, Thomas Graf wrote:
> * David Miller <davem@davemloft.net> 2007-03-28 11:24
> > Another idea Thomas and I tossed around was to have some kind of way
> > for the rule insertion to indicate that the flush should be deferred
> > and I kind of prefer that explicitness.
> 
> Right, although I believe the flag should not only defer it
> but not flush at all. This would be the optimal solution
> for scripts which can do a ip ro flush cache as they know
> what they're doing.
> 
> > By default it's better the flush immediately, because the old
> > behavior is totally unexpected.  "I insert a rule and it dosn't
> > show up?", nobody expects that.
> 
> It's a tough call, I'd favour immediate flush as well but I can
> see the point in delaying by ip_rt_min_delay which can be
> configured by the user. So people can choose to immediately flush
> by setting it to 0. It would also be consistent to the flush
> after route changes, the same delay is used there.
> 

Of course you both are right - but (...I've some doubts):

- there is a difference between tools: route or ip route
(as a successor) and ip rule; the latter is intended for
advanced things, so users have to expect... (or RTFM!).
 
- of course immediate flush seems to be more natural, but
it isn't like that and rules (other rules) are changed,
so maybe some transitory way is needed; these 2s look
like a good compromise, but after looking into
rt_cache_flush - it's not for all (I know - we don't like
multipath - but untill it's here...) and these locks and
timers aren't for free, too; so, IMHO, something like
-n[oflush] option is a mustbe.

- for consistency probably all ip "objects" should be
verified: "to flush or not to flush" by default.

Cheers,
Jarek P.

Re: [RESEND] [NET] fib_rules: Flush route cache after rule modifications

[Jarek Poplawski]

On Thu, Mar 29, 2007 at 12:03:26PM +0200, Jarek Poplawski wrote:
...
> rt_cache_flush - it's not for all (I know - we don't like
> multipath - but untill it's here...)[...] 

Sorry, I forgot it's already not there...

Jarek P.