Skip to content

Commit

Permalink
VRF state not deleted in DelPeer walk.
Browse files Browse the repository at this point in the history
Problem statement remains same as in this commit:
8e302fc

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
  • Loading branch information
manishsing committed May 22, 2015
1 parent f514603 commit 7051658
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 15 deletions.
42 changes: 29 additions & 13 deletions src/vnsw/agent/oper/agent_route_walker.cc
Expand Up @@ -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++) {
Expand All @@ -39,18 +41,21 @@ bool AgentRouteWalker::RouteWalker(boost::shared_ptr<AgentRouteWalkerQueueEntry>
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:
Expand Down Expand Up @@ -122,7 +127,7 @@ void AgentRouteWalker::StartVrfWalk() {
boost::shared_ptr<AgentRouteWalkerQueueEntry> data(new AgentRouteWalkerQueueEntry(NULL,
AgentRouteWalkerQueueEntry::START_VRF_WALK,
false));
IncrementWalkCount();
IncrementQueuedWalkCount();
work_queue_.Enqueue(data);
}

Expand All @@ -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();
}
}

Expand All @@ -159,7 +161,7 @@ void AgentRouteWalker::StartRouteWalk(VrfEntry *vrf) {
boost::shared_ptr<AgentRouteWalkerQueueEntry> data(new AgentRouteWalkerQueueEntry(vrf,
AgentRouteWalkerQueueEntry::START_ROUTE_WALK,
false));
IncrementWalkCount();
IncrementQueuedWalkCount();
work_queue_.Enqueue(data);
}

Expand All @@ -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();
}
}
}
Expand Down Expand Up @@ -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();
}
}

Expand All @@ -293,6 +304,7 @@ void AgentRouteWalker::Callback(VrfEntry *vrf) {
(new AgentRouteWalkerQueueEntry(vrf,
AgentRouteWalkerQueueEntry::DONE_WALK,
AreAllWalksDone()));
IncrementQueuedWalkDoneCount();
work_queue_.Enqueue(data);
}

Expand Down Expand Up @@ -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;
Expand All @@ -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_();
}
}
Expand Down
16 changes: 14 additions & 2 deletions src/vnsw/agent/oper/agent_route_walker.h
Expand Up @@ -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<AgentRouteWalkerQueueEntry> data);
Expand All @@ -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<int> queued_walk_count_;
tbb::atomic<int> queued_walk_done_count_;
tbb::atomic<int> walk_count_;
DBTableWalker::WalkId vrf_walkid_;
VrfRouteWalkerIdMap route_walkid_[Agent::ROUTE_TABLE_MAX];
Expand Down
2 changes: 2 additions & 0 deletions src/vnsw/agent/test/test_agent_route_walker.cc
Expand Up @@ -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;
}

Expand All @@ -178,6 +179,7 @@ class AgentRouteWalkerTest : public AgentRouteWalker, public ::testing::Test {
vrf_notifications_++;
VrfEntry *vrf = static_cast<VrfEntry *>(e);
StartRouteWalk(vrf);
assert(AreAllWalksDone() == false);
return true;
}

Expand Down

0 comments on commit 7051658

Please sign in to comment.