Skip to content

Commit

Permalink
Merge "Concurrency issue in unregister of static route" into R3.0
Browse files Browse the repository at this point in the history
  • Loading branch information
@opencontrail-ci-admin
Zuul authored and opencontrail-ci-admin committed Feb 25, 2016
2 parents 37d5ed1 + 3a748ab commit c3e9ec2
Show file tree
Hide file tree
Showing 6 changed files with 417 additions and 67 deletions.
2 changes: 1 addition & 1 deletion src/bgp/bgp_server.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -639,7 +639,7 @@ void BgpServer::InsertStaticRouteMgr(IStaticRouteMgr *srt_manager) {
}

void BgpServer::RemoveStaticRouteMgr(IStaticRouteMgr *srt_manager) {
CHECK_CONCURRENCY("bgp::StaticRoute");
CHECK_CONCURRENCY("bgp::Config");
srt_manager_list_.erase(srt_manager);
}

Expand Down
4 changes: 2 additions & 2 deletions src/bgp/routing-instance/istatic_route_mgr.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ class IStaticRouteMgr {
private:
template <typename U> friend class StaticRouteTest;

virtual void DisableResolveTrigger() = 0;
virtual void EnableResolveTrigger() = 0;
virtual void DisableUnregisterTrigger() = 0;
virtual void EnableUnregisterTrigger() = 0;

virtual void DisableQueue() = 0;
virtual void EnableQueue() = 0;
Expand Down
89 changes: 46 additions & 43 deletions src/bgp/routing-instance/static_route.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -557,9 +557,9 @@ template <typename T>
StaticRouteMgr<T>::StaticRouteMgr(RoutingInstance *rtinstance)
: rtinstance_(rtinstance),
listener_(rtinstance_->server()->condition_listener(GetFamily())),
resolve_trigger_(new TaskTrigger(
boost::bind(&StaticRouteMgr::ResolvePendingStaticRouteConfig, this),
TaskScheduler::GetInstance()->GetTaskId("bgp::Config"), 0)) {
unregister_list_trigger_(new TaskTrigger(
boost::bind(&StaticRouteMgr::ProcessUnregisterList, this),
TaskScheduler::GetInstance()->GetTaskId("bgp::Config"), 0)) {
if (static_route_task_id_ == -1) {
TaskScheduler *scheduler = TaskScheduler::GetInstance();
static_route_task_id_ = scheduler->GetTaskId("bgp::StaticRoute");
Expand Down Expand Up @@ -638,20 +638,6 @@ bool StaticRouteMgr<T>::StaticRouteEventCallback(StaticRouteRequest *req) {
info->set_nexthop_route(NULL);
break;
}
case StaticRouteRequest::DELETE_STATIC_ROUTE_DONE: {
info->set_unregistered();
if (!info->num_matchstate()) {
listener_->UnregisterMatchCondition(table, info);
static_route_map_.erase(info->static_route_prefix());
if (static_route_map_.empty())
rtinstance_->server()->RemoveStaticRouteMgr(this);
if (!routing_instance()->deleted() &&
routing_instance()->config()) {
resolve_trigger_->Set();
}
}
break;
}
default: {
assert(0);
break;
Expand All @@ -664,14 +650,7 @@ bool StaticRouteMgr<T>::StaticRouteEventCallback(StaticRouteRequest *req) {
listener_->RemoveMatchState(table, route, info);
delete state;
if (!info->num_matchstate() && info->unregistered()) {
listener_->UnregisterMatchCondition(table, info);
static_route_map_.erase(info->static_route_prefix());
if (static_route_map_.empty())
rtinstance_->server()->RemoveStaticRouteMgr(this);
if (!routing_instance()->deleted() &&
routing_instance()->config()) {
resolve_trigger_->Set();
}
UnregisterAndResolveStaticRoute(info);
}
}
}
Expand All @@ -680,6 +659,38 @@ bool StaticRouteMgr<T>::StaticRouteEventCallback(StaticRouteRequest *req) {
return true;
}

template <typename T>
bool StaticRouteMgr<T>::ProcessUnregisterList() {
CHECK_CONCURRENCY("bgp::Config");

for (StaticRouteProcessList::iterator
it = unregister_static_route_list_.begin();
it != unregister_static_route_list_.end(); ++it) {
StaticRouteT *info = static_cast<StaticRouteT *>(it->get());
listener_->UnregisterMatchCondition(info->bgp_table(), info);
static_route_map_.erase(info->static_route_prefix());
}

unregister_static_route_list_.clear();

if (static_route_map_.empty()) {
rtinstance_->server()->RemoveStaticRouteMgr(this);
}

if (!routing_instance()->deleted() &&
routing_instance()->config()) {
ProcessStaticRouteConfig();
}
return true;
}

template <typename T>
void StaticRouteMgr<T>::UnregisterAndResolveStaticRoute(StaticRoutePtr entry) {
tbb::mutex::scoped_lock lock(mutex_);
unregister_static_route_list_.insert(entry);
unregister_list_trigger_->Set();
}

template <typename T>
void StaticRouteMgr<T>::LocateStaticRoutePrefix(const StaticRouteConfig &cfg) {
CHECK_CONCURRENCY("bgp::Config");
Expand Down Expand Up @@ -737,12 +748,12 @@ void StaticRouteMgr<T>::LocateStaticRoutePrefix(const StaticRouteConfig &cfg) {
template <typename T>
void StaticRouteMgr<T>::StopStaticRouteDone(BgpTable *table,
ConditionMatch *info) {
// Post the RequestDone event to StaticRoute task to take Action
StaticRouteRequest *req =
new StaticRouteRequest(StaticRouteRequest::DELETE_STATIC_ROUTE_DONE,
table, NULL, StaticRoutePtr(info));

EnqueueStaticRouteReq(req);
CHECK_CONCURRENCY("db::DBTable");
StaticRoute<T> *match = static_cast<StaticRoute<T> *>(info);
match->set_unregistered();
if (!match->num_matchstate() && match->unregistered()) {
UnregisterAndResolveStaticRoute(match);
}
return;
}

Expand Down Expand Up @@ -773,15 +784,8 @@ void StaticRouteMgr<T>::ProcessStaticRouteConfig() {
}
}

template <typename T>
bool StaticRouteMgr<T>::ResolvePendingStaticRouteConfig() {
ProcessStaticRouteConfig();
return true;
}

template <typename T>
StaticRouteMgr<T>::~StaticRouteMgr() {
resolve_trigger_->Reset();
if (static_route_queue_)
delete static_route_queue_;
}
Expand All @@ -793,7 +797,6 @@ bool CompareStaticRouteConfig(const StaticRouteConfig &lhs,
return false;
}


template <typename T>
void StaticRouteMgr<T>::UpdateStaticRouteConfig() {
CHECK_CONCURRENCY("bgp::Config");
Expand Down Expand Up @@ -867,13 +870,13 @@ void StaticRouteMgr<T>::UpdateAllRoutes() {
}

template <typename T>
void StaticRouteMgr<T>::DisableResolveTrigger() {
resolve_trigger_->set_disable();
void StaticRouteMgr<T>::DisableUnregisterTrigger() {
unregister_list_trigger_->set_disable();
}

template <typename T>
void StaticRouteMgr<T>::EnableResolveTrigger() {
resolve_trigger_->set_enable();
void StaticRouteMgr<T>::EnableUnregisterTrigger() {
unregister_list_trigger_->set_enable();
}

template <typename T>
Expand Down
16 changes: 10 additions & 6 deletions src/bgp/routing-instance/static_route.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ typedef ConditionMatchPtr StaticRoutePtr;
struct StaticRouteRequest {
enum RequestType {
NEXTHOP_ADD_CHG,
NEXTHOP_DELETE,
DELETE_STATIC_ROUTE_DONE
NEXTHOP_DELETE
};

StaticRouteRequest(RequestType type, BgpTable *table, BgpRoute *route,
Expand Down Expand Up @@ -95,6 +94,7 @@ class StaticRouteMgr : public IStaticRouteMgr {

private:
template <typename U> friend class StaticRouteTest;
typedef std::set<StaticRoutePtr> StaticRouteProcessList;

// All static route related actions are performed in the context
// of this task. This task has exclusion with db::DBTable task.
Expand All @@ -103,11 +103,13 @@ class StaticRouteMgr : public IStaticRouteMgr {
void LocateStaticRoutePrefix(const StaticRouteConfig &cfg);
void RemoveStaticRoutePrefix(const PrefixT &static_route);
void StopStaticRouteDone(BgpTable *table, ConditionMatch *info);
bool ResolvePendingStaticRouteConfig();
void UnregisterAndResolveStaticRoute(StaticRoutePtr entry);
bool StaticRouteEventCallback(StaticRouteRequest *req);

virtual void DisableResolveTrigger();
virtual void EnableResolveTrigger();
bool ProcessUnregisterList();

virtual void DisableUnregisterTrigger();
virtual void EnableUnregisterTrigger();

virtual void DisableQueue() { static_route_queue_->set_disable(true); }
virtual void EnableQueue() { static_route_queue_->set_disable(false); }
Expand All @@ -118,7 +120,9 @@ class StaticRouteMgr : public IStaticRouteMgr {
BgpConditionListener *listener_;
StaticRouteMap static_route_map_;
WorkQueue<StaticRouteRequest *> *static_route_queue_;
boost::scoped_ptr<TaskTrigger> resolve_trigger_;
tbb::mutex mutex_;
StaticRouteProcessList unregister_static_route_list_;
boost::scoped_ptr<TaskTrigger> unregister_list_trigger_;

DISALLOW_COPY_AND_ASSIGN(StaticRouteMgr);
};
Expand Down

0 comments on commit c3e9ec2

Please sign in to comment.