Hi,
Only a part review but...
1. The commit subject line and body seem a bit terse. The body could give
a summary of what this is about at least.
2. There's a cleanup/consolidation of nexthop data-structure here, that
probably should go first, as its own commit.
3. Couple of things inline:
On Fri, 13 Nov 2015, Donald Sharp wrote:
+#if BGP_SCAN_NEXTHOP
???
Just delete all that dead code?
+#ifdef HAVE_IPV6
+ case ZEBRA_NEXTHOP_IPV6:
+ stream_get (&nexthop->gate.ipv6, s, 16);
+ break;
+ case ZEBRA_NEXTHOP_IPV6_IFINDEX:
+ case ZEBRA_NEXTHOP_IPV6_IFNAME:
+ stream_get (&nexthop->gate.ipv6, s, 16);
+ nexthop->ifindex = stream_getl (s);
+ break;
+#endif
I can't remember, but were we getting rid of HAVE_IPV6 before?
+ default:
+ /* do nothing */
+ break;
+ }
+
+ if (nhlist_tail)
+ {
+ nhlist_tail->next = nexthop;
+ nhlist_tail = nexthop;
+ }
+ else
+ {
+ nhlist_tail = nexthop;
+ nhlist_head = nexthop;
+ }
Seems like nhlist_tail assignment doesn't need to be conditional.
+static struct bgp_info *
+info_make (int type, int sub_type, struct peer *peer, struct attr *attr,
+ struct bgp_node *rn)
bgp_info_make.
- if (bgp_nexthop_lookup (afi, peer, ri, NULL, NULL))
+ if (bgp_find_or_add_nexthop (afi, ri, NULL, NULL))
bgp_info_set_flag (rn, ri, BGP_INFO_VALID);
else
bgp_info_unset_flag (rn, ri, BGP_INFO_VALID);
Reading that makes me wonder about "bgp_find_or_add_nexthop" - is that
really a good name?
Elsewhere Quagga uses the more succinct "_get" for "find_or_add", and
seems easier on the brain, and more consistent?
<snip lots of info_make changes>
I'd suggest just spinning those out to a separate cleanup commit too. The
NHT patch is pretty big, and every bit that can go out to separate
cleanups will make it easier to review (which may well include many cases
in the future trying to understand changes).
diff --git a/bgpd/bgp_route.h b/bgpd/bgp_route.h
index fea18dd..f8cc34f 100644
--- a/bgpd/bgp_route.h
+++ b/bgpd/bgp_route.h
@@ -21,8 +21,11 @@ Software Foundation, Inc., 59 Temple Place - Suite 330,
Boston, MA
#ifndef _QUAGGA_BGP_ROUTE_H
#define _QUAGGA_BGP_ROUTE_H
+#include "queue.h"
#include "bgp_table.h"
+struct bgp_nexthop_cache;
+
/* Ancillary information to struct bgp_info,
* used for uncommonly used data (aggregation, MPLS, etc.)
* and lazily allocated to save memory.
@@ -47,7 +50,16 @@ struct bgp_info
/* For linked list. */
struct bgp_info *next;
struct bgp_info *prev;
-
+
+ /* For nexthop linked list */
+ LIST_ENTRY(bgp_info) nh_thread;
+
+ /* Back pointer to the prefix node */
+ struct bgp_node *net;
+
+ /* Back pointer to the nexthop structure */
+ struct bgp_nexthop_cache *nexthop;
+
How necessary are these pointers?
There could be 0.5M or more (struct bgp_info)'s allocated, and this
increases its size by about 40% on 64bit, or 28% to 31% on 32bit
(depending on sizeof(time_t)).
diff --git a/lib/nexthop.h b/lib/nexthop.h
new file mode 100644
index 0000000..bddac65
--- /dev/null
+++ b/lib/nexthop.h
@@ -0,0 +1,89 @@
+/*
+ * Nexthop structure definition.
+ * Copyright (C) 2013 Cumulus Networks, Inc.
I think you need to keep the copyright string from zebra/rib.h, from which
the data-struct came.
+ *
+ * This file is part of GNU Zebra.
s/GNU Zebra/Quagga/ on licence notifications for new files.
+#define nexthop_new() \
+({ \
+ struct nexthop *n = XCALLOC (MTYPE_NEXTHOP, sizeof (struct nexthop)); \
+ n; \
+})
I'm generally extremely sceptical of code inlined in headers, unless it
comes with comprehensive performance evaluations. (And I'd be especially
unconvinced that any call to malloc would benefit from an inlined wrapper)
Why not move nexthop_{new,add,delete,free} out of zebra/zebra_rib.c and
into lib/nexthop.c?
diff --git a/zebra/rib.h b/zebra/rib.h
index 75575ed..115db4b 100644
--- a/zebra/rib.h
+++ b/zebra/rib.h
@@ -27,18 +27,10 @@
+extern void nexthop_free (struct nexthop *nexthop);
+extern void nexthops_free (struct nexthop *nexthop);
+extern void nexthop_add (struct rib *rib, struct nexthop *nexthop);
+
E.g. there's other changes where you're adding include on nexthop.h, but
not getting these.
+ /* We need to verify that the nexthops for r1 match the nexthops for r2.
+ * Since it is possible for a rib entry to have the same nexthop multiple
+ * times (Example: [a,a]) we need to keep track of which r2 nexthops we have
+ * already used as a match against a r1 nexthop. We track this
+ * via NEXTHOP_FLAG_MATCHED. Clear this flag for all r2 nexthops when you
+ * are finished.
+ *
+ * TRUE: r1 [a,b], r2 [a,b]
+ * TRUE: r1 [a,b], r2 [b,a]
+ * FALSE: r1 [a,b], r2 [a,c]
+ * FALSE: r1 [a,a], r2 [a,b]
+ */
+ for (nh1 = r1->nexthop; nh1; nh1 = nh1->next)
+ {
+ found_nh = 0;
+ for (nh2 = r2->nexthop; nh2; nh2 = nh2->next)
+ {
+ if (CHECK_FLAG (nh2->flags, NEXTHOP_FLAG_MATCHED))
+ continue;
+
+ if (nexthop_same_no_recurse(nh1, nh2))
+ {
+ SET_FLAG (nh2->flags, NEXTHOP_FLAG_MATCHED);
+ found_nh = 1;
+ break;
+ }
+ }
+
+ if (!found_nh)
+ {
+ for (nh2 = r2->nexthop; nh2; nh2 = nh2->next)
+ if (CHECK_FLAG (nh2->flags, NEXTHOP_FLAG_MATCHED))
+ UNSET_FLAG (nh2->flags, NEXTHOP_FLAG_MATCHED);
+ return 1;
+ }
+ }
+
+ for (nh2 = r2->nexthop; nh2; nh2 = nh2->next)
+ if (CHECK_FLAG (nh2->flags, NEXTHOP_FLAG_MATCHED))
+ UNSET_FLAG (nh2->flags, NEXTHOP_FLAG_MATCHED);
+
+ return 0;
+}
Hmm, might it be better instead to make nexthop_add do an insertion sort
(O(n) even with a linked-list), so that the above becomes an O(n) prefix
comparison rather than O(n^2) (and NEXTHOP_FLAG_MATCHED disappears).
+ * This file is part of GNU Zebra.
S/Quagga/GNU Zebra/
regards,
--
Paul Jakma [email protected] @pjakma Key ID: 64A2FF6A
Fortune:
The opposite of a correct statement is a false statement. But the opposite
of a profound truth may well be another profound truth.
-- Niels Bohr
_______________________________________________
Quagga-dev mailing list
[email protected]
https://lists.quagga.net/mailman/listinfo/quagga-dev
