From 07a60405761f849c2a0a23ddc7a812c905f41b89 Mon Sep 17 00:00:00 2001 From: Nischal Sheth Date: Mon, 22 Jun 2015 17:33:21 -0700 Subject: [PATCH] Don't delete BgpTable till all walkers are done Add infra to keep track of number of walkers per DBTableBase and invoke the RetryDelete method when the last walker is done. Modify BgpTable to use the new infra. Change-Id: Id33c84c7fac76ab99fdf919045da280b0b5038b4 Closes-Bug: 1468052 --- src/bgp/bgp_table.cc | 22 ++++++++----- src/bgp/bgp_table.h | 2 +- src/bgp/test/bgp_xmpp_deferq_test.cc | 48 ++++++++++++++++++++++++++++ src/db/db_table.cc | 17 +++++++++- src/db/db_table.h | 8 ++++- src/db/db_table_walker.cc | 7 ++++ 6 files changed, 93 insertions(+), 11 deletions(-) diff --git a/src/bgp/bgp_table.cc b/src/bgp/bgp_table.cc index 2820a73e5e7..33abdd353af 100644 --- a/src/bgp/bgp_table.cc +++ b/src/bgp/bgp_table.cc @@ -414,26 +414,32 @@ void BgpTable::Input(DBTablePartition *root, DBClient *client, bool BgpTable::MayDelete() const { CHECK_CONCURRENCY("bgp::Config"); - // - // The route replicator may be still in the processes of cleaning up - // the table. + // Bail if the table has listeners. if (HasListeners()) { BGP_LOG_TABLE(this, SandeshLevel::SYS_DEBUG, BGP_LOG_FLAG_ALL, "Paused table deletion due to pending listeners"); return false; } - // - // This table cannot be deleted yet if any route is still present. - // + // Bail if the table has walkers. + if (HasWalkers()) { + BGP_LOG_TABLE(this, SandeshLevel::SYS_DEBUG, BGP_LOG_FLAG_ALL, + "Paused table deletion due to pending walkers"); + return false; + } + + // Bail if the table is not empty. size_t size = Size(); if (size > 0) { BGP_LOG_TABLE(this, SandeshLevel::SYS_DEBUG, BGP_LOG_FLAG_ALL, "Paused table deletion due to " << size << - " pending entries"); + " pending routes"); return false; } - return true; + + // Check the base class at the end so that we add custom checks + // before this if needed and to get more informative log message. + return DBTableBase::MayDelete(); } void BgpTable::Shutdown() { diff --git a/src/bgp/bgp_table.h b/src/bgp/bgp_table.h index 5c58b4a2130..6230b9417bc 100644 --- a/src/bgp/bgp_table.h +++ b/src/bgp/bgp_table.h @@ -97,7 +97,7 @@ class BgpTable : public RouteTable { void ManagedDelete(); virtual void RetryDelete(); void Shutdown(); - bool MayDelete() const; + virtual bool MayDelete() const; bool IsDeleted() const { return deleter()->IsDeleted(); } RoutingInstance *routing_instance() { return rtinstance_; } diff --git a/src/bgp/test/bgp_xmpp_deferq_test.cc b/src/bgp/test/bgp_xmpp_deferq_test.cc index 968ff73bc10..2e6dc1a31b1 100644 --- a/src/bgp/test/bgp_xmpp_deferq_test.cc +++ b/src/bgp/test/bgp_xmpp_deferq_test.cc @@ -408,10 +408,58 @@ class BgpXmppSerializeMembershipReqTest : public BgpXmppUnitTest { } }; +static bool DummyWalkFunction(DBTablePartBase *root, DBEntryBase *entry) { + return true; +} + +static void DummyWalkDoneFunction(DBTableBase *table) { +} + int BgpXmppUnitTest::validate_done_; namespace { +TEST_F(BgpXmppUnitTest, TableDeleteWithPendingWalk) { + Configure(); + task_util::WaitForIdle(); + + // create an XMPP client in server A + agent_a_.reset( + new test::NetworkAgentMock(&evm_, SUB_ADDR, xs_a_->GetPort())); + + // Pause deletion for blue inet table. + BgpTable *blue_table = VerifyBgpTable("blue", Address::INET); + PauseDelete(blue_table->deleter()); + + // Unconfigure all instances. + // The blue instance and blue inet table should exist in deleted state. + UnconfigureRoutingInstances(); + task_util::WaitForIdle(); + RoutingInstance *blue = VerifyRoutingInstance("blue"); + TASK_UTIL_EXPECT_TRUE(blue->deleted()); + TASK_UTIL_EXPECT_TRUE(blue_table->IsDeleted()); + + // Stop the scheduler and resume delete of the table. + // This should create the bgp::Config Task to process lifetime manager + // work queue. + TaskScheduler::GetInstance()->Stop(); + blue_table->deleter()->ResumeDelete(); + + // Start a bunch of table walks. + DBTableWalker *walker = a_->database()->GetWalker(); + for (int idx = 0; idx < 128; ++idx) { + walker->WalkTable( + blue_table, NULL, DummyWalkFunction, DummyWalkDoneFunction); + } + + // Start the scheduler. + // Table should get deleted only after all the walks are done. + TaskScheduler::GetInstance()->Start(); + + // The blue instance should have been destroyed. + VerifyNoRoutingInstance("blue"); +} + TEST_F(BgpXmppUnitTest, Connection) { Configure(); task_util::WaitForIdle(); diff --git a/src/db/db_table.cc b/src/db/db_table.cc index 291ff9b58c3..038bf05e43d 100644 --- a/src/db/db_table.cc +++ b/src/db/db_table.cc @@ -158,7 +158,8 @@ class DBTableBase::ListenerInfo { DBTableBase::DBTableBase(DB *db, const string &name) : db_(db), name_(name), info_(new ListenerInfo(name)), - enqueue_count_(0), input_count_(0), notify_count_(0) { + enqueue_count_(0), input_count_(0), notify_count_(0), + walker_count_(0) { walk_request_count_ = 0; walk_complete_count_ = 0; walk_cancel_count_ = 0; @@ -202,6 +203,20 @@ void DBTableBase::AddToDBStateCount(ListenerId listener, int count) { info_->AddToDBStateCount(listener, count); } +bool DBTableBase::MayDelete() const { + if (HasListeners()) { + return false; + } + if (HasWalkers()) { + return false; + } + if (!empty()) { + return false; + } + + return true; +} + bool DBTableBase::HasListeners() const { return !info_->empty(); } diff --git a/src/db/db_table.h b/src/db/db_table.h index caacc7af96d..d815d38189c 100644 --- a/src/db/db_table.h +++ b/src/db/db_table.h @@ -83,7 +83,7 @@ class DBTableBase { void RunNotify(DBTablePartBase *tpart, DBEntryBase *entry); - // manage db state count for a listner + // Manage db state count for a listener. void AddToDBStateCount(ListenerId listener, int count); // Calculate the size across all partitions. @@ -93,6 +93,7 @@ class DBTableBase { // Suspended deletion resume hook for user function virtual void RetryDelete() { } + virtual bool MayDelete() const; DB *database() { return db_; } const DB *database() const { return db_; } @@ -115,6 +116,10 @@ class DBTableBase { void incr_notify_count() { notify_count_++; } void reset_notify_count() { notify_count_ = 0; } + bool HasWalkers() const { return walker_count_ != 0; } + void incr_walker_count() { walker_count_++; } + uint64_t decr_walker_count() { return --walker_count_; } + uint64_t walk_request_count() const { return walk_request_count_; } uint64_t walk_complete_count() const { return walk_complete_count_; } uint64_t walk_cancel_count() const { return walk_cancel_count_; } @@ -130,6 +135,7 @@ class DBTableBase { uint64_t enqueue_count_; uint64_t input_count_; uint64_t notify_count_; + uint64_t walker_count_; tbb::atomic walk_request_count_; tbb::atomic walk_complete_count_; tbb::atomic walk_cancel_count_; diff --git a/src/db/db_table_walker.cc b/src/db/db_table_walker.cc index ea6c4549a58..017388018d6 100644 --- a/src/db/db_table_walker.cc +++ b/src/db/db_table_walker.cc @@ -202,6 +202,7 @@ DBTableWalker::WalkId DBTableWalker::WalkTable(DBTable *table, walkerfn, walk_complete); walkers_[i] = walker; } + table->incr_walker_count(); return i; } @@ -214,6 +215,7 @@ void DBTableWalker::WalkCancel(WalkId id) { void DBTableWalker::PurgeWalker(WalkId id) { tbb::mutex::scoped_lock lock(walkers_mutex_); Walker *walker = walkers_[id]; + DBTable *table = walker->table_; delete walker; walkers_[id] = NULL; if ((size_t) id == walkers_.size() - 1) { @@ -229,4 +231,9 @@ void DBTableWalker::PurgeWalker(WalkId id) { } walker_map_.set(id); } + + // Retry table deletion when the last walker is purged. + if (table->decr_walker_count() == 0) { + table->RetryDelete(); + } }