----- Original Message -----
> Updated Branches:
> refs/heads/master f760dac42 -> 6da424faa
>
>
> 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/master
> 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)),
ConfigInfoReleaser... This feels like Java :\
(even more so than ConfigProcessor ;)
> 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);
>
>
>
--
Igor Galić
Tel: +43 (0) 664 886 22 883
Mail: i.ga...@brainsware.org
URL: http://brainsware.org/
GPG: 6880 4155 74BD FD7C B515 2EA5 4B1D 9E08 A097 C9AE
