Inline...
On Tue, Feb 2, 2016 at 7:40 AM, Paul Jakma <[email protected]> wrote:
> On Mon, 1 Feb 2016, Donald Sharp wrote:
>
> I was looking at the bgp code and noticed that we have different places
>> that we store nexthop information:
>>
>
>
>> In 'struct attr'
>> struct in_addr nexthop; <------IPv4 Nexthop
>> address
>>
>> In 'struct attr_extra':
>>
>> #ifdef HAVE_IPV6
>> struct in6_addr mp_nexthop_global; <-----IPv6 Nexthop Global
>> address
>> struct in6_addr mp_nexthop_local; <-----IPv6 Nexthop Link Local
>> address
>> #endif /* HAVE_IPV6 */
>> #ifdef HAVE_IPV6
>> struct in6_addr mp_nexthop_global; <-----VPNV4 Nexthop Address
>> struct in6_addr mp_nexthop_local; <-----Never Used
>> #endif /* HAVE_IPV6 */
>>
>
> For the latter two you mean _in?
Yeah.. I fat fingered the cut-n-paste.
>
>
> Does anyone know the historical reasoning for this? Does anyone mind if I
>> combine these data structures, in a somewhat more logical manner?
>>
>
> I don't know why VPNv4 doesn't re-use the same in_addr as for v6. Looks
> like maybe it could?
>
I believe so too.
>
> The reason they're in attr_extra is that IPv4 paths did and likely will
> continue to for at least a while, greatly outnumber IPv6 paths. With those
> addresses in the main attr, the size of (struct attr) increases by (if my
> counts are right):
>
> 32 bit:
> - 40%, with just the 2 extra addresses (29 to 41 bytes)
> - 96%, with all 4 of those (29 to 57)
>
> 64:
> - 17%, with just the 2
> - 53%, with all 4
>
>
Here's my refactored bgp_attr.h:
diff --git a/bgpd/bgp_attr.h b/bgpd/bgp_attr.h
index 789422f..9f825ea 100644
--- a/bgpd/bgp_attr.h
+++ b/bgpd/bgp_attr.h
@@ -60,10 +60,7 @@ Software Foundation, Inc., 59 Temple Place - Suite 330,
Boston, MA
struct attr_extra
{
/* Multi-Protocol Nexthop, AFI IPv6 */
-#ifdef HAVE_IPV6
- struct in6_addr mp_nexthop_global;
struct in6_addr mp_nexthop_local;
-#endif /* HAVE_IPV6 */
/* Extended Communities attribute. */
struct ecommunity *ecommunity;
@@ -74,9 +71,6 @@ struct attr_extra
/* Unknown transitive attribute. */
struct transit *transit;
- struct in_addr mp_nexthop_global_in;
- struct in_addr mp_nexthop_local_in;
-
/* Aggregator Router ID attribute */
struct in_addr aggregator_addr;
@@ -115,7 +109,7 @@ struct attr
u_int32_t flag;
/* Apart from in6_addr, the remaining static attributes */
- struct in_addr nexthop;
+ union g_addr nexthop;
u_int32_t med;
u_int32_t local_pref;
u_int32_t nh_ifindex;
In my refactored code I'm testing right now, the V4 data structure grows
from 4 bytes to 16 per route, so 12 bytes increase.
While If you have a V6 or VPNV4 address we loose 12 bytes per route
I didn't fancy digging into VPNv4 to see what the hell those 2 extra
> addresses were about when I tried to slim down (struct attr).
>
>
mp_nexthop_local_in is never used. That can just be removed from the data
structure, imo.
> The reason I tried to slim down (struct attr) was that some people from
> another free BGP implementation were going around at conferences saying
> that Quagga was bloated memory wise (never mind Quagga implemented stuff
> they didn't, including soft-refresh...).
>
>
I'm not sure I care about what other people are saying, if they are just
complaining. Let's make the code structure right and fix performance
issues and this becomes a non-talking point from my perspective. There is
something to be said about cleanliness of code as well and I think we can
all agree that how nexthop's are stored is a bit non-intuitive.
donald
> I could imagine getting rid of (struct attr_extra), and making (struct
> attr) more dynamically sized according to the attrs present, and
> encapsulating access to its members via helpers to pull desired fields out
> based on the "attrs set" bitmap. I wouldn't get /too/ radical cleaning this
> up right now though, given there's a lot of other stuff still to get in...
>
> regards,
> --
> Paul Jakma [email protected] @pjakma Key ID: 64A2FF6A
> Fortune:
> You now have Asian Flu.
>
_______________________________________________
Quagga-dev mailing list
[email protected]
https://lists.quagga.net/mailman/listinfo/quagga-dev
