BIND 10 trac2202, updated. fc7462103f1fb344bf91707a1e2de99534e237b9 [2202] Change comment style
BIND 10 source code commits
bind10-changes at lists.isc.org
Tue Oct 2 17:25:09 UTC 2012
The branch, trac2202 has been updated
via fc7462103f1fb344bf91707a1e2de99534e237b9 (commit)
via 1ade0a0c0c8ead227aa7f8d374a1748de2535b15 (commit)
from a2bcf6e5b22df3c9bed473d342485548c60c8773 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit fc7462103f1fb344bf91707a1e2de99534e237b9
Author: JINMEI Tatuya <jinmei at isc.org>
Date: Tue Oct 2 16:53:56 2012 +0200
[2202] Change comment style
So it is consistent with the rest.
commit 1ade0a0c0c8ead227aa7f8d374a1748de2535b15
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Tue Oct 2 16:21:44 2012 +0200
[2202] Use simpler test for mutex
As Jinmei suggests, a double seems to be not atomic enough, so we can
use that. This is mostly his code, slightly modified (and comments
rewritten).
-----------------------------------------------------------------------
Summary of changes:
src/bin/auth/auth_srv.h | 63 +++++++++----------
src/lib/util/threads/tests/lock_unittest.cc | 87 +++++++++------------------
2 files changed, 58 insertions(+), 92 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/auth/auth_srv.h b/src/bin/auth/auth_srv.h
index 8f2fcec..ea658bc 100644
--- a/src/bin/auth/auth_srv.h
+++ b/src/bin/auth/auth_srv.h
@@ -322,39 +322,36 @@ public:
/// has been set by setClientList.
std::vector<isc::dns::RRClass> getClientListClasses() const;
- /**
- * \brief Return a mutex for the client lists.
- *
- * Background loading of data uses threads. Therefore we need to protect
- * the client lists by a mutex, so they don't change (or get destroyed)
- * during query processing. Get (and lock) this mutex whenever you do
- * something with the lists and keep it locked until you finish. This
- * is correct:
- * \code
- {
- Mutex::Locker locker(auth->getClientListMutex());
- boost::shared_ptr<isc::datasrc::ConfigurableClientList>
- list(auth->getClientList(RRClass::IN()));
- // Do some processing here
- }
- \endcode
- *
- * But this is not (it releases the mutex too soon):
- * \code
- boost::shared_ptr<isc::datasrc::ConfigurableClientList>
- list;
- {
- Mutex::Locker locker(auth->getClientListMutex());
- list = auth->getClientList(RRClass::IN()));
- }
- // Do some processing here
- * \endcode
- *
- * \note This method is const even if you are allowed to modify
- * (lock) the mutex. It's because locking of the mutex is not really
- * a modification of the server object and it is needed to protect the
- * lists even on read-only operations.
- */
+ /// \brief Return a mutex for the client lists.
+ ///
+ /// Background loading of data uses threads. Therefore we need to protect
+ /// the client lists by a mutex, so they don't change (or get destroyed)
+ /// during query processing. Get (and lock) this mutex whenever you do
+ /// something with the lists and keep it locked until you finish. This
+ /// is correct:
+ /// \code
+ /// {
+ /// Mutex::Locker locker(auth->getClientListMutex());
+ /// boost::shared_ptr<isc::datasrc::ConfigurableClientList>
+ /// list(auth->getClientList(RRClass::IN()));
+ /// // Do some processing here
+ /// }
+ /// \endcode
+ ///
+ /// But this is not (it releases the mutex too soon):
+ /// \code
+ /// boost::shared_ptr<isc::datasrc::ConfigurableClientList> list;
+ /// {
+ /// Mutex::Locker locker(auth->getClientListMutex());
+ /// list = auth->getClientList(RRClass::IN()));
+ /// }
+ /// // Do some processing here
+ /// \endcode
+ ///
+ /// \note This method is const even if you are allowed to modify
+ /// (lock) the mutex. It's because locking of the mutex is not really
+ /// a modification of the server object and it is needed to protect the
+ /// lists even on read-only operations.
isc::util::thread::Mutex& getClientListMutex() const;
private:
diff --git a/src/lib/util/threads/tests/lock_unittest.cc b/src/lib/util/threads/tests/lock_unittest.cc
index ea8f6af..fbf5df5 100644
--- a/src/lib/util/threads/tests/lock_unittest.cc
+++ b/src/lib/util/threads/tests/lock_unittest.cc
@@ -52,58 +52,30 @@ TEST(MutexTest, destroyLocked) {
}
#endif
-// This test tries if a mutex really locks. We could try that with a deadlock,
-// but that's not practical (the test would not end).
+// In this test, we try to check if a mutex really locks. We could try that with
+// a deadlock, but that's not practical (the test would not end).
//
-// So instead, we try to do some operations from multiple threads that are
-// likely to break if not locked. Also, they must run for some time so all
-// threads are started and the operation must be complicated enough so the
-// compiler won't turn it into some kind of single atomic instruction.
+// Instead, we try do to some operation on the same data from multiple threads
+// that's likely to break if not locked. Also, the test must run for a while
+// to have an opportunity to manifest.
//
-// FIXME: Any idea for a simpler but non-atomic operation that keeps an
-// invariant?
-//
-// So, we'll have an array of numbers. Each thread will try to repeatedly
-// find a number large at least as half of the average, take the number,
-// distribute the value across the rest of the positions of the array and
-// zero the found position. This operation keeps the sum of the array
-// the same. But if two threads access it at the same time, it is likely
-// one will add something to the position another one have chosen and
-// the other one will then zero it, not taking the new value into account.
-// That'd lower the total value of the array.
-//
-// We run the threads in opposite directions (so we have no chance of them
-// keeping the same distance to each other and not meeting). Also, the indexing
-// is performed in a circular manner like with a ring buffer.
-const unsigned long long length = 1000;
-const unsigned long long iterations = 10000;
-const unsigned long long value = 2000;
+// Currently we try incrementing a double variable. That one is large enough
+// and complex enough so it should not be possible for the CPU to do it as an
+// atomic operation, at least on common architectures.
+const size_t iterations = 100000;
+
void
-performStrangeOperation(std::vector<long long unsigned>& array, int direction,
- Mutex* mutex)
+performIncrement(volatile double* canary, volatile bool* ready_me,
+ volatile bool* ready_other, Mutex* mutex)
{
- unsigned long long position = 0;
+ // Loosely (busy) wait for the other thread so both will start
+ // approximately at the same time.
+ *ready_me = true;
+ while (!*ready_other) {}
+
for (size_t i = 0; i < iterations; ++i) {
Mutex::Locker lock(*mutex);
- // Find a place with large enough value
- while (array[position % length] < value) {
- position += direction;
- }
- // Take the value
- unsigned long long found_value = array[position % length];
- // And distribute it to places following the found one, by
- // adding 1 to each.
- unsigned long long p2 = position;
- while (found_value > 0) {
- p2 += direction;
- if (p2 % length == position % length) {
- continue;
- }
- ++array[p2 % length];
- --found_value;
- }
- // Zero the distributed value
- array[position % length] = 0;
+ *canary += 1;
}
}
@@ -121,22 +93,19 @@ TEST(MutexTest, swarm) {
alarm(10);
// This type has a low chance of being atomic itself, further raising
// the chance of problems appearing.
- std::vector<long long unsigned> array(length);
- for (size_t i = 0; i < length; ++i) {
- array[i] = value;
- }
+ double canary = 0;
Mutex mutex;
- // Run two parallel threads, each in one direction
- Thread t1(boost::bind(&performStrangeOperation, array, 1, &mutex));
- Thread t2(boost::bind(&performStrangeOperation, array, -1, &mutex));
+ // Run two parallel threads
+ bool ready1 = false;
+ bool ready2 = false;
+ Thread t1(boost::bind(&performIncrement, &canary, &ready1, &ready2,
+ &mutex));
+ Thread t2(boost::bind(&performIncrement, &canary, &ready2, &ready1,
+ &mutex));
t1.wait();
t2.wait();
- // Check the sum didn't change.
- long long unsigned sum = 0;
- for (size_t i = 0; i < length; ++i) {
- sum += array[i];
- }
- EXPECT_EQ(length * value, sum) << "Threads are badly synchronized";
+ // Check it the sum is the expected value.
+ EXPECT_EQ(iterations * 2, canary) << "Threads are badly synchronized";
// Cancel the alarm and return the original handler
alarm(0);
if (sigaction(SIGALRM, &original, NULL)) {
More information about the bind10-changes
mailing list