From 705165854bbdaff9c47a2a9443410eec53e4fb37 Mon Sep 17 00:00:00 2001 From: Manish Date: Wed, 20 May 2015 16:42:03 +0530 Subject: [PATCH] VRF state not deleted in DelPeer walk. Problem statement remains same as in this commit: https://github.com/Juniper/contrail-controller/commit/8e302fcb991c8f5d8f5defb85b9851f8cde5f479 However above commit does not solve the issue. Reason being, walk count was being incremented on enqueue of walk but when walk is processed it calls Cancel for any previously started walk. This Cancel decrements the walk count. This defeats the purpose of moving walk count increment to enqueue in above commit. Also consider a walk for VRF where there are four route tables. This should result in walk count to be 5 (1 for vrf table + 4 for route tables). With above fix this will be 2 (1 for Vrf + 1 for route table). It didnt take into consideration that route walk count needs to be incremented for each route table. Solution: Use a seperate enqueue walk count and restore the walk_count as it was before the above commit. Use both of them to check for walk done. Closes-bug: 1455862 Change-Id: I8d96732375f649e70d6754cb6c5b34c24867ce0c --- src/vnsw/agent/oper/agent_route_walker.cc | 42 +++++++++++++------ src/vnsw/agent/oper/agent_route_walker.h | 16 ++++++- .../agent/test/test_agent_route_walker.cc | 2 + 3 files changed, 45 insertions(+), 15 deletions(-) diff --git a/src/vnsw/agent/oper/agent_route_walker.cc b/src/vnsw/agent/oper/agent_route_walker.cc index aab3996b176..4dd4b6ab313 100644 --- a/src/vnsw/agent/oper/agent_route_walker.cc +++ b/src/vnsw/agent/oper/agent_route_walker.cc @@ -24,6 +24,8 @@ AgentRouteWalker::AgentRouteWalker(Agent *agent, WalkType type) : GetTaskId("Agent::RouteWalker"), 0, boost::bind(&AgentRouteWalker::RouteWalker, this, _1)) { walk_count_ = AgentRouteWalker::kInvalidWalkCount; + queued_walk_count_ = AgentRouteWalker::kInvalidWalkCount; + queued_walk_done_count_ = AgentRouteWalker::kInvalidWalkCount; for (uint8_t table_type = (Agent::INVALID + 1); table_type < Agent::ROUTE_TABLE_MAX; table_type++) { @@ -39,18 +41,21 @@ bool AgentRouteWalker::RouteWalker(boost::shared_ptr VrfEntry *vrf = data->vrf_ref_.get(); switch (data->type_) { case AgentRouteWalkerQueueEntry::START_VRF_WALK: + DecrementQueuedWalkCount(); StartVrfWalkInternal(); break; case AgentRouteWalkerQueueEntry::CANCEL_VRF_WALK: CancelVrfWalkInternal(); break; case AgentRouteWalkerQueueEntry::START_ROUTE_WALK: + DecrementQueuedWalkCount(); StartRouteWalkInternal(vrf); break; case AgentRouteWalkerQueueEntry::CANCEL_ROUTE_WALK: CancelRouteWalkInternal(vrf); break; case AgentRouteWalkerQueueEntry::DONE_WALK: + DecrementQueuedWalkDoneCount(); CallbackInternal(vrf, data->all_walks_done_); break; default: @@ -122,7 +127,7 @@ void AgentRouteWalker::StartVrfWalk() { boost::shared_ptr data(new AgentRouteWalkerQueueEntry(NULL, AgentRouteWalkerQueueEntry::START_VRF_WALK, false)); - IncrementWalkCount(); + IncrementQueuedWalkCount(); work_queue_.Enqueue(data); } @@ -140,14 +145,11 @@ void AgentRouteWalker::StartVrfWalkInternal() boost::bind(&AgentRouteWalker::VrfWalkDone, this, _1)); if (vrf_walkid_ != DBTableWalker::kInvalidWalkerId) { + IncrementWalkCount(); AGENT_DBWALK_TRACE(AgentRouteWalkerTrace, "VRF table walk started", walk_type_, "", vrf_walkid_, 0, "", DBTableWalker::kInvalidWalkerId); - } else { - //As walk didnt start because of some failure decrement the - //walk count, as there will be no walk done. - DecrementWalkCount(); } } @@ -159,7 +161,7 @@ void AgentRouteWalker::StartRouteWalk(VrfEntry *vrf) { boost::shared_ptr data(new AgentRouteWalkerQueueEntry(vrf, AgentRouteWalkerQueueEntry::START_ROUTE_WALK, false)); - IncrementWalkCount(); + IncrementQueuedWalkCount(); work_queue_.Enqueue(data); } @@ -185,14 +187,11 @@ void AgentRouteWalker::StartRouteWalkInternal(const VrfEntry *vrf) { this, _1)); if (walkid != DBTableWalker::kInvalidWalkerId) { route_walkid_[table_type][vrf_id] = walkid; + IncrementWalkCount(); AGENT_DBWALK_TRACE(AgentRouteWalkerTrace, "Route table walk started for vrf", walk_type_, (vrf != NULL) ? vrf->GetName() : "Unknown", vrf_walkid_, table_type, "", walkid); - } else { - //As walk didnt start because of some failure decrement the - //walk count, as there will be no walk done. - DecrementWalkCount(); } } } @@ -284,7 +283,19 @@ void AgentRouteWalker::RouteWalkDone(DBTableBase *part) { void AgentRouteWalker::DecrementWalkCount() { if (walk_count_ != AgentRouteWalker::kInvalidWalkCount) { - walk_count_--; + walk_count_.fetch_and_decrement(); + } +} + +void AgentRouteWalker::DecrementQueuedWalkCount() { + if (queued_walk_count_ != AgentRouteWalker::kInvalidWalkCount) { + queued_walk_count_.fetch_and_decrement(); + } +} + +void AgentRouteWalker::DecrementQueuedWalkDoneCount() { + if (queued_walk_done_count_ != AgentRouteWalker::kInvalidWalkCount) { + queued_walk_done_count_.fetch_and_decrement(); } } @@ -293,6 +304,7 @@ void AgentRouteWalker::Callback(VrfEntry *vrf) { (new AgentRouteWalkerQueueEntry(vrf, AgentRouteWalkerQueueEntry::DONE_WALK, AreAllWalksDone())); + IncrementQueuedWalkDoneCount(); work_queue_.Enqueue(data); } @@ -341,7 +353,8 @@ bool AgentRouteWalker::AreAllWalksDone() const { } } } - if (walk_done && walk_count_) { + if (walk_done && (walk_count_ != AgentRouteWalker::kInvalidWalkCount) + && (queued_walk_count_ != AgentRouteWalker::kInvalidWalkCount)) { walk_done = false; } return walk_done; @@ -351,7 +364,10 @@ bool AgentRouteWalker::AreAllWalksDone() const { * Check if all walks are over. */ void AgentRouteWalker::OnWalkComplete() { - if (!walk_count_ && walk_done_cb_) { + if ((walk_count_ == AgentRouteWalker::kInvalidWalkCount) && + (queued_walk_count_ == AgentRouteWalker::kInvalidWalkCount) && + (queued_walk_done_count_ == AgentRouteWalker::kInvalidWalkCount) && + walk_done_cb_) { walk_done_cb_(); } } diff --git a/src/vnsw/agent/oper/agent_route_walker.h b/src/vnsw/agent/oper/agent_route_walker.h index 45a378d4c7b..189f2b5c1f9 100644 --- a/src/vnsw/agent/oper/agent_route_walker.h +++ b/src/vnsw/agent/oper/agent_route_walker.h @@ -88,8 +88,14 @@ class AgentRouteWalker { void WalkDoneCallback(WalkDone cb); void RouteWalkDoneForVrfCallback(RouteWalkDoneCb cb); + int queued_walk_done_count() const {return queued_walk_done_count_;} + bool AllWalkDoneDequeued() const {return (queued_walk_done_count_ == + kInvalidWalkCount);} + int queued_walk_count() const {return queued_walk_count_;} + bool AllWalksDequeued() const {return (queued_walk_count_ == + kInvalidWalkCount);} int walk_count() const {return walk_count_;} - bool IsWalkCompleted() const {return (walk_count_ == 0);} + bool IsWalkCompleted() const {return (walk_count_ == kInvalidWalkCount);} //Callback for start of a walk issued from Agent::RouteWalker //task context. virtual bool RouteWalker(boost::shared_ptr data); @@ -107,10 +113,16 @@ class AgentRouteWalker { void OnWalkComplete(); void OnRouteTableWalkCompleteForVrf(VrfEntry *vrf); void DecrementWalkCount(); - void IncrementWalkCount() {walk_count_++;} + void IncrementWalkCount() {walk_count_.fetch_and_increment();} + void IncrementQueuedWalkCount() {queued_walk_count_.fetch_and_increment();} + void DecrementQueuedWalkCount(); + void IncrementQueuedWalkDoneCount() {queued_walk_done_count_.fetch_and_increment();} + void DecrementQueuedWalkDoneCount(); Agent *agent_; AgentRouteWalker::WalkType walk_type_; + tbb::atomic queued_walk_count_; + tbb::atomic queued_walk_done_count_; tbb::atomic walk_count_; DBTableWalker::WalkId vrf_walkid_; VrfRouteWalkerIdMap route_walkid_[Agent::ROUTE_TABLE_MAX]; diff --git a/src/vnsw/agent/test/test_agent_route_walker.cc b/src/vnsw/agent/test/test_agent_route_walker.cc index 077c19b7857..f043db493ee 100644 --- a/src/vnsw/agent/test/test_agent_route_walker.cc +++ b/src/vnsw/agent/test/test_agent_route_walker.cc @@ -166,6 +166,7 @@ class AgentRouteWalkerTest : public AgentRouteWalker, public ::testing::Test { //2.2.2.20/32; 255.255.255.255; 0:0:2:2:2:20; ff:ff:ff:ff:ff:ff route_notifications_++; route_table_walk_started_ = true; + assert(AreAllWalksDone() == false); return true; } @@ -178,6 +179,7 @@ class AgentRouteWalkerTest : public AgentRouteWalker, public ::testing::Test { vrf_notifications_++; VrfEntry *vrf = static_cast(e); StartRouteWalk(vrf); + assert(AreAllWalksDone() == false); return true; }