You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by jp...@apache.org on 2013/10/23 18:22:08 UTC
git commit: TS-2296: improve ConfigProcessor reference counting
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 <jp...@apache.org>
Authored: Mon Oct 7 16:08:36 2013 -0700
Committer: James Peach <jp...@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);
Re: git commit: TS-2296: improve ConfigProcessor reference counting
Posted by Igor Galić <i....@brainsware.org>.
----- 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 <jp...@apache.org>
> Authored: Mon Oct 7 16:08:36 2013 -0700
> Committer: James Peach <jp...@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.galic@brainsware.org
URL: http://brainsware.org/
GPG: 6880 4155 74BD FD7C B515 2EA5 4B1D 9E08 A097 C9AE