Re: multipath / route priority support for ospf6d
On Mon, May 15, 2017 at 08:43:02PM +0200, Florian Riehm wrote:
> > The attached patch ports the multipath support from ospfd to ospf6d.
> @@ -171,48 +260,30 @@ kr_change(struct kroute *kroute)
>* - zero out ifindex (this is no longer relevant)
>*/
> action = RTM_CHANGE;
> - kr->r.flags = kroute->flags | F_OSPFD_INSERTED;
> - kr->r.ifindex = 0;
> - rtlabel_unref(kr->r.rtlabel);
> - kr->r.ext_tag = kroute->ext_tag;
> - kr->r.rtlabel = kroute->rtlabel;
> - }
> + } else if (kr->next == NULL)/* single path OSPF route */
> + action = RTM_CHANGE;
I wonder wether this check should be
} else if (kr->next == NULL && krcount == 1)
At least this is what ospfd is doing.
> RB_FOREACH(kr, kroute_tree, )
> if (!(kr->r.flags & F_KERNEL))
> - send_rtmsg(kr_state.fd, RTM_ADD, >r);
> + for (kn = kr; kn != NULL; kn = kn->next) {
> + send_rtmsg(kr_state.fd, RTM_ADD, >r);
> + }
> - RB_FOREACH(kr, kroute_tree, )
> + RB_FOREACH(kr, kroute_tree, ) {
> if (!(kr->r.flags & F_KERNEL))
> - send_rtmsg(kr_state.fd, RTM_DELETE, >r);
> + for (kn = kr; kn != NULL; kn = kn->next) {
> + send_rtmsg(kr_state.fd, RTM_DELETE, >r);
> + }
> + }
The braces { } are used a bit inconsistently here.
Anyway I think this diff should go in as it is.
OK bluhm@
Re: multipath / route priority support for ospf6d
On 05/12/17 18:07, Florian Riehm wrote:
> Hi,
>
> our QA reports issues with the ospf6d since the kernel uses more multipath
> routes.
> It exits after certain topology changes with:
> rde_send_change_kroute: no valid nexthop found
>
> Since the kernel uses more multipath routes, the lack of multipath support in
> ospf6d became a problem.
>
> The attached patch ports the multipath support from ospfd to ospf6d.
> It bases on the following ospfd commits:
> cvs diff -D "2007-09-24" -D "2007-09-26"
> cvs diff -r1.65 -r1.67 kroute.c
>
> A similar patch was suggested by Manues Guesdon a couple of years ago.
>
> This patch doesn't fix all problems I am seeing, but it improves the situation
> a lot. A second patch porting priority support will follow and fix further
> issues. I decided to split my fixes into two parts to make review easier.
>
Sorry,
Mailclient messed up whitespaces.
Fixed diff below.
friehm
Index: kroute.c
===
RCS file: /openbsd/src/usr.sbin/ospf6d/kroute.c,v
retrieving revision 1.50
diff -u -p -r1.50 kroute.c
--- kroute.c27 Dec 2016 17:18:56 - 1.50
+++ kroute.c11 May 2017 20:48:49 -
@@ -59,6 +59,8 @@ void kr_redist_remove(struct kroute_node
intkr_redist_eval(struct kroute *, struct kroute *);
void kr_redistribute(struct kroute_node *);
intkroute_compare(struct kroute_node *, struct kroute_node *);
+intkr_change_fib(struct kroute_node *, struct kroute *, int, int);
+intkr_delete_fib(struct kroute_node *);
struct kroute_node *kroute_find(const struct in6_addr *, u_int8_t);
struct kroute_node *kroute_matchgw(struct kroute_node *,
@@ -141,18 +143,105 @@ kr_init(int fs)
}
int
-kr_change(struct kroute *kroute)
+kr_change_fib(struct kroute_node *kr, struct kroute *kroute, int krcount,
+int action)
+{
+ int i;
+ struct kroute_node *kn, *nkn;
+
+ if (action == RTM_ADD) {
+ /*
+* First remove all stale multipath routes.
+* This step must be skipped when the action is RTM_CHANGE
+* because it is already a single path route that will be
+* changed.
+*/
+ for (kn = kr; kn != NULL; kn = nkn) {
+ for (i = 0; i < krcount; i++) {
+ if (kn->r.scope == kroute[i].scope &&
+ IN6_ARE_ADDR_EQUAL(>r.nexthop,
+ [i].nexthop))
+ break;
+ }
+ nkn = kn->next;
+ if (i == krcount) {
+ /* stale route */
+ if (kr_delete_fib(kn) == -1)
+ log_warnx("kr_delete_fib failed");
+ /*
+* if head element was removed we need to adjust
+* the head
+*/
+ if (kr == kn)
+ kr = nkn;
+ }
+ }
+ }
+
+ /*
+* now add or change the route
+*/
+ for (i = 0; i < krcount; i++) {
+ /* nexthop ::1 -> ignore silently */
+ if (IN6_IS_ADDR_LOOPBACK([i].nexthop))
+ continue;
+
+ if (action == RTM_ADD && kr) {
+ for (kn = kr; kn != NULL; kn = kn->next) {
+ if (kn->r.scope == kroute[i].scope &&
+ IN6_ARE_ADDR_EQUAL(>r.nexthop,
+ [i].nexthop))
+ break;
+ }
+
+ if (kn != NULL)
+ /* nexthop already present, skip it */
+ continue;
+ } else
+ /* modify first entry */
+ kn = kr;
+
+ /* send update */
+ if (send_rtmsg(kr_state.fd, action, [i]) == -1)
+ return (-1);
+
+ /* create new entry unless we are changing the first entry */
+ if (action == RTM_ADD)
+ if ((kn = calloc(1, sizeof(*kn))) == NULL)
+ fatal(NULL);
+
+ kn->r.prefix = kroute[i].prefix;
+ kn->r.prefixlen = kroute[i].prefixlen;
+ kn->r.nexthop = kroute[i].nexthop;
+ kn->r.scope = kroute[i].scope;
+ kn->r.flags = kroute[i].flags | F_OSPFD_INSERTED;
+ kn->r.ext_tag = kroute[i].ext_tag;
+ rtlabel_unref(kn->r.rtlabel); /* for RTM_CHANGE */
+ kn->r.rtlabel = kroute[i].rtlabel;
+
+ if (action == RTM_ADD)
+
multipath / route priority support for ospf6d
Hi,
our QA reports issues with the ospf6d since the kernel uses more multipath
routes.
It exits after certain topology changes with:
rde_send_change_kroute: no valid nexthop found
Since the kernel uses more multipath routes, the lack of multipath support in
ospf6d became a problem.
The attached patch ports the multipath support from ospfd to ospf6d.
It bases on the following ospfd commits:
cvs diff -D "2007-09-24" -D "2007-09-26"
cvs diff -r1.65 -r1.67 kroute.c
A similar patch was suggested by Manues Guesdon a couple of years ago.
This patch doesn't fix all problems I am seeing, but it improves the situation
a lot. A second patch porting priority support will follow and fix further
issues. I decided to split my fixes into two parts to make review easier.
Thanks & Regards
Florian
Index: kroute.c
===
RCS file: /openbsd/src/usr.sbin/ospf6d/kroute.c,v
retrieving revision 1.50
diff -u -p -r1.50 kroute.c
--- kroute.c27 Dec 2016 17:18:56 - 1.50
+++ kroute.c11 May 2017 20:48:49 -
@@ -59,6 +59,8 @@ void kr_redist_remove(struct kroute_node
intkr_redist_eval(struct kroute *, struct kroute *);
void kr_redistribute(struct kroute_node *);
intkroute_compare(struct kroute_node *, struct kroute_node *);
+intkr_change_fib(struct kroute_node *, struct kroute *, int, int);
+intkr_delete_fib(struct kroute_node *);
struct kroute_node *kroute_find(const struct in6_addr *, u_int8_t);
struct kroute_node *kroute_matchgw(struct kroute_node *,
@@ -141,18 +143,105 @@ kr_init(int fs)
}
int
-kr_change(struct kroute *kroute)
+kr_change_fib(struct kroute_node *kr, struct kroute *kroute, int krcount,
+int action)
+{
+ int i;
+ struct kroute_node *kn, *nkn;
+
+ if (action == RTM_ADD) {
+ /*
+* First remove all stale multipath routes.
+* This step must be skipped when the action is RTM_CHANGE
+* because it is already a single path route that will be
+* changed.
+*/
+ for (kn = kr; kn != NULL; kn = nkn) {
+ for (i = 0; i < krcount; i++) {
+ if (kn->r.scope == kroute[i].scope &&
+ IN6_ARE_ADDR_EQUAL(>r.nexthop,
+ [i].nexthop))
+ break;
+ }
+ nkn = kn->next;
+ if (i == krcount) {
+ /* stale route */
+ if (kr_delete_fib(kn) == -1)
+ log_warnx("kr_delete_fib failed");
+ /*
+* if head element was removed we need to adjust
+* the head
+*/
+ if (kr == kn)
+ kr = nkn;
+ }
+ }
+ }
+
+ /*
+* now add or change the route
+*/
+ for (i = 0; i < krcount; i++) {
+ /* nexthop ::1 -> ignore silently */
+ if (IN6_IS_ADDR_LOOPBACK([i].nexthop))
+ continue;
+
+ if (action == RTM_ADD && kr) {
+ for (kn = kr; kn != NULL; kn = kn->next) {
+ if (kn->r.scope == kroute[i].scope &&
+ IN6_ARE_ADDR_EQUAL(>r.nexthop,
+ [i].nexthop))
+ break;
+ }
+
+ if (kn != NULL)
+ /* nexthop already present, skip it */
+ continue;
+ } else
+ /* modify first entry */
+ kn = kr;
+
+ /* send update */
+ if (send_rtmsg(kr_state.fd, action, [i]) == -1)
+ return (-1);
+
+ /* create new entry unless we are changing the first entry */
+ if (action == RTM_ADD)
+ if ((kn = calloc(1, sizeof(*kn))) == NULL)
+ fatal(NULL);
+
+ kn->r.prefix = kroute[i].prefix;
+ kn->r.prefixlen = kroute[i].prefixlen;
+ kn->r.nexthop = kroute[i].nexthop;
+ kn->r.scope = kroute[i].scope;
+ kn->r.flags = kroute[i].flags | F_OSPFD_INSERTED;
+ kn->r.ext_tag = kroute[i].ext_tag;
+ rtlabel_unref(kn->r.rtlabel);/* for RTM_CHANGE */
+ kn->r.rtlabel = kroute[i].rtlabel;
+
+ if (action == RTM_ADD)
+ if (kroute_insert(kn) == -1) {
+ log_debug("kr_update_fib: cannot insert %s",
+
