| Message ID | 87fv9tvrgq.fsf@x220.int.ebiederm.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 2/25/15, 9:19 AM, Eric W. Biederman wrote: > Unlike IPv4 this code notifies on all cases where mpls routes > are added or removed as that was the simplest to implement. > > In particular routes being removed because a network interface > goes down or is removed are notified about. Are there technical > arguments for handling this differently ? Userspace developers > don't particularly like the way IPv4 handles route removal > on ifdown. that is true. However, from previous emails on this topic on netdev, there is no reason to notify these deletes to userspace thereby creating a notification storm when userspace can figure this out. Which seems like a valid reason. (Your approach resembles IPv6 which does generate these notifications and userspace is usually happy with this). Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
roopa <roopa@cumulusnetworks.com> writes: > On 2/25/15, 9:19 AM, Eric W. Biederman wrote: >> Unlike IPv4 this code notifies on all cases where mpls routes >> are added or removed as that was the simplest to implement. >> >> In particular routes being removed because a network interface >> goes down or is removed are notified about. Are there technical >> arguments for handling this differently ? Userspace developers >> don't particularly like the way IPv4 handles route removal >> on ifdown. > that is true. However, from previous emails on this topic on netdev, > there is no reason to notify these deletes to userspace thereby creating a > notification storm > when userspace can figure this out. Which seems like a valid reason. > (Your approach resembles IPv6 which does generate these notifications and > userspace is usually happy with this). Grr. There is an even better way to do this. The semantically best way to handle this is to simply not use routes for forwarding where the network inteface is down, the carrier is down, or the network device has gone away for forwarding. Apparently there are some multi-path scenearios that already do this legitimately, and routes going away auto-matically can cause userspace other kinds of problems. In MPLS I especially don't want to free the routing table slot until I know that the change has propagated in the network and I can be reasonably confident that no-one will send me traffic on that label. Otherwise there is a chance the label will be reused too soon. Grumble. That is a code change I need to make. Grumble. I also need to look and see if those multi-path scenarios report a next hop as dead or just rely on the network interface state (which I think it is) to be sufficient information relayed to userspace Eric -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2/26/15, 6:03 AM, Eric W. Biederman wrote: > roopa <roopa@cumulusnetworks.com> writes: > >> On 2/25/15, 9:19 AM, Eric W. Biederman wrote: >>> Unlike IPv4 this code notifies on all cases where mpls routes >>> are added or removed as that was the simplest to implement. >>> >>> In particular routes being removed because a network interface >>> goes down or is removed are notified about. Are there technical >>> arguments for handling this differently ? Userspace developers >>> don't particularly like the way IPv4 handles route removal >>> on ifdown. >> that is true. However, from previous emails on this topic on netdev, >> there is no reason to notify these deletes to userspace thereby creating a >> notification storm >> when userspace can figure this out. Which seems like a valid reason. >> (Your approach resembles IPv6 which does generate these notifications and >> userspace is usually happy with this). > Grr. There is an even better way to do this. > > The semantically best way to handle this is to simply not use routes for > forwarding where the network inteface is down, the carrier is down, or > the network device has gone away for forwarding. agreed, And we have an internal patch that does this for regular routing on carrier down (which we will upstream soon). > > Apparently there are some multi-path scenearios that already do this > legitimately, and routes going away auto-matically can cause userspace > other kinds of problems. > > In MPLS I especially don't want to free the routing table slot until I > know that the change has propagated in the network and I can be > reasonably confident that no-one will send me traffic on that label. > Otherwise there is a chance the label will be reused too soon. ack > > Grumble. That is a code change I need to make. Grumble. > > I also need to look and see if those multi-path scenarios report a next > hop as dead or just rely on the network interface state (which I think > it is) to be sufficient information relayed to userspace > they are marked DEAD on ifdown today (AFAIR they dont generate a notification in IPv4) and are skipped during route lookup. Only when all the nexthops in a multi-path route are dead, is the route multipath route declared dead and is deleted today (with no notification to userspace in the IPv4 case). Thanks, Roopa -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 26, 2015 at 07:12:34AM -0800, roopa wrote: > On 2/26/15, 6:03 AM, Eric W. Biederman wrote: > >roopa <roopa@cumulusnetworks.com> writes: > > > >>On 2/25/15, 9:19 AM, Eric W. Biederman wrote: > >>>Unlike IPv4 this code notifies on all cases where mpls routes > >>>are added or removed as that was the simplest to implement. > >>> > >>>In particular routes being removed because a network interface > >>>goes down or is removed are notified about. Are there technical > >>>arguments for handling this differently ? Userspace developers > >>>don't particularly like the way IPv4 handles route removal > >>>on ifdown. > >>that is true. However, from previous emails on this topic on netdev, > >>there is no reason to notify these deletes to userspace thereby creating a > >>notification storm > >>when userspace can figure this out. Which seems like a valid reason. > >>(Your approach resembles IPv6 which does generate these notifications and > >>userspace is usually happy with this). > >Grr. There is an even better way to do this. > > > >The semantically best way to handle this is to simply not use routes for > >forwarding where the network inteface is down, the carrier is down, or > >the network device has gone away for forwarding. > > agreed, And we have an internal patch that does this for regular routing > on carrier down (which we will upstream soon). Yep, I should be able to easily forward-port it from 3.17 to net-next without much issue. Eric feel free to email me directly if you want to see what I've got now. > > > >Apparently there are some multi-path scenearios that already do this > >legitimately, and routes going away auto-matically can cause userspace > >other kinds of problems. > > > >In MPLS I especially don't want to free the routing table slot until I > >know that the change has propagated in the network and I can be > >reasonably confident that no-one will send me traffic on that label. > >Otherwise there is a chance the label will be reused too soon. > ack > > > >Grumble. That is a code change I need to make. Grumble. > > > >I also need to look and see if those multi-path scenarios report a next > >hop as dead or just rely on the network interface state (which I think > >it is) to be sufficient information relayed to userspace > > > they are marked DEAD on ifdown today (AFAIR they dont generate a > notification in IPv4) and are skipped during route lookup. > Only when all the nexthops in a multi-path route are dead, is the route > multipath route declared dead > and is deleted today (with no notification to userspace in the IPv4 case). > > Thanks, > Roopa > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h index da9889a4dec0..481d2516ccd0 100644 --- a/include/uapi/linux/rtnetlink.h +++ b/include/uapi/linux/rtnetlink.h @@ -625,6 +625,8 @@ enum rtnetlink_groups { #define RTNLGRP_IPV6_NETCONF RTNLGRP_IPV6_NETCONF RTNLGRP_MDB, #define RTNLGRP_MDB RTNLGRP_MDB + RTNLGRP_MPLS_ROUTE, +#define RTNLGRP_MPLS_ROUTE RTNLGRP_MPLS_ROUTE __RTNLGRP_MAX }; #define RTNLGRP_MAX (__RTNLGRP_MAX - 1) diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c index 5cf9aa68c32f..90e45461c8e2 100644 --- a/net/mpls/af_mpls.c +++ b/net/mpls/af_mpls.c @@ -34,6 +34,10 @@ struct mpls_route { /* next hop label forwarding entry */ static int zero = 0; static int label_limit = (1 << 20) - 1; +static void rtmsg_lfib(int event, u32 label, struct mpls_route *rt, + struct nlmsghdr *nlh, struct net *net, u32 portid, + unsigned int nlm_flags); + static struct mpls_route *mpls_route_input_rcu(struct net *net, unsigned index) { struct mpls_route *rt = NULL; @@ -237,6 +241,20 @@ static void mpls_rt_free(struct mpls_route *rt) kfree_rcu(rt, rt_rcu); } +static void mpls_notify_route(struct net *net, unsigned index, + struct mpls_route *old, struct mpls_route *new, + const struct nl_info *info) +{ + struct nlmsghdr *nlh = info ? info->nlh : NULL; + unsigned portid = info ? info->portid : 0; + int event = new ? RTM_NEWROUTE : RTM_DELROUTE; + struct mpls_route *rt = new ? new : old; + unsigned nlm_flags = (old && new) ? NLM_F_REPLACE : 0; + /* Ignore reserved labels for now */ + if (rt && (index >= 16)) + rtmsg_lfib(event, index, rt, nlh, net, portid, nlm_flags); +} + static void mpls_route_update(struct net *net, unsigned index, struct net_device *dev, struct mpls_route *new, const struct nl_info *info) @@ -251,6 +269,8 @@ static void mpls_route_update(struct net *net, unsigned index, old = rt; } + mpls_notify_route(net, index, old, new, info); + /* If we removed a route free it now */ mpls_rt_free(old); } @@ -643,6 +663,46 @@ static int mpls_dump_routes(struct sk_buff *skb, struct netlink_callback *cb) return skb->len; } +static inline size_t lfib_nlmsg_size(struct mpls_route *rt) +{ + size_t payload = + NLMSG_ALIGN(sizeof(struct rtmsg)) + + ((rt->rt_labels == 0) ? 0 : /* RTA_NEWDST */ + nla_total_size(rt->rt_labels *4)) + + nla_total_size(rt->rt_dev->addr_len) /* RTA_LLGATEWAY */ + + nla_total_size(4) /* RTA_OIF */ + + nla_total_size(4); /* RTA_DST */ + + return payload; +} + +static void rtmsg_lfib(int event, u32 label, struct mpls_route *rt, + struct nlmsghdr *nlh, struct net *net, u32 portid, + unsigned int nlm_flags) +{ + struct sk_buff *skb; + u32 seq = nlh ? nlh->nlmsg_seq : 0; + int err = -ENOBUFS; + + skb = nlmsg_new(lfib_nlmsg_size(rt), GFP_KERNEL); + if (skb == NULL) + goto errout; + + err = mpls_dump_route(skb, portid, seq, event, label, rt, nlm_flags); + if (err < 0) { + /* -EMSGSIZE implies BUG in lfib_nlmsg_size */ + WARN_ON(err == -EMSGSIZE); + kfree_skb(skb); + goto errout; + } + rtnl_notify(skb, net, portid, RTNLGRP_MPLS_ROUTE, nlh, GFP_KERNEL); + + return; +errout: + if (err < 0) + rtnl_set_sk_err(net, RTNLGRP_MPLS_ROUTE, err); +} + static int resize_platform_label_table(struct net *net, size_t limit) { size_t size = sizeof(struct mpls_route *) * limit;
Unlike IPv4 this code notifies on all cases where mpls routes are added or removed as that was the simplest to implement. In particular routes being removed because a network interface goes down or is removed are notified about. Are there technical arguments for handling this differently? Userspace developers don't particularly like the way IPv4 handles route removal on ifdown. For now reserved labels are handled automatically and userspace is not notified. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- include/uapi/linux/rtnetlink.h | 2 ++ net/mpls/af_mpls.c | 60 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+)