From 9eb7fc03cea177ed2a67452bad316053b6a9aecd Mon Sep 17 00:00:00 2001 From: Patrick Niklaus Date: Fri, 7 Oct 2016 16:08:08 +0200 Subject: [PATCH] Use a shared (!) reader writer lock to protect CURRENT_REGIONS This fixes issue #3016. --- include/engine/engine.hpp | 11 ++--- include/storage/shared_barriers.hpp | 17 ++------ src/engine/engine.cpp | 66 ++++------------------------- src/storage/storage.cpp | 29 ++++++------- src/tools/unlock_all_mutexes.cpp | 1 - 5 files changed, 30 insertions(+), 94 deletions(-) diff --git a/include/engine/engine.hpp b/include/engine/engine.hpp index 85e576054..aece1ac1a 100644 --- a/include/engine/engine.hpp +++ b/include/engine/engine.hpp @@ -1,7 +1,6 @@ #ifndef ENGINE_HPP #define ENGINE_HPP -#include "storage/shared_barriers.hpp" #include "engine/status.hpp" #include "util/json_container.hpp" @@ -20,6 +19,11 @@ struct Object; } } +namespace storage +{ +struct SharedBarriers; +} + // Fwd decls namespace engine { @@ -52,9 +56,6 @@ class BaseDataFacade; class Engine final { public: - // Needs to be public - struct EngineLock; - explicit Engine(const EngineConfig &config); Engine(Engine &&) noexcept; @@ -71,7 +72,7 @@ class Engine final Status Tile(const api::TileParameters ¶meters, std::string &result) const; private: - std::unique_ptr lock; + std::unique_ptr lock; std::unique_ptr route_plugin; std::unique_ptr table_plugin; diff --git a/include/storage/shared_barriers.hpp b/include/storage/shared_barriers.hpp index d474e27f6..0cb7f84d7 100644 --- a/include/storage/shared_barriers.hpp +++ b/include/storage/shared_barriers.hpp @@ -3,6 +3,7 @@ #include #include +#include namespace osrm { @@ -13,25 +14,13 @@ struct SharedBarriers SharedBarriers() : pending_update_mutex(boost::interprocess::open_or_create, "pending_update"), - update_mutex(boost::interprocess::open_or_create, "update"), - query_mutex(boost::interprocess::open_or_create, "query"), - no_running_queries_condition(boost::interprocess::open_or_create, "no_running_queries"), - update_ongoing(false), number_of_queries(0) + query_mutex(boost::interprocess::open_or_create, "query") { } // Mutex to protect access to the boolean variable boost::interprocess::named_mutex pending_update_mutex; - boost::interprocess::named_mutex update_mutex; - boost::interprocess::named_mutex query_mutex; - - // Condition that no update is running - boost::interprocess::named_condition no_running_queries_condition; - - // Is there an ongoing update? - bool update_ongoing; - // Is there any query? - int number_of_queries; + boost::interprocess::named_sharable_mutex query_mutex; }; } } diff --git a/src/engine/engine.cpp b/src/engine/engine.cpp index 413a75c15..429f68bb2 100644 --- a/src/engine/engine.cpp +++ b/src/engine/engine.cpp @@ -1,5 +1,5 @@ -#include "engine/engine.hpp" #include "engine/api/route_parameters.hpp" +#include "engine/engine.hpp" #include "engine/engine_config.hpp" #include "engine/status.hpp" @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -27,65 +28,13 @@ #include #include -namespace osrm -{ -namespace engine -{ -struct Engine::EngineLock -{ - // will only be initialized if shared memory is used - storage::SharedBarriers barrier; - // decrease number of concurrent queries - void DecreaseQueryCount(); - // increase number of concurrent queries - void IncreaseQueryCount(); -}; - -// decrease number of concurrent queries -void Engine::EngineLock::DecreaseQueryCount() -{ - // lock query - boost::interprocess::scoped_lock query_lock( - barrier.query_mutex); - - // decrement query count - --(barrier.number_of_queries); - BOOST_ASSERT_MSG(0 <= barrier.number_of_queries, "invalid number of queries"); - - // notify all processes that were waiting for this condition - if (0 == barrier.number_of_queries) - { - barrier.no_running_queries_condition.notify_all(); - } -} - -// increase number of concurrent queries -void Engine::EngineLock::IncreaseQueryCount() -{ - // lock update pending - boost::interprocess::scoped_lock pending_lock( - barrier.pending_update_mutex); - - // lock query - boost::interprocess::scoped_lock query_lock( - barrier.query_mutex); - - // unlock update pending - pending_lock.unlock(); - - // increment query count - ++(barrier.number_of_queries); -} -} // ns engine -} // ns osrm - namespace { // Abstracted away the query locking into a template function // Works the same for every plugin. template osrm::engine::Status -RunQuery(const std::unique_ptr &lock, +RunQuery(const std::unique_ptr &lock, const std::shared_ptr &facade, const ParameterT ¶meters, PluginT &plugin, @@ -97,7 +46,10 @@ RunQuery(const std::unique_ptr &lock, } BOOST_ASSERT(lock); - lock->IncreaseQueryCount(); + // this locks aquires shared ownership of the query mutex: other requets are allowed + // to run, but data updates need to wait for all queries to finish until they can aquire an exclusive lock + boost::interprocess::sharable_lock query_lock( + lock->query_mutex); auto &shared_facade = static_cast(*facade); shared_facade.CheckAndReloadFacade(); @@ -107,7 +59,6 @@ RunQuery(const std::unique_ptr &lock, osrm::engine::Status status = plugin.HandleRequest(facade, parameters, result); - lock->DecreaseQueryCount(); return status; } @@ -119,10 +70,11 @@ namespace engine { Engine::Engine(const EngineConfig &config) + : lock(config.use_shared_memory ? std::make_unique() + : std::unique_ptr()) { if (config.use_shared_memory) { - lock = std::make_unique(); query_data_facade = std::make_shared(); } else diff --git a/src/storage/storage.cpp b/src/storage/storage.cpp index f42c64fcf..c5de38d77 100644 --- a/src/storage/storage.cpp +++ b/src/storage/storage.cpp @@ -42,11 +42,6 @@ namespace osrm namespace storage { -using RTreeLeaf = engine::datafacade::BaseDataFacade::RTreeLeaf; -using RTreeNode = - util::StaticRTree::vector, true>::TreeNode; -using QueryGraph = util::StaticGraph; - // delete a shared memory region. report warning if it could not be deleted void deleteRegion(const SharedDataType region) { @@ -76,6 +71,11 @@ void deleteRegion(const SharedDataType region) } } +using RTreeLeaf = engine::datafacade::BaseDataFacade::RTreeLeaf; +using RTreeNode = + util::StaticRTree::vector, true>::TreeNode; +using QueryGraph = util::StaticGraph; + Storage::Storage(StorageConfig config_) : config(std::move(config_)) {} int Storage::Run() @@ -738,20 +738,15 @@ int Storage::Run() SharedDataTimestamp *data_timestamp_ptr = static_cast(data_type_memory->Ptr()); - boost::interprocess::scoped_lock query_lock( - barrier.query_mutex); - - // notify all processes that were waiting for this condition - if (0 < barrier.number_of_queries) { - barrier.no_running_queries_condition.wait(query_lock); - } + boost::interprocess::scoped_lock query_lock(barrier.query_mutex); - data_timestamp_ptr->layout = layout_region; - data_timestamp_ptr->data = data_region; - data_timestamp_ptr->timestamp += 1; - deleteRegion(previous_data_region); - deleteRegion(previous_layout_region); + data_timestamp_ptr->layout = layout_region; + data_timestamp_ptr->data = data_region; + data_timestamp_ptr->timestamp += 1; + deleteRegion(previous_data_region); + deleteRegion(previous_layout_region); + } util::SimpleLogger().Write() << "all data loaded"; return EXIT_SUCCESS; diff --git a/src/tools/unlock_all_mutexes.cpp b/src/tools/unlock_all_mutexes.cpp index 556f5b75e..abb2e065e 100644 --- a/src/tools/unlock_all_mutexes.cpp +++ b/src/tools/unlock_all_mutexes.cpp @@ -10,6 +10,5 @@ int main() osrm::storage::SharedBarriers barrier; barrier.pending_update_mutex.unlock(); barrier.query_mutex.unlock(); - barrier.update_mutex.unlock(); return 0; }