TS-2296: improve ConfigProcessor reference counting Use the standard RefCountObj reference counting of ConfigInfo objects. Add regression tests so that we can understand how this is supposed to work better.
Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/6da424fa Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/6da424fa Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/6da424fa Branch: refs/heads/5.0.x Commit: 6da424faa886e12b658dbe426aad9d19512065c6 Parents: f760dac Author: James Peach <jpe...@apache.org> Authored: Mon Oct 7 16:08:36 2013 -0700 Committer: James Peach <jpe...@apache.org> Committed: Wed Oct 23 09:20:20 2013 -0700 ---------------------------------------------------------------------- CHANGES | 2 + lib/ts/Ptr.h | 2 + mgmt/ProxyConfig.cc | 115 ++++++++++++++++++++++++++++++++++++++++++----- mgmt/ProxyConfig.h | 18 ++++---- 4 files changed, 115 insertions(+), 22 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/trafficserver/blob/6da424fa/CHANGES ---------------------------------------------------------------------- diff --git a/CHANGES b/CHANGES index c66e53e..84b8e1b 100644 --- a/CHANGES +++ b/CHANGES @@ -2,6 +2,8 @@ Changes with Apache Traffic Server 4.1.0 + *) [TS-2296] improve ConfigProcessor reference counting + *) [ TS-2295] update statvfs usage *) [TS-2139] Fix PURGE twice to remove object in cache if enable-interim-cache http://git-wip-us.apache.org/repos/asf/trafficserver/blob/6da424fa/lib/ts/Ptr.h ---------------------------------------------------------------------- diff --git a/lib/ts/Ptr.h b/lib/ts/Ptr.h index c14759a..7cea36c 100644 --- a/lib/ts/Ptr.h +++ b/lib/ts/Ptr.h @@ -272,6 +272,7 @@ public: volatile int m_refcount; }; +// Increment the reference count, returning the new count. inline int RefCountObj::refcount_inc() { @@ -280,6 +281,7 @@ RefCountObj::refcount_inc() #define REF_COUNT_OBJ_REFCOUNT_INC(_x) (_x)->refcount_inc() +// Decrement the reference count, returning the new count. inline int RefCountObj::refcount_dec() { http://git-wip-us.apache.org/repos/asf/trafficserver/blob/6da424fa/mgmt/ProxyConfig.cc ---------------------------------------------------------------------- diff --git a/mgmt/ProxyConfig.cc b/mgmt/ProxyConfig.cc index e236897..2c6f7be 100644 --- a/mgmt/ProxyConfig.cc +++ b/mgmt/ProxyConfig.cc @@ -24,6 +24,7 @@ #include "libts.h" #include "ProxyConfig.h" #include "P_EventSystem.h" +#include "ts/TestBox.h" ConfigProcessor configProcessor; @@ -124,7 +125,7 @@ ConfigProcessor::ConfigProcessor() } unsigned int -ConfigProcessor::set(unsigned int id, ConfigInfo * info) +ConfigProcessor::set(unsigned int id, ConfigInfo * info, unsigned timeout_secs) { ConfigInfo *old_info; int idx; @@ -135,7 +136,13 @@ ConfigProcessor::set(unsigned int id, ConfigInfo * info) ink_assert(id <= MAX_CONFIGS); } - info->m_refcount = 1; + // Don't be an idiot and use a zero timeout ... + ink_assert(timeout_secs > 0); + + // New objects *must* start with a zero refcount. The config + // processor holds it's own refcount. We should be the only + // refcount holder at this point. + ink_release_assert(info->refcount_inc() == 1); if (id > MAX_CONFIGS) { // invalid index @@ -146,11 +153,14 @@ ConfigProcessor::set(unsigned int id, ConfigInfo * info) idx = id - 1; do { - old_info = (ConfigInfo *) infos[idx]; - } while (!ink_atomic_cas( & infos[idx], old_info, info)); + old_info = infos[idx]; + } while (!ink_atomic_cas(&infos[idx], old_info, info)); if (old_info) { - eventProcessor.schedule_in(NEW(new ConfigInfoReleaser(id, old_info)), HRTIME_SECONDS(60)); + // The ConfigInfoReleaser now takes our refcount, but + // someother thread might also have one ... + ink_assert(old_info->refcount() > 0); + eventProcessor.schedule_in(NEW(new ConfigInfoReleaser(id, old_info)), HRTIME_SECONDS(timeout_secs)); } return id; @@ -171,18 +181,17 @@ ConfigProcessor::get(unsigned int id) } idx = id - 1; - info = (ConfigInfo *) infos[idx]; - if (ink_atomic_increment((int *) &info->m_refcount, 1) < 0) { - ink_assert(!"not reached"); - } + info = infos[idx]; + // Hand out a refcount to the caller. We should still have out + // own refcount, so it should be at least 2. + ink_release_assert(info->refcount_inc() > 1); return info; } void ConfigProcessor::release(unsigned int id, ConfigInfo * info) { - int val; int idx; ink_assert(id != 0); @@ -194,9 +203,91 @@ ConfigProcessor::release(unsigned int id, ConfigInfo * info) } idx = id - 1; - val = ink_atomic_increment((int *) &info->m_refcount, -1); - if ((infos[idx] != info) && (val == 1)) { + if (info->refcount_dec() == 0) { + // When we release, we should already have replaced this object in the index. + ink_release_assert(info != this->infos[idx]); delete info; } } + +#if TS_HAS_TESTS + +enum { + REGRESSION_CONFIG_FIRST = 1, // last config in a sequence + REGRESSION_CONFIG_LAST = 2, // last config in a sequence + REGRESSION_CONFIG_SINGLE = 4, // single-owner config +}; + +struct RegressionConfig : public ConfigInfo +{ + + RegressionConfig(RegressionTest * r, int * ps, unsigned f) + : test(r), pstatus(ps), flags(f) { + if (this->flags & REGRESSION_CONFIG_SINGLE) { + TestBox box(this->test, this->pstatus); + box.check(this->refcount() == 1, "invalid refcount %d (should be 1)", this->refcount()); + } + } + + ~RegressionConfig() { + TestBox box(this->test, this->pstatus); + + box.check(this->refcount() == 0, "invalid refcount %d (should be 0)", this->refcount()); + + // If we are the last config to be scheduled, pass the test. + // Otherwise, verify that the test is still running ... + if (REGRESSION_CONFIG_LAST & flags) { + *this->pstatus = REGRESSION_TEST_PASSED; + } else { + box.check(*this->pstatus == REGRESSION_TEST_INPROGRESS, "intermediate config out of sequence"); + } + } + + RegressionTest * test; + int * pstatus; + unsigned flags; +}; + +// Test that ConfigProcessor::set() correctly releases the old ConfigInfo after a timeout. +REGRESSION_TEST(ProxyConfig_Set)(RegressionTest * test, int /* atype ATS_UNUSED */, int * pstatus) +{ + int configid = 0; + *pstatus = REGRESSION_TEST_INPROGRESS; + configid = configProcessor.set(configid, new RegressionConfig(test, pstatus, REGRESSION_CONFIG_FIRST), 1); + configid = configProcessor.set(configid, new RegressionConfig(test, pstatus, REGRESSION_CONFIG_FIRST), 1); + configid = configProcessor.set(configid, new RegressionConfig(test, pstatus, REGRESSION_CONFIG_FIRST), 1); + configid = configProcessor.set(configid, new RegressionConfig(test, pstatus, REGRESSION_CONFIG_LAST), 2); + + // Push one more RegressionConfig to force the tagged one to + // get destroyed. + configid = configProcessor.set(configid, new RegressionConfig(test, pstatus, 0), 4); + + return; +} + +// Test that ConfigProcessor::release() correctly releases the old ConfigInfo across an implicit +// release timeout. +REGRESSION_TEST(ProxyConfig_Release)(RegressionTest * test, int /* atype ATS_UNUSED */, int * pstatus) +{ + int configid = 0; + RegressionConfig * config; + + *pstatus = REGRESSION_TEST_INPROGRESS; + + // Set an initial config, then get it back to hold a reference count. + configid = configProcessor.set(configid, new RegressionConfig(test, pstatus, REGRESSION_CONFIG_LAST), 1); + config = (RegressionConfig *)configProcessor.get(configid); + + // Now update the config a few times. + configid = configProcessor.set(configid, new RegressionConfig(test, pstatus, REGRESSION_CONFIG_FIRST), 1); + configid = configProcessor.set(configid, new RegressionConfig(test, pstatus, REGRESSION_CONFIG_FIRST), 1); + configid = configProcessor.set(configid, new RegressionConfig(test, pstatus, REGRESSION_CONFIG_FIRST), 1); + + sleep(2); + + // Release the reference count. Since we were keeping this alive, it should be the last to die. + configProcessor.release(configid, config); +} + +#endif /* TS_HAS_TESTS */ http://git-wip-us.apache.org/repos/asf/trafficserver/blob/6da424fa/mgmt/ProxyConfig.h ---------------------------------------------------------------------- diff --git a/mgmt/ProxyConfig.h b/mgmt/ProxyConfig.h index 8fd6aa9..bb55f1b 100644 --- a/mgmt/ProxyConfig.h +++ b/mgmt/ProxyConfig.h @@ -54,21 +54,19 @@ pmgmt->registerMgmtCallback(_signal,_fn,_data) #define MAX_CONFIGS 100 -struct ConfigInfo -{ - volatile int m_refcount; - - virtual ~ ConfigInfo() - { - } -}; - +typedef RefCountObj ConfigInfo; class ConfigProcessor { public: ConfigProcessor(); + enum { + // The number of seconds to wait before garbage collecting stale ConfigInfo objects. There's + // no good reason to tune this, outside of regression tests, so don't. + CONFIG_PROCESSOR_RELEASE_SECS = 60 + }; + template <typename ClassType, typename ConfigType> struct scoped_config { scoped_config() : ptr(ClassType::acquire()) {} @@ -82,7 +80,7 @@ public: ConfigType * ptr; }; - unsigned int set(unsigned int id, ConfigInfo * info); + unsigned int set(unsigned int id, ConfigInfo * info, unsigned timeout_secs = CONFIG_PROCESSOR_RELEASE_SECS); ConfigInfo *get(unsigned int id); void release(unsigned int id, ConfigInfo * data);
