BGP: Fix nexthop registration churn

When a BGP nexthop is registered for resolution, if it is learnt from an
EBGP peer and other conditions warrant (non-multihop peer and connected check
is not disabled), the registration includes a flag that indicates that the
nexthop must be resolved only if it is directly connected. In peculiar
situations - e.g., third-party nexthop or policy configuration - the same
nexthop could be learnt from an IBGP peer, and in general, nexthops learnt
from IBGP peers can be resolved over any route. This scenario was causing
a churn in the nexthop registration with the 'must-be-connected' flag being
repeatedly toggled as routes are received from both peers. The registrations
would in turn trigger significant processing.

The fix is to treat 'must-be-connected' as an overriding condition.

The repeated registration and related processing was also causing heavy memory
usage by BGP - for memory buffers used to hold registration information. This
fix will ensure that is no longer the case.

Signed-off-by: Vivek Venkatraman <vivek@cumulusnetworks.com>
Reviewed-by: Donald Sharp <sharpd@cumulusnetworks.com>
Ticket: CM-8005, CM-8013
Reviewed By: CCR-3772
Testing Done: Manual, bgpsmoke (on 2.5-br)

diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c
index 197dd87585fe31b2548c27b1bdf8517956c9dc6e..25969da3d2968fa7c75caaf5d1d5a99d931480ad 100644 (file)
--- a/bgpd/bgp_nht.c
+++ b/bgpd/bgp_nht.c
@@ -191,13 +191,23 @@ bgp_find_or_add_nexthop (struct bgp *bgp, afi_t afi, struct bgp_info *ri,
          UNSET_FLAG(bnc->flags, BGP_NEXTHOP_VALID);
        }
     }
+  /* When nexthop is already known, but now requires 'connected' resolution,
+   * re-register it. The reverse scenario where the nexthop currently requires
+   * 'connected' resolution does not need a re-register (i.e., we treat
+   * 'connected-required' as an override) except in the scenario where this
+   * is actually a case of tracking a peer for connectivity (e.g., after
+   * disable connected-check).
+   * NOTE: We don't track the number of paths separately for 'connected-
+   * required' vs 'connected-not-required' as this change is not a common
+   * scenario.
+   */
   else if (connected && ! CHECK_FLAG(bnc->flags, BGP_NEXTHOP_CONNECTED))
     {
       SET_FLAG(bnc->flags, BGP_NEXTHOP_CONNECTED);
       UNSET_FLAG(bnc->flags, BGP_NEXTHOP_REGISTERED);
       UNSET_FLAG(bnc->flags, BGP_NEXTHOP_VALID);
     }
-  else if (!connected && CHECK_FLAG(bnc->flags, BGP_NEXTHOP_CONNECTED))
+  else if (peer && !connected && CHECK_FLAG(bnc->flags, BGP_NEXTHOP_CONNECTED))
     {
       UNSET_FLAG(bnc->flags, BGP_NEXTHOP_CONNECTED);
       UNSET_FLAG(bnc->flags, BGP_NEXTHOP_REGISTERED);