Merge "Handle more than 1 level of resolution in PathResolver" into R3.2

src/bgp/routing-instance/path_resolver.cc

@@ -205,6 +205,31 @@ PathResolverPartition *PathResolver::GetPartition(int part_id) {
     return partitions_[part_id];
 }
 
+//
+// Find ResolverRouteState for the given BgpRoute.
+//
+ResolverRouteState *PathResolver::FindResolverRouteState(BgpRoute *route) {
+    ResolverRouteState *state = static_cast<ResolverRouteState *>(
+        route->GetState(table_, listener_id_));
+    return state;
+}
+
+//
+// Find or create ResolverRouteState for the given BgpRoute.
+//
+// Note that the refcount for ResolverRouteState gets incremented when the
+// ResolverPath takes an intrusive_ptr to it.
+//
+ResolverRouteState *PathResolver::LocateResolverRouteState(BgpRoute *route) {
+    ResolverRouteState *state = static_cast<ResolverRouteState *>(
+        route->GetState(table_, listener_id_));
+    if (state) {
+        return state;
+    } else {
+        return (new ResolverRouteState(this, route));
+    }
+}
+
 //
 // Find or create the ResolverNexthop with the given IpAddress.
 // Called when a new ResolverPath is being created.
@@ -686,6 +711,18 @@ void PathResolverPartition::TriggerPathResolution(ResolverPath *rpath) {
     rpath_update_trigger_->Set();
 }
 
+//
+// Add a ResolverPath to the update list and start Task to process the list.
+// This is used to defer re-evaluation of the ResolverPath when the update
+// list is being processed.
+//
+void PathResolverPartition::DeferPathResolution(ResolverPath *rpath) {
+    CHECK_CONCURRENCY("bgp::ResolverPath");
+
+    rpath_update_list_.insert(rpath);
+    rpath_update_trigger_->Set();
+}
+
 //
 // Get the BgpTable partition corresponding to this PathResolverPartition.
 //
@@ -736,14 +773,16 @@ ResolverPath *PathResolverPartition::RemoveResolverPath(const BgpPath *path) {
 bool PathResolverPartition::ProcessResolverPathUpdateList() {
     CHECK_CONCURRENCY("bgp::ResolverPath");
 
-    for (ResolverPathList::iterator it = rpath_update_list_.begin();
-         it != rpath_update_list_.end(); ++it) {
+    ResolverPathList update_list;
+    rpath_update_list_.swap(update_list);
+    for (ResolverPathList::iterator it = update_list.begin();
+         it != update_list.end(); ++it) {
         ResolverPath *rpath = *it;
         if (rpath->UpdateResolvedPaths())
             delete rpath;
     }
-    rpath_update_list_.clear();
-    return true;
+
+    return rpath_update_list_.empty();
 }
 
 //
@@ -788,17 +827,16 @@ size_t PathResolverPartition::GetResolverPathUpdateListSize() const {
 
 //
 // Constructor for ResolverRouteState.
-// Gets called via static method LocateState when the first ResolverPath for
-// a BgpRoute is created.
+// Gets called when the first ResolverPath for a BgpRoute is created.
 //
 // Set State on the BgpRoute to ensure that it doesn't go away.
 //
-ResolverRouteState::ResolverRouteState(PathResolverPartition *partition,
+ResolverRouteState::ResolverRouteState(PathResolver *resolver,
     BgpRoute *route)
-    : partition_(partition),
+    : resolver_(resolver),
       route_(route),
       refcount_(0) {
-    route_->SetState(partition_->table(), partition_->listener_id(), this);
+    route_->SetState(resolver_->table(), resolver_->listener_id(), this);
 }
 
 //
@@ -809,24 +847,7 @@ ResolverRouteState::ResolverRouteState(PathResolverPartition *partition,
 // Remove State on the BgpRoute so that deletion can proceed.
 //
 ResolverRouteState::~ResolverRouteState() {
-    route_->ClearState(partition_->table(), partition_->listener_id());
-}
-
-//
-// Find or create ResolverRouteState for the given BgpRoute.
-//
-// Note that the refcount for ResolverRouteState gets incremented when the
-// ResolverPath takes an intrusive_ptr to it.
-//
-ResolverRouteState *ResolverRouteState::LocateState(
-    PathResolverPartition *partition, BgpRoute *route) {
-    ResolverRouteState *state = static_cast<ResolverRouteState *>(
-        route->GetState(partition->table(), partition->listener_id()));
-    if (state) {
-        return state;
-    } else {
-        return (new ResolverRouteState(partition, route));
-    }
+    route_->ClearState(resolver_->table(), resolver_->listener_id());
 }
 
 //
@@ -842,7 +863,7 @@ ResolverPath::ResolverPath(PathResolverPartition *partition,
       path_(path),
       route_(route),
       rnexthop_(rnexthop),
-      state_(ResolverRouteState::LocateState(partition, route)) {
+      state_(partition_->resolver()->LocateResolverRouteState(route)) {
     rnexthop->AddResolverPath(partition->part_id(), this);
 }
 
@@ -972,6 +993,24 @@ static ExtCommunityPtr UpdateExtendedCommunity(ExtCommunityDB *extcomm_db,
 bool ResolverPath::UpdateResolvedPaths() {
     CHECK_CONCURRENCY("bgp::ResolverPath");
 
+    // Defer updates if we can't get a write lock on the ResolverRouteState
+    // for the BgpRoute we're trying to modify.
+    tbb::spin_rw_mutex::scoped_lock rt_write_lock;
+    if (!rt_write_lock.try_acquire(state_->rw_mutex(), true)) {
+        partition_->DeferPathResolution(this);
+        return false;
+    }
+
+    // Defer updates if we can't get a read lock on the ResolverRouteState
+    // for the BgpRoute to the ResolverNexthop. This ResolverRouteState will
+    // exist only if the BgpRoute in question itself has resolved paths.
+    tbb::spin_rw_mutex::scoped_lock nh_read_lock;
+    ResolverRouteState *nh_state = rnexthop_->GetResolverRouteState();
+    if (nh_state && !nh_read_lock.try_acquire(nh_state->rw_mutex(), false)) {
+        partition_->DeferPathResolution(this);
+        return false;
+    }
+
     BgpServer *server = partition_->table()->server();
     BgpAttrDB *attr_db = server->attr_db();
     ExtCommunityDB *extcomm_db = server->extcomm_db();
@@ -1151,6 +1190,15 @@ void ResolverNexthop::RemoveResolverPath(int part_id, ResolverPath *rpath) {
         resolver_->RegisterUnregisterResolverNexthop(this);
 }
 
+ResolverRouteState *ResolverNexthop::GetResolverRouteState() {
+    if (!route_)
+        return NULL;
+    PathResolver *nh_resolver = table_->path_resolver();
+    if (!nh_resolver)
+        return NULL;
+    return nh_resolver->FindResolverRouteState(route_);
+}
+
 //
 // Trigger update of resolved BgpPaths for all ResolverPaths that depend on
 // the ResolverNexthop. Actual update of the resolved BgpPaths happens when

src/bgp/routing-instance/path_resolver.h

@@ -7,6 +7,7 @@
 
 #include <boost/scoped_ptr.hpp>
 #include <tbb/mutex.h>
+#include <tbb/spin_rw_mutex.h>
 
 #include <map>
 #include <set>
@@ -33,6 +34,7 @@ class IPeer;
 class PathResolverPartition;
 class ResolverNexthop;
 class ResolverPath;
+class ResolverRouteState;
 class ShowPathResolver;
 class TaskTrigger;
 
@@ -98,6 +100,9 @@ class PathResolver {
         BgpTable *nh_table = NULL);
     void StopPathResolution(int part_id, const BgpPath *path);
 
+    ResolverRouteState *FindResolverRouteState(BgpRoute *route);
+    ResolverRouteState *LocateResolverRouteState(BgpRoute *route);
+
     BgpTable *table() { return table_; }
     Address::Family family() const;
     DBTableBase::ListenerId listener_id() const { return listener_id_; }
@@ -182,7 +187,9 @@ class PathResolver {
 // Mutual exclusion of db::DBTable and bgp::ResolverPath Tasks ensures that
 // it's safe to add/delete/update resolved BgpPaths from the bgp::ResolverPath
 // Task. It also ensures that it's safe to access the BgpPaths of the nexthop
-// BgpRoute from the bgp::ResolverPath Task.
+// BgpRoute from the bgp::ResolverPath Task. The only exception is where the
+// BgpRoute itself is being modified by another bgp::ResolverPath Task. This
+// is handled by using a read-write mutex in the ResolverRouteState.
 //
 class PathResolverPartition {
 public:
@@ -196,6 +203,7 @@ class PathResolverPartition {
     void StopPathResolution(const BgpPath *path);
 
     void TriggerPathResolution(ResolverPath *rpath);
+    void DeferPathResolution(ResolverPath *rpath);
 
     int part_id() const { return part_id_; }
     DBTableBase::ListenerId listener_id() const {
@@ -240,20 +248,26 @@ class PathResolverPartition {
 // The refcount doesn't need to be atomic because it's updated/accessed from
 // exactly one DBPartition or PathResolverPartition.
 //
+// The rw_mutex is used to prevent a PathResolverPartition from modifying the
+// associated BgpRoute while another PathResolverPartition is accessing the
+// BgpRoute. This can happen when there's more than 1 levels of resolution in
+// use i.e. a BgpRoute with resolved paths is itself being used to resolve a
+// nexthop. Note that the two PathResolverPartitions could be in the same or
+// different PathResolvers.
+//
 class ResolverRouteState : public DBState {
 public:
-    ResolverRouteState(PathResolverPartition *partition, BgpRoute *route);
+    ResolverRouteState(PathResolver *resolver, BgpRoute *route);
     virtual ~ResolverRouteState();
-
-    static ResolverRouteState *LocateState(PathResolverPartition *partition,
-        BgpRoute *route);
+    tbb::spin_rw_mutex &rw_mutex() { return rw_mutex_; }
 
 private:
     friend void intrusive_ptr_add_ref(ResolverRouteState *state);
     friend void intrusive_ptr_release(ResolverRouteState *state);
 
-    PathResolverPartition *partition_;
+    PathResolver *resolver_;
     BgpRoute *route_;
+    tbb::spin_rw_mutex rw_mutex_;
     uint32_t refcount_;
 };
 
@@ -380,6 +394,7 @@ class ResolverNexthop : public ConditionMatch {
         bool deleted);
     void AddResolverPath(int part_id, ResolverPath *rpath);
     void RemoveResolverPath(int part_id, ResolverPath *rpath);
+    ResolverRouteState *GetResolverRouteState();
 
     void TriggerAllResolverPaths() const;
 
@@ -388,6 +403,7 @@ class ResolverNexthop : public ConditionMatch {
     IpAddress address() const { return address_; }
     BgpTable *table() const { return table_; }
     const BgpRoute *route() const { return route_; }
+    BgpRoute *route() { return route_; }
     bool empty() const;
     bool registered() const { return registered_; }
     void set_registered() { registered_ = true; }