Message-ID: <20150603150223.GD10203@neilslaptop.think-freely.org>
Date:	Wed, 3 Jun 2015 11:02:23 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Andy Gospodarek <gospo@...ulusnetworks.com>
Cc:	Hannes Frederic Sowa <hannes@...hat.com>, netdev@...r.kernel.org,
	davem@...emloft.net, ddutt@...ulusnetworks.com
Subject: Re: [PATCH net-next] net: change fib behavior based on interface
 link status

On Wed, Jun 03, 2015 at 10:46:22AM -0400, Andy Gospodarek wrote:
> On Wed, Jun 03, 2015 at 10:21:31AM -0400, Neil Horman wrote:
> > On Wed, Jun 03, 2015 at 04:13:08PM +0200, Hannes Frederic Sowa wrote:
> > > On Mi, 2015-06-03 at 09:53 -0400, Neil Horman wrote:
> > > > On Tue, Jun 02, 2015 at 11:07:19PM -0400, Andy Gospodarek wrote:
> > > > > This patch adds the ability to have the Linux kernel track whether or
> > > > > not a particular route should be used based on the link-status of the
> > > > > interface associated with the next-hop.
> > > > > 
> > > > > Before this patch any link-failure on an interface that was serving as a
> > > > > gateway for some systems could result in those systems being isolated
> > > > > from the rest of the network as the stack would continue to attempt to
> > > > > send frames out of an interface that is actually linked-down.  When the
> > > > > kernel is responsible for all forwarding, it should also be responsible
> > > > > for taking action when the traffic can no longer be forwarded -- there
> > > > > is no real need to outsource link-monitoring to userspace anymore.
> > > > > 
> > > > > This feature is only enabled with the new sysctl set (default is off):
> > > > > net.core.kill_routes_on_linkdown = 1
> > > > > 
> > > > > When this is set, the following behavior can be observed (interface p8p1
> > > > > is link-down):
> > > > > 
> > > > > # ip route show 
> > > > > default via 10.0.5.2 dev p9p1 
> > > > > 10.0.5.0/24 dev p9p1  proto kernel  scope link  src 10.0.5.15 
> > > > > 70.0.0.0/24 dev p7p1  proto kernel  scope link  src 70.0.0.1 
> > > > > 80.0.0.0/24 dev p8p1  proto kernel  scope link  src 80.0.0.1 dead 
> > > > > 90.0.0.0/24 via 80.0.0.2 dev p8p1  metric 1 dead 
> > > > > 90.0.0.0/24 via 70.0.0.2 dev p7p1  metric 2 
> > > > > # ip route get 90.0.0.1 
> > > > > 90.0.0.1 via 70.0.0.2 dev p7p1  src 70.0.0.1 
> > > > >     cache 
> > > > > # ip route get 80.0.0.1 
> > > > > local 80.0.0.1 dev lo  src 80.0.0.1 
> > > > >     cache <local> 
> > > > > # ip route get 80.0.0.2
> > > > > 80.0.0.2 via 10.0.5.2 dev p9p1  src 10.0.5.15 
> > > > >     cache 
> > > > > 
> > > > > While the route does remain in the table (so it can be modified if
> > > > > needed rather than being wiped away as it would be if IFF_UP was
> > > > > cleared), the proper next-hop is chosen automatically when the link is
> > > > > down.  Now interface p8p1 is linked-up:
> > > > > 
> > > > > # ip route show 
> > > > > default via 10.0.5.2 dev p9p1 
> > > > > 10.0.5.0/24 dev p9p1  proto kernel  scope link  src 10.0.5.15 
> > > > > 70.0.0.0/24 dev p7p1  proto kernel  scope link  src 70.0.0.1 
> > > > > 80.0.0.0/24 dev p8p1  proto kernel  scope link  src 80.0.0.1 
> > > > > 90.0.0.0/24 via 80.0.0.2 dev p8p1  metric 1 
> > > > > 90.0.0.0/24 via 70.0.0.2 dev p7p1  metric 2 
> > > > > 192.168.56.0/24 dev p2p1  proto kernel  scope link  src 192.168.56.2 
> > > > > # ip route get 90.0.0.1 
> > > > > 90.0.0.1 via 80.0.0.2 dev p8p1  src 80.0.0.1 
> > > > >     cache 
> > > > > # ip route get 80.0.0.1 
> > > > > local 80.0.0.1 dev lo  src 80.0.0.1 
> > > > >     cache <local> 
> > > > > # ip route get 80.0.0.2
> > > > > 80.0.0.2 dev p8p1  src 80.0.0.1 
> > > > >     cache 
> > > > > 
> > > > > and the output changes to what one would expect.
> > > > > 
> > > > > Signed-off-by: Andy Gospodarek <gospo@...ulusnetworks.com>
> > > > > Suggested-by: Dinesh Dutt <ddutt@...ulusnetworks.com>
> > > > > 
> > > > > ---
> > > > > Though there were some that preferred not to have a configuration option
> > > > > and to make this behavior the default when it was discussed in Ottawa
> > > > > earlier this year since "it was time to do this."  I wanted to propose
> > > > > the config option to preserve the current behavior for those that desire
> > > > > it.  I'll happily remove it if Dave and Linus approve.
> > > > > 
> > > > > An IPv6 implementation is also needed (DECnet too!), but I wanted to
> > > > > start with the IPv4 implementation to get people comfortable with the
> > > > > idea before moving forward.  If this is accepted the IPv6 implementation
> > > > > can be posted shortly.  
> > > > > 
> > > > > FWIW, we have been running this patch with the sysctl setting above and
> > > > > our customers have been happily using a backported version for IPv4 and
> > > > > IPv6 for >6 months.
> > > > > 
> > > > >  include/linux/netdevice.h      |  1 +
> > > > >  include/net/fib_rules.h        |  1 +
> > > > >  include/net/ip_fib.h           |  1 +
> > > > >  include/uapi/linux/rtnetlink.h |  1 +
> > > > >  include/uapi/linux/sysctl.h    |  1 +
> > > > >  kernel/sysctl_binary.c         |  1 +
> > > > >  net/core/dev.c                 |  2 ++
> > > > >  net/core/sysctl_net_core.c     |  7 +++++++
> > > > >  net/ipv4/fib_frontend.c        | 12 +++++++++--
> > > > >  net/ipv4/fib_rules.c           |  7 ++++++-
> > > > >  net/ipv4/fib_semantics.c       | 46 ++++++++++++++++++++++++++++++++++++------
> > > > >  net/ipv4/fib_trie.c            | 19 +++++++++++++----
> > > > >  12 files changed, 86 insertions(+), 13 deletions(-)
> > > > > 
> > > > 
> > > > Hey Andy-
> > > > 	So, the implementation looks great.  That said, a question comes up in
> > > > my mind that I can't quite resolve.  In looking at your code, and seeing how it
> > > > fit into the routing path, I came accross the function sync_fib_down_dev, which
> > > > appears to be called in response to NETDEV_DOWN events.  Its purpose from my
> > > > read is to interrogate the fib for routes that specify the device going down as
> > > > the egress device, and mark said routes as dead.  I'm guessing that I'm missing
> > > > something here, but it seems as though this patch shouldn't be needed in light
> > > > of that behavior (unless the existing code is somehow broken, or I'm confused
> > > > about its purpose).  Could it be that whats really needed here is for Network
> > > > Manager (or other user space programs ) to stop removing routes in response to
> > > > NETDEV_DOWN events so that the above kernel code can do its job?
> > > 
> > > We don't care about DEAD flag during normal fib lookup, only when
> > > considering multipath routes.
> > > 
> > 
> > I get that, but I think the question still applies.  If you have multiple routes
> > to the same destination, the sync_fib_down_dev code should still be doing the
> > job that andy's patch does, shouldn't it?  I.e. if you have two interfaces with
> > ip addresses on the same subnet, and a route for each interface, only the first
> > route in the table typically gets used, because its found first.  But if that
> > link goes down, the sync_fib_down_dev code should mark the links route as dead,
> > and subsequent lookups should use the alternate interface.  But it seems like
> > thats not happening, so I'm wondering why that is.
> 
> Neil, I think I see what you are asking, let me see if I can clarify
> this.  The confusing part here is that NETDEV_DOWN events are done when
> IFF_UP is cleared, not when link changes (those are NETDEV_CHANGE
> events), but sync_fib_down_dev() is called by both with this patch.
> 
Ok, I can see that, but shouldn't we get IFF_UP cleared when we loose link
anyway? I.e. if we loose link, we loose carrier, and as a result we get a
linkwatch event that takes down the interface, no?

> When NETDEV_DOWN events happen the DEAD_LINKDOWN flag needs to be
> cleared so that proper cleanup happens later when flushing routes later.
> This is why some action is needed in sync_fib_down_dev to handle this
> new flag.
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists