From 783787063232cd167247b9dd3fdd4ca6444cc7df Mon Sep 17 00:00:00 2001 From: Jake Fevold Date: Fri, 26 Apr 2024 09:28:45 -0400 Subject: [PATCH] Ball log set category tsan race drqs 171004031 (#4560) Fix TSAN failure with ball categories and refactor parts of ball_category.h --- groups/bal/ball/ball_category.cpp | 30 ++- groups/bal/ball/ball_category.h | 69 ++++-- groups/bal/ball/ball_categorymanager.cpp | 237 +++++++++++---------- groups/bal/ball/ball_categorymanager.h | 67 +++--- groups/bal/ball/ball_categorymanager.t.cpp | 138 ++++++++++-- groups/bal/ball/ball_ruleset.cpp | 11 +- 6 files changed, 371 insertions(+), 181 deletions(-) diff --git a/groups/bal/ball/ball_category.cpp b/groups/bal/ball/ball_category.cpp index e04d7217ef..b0072b2568 100644 --- a/groups/bal/ball/ball_category.cpp +++ b/groups/bal/ball/ball_category.cpp @@ -11,6 +11,8 @@ BSLS_IDENT_RCSID(ball_category_cpp,"$Id$ $CSID$") #include #include +#include + #include #include @@ -44,9 +46,10 @@ Category::Category(const char *categoryName, triggerLevel, triggerAllLevel)) , d_categoryName(categoryName, basicAllocator) -, d_categoryHolder(0) +, d_categoryHolder_p(0) , d_relevantRuleMask() , d_ruleThreshold(0) +, d_mutex() { BSLS_ASSERT(categoryName); bsls::AtomicOperations::initUint(&d_relevantRuleMask, 0); @@ -58,31 +61,39 @@ Category::linkCategoryHolder(CategoryHolder *categoryHolder) { BSLS_ASSERT(categoryHolder); + bslmt::LockGuard guard(&d_mutex); if (!categoryHolder->category()) { - categoryHolder->setThreshold(bsl::max(d_threshold, d_ruleThreshold)); + categoryHolder->setThreshold(bsl::max(d_threshold.loadRelaxed(), + d_ruleThreshold)); categoryHolder->setCategory(this); - categoryHolder->setNext(d_categoryHolder); - d_categoryHolder = categoryHolder; + categoryHolder->setNext(d_categoryHolder_p); + d_categoryHolder_p = categoryHolder; } } void Category::resetCategoryHolders() { - CategoryHolder *holder = d_categoryHolder; + bslmt::LockGuard guard(&d_mutex); + + CategoryHolder *holder = d_categoryHolder_p; + while (holder) { CategoryHolder *nextHolder = holder->next(); holder->reset(); holder = nextHolder; } - d_categoryHolder = 0; + d_categoryHolder_p = 0; } // CLASS METHODS void Category::updateThresholdForHolders() { - if (d_categoryHolder) { - CategoryHolder *holder = d_categoryHolder; - const int threshold = bsl::max(d_threshold, d_ruleThreshold); + BSLMT_MUTEXASSERT_IS_LOCKED(&d_mutex); + + if (d_categoryHolder_p) { + CategoryHolder *holder = d_categoryHolder_p; + const int threshold = bsl::max(d_threshold.loadRelaxed(), + d_ruleThreshold); if (threshold != holder->threshold()) { do { holder->setThreshold(threshold); @@ -102,6 +113,7 @@ int Category::setLevels(int recordLevel, passLevel, triggerLevel, triggerAllLevel)) { + bslmt::LockGuard guard(&d_mutex); d_thresholdLevels.setLevels(recordLevel, passLevel, diff --git a/groups/bal/ball/ball_category.h b/groups/bal/ball/ball_category.h index 167e68c737..0c511eddc1 100644 --- a/groups/bal/ball/ball_category.h +++ b/groups/bal/ball/ball_category.h @@ -106,7 +106,11 @@ BSLS_IDENT("$Id: $") #include +#include +#include + #include +#include #include #include #include @@ -134,23 +138,29 @@ class Category { // logging system. // DATA - ThresholdAggregate d_thresholdLevels; // record, pass, trigger, and - // trigger-all levels + ThresholdAggregate d_thresholdLevels; // record, pass, trigger, and + // trigger-all levels + + bsls::AtomicInt d_threshold; // numerical maximum of the four + // levels - int d_threshold; // numerical maximum of the four - // levels + const bsl::string d_categoryName; // category name - bsl::string d_categoryName; // category name + CategoryHolder *d_categoryHolder_p; // linked list of holders of this + // category - CategoryHolder *d_categoryHolder; // linked list of holders of this - // category mutable bsls::AtomicOperations::AtomicTypes::Uint - d_relevantRuleMask; // the mask indicating which rules - // are relevant (i.e., have been - // attached to this category) + d_relevantRuleMask; // the mask indicating which + // rules are relevant (i.e., have + // been attached to this + // category) - mutable int d_ruleThreshold; // numerical maximum of all four - // levels for all relevant rules + mutable int d_ruleThreshold; // numerical maximum of all four + // levels for all relevant rules + + mutable bslmt::Mutex d_mutex; // mutex providing mutually + // exclusive access to all + // non-atomic members // FRIENDS friend class CategoryManagerImpUtil; @@ -175,7 +185,8 @@ class Category { void updateThresholdForHolders(); // Update the threshold of all category holders that hold the address // of this object to the maximum of 'd_threshold' and - // 'd_ruleThreshold'. + // 'd_ruleThreshold'. The behavior is undefined unless 'd_mutex' is + // locked. public: // CLASS METHODS @@ -221,6 +232,8 @@ class Category { // otherwise (with no effect on the threshold levels of this category). // ACCESSORS + // BDE_VERIFY pragma: push + // BDE_VERIFY pragma: -FABC01: Functions not in alphanumeric order const char *categoryName() const; // Return the name of this category. @@ -245,7 +258,7 @@ class Category { int triggerAllLevel() const; // Return the trigger-all level of this category. - const ThresholdAggregate& thresholdLevels() const; + ThresholdAggregate thresholdLevels() const; // Return the aggregate threshold levels of this category. int threshold() const; @@ -271,6 +284,7 @@ class Category { // owns this category) applies at this category. Note that a rule // applies to this category if the rule's pattern matches the name // returned by 'categoryName'. + // BDE_VERIFY pragma: pop }; // ==================== @@ -296,6 +310,7 @@ class CategoryHolder { // NOT IMPLEMENTED CategoryHolder& operator=(const CategoryHolder&) BSLS_KEYWORD_DELETED; + // PRIVATE TYPES typedef bsls::AtomicOperations AtomicOps; typedef bsls::AtomicOperations::AtomicTypes::Int AtomicInt; typedef bsls::AtomicOperations::AtomicTypes::Pointer AtomicPointer; @@ -317,9 +332,12 @@ class CategoryHolder { // the range '[0 .. 255]'. // PUBLIC DATA + // BDE_VERIFY pragma: push + // BDE_VERIFY pragma: -MN01 // Class data members must be private AtomicInt d_threshold; // threshold level AtomicPointer d_category_p; // held category (not owned) AtomicPointer d_next_p; // next category holder in linked list + // BDE_VERIFY pragma: pop // CREATORS @@ -327,6 +345,8 @@ class CategoryHolder { // initialization of instances of this class. // MANIPULATORS + // BDE_VERIFY pragma: push + // BDE_VERIFY pragma: -FABC01: Functions not in alphanumeric order void reset(); // Reset this object to its default value. The default value is: //.. @@ -354,6 +374,7 @@ class CategoryHolder { CategoryHolder *next() const; // Return the address of the modifiable holder held by this holder. + // BDE_VERIFY pragma: pop }; // ============================ @@ -368,7 +389,9 @@ class CategoryManagerImpUtil { // implementation detail of the 'ball' logging system. public: - // PRIVATE MANIPULATORS + // CLASS METHODS + // BDE_VERIFY pragma: push + // BDE_VERIFY pragma: -FABC01: Functions not in alphanumeric order static void linkCategoryHolder(Category *category, CategoryHolder *categoryHolder); // Load the specified 'category' and its corresponding 'maxLevel()' @@ -402,6 +425,7 @@ class CategoryManagerImpUtil { RuleSet::MaskType mask); // Set the rule-mask for the specified 'category' to the specified // 'mask'. + // BDE_VERIFY pragma: pop }; // ============================================================================ @@ -412,6 +436,9 @@ class CategoryManagerImpUtil { // class Category // -------------- +// BDE_VERIFY pragma: push +// BDE_VERIFY pragma: -FABC01: Functions not in alphanumeric order + // CLASS METHODS inline bool Category::areValidThresholdLevels(int recordLevel, @@ -447,30 +474,35 @@ int Category::maxLevel() const inline int Category::recordLevel() const { + bslmt::LockGuard guard(&d_mutex); return d_thresholdLevels.recordLevel(); } inline int Category::passLevel() const { + bslmt::LockGuard guard(&d_mutex); return d_thresholdLevels.passLevel(); } inline int Category::triggerLevel() const { + bslmt::LockGuard guard(&d_mutex); return d_thresholdLevels.triggerLevel(); } inline int Category::triggerAllLevel() const { + bslmt::LockGuard guard(&d_mutex); return d_thresholdLevels.triggerAllLevel(); } inline -const ThresholdAggregate& Category::thresholdLevels() const +ThresholdAggregate Category::thresholdLevels() const { + bslmt::LockGuard guard(&d_mutex); return d_thresholdLevels; } @@ -483,6 +515,7 @@ int Category::threshold() const inline int Category::ruleThreshold() const { + bslmt::LockGuard guard(&d_mutex); return d_ruleThreshold; } @@ -564,6 +597,7 @@ void CategoryManagerImpUtil::updateThresholdForHolders(Category *category) { BSLS_ASSERT(category); + bslmt::LockGuard guard(&category->d_mutex); category->updateThresholdForHolders(); } @@ -571,6 +605,7 @@ inline void CategoryManagerImpUtil::setRuleThreshold(Category *category, int ruleThreshold) { + bslmt::LockGuard guard(&category->d_mutex); category->d_ruleThreshold = ruleThreshold; } @@ -616,6 +651,8 @@ void CategoryManagerImpUtil::setRelevantRuleMask(Category *category, mask); } +// BDE_VERIFY pragma: pop + } // close package namespace } // close enterprise namespace diff --git a/groups/bal/ball/ball_categorymanager.cpp b/groups/bal/ball/ball_categorymanager.cpp index 391b869045..599213647d 100644 --- a/groups/bal/ball/ball_categorymanager.cpp +++ b/groups/bal/ball/ball_categorymanager.cpp @@ -7,6 +7,8 @@ BSLS_IDENT_RCSID(ball_categorymanager_cpp,"$Id$ $CSID$") #include #include +#include + #include #include @@ -75,13 +77,19 @@ class CategoryProctor { // Rollback the owned objects to their initial state on failure. // MANIPULATORS + + // BDE_VERIFY pragma: push + // BDE_VERIFY pragma: -FABC01: Functions not in alphanumeric order + void setCategories(CategoryVector *categories); - // Take ownership of the 'categories' object to roll it back on - // failure. + // Take ownership of the specified 'categories' object to roll it back + // on failure. void release(); // Release the ownership of all objects currently managed by this // proctor. + + // BDE_VERIFY pragma: pop }; // --------------------- @@ -112,6 +120,10 @@ CategoryProctor::~CategoryProctor() } // MANIPULATORS + +// BDE_VERIFY pragma: push +// BDE_VERIFY pragma: -FABC01: Functions not in alphanumeric order + inline void CategoryProctor::setCategories(CategoryVector *categories) { @@ -125,6 +137,8 @@ void CategoryProctor::release() d_categories_p = 0; } +// BDE_VERIFY pragma: pop + } // close unnamed namespace // --------------------- @@ -146,6 +160,7 @@ Category *CategoryManager::addNewCategory(const char *categoryName, triggerLevel, triggerAllLevel, d_allocator_p); + CategoryProctor proctor(category, d_allocator_p); // rollback on exception d_categories.push_back(category); @@ -158,6 +173,71 @@ Category *CategoryManager::addNewCategory(const char *categoryName, return category; } +void CategoryManager::privateApplyRulesToCategory(Category* category) +{ + RuleSet::MaskType mask = 0; + int threshold = 0; + for (int i = 0; i < RuleSet::maxNumRules(); ++i) { + const Rule *rule = d_ruleSet.getRuleById(i); + if (rule && rule->isMatch(category->categoryName())) { + mask = bdlb::BitUtil::withBitSet(mask, i); + threshold = bsl::max(threshold, + ThresholdAggregate::maxLevel( + rule->recordLevel(), + rule->passLevel(), + rule->triggerLevel(), + rule->triggerAllLevel())); + } + } + CategoryManagerImpUtil::setRelevantRuleMask(category, mask); + if (threshold != category->ruleThreshold()) { + CategoryManagerImpUtil::setRuleThreshold(category, threshold); + CategoryManagerImpUtil::updateThresholdForHolders(category); + } +} + +void CategoryManager::privateApplyRulesToAllCategories( + bslmt::LockGuard& ruleGuard) +{ + typedef bsls::Types::Int64 Int64; + BSLMT_MUTEXASSERT_IS_LOCKED(&d_ruleSetMutex); + BSLS_ASSERT(ruleGuard.ptr() == &d_ruleSetMutex); + bsls::Types::Int64 localRulesetSequenceNumber = ++d_ruleSetSequenceNumber; + ruleGuard.release()->unlock(); + + const Int64 k_BATCH_SIZE = 4096; + Int64 offset = 0; + Int64 batch; + bool done = false; + + bsl::vector cachedCategories; + while (!done) { + cachedCategories.clear(); + { + bslmt::ReadLockGuard guard( + &d_registryLock); + Int64 remaining = d_categories.size() - offset; + batch = std::min(k_BATCH_SIZE, remaining); + done = (remaining == batch); + cachedCategories.assign(d_categories.begin() + offset, + d_categories.begin() + offset + batch); + } + { + bslmt::LockGuard guard(&d_ruleSetMutex); + if (localRulesetSequenceNumber != d_ruleSetSequenceNumber) { + // Another thread is applying newer rules. + return; // RETURN + } + for (bsl::vector::iterator iter = + cachedCategories.begin(); + iter != cachedCategories.end(); + ++iter) { + privateApplyRulesToCategory(*iter); + } + } + } +} + // CREATORS CategoryManager::CategoryManager(bslma::Allocator *basicAllocator) : d_registry(1, bdlb::CStringHash(), bdlb::CStringEqualTo(), basicAllocator) @@ -180,6 +260,10 @@ CategoryManager::~CategoryManager() } // MANIPULATORS + +// BDE_VERIFY pragma: push +// BDE_VERIFY pragma: -FABC01: Functions not in alphanumeric order + Category *CategoryManager::addCategory(const char *categoryName, int recordLevel, int passLevel, @@ -233,26 +317,12 @@ Category *CategoryManager::addCategory(CategoryHolder *categoryHolder, bslmt::LockGuard ruleSetGuard(&d_ruleSetMutex); - for (int i = 0; i < RuleSet::maxNumRules(); ++i) { - const Rule *rule = d_ruleSet.getRuleById(i); - if (rule && rule->isMatch(category->categoryName())) { - CategoryManagerImpUtil::enableRule(category, i); - int threshold = ThresholdAggregate::maxLevel( - rule->recordLevel(), - rule->passLevel(), - rule->triggerLevel(), - rule->triggerAllLevel()); - - if (threshold > category->ruleThreshold()) { - CategoryManagerImpUtil::setRuleThreshold(category, - threshold); - } - } - } + privateApplyRulesToCategory(category); // We have a write lock on 'd_registryLock', so the supplied category // holder is the only holder for the created category. + // Despite the above comment, no such lock is held. if (categoryHolder) { categoryHolder->setThreshold(bsl::max(category->threshold(), category->ruleThreshold())); @@ -279,8 +349,8 @@ Category *CategoryManager::lookupCategory(CategoryHolder *categoryHolder, bslmt::WriteLockGuard registryGuard( &d_registryLock, 1); - Category *category = 0; - CategoryMap::const_iterator iter = d_registry.find(categoryName); + Category *category = 0; + CategoryMap::const_iterator iter = d_registry.find(categoryName); if (iter != d_registry.end()) { category = d_categories[iter->second]; if (categoryHolder && !categoryHolder->category()) { @@ -322,6 +392,7 @@ Category *CategoryManager::setThresholdLevels(const char *categoryName, d_registryLock.lockReadReserveWrite(); bslmt::WriteLockGuard registryGuard( &d_registryLock, 1); + CategoryMap::iterator iter = d_registry.find(categoryName); if (iter != d_registry.end()) { Category *category = d_categories[iter->second]; @@ -344,128 +415,62 @@ Category *CategoryManager::setThresholdLevels(const char *categoryName, d_registryLock.unlock(); bslmt::LockGuard ruleSetGuard(&d_ruleSetMutex); - for (int i = 0; i < RuleSet::maxNumRules(); ++i) { - const Rule *rule = d_ruleSet.getRuleById(i); - if (rule && rule->isMatch(category->categoryName())) { - CategoryManagerImpUtil::enableRule(category, i); - int threshold = ThresholdAggregate::maxLevel( - rule->recordLevel(), - rule->passLevel(), - rule->triggerLevel(), - rule->triggerAllLevel()); - if (threshold > category->ruleThreshold()) { - CategoryManagerImpUtil::setRuleThreshold(category, - threshold); - } - } - } - - // No need to update holders since the category was just newly created - // and thus does not have any linked holders. + privateApplyRulesToCategory(category); return category; // RETURN } } -int CategoryManager::addRule(const Rule& value) +int CategoryManager::addRule(const Rule& ruleToAdd) { bslmt::LockGuard guard(&d_ruleSetMutex); - const int ruleId = d_ruleSet.addRule(value); + const int ruleId = d_ruleSet.addRule(ruleToAdd); if (ruleId < 0) { return 0; // RETURN } - ++d_ruleSetSequenceNumber; + privateApplyRulesToAllCategories(guard); - const Rule *rule = d_ruleSet.getRuleById(ruleId); - - for (int i = 0; i < length(); ++i) { - Category *category = d_categories[i]; - if (rule->isMatch(category->categoryName())) { - CategoryManagerImpUtil::enableRule(category, ruleId); - int threshold = ThresholdAggregate::maxLevel( - rule->recordLevel(), - rule->passLevel(), - rule->triggerLevel(), - rule->triggerAllLevel()); - if (threshold > category->ruleThreshold()) { - CategoryManagerImpUtil::setRuleThreshold(category, threshold); - CategoryManagerImpUtil::updateThresholdForHolders(category); - } - } - } return 1; } int CategoryManager::addRules(const RuleSet& ruleSet) { - int count = 0; - for (int i = 0; i < ruleSet.maxNumRules(); ++i) { - const Rule *rule = ruleSet.getRuleById(i); - if (rule) { - count += addRule(*rule); - } + bslmt::LockGuard guard(&d_ruleSetMutex); + + int count = d_ruleSet.addRules(ruleSet); + + if (count) { + privateApplyRulesToAllCategories(guard); } + return count; } -int CategoryManager::removeRule(const Rule& value) +int CategoryManager::removeRule(const Rule& ruleToRemove) { bslmt::LockGuard guard(&d_ruleSetMutex); - const int ruleId = d_ruleSet.ruleId(value); - if (ruleId < 0) { - return 0; // RETURN - } - - ++d_ruleSetSequenceNumber; - - const Rule *rule = d_ruleSet.getRuleById(ruleId); + int count = d_ruleSet.removeRule(ruleToRemove); - for (int i = 0; i < length(); ++i) { - Category *category = d_categories[i]; - if (rule->isMatch(category->categoryName())) { - CategoryManagerImpUtil::disableRule(category, ruleId); - CategoryManagerImpUtil::setRuleThreshold(category, 0); - - RuleSet::MaskType relevantRuleMask = category->relevantRuleMask(); - - int j; - - while ((j = bdlb::BitUtil::numTrailingUnsetBits(relevantRuleMask)) - != RuleSet::e_MAX_NUM_RULES) { - relevantRuleMask = - bdlb::BitUtil::withBitCleared(relevantRuleMask, j); - - const Rule *r = d_ruleSet.getRuleById(j); - int threshold = ThresholdAggregate::maxLevel( - r->recordLevel(), - r->passLevel(), - r->triggerLevel(), - r->triggerAllLevel()); - if (threshold > category->ruleThreshold()) { - CategoryManagerImpUtil::setRuleThreshold(category, - threshold); - } - } - CategoryManagerImpUtil::updateThresholdForHolders(category); - } + if (count) { + privateApplyRulesToAllCategories(guard); } - d_ruleSet.removeRuleById(ruleId); - return 1; + return count; } int CategoryManager::removeRules(const RuleSet& ruleSet) { - int count = 0; - for (int i = 0; i < ruleSet.maxNumRules(); ++i) { - const Rule *rule = ruleSet.getRuleById(i); - if (rule) { - count += removeRule(*rule); - } + bslmt::LockGuard guard(&d_ruleSetMutex); + + int count = d_ruleSet.removeRules(ruleSet); + + if (count) { + privateApplyRulesToAllCategories(guard); } + return count; } @@ -473,18 +478,16 @@ void CategoryManager::removeAllRules() { bslmt::LockGuard guard(&d_ruleSetMutex); - ++d_ruleSetSequenceNumber; + int count = d_ruleSet.numRules(); - for (int i = 0; i < length(); ++i) { - if (d_categories[i]->relevantRuleMask()) { - CategoryManagerImpUtil::setRelevantRuleMask(d_categories[i], 0); - CategoryManagerImpUtil::setRuleThreshold(d_categories[i], 0); - CategoryManagerImpUtil::updateThresholdForHolders(d_categories[i]); - } + if (count) { + d_ruleSet.removeAllRules(); + privateApplyRulesToAllCategories(guard); } - d_ruleSet.removeAllRules(); } +// BDE_VERIFY pragma: pop + // ACCESSORS const Category *CategoryManager::lookupCategory(const char *categoryName) const { diff --git a/groups/bal/ball/ball_categorymanager.h b/groups/bal/ball/ball_categorymanager.h index a8dbbc4948..5b5d66fb0a 100644 --- a/groups/bal/ball/ball_categorymanager.h +++ b/groups/bal/ball/ball_categorymanager.h @@ -15,12 +15,12 @@ BSLS_IDENT("$Id: $") //@DESCRIPTION: This component provides a registry for category information and // functions to manage the registry and its members. By "category" we mean a // named entity that identifies a region or functional area of a program. A -// category name can be an arbitrary string, including the empty string. -// Note that category names are case-sensitive. +// category name can be an arbitrary string, including the empty string. Note +// that category names are case-sensitive. // // Associated with each category, besides its name, are four threshold levels -// known as "record", "pass", "trigger", and "trigger-all". Threshold -// levels are values in the range '[0 .. 255]'. (See the 'ball_loggermanager' +// known as "record", "pass", "trigger", and "trigger-all". Threshold levels +// are values in the range '[0 .. 255]'. (See the 'ball_loggermanager' // component-level documentation for a typical interpretation of these four // thresholds.) // @@ -96,8 +96,8 @@ BSLS_IDENT("$Id: $") // [ EQUITY.GRAPHICS.MATH.ACKERMANN, 195, 99, 67, 35 ] //.. // We next use the 'setLevels' method of 'ball::Category' to adjust the -// threshold levels of our categories. The following also demonstrates use -// of the 'recordLevel', etc., accessors of 'ball::Category': +// threshold levels of our categories. The following also demonstrates use of +// the 'recordLevel', etc., accessors of 'ball::Category': //.. // for (int i = 0; i < NUM_CATEGORIES; ++i) { // ball::Category *category = manager.lookupCategory(myCategories[i]); @@ -187,7 +187,7 @@ class CategoryManager { // threshold levels of existing categories may be accessed and modified // directly. - // TYPES + // PRIVATE TYPES typedef bsl::unordered_map& ruleGuard); + // Apply all rules in the rule set to all categories. Use the + // specified 'ruleGuard' to provide synchronization. The behavior is + // undefined unless 'ruleGuard' holds a lock on 'd_ruleSetMutex'. Note + // that all methods that modify the rule set must lock 'd_ruleSetMutex' + // to perform that modification, so passing the guard avoids needlessly + // unlocking and re-locking the mutex. + public: + // BDE_VERIFY pragma: push + // BDE_VERIFY pragma: -FABC01 // Functions not in alphanumeric order + // CREATORS explicit CategoryManager(bslma::Allocator *basicAllocator = 0); // Create a category manager. Optionally specify a 'basicAllocator' @@ -324,21 +341,21 @@ class CategoryManager { // 'triggerAllLevel' values, respectively, if a category having // 'categoryName' exists and each of the specified threshold values is // in the range '[0 .. 255]'. Otherwise, add to the registry a - // category having the specified 'categoryName' and the specified - // 'recordLevel', 'passLevel', 'triggerLevel', and 'triggerAllLevel' - // threshold values, respectively, if there is no category having - // 'categoryName' and each of the specified threshold values is in the - // range '[0 .. 255]'. Return the address of the (possibly - // newly-created) modifiable category on success, and 0 otherwise (with - // no effect on any category). The behavior is undefined unless a lock - // is not held by this thread on the mutex returned by 'rulesetMutex'. - - int addRule(const Rule& rule); - // Add the specified 'rule' to the set of (unique) rules maintained by - // this object. Return the number of rules added (i.e., 1 on success - // and 0 if a rule with the same value is already present). The - // behavior is undefined unless a lock is not held by this thread on - // the mutex returned by 'rulesetMutex'. + // category having 'categoryName' and 'recordLevel', 'passLevel', + // 'triggerLevel', and 'triggerAllLevel' threshold values, + // respectively, if there is no category having 'categoryName' and each + // of the specified threshold values is in the range '[0 .. 255]'. + // Return the address of the (possibly newly-created) modifiable + // category on success, and 0 otherwise (with no effect on any + // category). The behavior is undefined unless a lock is not held by + // this thread on the mutex returned by 'rulesetMutex'. + + int addRule(const Rule& ruleToAdd); + // Add the specified 'ruleToAdd' to the set of (unique) rules + // maintained by // this object. Return the number of rules added + // (i.e., 1 on success and 0 if a rule with the same value is already + // present). The behavior is undefined unless a lock is not held by + // this thread on the mutex returned by 'rulesetMutex'. int addRules(const RuleSet& ruleSet); // Add each rule in the specified 'ruleSet' to the set of (unique) @@ -347,8 +364,8 @@ class CategoryManager { // on the mutex returned by 'rulesetMutex'. Note that each rule having // the same value as an existing rule will be ignored. - int removeRule(const Rule& rule); - // Remove the specified 'rule' from the set of (unique) rules + int removeRule(const Rule& ruleToRemove); + // Remove the specified 'ruleToRemove' from the set of (unique) rules // maintained by this object. Return the number of rules removed // (i.e., 1 on success and 0 if no rule having the same value is // found). The behavior is undefined unless a lock is not held by this @@ -418,6 +435,8 @@ class CategoryManager { //.. // void operator()(const Category *); //.. + + // BDE_VERIFY pragma: pop }; #ifndef BDE_OMIT_INTERNAL_DEPRECATED diff --git a/groups/bal/ball/ball_categorymanager.t.cpp b/groups/bal/ball/ball_categorymanager.t.cpp index ecb5a15a71..57b3e762cd 100644 --- a/groups/bal/ball/ball_categorymanager.t.cpp +++ b/groups/bal/ball/ball_categorymanager.t.cpp @@ -102,18 +102,18 @@ using namespace bsl; // #ifndef BDE_OMIT_INTERNAL_DEPRECATED // 'ball::CategoryManagerIter' public interface: -// [16] CategoryManagerIter(const CategoryManager& cm); -// [16] ~CategoryManagerIter(); -// [16] void operator++(); -// [16] operator const void *() const; -// [16] const Category& operator()() const; +// [17] CategoryManagerIter(const CategoryManager& cm); +// [17] ~CategoryManagerIter(); +// [17] void operator++(); +// [17] operator const void *() const; +// [17] const Category& operator()() const; // // 'ball::CategoryManagerManip' public interface: -// [17] CategoryManagerManip(CategoryManager *cm); -// [17] ~CategoryManagerManip(); -// [17] void advance(); -// [17] Category& operator()(); -// [17] operator const void *() const; +// [18] CategoryManagerManip(CategoryManager *cm); +// [18] ~CategoryManagerManip(); +// [18] void advance(); +// [18] Category& operator()(); +// [18] operator const void *() const; #endif // BDE_OMIT_INTERNAL_DEPRECATED //----------------------------------------------------------------------------- // [ 1] BREATHING TEST @@ -124,6 +124,7 @@ using namespace bsl; // [13] CONCURRENCY TEST: RULES // [14] UNIQUENESS OF INITIAL RULE SET SEQUENCE NUMBER // [15] USAGE EXAMPLE +// [16] DRQS 171004031 - TSAN test // ============================================================================ // STANDARD BDE ASSERT TEST FUNCTION @@ -480,6 +481,7 @@ extern "C" void *ruleThreadTest(void *args) ASSERT(0 == mX.addRule(rule3)); ASSERT(1 == mX.addRule(rule4)); ASSERT(0 == mX.addRule(rule4)); + barrier.wait(); mX.rulesetMutex().lock(); int ruleId1 = X.ruleSet().ruleId(rule1); @@ -514,6 +516,7 @@ extern "C" void *ruleThreadTest(void *args) mX.rulesetMutex().lock(); ruleId4 = X.ruleSet().ruleId(rule4); mX.rulesetMutex().unlock(); + barrier.wait(); const Entry *entry = mX.lookupCategory(US); ASSERT(bdlb::BitUtil::isBitSet(entry->relevantRuleMask(), ruleId4)); @@ -588,6 +591,95 @@ extern "C" void *seqNumUniquenessThread(void *args) } // close namespace BALL_CATEGORYMANAGER_UNIQUENESS_OF_SEQUENCE_NUMBERS +struct TestDrqs171004031Data { + bslmt::Barrier d_barrier; // encourage collisions by sync start + Obj d_mx; // Object under test + const char * const d_categoryName; // category name + + TestDrqs171004031Data(unsigned int threads, + const char *categoryName); + // Create a TestDrqs171004031Data object with a barrier to synchronize + // the start of the specified 'threads', and the specified + // 'categoryName'. +}; + +TestDrqs171004031Data::TestDrqs171004031Data(unsigned int threads, + const char *categoryName) +: d_barrier(threads) +, d_mx() +, d_categoryName(categoryName) +{ +} + +void *lookupCategoryFunction(void* void_p) + // After waiting for a barrier to encourage collisions, invoke + // 'lookupCategory' on the 'CategoryManager' referenced by the + // 'TestDrqs171004031Data' object pointed to by the specified 'void_p'. +{ + TestDrqs171004031Data &testData = + *(static_cast(void_p)); + Obj& mX = testData.d_mx; + static ball::CategoryHolder holder; + testData.d_barrier.wait(); + mX.lookupCategory(&holder, testData.d_categoryName); + return 0; +} + +void *addCategoryFunction(void* void_p) + // After waiting for a barrier to encourage collisions, invoke + // 'addCategory' on the 'CategoryManager' referenced by the + // 'TestDrqs171004031Data' object pointed to by the specified 'void_p'. +{ + TestDrqs171004031Data &testData = + *(static_cast(void_p)); + Obj& mX = testData.d_mx; + static ball::CategoryHolder holder; + testData.d_barrier.wait(); + mX.addCategory(&holder, testData.d_categoryName, 0, 0, 0, 0); + return 0; +} + +void testDrqs171004031(unsigned int addCategoryThreads, + unsigned int lookupCategoryThreads) + // Create test data and run the specified 'addCategoryThreads' number of + // threads adding categories and the specified 'lookupCategoryThreads' + // number of threads looking up categories. +{ + TestDrqs171004031Data testData(addCategoryThreads + lookupCategoryThreads, + "TSAN category"); + + bsl::vector handles; + handles.reserve(addCategoryThreads + lookupCategoryThreads); + + // There must be rules in the system to trigger the collision. + for (int rule = 0; rule < ball::RuleSet::maxNumRules(); ++rule) { + testData.d_mx.addRule(ball::Rule("TSAN*", + rule+2, + rule+2, + rule+2, + rule+2)); + } + + for (unsigned int add = 0; add < addCategoryThreads; ++add) { + handles.push_back(bslmt::ThreadUtil::Handle()); + bslmt::ThreadUtil::create(&(handles.back()), + addCategoryFunction, + static_cast(&testData)); + } + for (unsigned int lookup = 0; lookup < lookupCategoryThreads; ++lookup) { + handles.push_back(bslmt::ThreadUtil::Handle()); + bslmt::ThreadUtil::create(&(handles.back()), + lookupCategoryFunction, + static_cast(&testData)); + } + for (bsl::vector::iterator it = handles.begin(); + it != handles.end(); + ++it) + { + bslmt::ThreadUtil::join(*it, 0); + } +} + //============================================================================= // MAIN PROGRAM //----------------------------------------------------------------------------- @@ -606,7 +698,7 @@ int main(int argc, char *argv[]) switch (test) { case 0: // Zero is always the leading case. #ifndef BDE_OMIT_INTERNAL_DEPRECATED - case 17: { + case 18: { // -------------------------------------------------------------------- // TESTING 'ball::CategoryManagerManip' // @@ -680,7 +772,7 @@ int main(int argc, char *argv[]) ASSERT(triggerAll == p->triggerAllLevel()); } } break; - case 16: { + case 17: { // -------------------------------------------------------------------- // TESTING 'ball::CategoryManagerIter' // @@ -744,6 +836,28 @@ int main(int argc, char *argv[]) } break; #endif // BDE_OMIT_INTERNAL_DEPRECATED + case 16: { + // -------------------------------------------------------------------- + // DRQS 171004031 - TSAN test + // + // Concerns: + //: 1 DRQS 171004031 showed the presence of data races, those were + // confirmed by this test prior to fixing them, and can now be shown + // to be fixed by running this test under TSAN. + // + // Plan: + //: 1 Populate the rule set, then simultaneously add categories and + // look up categories. + // + // Testing: + // DRQS 171004031 - TSAN test + // -------------------------------------------------------------------- + + if (verbose) cout << endl << "DRQS 171004031 - TSAN TEST" << endl + << "==========================" << endl; + + testDrqs171004031(4, 4); + } break; case 15: { // -------------------------------------------------------------------- // TESTING USAGE EXAMPLE diff --git a/groups/bal/ball/ball_ruleset.cpp b/groups/bal/ball/ball_ruleset.cpp index 0f2cfd6e15..9428e0c74d 100644 --- a/groups/bal/ball/ball_ruleset.cpp +++ b/groups/bal/ball/ball_ruleset.cpp @@ -54,6 +54,8 @@ RuleSet::RuleSet(bslma::Allocator *basicAllocator) , d_freeRuleIds(basicAllocator) , d_numPredicates(0) { + d_ruleAddresses.reserve(maxNumRules()); + d_freeRuleIds.reserve(maxNumRules()); for (int i = 0; i < maxNumRules(); ++i) { d_ruleAddresses.push_back(0); d_freeRuleIds.push_back(i); @@ -68,6 +70,8 @@ RuleSet::RuleSet(const RuleSet& original, bslma::Allocator *basicAllocator) , d_ruleAddresses(basicAllocator) , d_freeRuleIds(basicAllocator) { + d_ruleAddresses.reserve(maxNumRules()); + d_freeRuleIds.reserve(maxNumRules()); for (int i = 0; i < maxNumRules(); ++i) { d_ruleAddresses.push_back(0); d_freeRuleIds.push_back(i); @@ -81,15 +85,16 @@ int RuleSet::addRule(const Rule& value) if (d_ruleHashtable.find(value) != d_ruleHashtable.end()) { return -1; // RETURN } - if ((int)d_ruleHashtable.size() >= maxNumRules()) { + if (static_cast(d_ruleHashtable.size()) >= maxNumRules()) { return -2; // RETURN } HashtableType::const_iterator iter = d_ruleHashtable.insert(value).first; + int ruleId = d_freeRuleIds.back(); d_freeRuleIds.pop_back(); d_ruleAddresses[ruleId] = &*iter; - d_numPredicates += value.numPredicates(); + d_numPredicates += value.numAttributes(); return ruleId; } @@ -125,7 +130,7 @@ int RuleSet::removeRuleById(int id) return 0; // RETURN } - d_numPredicates -= rule->numPredicates(); + d_numPredicates -= rule->numAttributes(); // Note that removing 'iter' from 'd_ruleHashTable' invalidates 'rule'. HashtableType::iterator iter = d_ruleHashtable.find(*rule);