From 5a4ffacebfd5c2c9bbf0accbe8f01fe116814a68 Mon Sep 17 00:00:00 2001 From: Ilho Kim Date: Tue, 20 Sep 2022 14:55:15 +0900 Subject: [PATCH] Remove unnecessary unique_lock from read operation In some case, unique_lock is used in code that can be operated in multiple thread environments without changing memory state during read operation, which cause unnecessary starvation Change-Id: Ife3689a667929ac0506f4ced44e4a12cf16ba3dc Signed-off-by: Ilho Kim --- src/server/cache_flag.hh | 19 ++++--- src/server/database/abstract_db_handler.cc | 2 +- src/server/database/abstract_db_handler.hh | 2 +- src/server/database/appinfo_db_handler.cc | 75 ++++++++++++++------------ src/server/database/appinfo_db_handler.hh | 5 +- src/server/database/cache_db_handler.cc | 2 +- src/server/database/cert_get_db_handler.cc | 2 +- src/server/database/cert_set_db_handler.cc | 2 +- src/server/database/create_db_handler.cc | 2 +- src/server/database/db_handle_provider.cc | 26 +++++---- src/server/database/db_handle_provider.hh | 10 ++-- src/server/database/depinfo_db_handler.cc | 2 +- src/server/database/pkg_get_db_handler.cc | 66 ++++++++++++----------- src/server/database/pkg_get_db_handler.hh | 5 +- src/server/database/pkg_set_db_handler.cc | 2 +- src/server/database/query_handler.cc | 2 +- src/server/database/remove_cache_db_handler.cc | 2 +- 17 files changed, 124 insertions(+), 102 deletions(-) diff --git a/src/server/cache_flag.hh b/src/server/cache_flag.hh index 67aa9a0..5ddcb0c 100644 --- a/src/server/cache_flag.hh +++ b/src/server/cache_flag.hh @@ -17,7 +17,6 @@ #ifndef CACHE_FLAG_HH_ #define CACHE_FLAG_HH_ -#include #include namespace pkgmgr_server { @@ -36,7 +35,7 @@ class EXPORT_API CacheFlag { static bool SetPreparing() { bool ret = false; - std::unique_lock u(status_lock_); + std::unique_lock u(status_lock_); if (status_ == Status::UNPREPARED) { status_ = Status::PREPARING; ret = true; @@ -45,25 +44,25 @@ class EXPORT_API CacheFlag { } static void SetStatus(Status status) { - std::unique_lock u(status_lock_); + std::unique_lock u(status_lock_); status_ = status; } static Status GetStatus() { - std::unique_lock u(status_lock_); + std::shared_lock s(status_lock_); return status_; } - static std::unique_lock GetWriterLock() { - return std::unique_lock(lock_); + static std::unique_lock GetWriterLock() { + return std::unique_lock(lock_); } static void WriterUnLock() { lock_.unlock(); } - static std::shared_lock GetReaderLock() { - return std::shared_lock(lock_, std::defer_lock); + static std::shared_lock GetReaderLock() { + return std::shared_lock(lock_, std::defer_lock); } static void ReaderUnLock() { @@ -72,8 +71,8 @@ class EXPORT_API CacheFlag { private: static inline Status status_; - static inline std::mutex status_lock_; - static inline std::shared_timed_mutex lock_; + static inline std::shared_mutex status_lock_; + static inline std::shared_mutex lock_; }; } // namespace pkgmgr_server diff --git a/src/server/database/abstract_db_handler.cc b/src/server/database/abstract_db_handler.cc index 2215c30..1e57815 100644 --- a/src/server/database/abstract_db_handler.cc +++ b/src/server/database/abstract_db_handler.cc @@ -155,7 +155,7 @@ uid_t ConvertUID(uid_t uid) { namespace pkgmgr_server { namespace database { -std::shared_timed_mutex AbstractDBHandler::lock_; +std::shared_mutex AbstractDBHandler::lock_; AbstractDBHandler::~AbstractDBHandler() { for (auto& db_handle : db_handle_list_) diff --git a/src/server/database/abstract_db_handler.hh b/src/server/database/abstract_db_handler.hh index 894e1de..6377b52 100644 --- a/src/server/database/abstract_db_handler.hh +++ b/src/server/database/abstract_db_handler.hh @@ -57,7 +57,7 @@ class EXPORT_API AbstractDBHandler { const std::string& GetLocale(); static uid_t GetDefaultUser(); - static std::shared_timed_mutex lock_; + static std::shared_mutex lock_; private: pkgmgr_common::DBType db_type_; diff --git a/src/server/database/appinfo_db_handler.cc b/src/server/database/appinfo_db_handler.cc index c6a891a..de4dcb3 100644 --- a/src/server/database/appinfo_db_handler.cc +++ b/src/server/database/appinfo_db_handler.cc @@ -27,6 +27,19 @@ #include "pkgmgrinfo_internal.h" #include "pkgmgrinfo_internal.hh" +namespace { + +uid_t globaluser_uid = -1; + +uid_t GetGlobalUID() { + if (globaluser_uid == (uid_t)-1) + globaluser_uid = tzplatform_getuid(TZ_SYS_GLOBALAPP_USER); + + return globaluser_uid; +} + +} // namespace + namespace pkgmgr_server { namespace database { @@ -43,8 +56,13 @@ void AppInfoDBHandler::SetFilter(pkgmgrinfo_filter_x* filter) { filter_ = filter; } -int AppInfoDBHandler::GetHandleFromDB( - std::vector>& conn_list) { +int AppInfoDBHandler::GetHandleFromDB() { + if (!Connect()) { + LOG(ERROR) << "Failed to connect database"; + return PMINFO_R_ERROR; + } + + std::vector> conn_list = GetConnection(); std::vector> list; int ret = PMINFO_R_OK; for (auto& conn : conn_list) { @@ -65,11 +83,18 @@ int AppInfoDBHandler::GetHandleFromDB( return ret; } -int AppInfoDBHandler::GetHandleFromCache( - std::vector>& conn_list) { - bool is_write_op = - GetOpType() == pkgmgr_common::DBOperationType::OPERATION_TYPE_WRITE; - int ret = PMINFO_R_OK; +void AppInfoDBHandler::GetApplicationFromCache(uid_t uid, + const std::string& application) { + std::vector> app_list; + app_list = DBHandleProvider::GetInst(uid).GetApplications(GetPID(), + filter_, application); + + handle_list_.reserve(app_list.size() + handle_list_.size()); + std::move(std::begin(app_list), std::end(app_list), + std::back_inserter(handle_list_)); +} + +int AppInfoDBHandler::GetHandleFromCache() { std::string application; for (auto* it = filter_->list; it != nullptr; it = g_slist_next(it)) { @@ -80,50 +105,34 @@ int AppInfoDBHandler::GetHandleFromCache( } } - std::vector> app_list; - for (auto& conn : conn_list) { - app_list = DBHandleProvider::GetInst(conn.second) - .GetApplications( - GetPID(), is_write_op, filter_, application); + if (uid_ > REGULAR_USER) + GetApplicationFromCache(uid_, application); - handle_list_.reserve(app_list.size() + handle_list_.size()); - std::move(std::begin(app_list), std::end(app_list), - std::back_inserter(handle_list_)); - } + GetApplicationFromCache(GetGlobalUID(), application); if (handle_list_.empty()) - ret = PMINFO_R_ENOENT; + return PMINFO_R_ENOENT; - return ret; + return PMINFO_R_OK; } int AppInfoDBHandler::Execute() { - std::shared_lock s(lock_); + std::shared_lock s(lock_); SetOpType(pkgmgr_common::DBOperationType::OPERATION_TYPE_READ); SetDBType(pkgmgr_common::DBType::DB_TYPE_FILE_PKGDB); - if (!Connect()) - return PMINFO_R_ERROR; - - std::vector> conn_list = GetConnection(); - - bool is_writer = false; - for (auto& conn : conn_list) { - if (DBHandleProvider::GetInst(conn.second).IsWriter(GetPID())) - is_writer = true; - } - if (is_writer) - return GetHandleFromDB(conn_list); + if (DBHandleProvider::IsWriter(GetPID())) + return GetHandleFromDB(); if (uid_ <= GetDefaultUser() && CacheFlag::GetStatus() == CacheFlag::Status::PREPARED) { auto cache_lock = CacheFlag::GetReaderLock(); if (cache_lock.try_lock() && CacheFlag::GetStatus() == CacheFlag::Status::PREPARED) - return GetHandleFromCache(conn_list); + return GetHandleFromCache(); } - return GetHandleFromDB(conn_list); + return GetHandleFromDB(); } } // namespace database diff --git a/src/server/database/appinfo_db_handler.hh b/src/server/database/appinfo_db_handler.hh index 1e68059..41ea386 100644 --- a/src/server/database/appinfo_db_handler.hh +++ b/src/server/database/appinfo_db_handler.hh @@ -42,8 +42,9 @@ class EXPORT_API AppInfoDBHandler : public AbstractDBHandler{ virtual int Execute(); protected: - int GetHandleFromDB(std::vector>& conn_list); - int GetHandleFromCache(std::vector>& conn_list); + int GetHandleFromDB(); + int GetHandleFromCache(); + void GetApplicationFromCache(uid_t uid, const std::string& application); pkgmgrinfo_filter_x* filter_; std::vector> handle_list_; uid_t uid_; diff --git a/src/server/database/cache_db_handler.cc b/src/server/database/cache_db_handler.cc index aaf1592..aacdbd5 100644 --- a/src/server/database/cache_db_handler.cc +++ b/src/server/database/cache_db_handler.cc @@ -32,7 +32,7 @@ CacheDBHandler::CacheDBHandler(uid_t uid, int pid) CacheDBHandler::~CacheDBHandler() {} int CacheDBHandler::Execute() { - std::shared_lock s(lock_); + std::shared_lock s(lock_); SetOpType(pkgmgr_common::DBOperationType::OPERATION_TYPE_READ); SetDBType(pkgmgr_common::DBType::DB_TYPE_FILE_PKGDB); diff --git a/src/server/database/cert_get_db_handler.cc b/src/server/database/cert_get_db_handler.cc index 2001bd1..881b84c 100644 --- a/src/server/database/cert_get_db_handler.cc +++ b/src/server/database/cert_get_db_handler.cc @@ -41,7 +41,7 @@ void CertGetDBHandler::SetPkgID(std::string pkgid) { } int CertGetDBHandler::Execute() { - std::shared_lock s(lock_); + std::shared_lock s(lock_); SetOpType(pkgmgr_common::DBOperationType::OPERATION_TYPE_READ); SetDBType(pkgmgr_common::DBType::DB_TYPE_FILE_CERTDB); diff --git a/src/server/database/cert_set_db_handler.cc b/src/server/database/cert_set_db_handler.cc index 179789a..97babdf 100644 --- a/src/server/database/cert_set_db_handler.cc +++ b/src/server/database/cert_set_db_handler.cc @@ -36,7 +36,7 @@ void CertSetDBHandler::SetCertHandle(pkgmgr_certinfo_x* cert_info) { } int CertSetDBHandler::Execute() { - std::unique_lock u(lock_); + std::unique_lock u(lock_); SetOpType(pkgmgr_common::DBOperationType::OPERATION_TYPE_WRITE); SetDBType(pkgmgr_common::DBType::DB_TYPE_FILE_CERTDB); diff --git a/src/server/database/create_db_handler.cc b/src/server/database/create_db_handler.cc index 8ad7f82..1e9a657 100644 --- a/src/server/database/create_db_handler.cc +++ b/src/server/database/create_db_handler.cc @@ -44,7 +44,7 @@ std::vector> CreateDBHandler::GetResult() { } int CreateDBHandler::Execute() { - std::unique_lock u(lock_); + std::unique_lock u(lock_); if (CreateParserDB() < 0) { LOG(ERROR) << "Failed to create parser db for uid : " << GetUID(); diff --git a/src/server/database/db_handle_provider.cc b/src/server/database/db_handle_provider.cc index 6720378..375c3eb 100644 --- a/src/server/database/db_handle_provider.cc +++ b/src/server/database/db_handle_provider.cc @@ -100,7 +100,7 @@ std::string DBHandleProvider::global_parser_filedb_path_; std::string DBHandleProvider::cert_filedb_path_; std::unordered_set DBHandleProvider::writer_pid_list_; std::recursive_mutex DBHandleProvider::lock_; -std::mutex DBHandleProvider::pid_list_lock_; +std::shared_mutex DBHandleProvider::pid_list_lock_; DBHandleProvider::DBHandleProvider(uid_t uid) : uid_(uid), @@ -130,10 +130,16 @@ DBHandleProvider::DBHandleProvider(uid_t uid) } DBHandleProvider& DBHandleProvider::GetInst(uid_t uid) { - static std::mutex singleton_lock_; - std::unique_lock u(singleton_lock_); + static std::shared_mutex singleton_lock_; + std::shared_lock s(singleton_lock_); uid = ConvertUID(uid); + auto it = provider_map_.find(uid); + if (it != provider_map_.end()) + return *(it->second); + + s.unlock(); + std::unique_lock u(singleton_lock_); auto& prov = provider_map_[uid]; if (prov == nullptr) prov.reset(new DBHandleProvider(uid)); @@ -142,7 +148,7 @@ DBHandleProvider& DBHandleProvider::GetInst(uid_t uid) { } bool DBHandleProvider::IsCrashedWriteRequest() { - std::unique_lock u(pid_list_lock_); + std::unique_lock u(pid_list_lock_); if (writer_pid_list_.empty()) return false; @@ -296,7 +302,7 @@ void DBHandleProvider::UnsetMemoryMode(pid_t pid) { } bool DBHandleProvider::IsMemoryDBActive(pid_t pid, bool write) { - std::unique_lock u(pid_list_lock_); + std::unique_lock u(pid_list_lock_); if (!is_user_memdb_set_) return false; @@ -327,7 +333,7 @@ void DBHandleProvider::ReleaseCache() { } bool DBHandleProvider::IsWriter(pid_t pid) { - std::unique_lock u(pid_list_lock_); + std::unique_lock s(pid_list_lock_); return writer_pid_list_.find(pid) != writer_pid_list_.end(); } @@ -414,7 +420,7 @@ int DBHandleProvider::UpdateCache(sqlite3* db, pid_t pid, uid_t uid, bool write, } std::vector> DBHandleProvider::GetPackages( - pid_t pid, bool write, pkgmgrinfo_filter_x* filter, + pid_t pid, pkgmgrinfo_filter_x* filter, const std::string& package) { std::vector> ret; auto map_it = pkg_map_.find(package); @@ -454,7 +460,7 @@ void DBHandleProvider::AddPackage(std::string package, package_x* info) { } std::vector> DBHandleProvider::GetApplications( - pid_t pid, bool write, pkgmgrinfo_filter_x* filter, + pid_t pid, pkgmgrinfo_filter_x* filter, const std::string& app) { /* make metadata filter map */ std::unordered_map metadata_map; @@ -524,13 +530,13 @@ void DBHandleProvider::AddApplication(std::string app, std::shared_ptr u(pid_list_lock_); + std::unique_lock u(pid_list_lock_); writer_pid_list_.insert(pid); } bool DBHandleProvider::ErasePID(pid_t pid) { - std::unique_lock u(pid_list_lock_); + std::unique_lock u(pid_list_lock_); return writer_pid_list_.erase(pid) == 1; } diff --git a/src/server/database/db_handle_provider.hh b/src/server/database/db_handle_provider.hh index 7795732..f5df1e3 100644 --- a/src/server/database/db_handle_provider.hh +++ b/src/server/database/db_handle_provider.hh @@ -22,7 +22,7 @@ #include #include -#include +#include #include #include #include @@ -45,6 +45,7 @@ class EXPORT_API DBHandleProvider { ~DBHandleProvider() = default; static DBHandleProvider& GetInst(uid_t uid); static bool IsCrashedWriteRequest(); + static bool IsWriter(pid_t pid); std::vector> GetParserDBPath(int pid, bool write); std::string GetCertDBPath(int pid, bool write); @@ -53,13 +54,12 @@ class EXPORT_API DBHandleProvider { int UpdateCache(sqlite3* db, pid_t pid, uid_t uid, bool write, const std::string& locale); std::vector> GetPackages( - pid_t pid, bool write, pkgmgrinfo_filter_x* filter, + pid_t pid, pkgmgrinfo_filter_x* filter, const std::string& package); std::vector> GetApplications( - pid_t pid, bool write, pkgmgrinfo_filter_x* filter, + pid_t pid, pkgmgrinfo_filter_x* filter, const std::string& app); void TrimCache(); - bool IsWriter(pid_t pid); private: explicit DBHandleProvider(uid_t uid); @@ -84,7 +84,7 @@ class EXPORT_API DBHandleProvider { static std::string cert_filedb_path_; static std::unordered_set writer_pid_list_; static std::recursive_mutex lock_; - static std::mutex pid_list_lock_; + static std::shared_mutex pid_list_lock_; uid_t uid_; bool is_user_memdb_set_; diff --git a/src/server/database/depinfo_db_handler.cc b/src/server/database/depinfo_db_handler.cc index 98d6b7e..3362a19 100644 --- a/src/server/database/depinfo_db_handler.cc +++ b/src/server/database/depinfo_db_handler.cc @@ -41,7 +41,7 @@ void DepInfoGetDBHandler::SetPkgID(const std::string& pkgid) { } int DepInfoGetDBHandler::Execute() { - std::shared_lock s(lock_); + std::shared_lock s(lock_); SetOpType(pkgmgr_common::DBOperationType::OPERATION_TYPE_READ); SetDBType(pkgmgr_common::DBType::DB_TYPE_FILE_PKGDB); diff --git a/src/server/database/pkg_get_db_handler.cc b/src/server/database/pkg_get_db_handler.cc index 9438dec..78cd3da 100644 --- a/src/server/database/pkg_get_db_handler.cc +++ b/src/server/database/pkg_get_db_handler.cc @@ -37,6 +37,15 @@ gboolean _move_func(gpointer key, gpointer value, gpointer user_data) { return true; } +uid_t globaluser_uid = -1; + +uid_t GetGlobalUID() { + if (globaluser_uid == (uid_t)-1) + globaluser_uid = tzplatform_getuid(TZ_SYS_GLOBALAPP_USER); + + return globaluser_uid; +} + } // namespace namespace pkgmgr_server { @@ -55,8 +64,13 @@ void PkgGetDBHandler::SetFilter(pkgmgrinfo_filter_x* filter) { filter_ = filter; } -int PkgGetDBHandler::GetHandleFromDB( - std::vector>& conn_list) { +int PkgGetDBHandler::GetHandleFromDB() { + if (!Connect()) { + LOG(ERROR) << "Failed to connect database"; + return PMINFO_R_ERROR; + } + + std::vector> conn_list = GetConnection(); GHashTable* list = g_hash_table_new_full(g_str_hash, g_str_equal, nullptr, nullptr); int ret = PMINFO_R_OK; @@ -80,9 +94,17 @@ int PkgGetDBHandler::GetHandleFromDB( return ret; } -int PkgGetDBHandler::GetHandleFromCache( - std::vector>& conn_list) { - int ret = PMINFO_R_OK; +void PkgGetDBHandler::GetPackageFromCache(uid_t uid, + const std::string& package) { + std::vector> pkg_list = + DBHandleProvider::GetInst(uid).GetPackages(GetPID(), filter_, package); + + handle_list_.reserve(pkg_list.size() + handle_list_.size()); + std::move(std::begin(pkg_list), std::end(pkg_list), + std::back_inserter(handle_list_)); +} + +int PkgGetDBHandler::GetHandleFromCache() { std::string package; for (auto* it = filter_->list; it != nullptr; it = g_slist_next(it)) { @@ -93,50 +115,34 @@ int PkgGetDBHandler::GetHandleFromCache( } } - std::vector> pkg_list; - for (auto& conn : conn_list) { - pkg_list = DBHandleProvider::GetInst(conn.second) - .GetPackages(GetPID(), false, filter_, package); + if (uid_ > REGULAR_USER) + GetPackageFromCache(uid_, package); - handle_list_.reserve(pkg_list.size() + handle_list_.size()); - std::move(std::begin(pkg_list), std::end(pkg_list), - std::back_inserter(handle_list_)); - } + GetPackageFromCache(GetGlobalUID(), package); if (handle_list_.empty()) return PMINFO_R_ENOENT; - return ret; + return PMINFO_R_OK; } int PkgGetDBHandler::Execute() { - std::shared_lock s(lock_); + std::shared_lock s(lock_); SetOpType(pkgmgr_common::DBOperationType::OPERATION_TYPE_READ); SetDBType(pkgmgr_common::DBType::DB_TYPE_FILE_PKGDB); - if (!Connect()) { - LOG(ERROR) << "Failed to connect database"; - return PMINFO_R_ERROR; - } - - std::vector> conn_list = GetConnection(); - bool is_writer = false; - for (auto& conn : conn_list) { - if (DBHandleProvider::GetInst(conn.second).IsWriter(GetPID())) - is_writer = true; - } - if (is_writer) - return GetHandleFromDB(conn_list); + if (DBHandleProvider::IsWriter(GetPID())) + return GetHandleFromDB(); if (uid_ <= GetDefaultUser() && CacheFlag::GetStatus() == CacheFlag::Status::PREPARED) { auto cache_lock = CacheFlag::GetReaderLock(); if (cache_lock.try_lock() && CacheFlag::GetStatus() == CacheFlag::Status::PREPARED) - return GetHandleFromCache(conn_list); + return GetHandleFromCache(); } - return GetHandleFromDB(conn_list); + return GetHandleFromDB(); } } // namespace database diff --git a/src/server/database/pkg_get_db_handler.hh b/src/server/database/pkg_get_db_handler.hh index ba16187..2a738dd 100644 --- a/src/server/database/pkg_get_db_handler.hh +++ b/src/server/database/pkg_get_db_handler.hh @@ -41,8 +41,9 @@ class EXPORT_API PkgGetDBHandler : public AbstractDBHandler { virtual int Execute(); protected: - int GetHandleFromDB(std::vector>& conn_list); - int GetHandleFromCache(std::vector>& conn_list); + int GetHandleFromDB(); + int GetHandleFromCache(); + void GetPackageFromCache(uid_t uid, const std::string& package); pkgmgrinfo_filter_x* filter_ = nullptr; std::vector> handle_list_; uid_t uid_; diff --git a/src/server/database/pkg_set_db_handler.cc b/src/server/database/pkg_set_db_handler.cc index f6840d7..9c8d0b9 100644 --- a/src/server/database/pkg_set_db_handler.cc +++ b/src/server/database/pkg_set_db_handler.cc @@ -49,7 +49,7 @@ std::vector> PkgSetDBHandler::GetResult() { } int PkgSetDBHandler::Execute() { - std::unique_lock u(lock_); + std::unique_lock u(lock_); SetOpType(pkgmgr_common::DBOperationType::OPERATION_TYPE_WRITE); SetDBType(pkgmgr_common::DBType::DB_TYPE_FILE_PKGDB); diff --git a/src/server/database/query_handler.cc b/src/server/database/query_handler.cc index d450751..9d3624f 100644 --- a/src/server/database/query_handler.cc +++ b/src/server/database/query_handler.cc @@ -220,7 +220,7 @@ std::vector QueryHandler::GetResult() { } int QueryHandler::Execute() { - std::shared_lock s(lock_); + std::shared_lock s(lock_); if (!Connect()) { LOG(ERROR) << "Failed to connect database"; return PMINFO_R_ERROR; diff --git a/src/server/database/remove_cache_db_handler.cc b/src/server/database/remove_cache_db_handler.cc index 3ce37b5..d4ca31a 100644 --- a/src/server/database/remove_cache_db_handler.cc +++ b/src/server/database/remove_cache_db_handler.cc @@ -25,7 +25,7 @@ RemoveCacheDBHandler::RemoveCacheDBHandler(uid_t uid, int pid) : AbstractDBHandler(uid, pid) {} int RemoveCacheDBHandler::Execute() { - std::unique_lock u(lock_); + std::unique_lock u(lock_); database::DBHandleProvider::GetInst(GetUID()).UnsetMemoryMode(GetPID()); database::DBHandleProvider::GetInst(GetUID()).TrimCache(); -- 2.7.4