Merge "Fix concurrency issue with access of refcount_ on routing-policy"

Juniper · Feb 17, 2016 · a806aef · a806aef
2 parents c6a9968 + bcfa11c
commit a806aef
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 5 deletions.
diff --git a/src/bgp/routing-policy/routing_policy.cc b/src/bgp/routing-policy/routing_policy.cc
@@ -341,7 +341,8 @@ RoutingPolicy::RoutingPolicy(std::string name, BgpServer *server,
                                  const BgpRoutingPolicyConfig *config)
     : name_(name), server_(server), mgr_(mgr), config_(config),
       deleter_(new DeleteActor(server, this)),
-      manager_delete_ref_(this, mgr->deleter()), refcount_(0), generation_(0) {
+      manager_delete_ref_(this, mgr->deleter()), generation_(0) {
+    refcount_ = 0;
 }
 
 RoutingPolicy::~RoutingPolicy() {

diff --git a/src/bgp/routing-policy/routing_policy.h b/src/bgp/routing-policy/routing_policy.h
@@ -8,6 +8,7 @@
 #include <boost/scoped_ptr.hpp>
 #include <boost/intrusive_ptr.hpp>
 #include <boost/shared_ptr.hpp>
+#include <tbb/atomic.h>
 
 #include <map>
 #include <string>
@@ -275,18 +276,19 @@ class RoutingPolicy {
     LifetimeRef<RoutingPolicy> manager_delete_ref_;
 
     // Updated when routing policy undergoes a change
-    uint32_t refcount_;
+    tbb::atomic<uint32_t> refcount_;
     uint32_t generation_;
     RoutingPolicyTermList terms_;
 };
 
 inline void intrusive_ptr_add_ref(RoutingPolicy *policy) {
-    policy->refcount_++;
+    policy->refcount_.fetch_and_increment();
+    return;
 }
 
 inline void intrusive_ptr_release(RoutingPolicy *policy) {
-    assert(policy->refcount_ != 0);
-    if (--policy->refcount_ == 0) {
+    int prev = policy->refcount_.fetch_and_decrement();
+    if (prev == 1) {
         if (policy->MayDelete())
             policy->RetryDelete();
     }

diff --git a/src/bgp/test/routing_policy_test.cc b/src/bgp/test/routing_policy_test.cc
@@ -2586,6 +2586,53 @@ TEST_F(RoutingPolicyTest, ShowPolicy_2) {
     TASK_UTIL_EXPECT_TRUE(validate_done);
 }
 
+//
+// Add routes with different hashes such that route belongs to different DBTable
+// partition. Validate the routing policy and update of routing policy on
+// routing instance
+//
+TEST_F(RoutingPolicyTest, MultipleRoutes_DifferentPartition) {
+    string content =
+        FileRead("controller/src/bgp/testdata/routing_policy_0.xml");
+    EXPECT_TRUE(parser_.Parse(content));
+    task_util::WaitForIdle();
+
+    boost::system::error_code ec;
+    peers_.push_back(
+        new BgpPeerMock(Ip4Address::from_string("192.168.0.1", ec)));
+
+    for (int i = 0; i < 255; i++) {
+        ostringstream oss;
+        oss << "10.0.1." << i << "/32";
+        AddRoute<InetDefinition>(peers_[0], "test.inet.0", oss.str(), 100);
+        oss.str("");
+    }
+    task_util::WaitForIdle();
+    VERIFY_EQ(255, RouteCount("test.inet.0"));
+
+    content = FileRead("controller/src/bgp/testdata/routing_policy_0d.xml");
+    EXPECT_TRUE(parser_.Parse(content));
+    task_util::WaitForIdle();
+
+    // Policy matches for 10.0.1.1/32 "exact". The local pref should be updated
+    BgpRoute *rt = RouteLookup<InetDefinition>("test.inet.0", "10.0.1.1/32");
+    ASSERT_TRUE(rt != NULL);
+    const BgpAttr *attr = rt->BestPath()->GetAttr();
+    const BgpAttr *orig_attr = rt->BestPath()->GetOriginalAttr();
+    uint32_t original_local_pref = orig_attr->local_pref();
+    uint32_t policy_local_pref = attr->local_pref();
+    ASSERT_TRUE(policy_local_pref == 102);
+    ASSERT_TRUE(original_local_pref == 100);
+
+    for (int i = 0; i < 255; i++) {
+        ostringstream oss;
+        oss << "10.0.1." << i << "/32";
+        DeleteRoute<InetDefinition>(peers_[0], "test.inet.0", oss.str());
+        oss.str("");
+    }
+    task_util::WaitForIdle();
+}
+
 class TestEnvironment : public ::testing::Environment {
     virtual ~TestEnvironment() { }
 };