summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--content/browser/gamepad/gamepad_provider.cc11
-rw-r--r--content/browser/gamepad/gamepad_provider_unittest.cc25
-rw-r--r--content/common/gamepad_hardware_buffer.h9
-rw-r--r--content/common/gamepad_seqlock.cc106
-rw-r--r--content/common/gamepad_seqlock.h88
-rw-r--r--content/common/gamepad_seqlock_unittest.cc26
-rw-r--r--content/content_tests.gypi1
-rw-r--r--content/renderer/gamepad_shared_memory_reader.cc26
-rw-r--r--tools/valgrind/tsan/ignores.txt2
9 files changed, 164 insertions, 130 deletions
diff --git a/content/browser/gamepad/gamepad_provider.cc b/content/browser/gamepad/gamepad_provider.cc
index 3ca996f..8de43f5 100644
--- a/content/browser/gamepad/gamepad_provider.cc
+++ b/content/browser/gamepad/gamepad_provider.cc
@@ -121,11 +121,6 @@ void GamepadProvider::DoPoll() {
bool changed;
GamepadHardwareBuffer* hwbuf = SharedMemoryAsHardwareBuffer();
- ANNOTATE_BENIGN_RACE_SIZED(
- &hwbuf->buffer,
- sizeof(WebKit::WebGamepads),
- "Racey reads are discarded");
-
{
base::AutoLock lock(devices_changed_lock_);
changed = devices_changed_;
@@ -134,9 +129,9 @@ void GamepadProvider::DoPoll() {
// Acquire the SeqLock. There is only ever one writer to this data.
// See gamepad_hardware_buffer.h.
- hwbuf->sequence.WriteBegin();
- data_fetcher_->GetGamepadData(&hwbuf->buffer, changed);
- hwbuf->sequence.WriteEnd();
+ WebKit::WebGamepads tmp;
+ data_fetcher_->GetGamepadData(&tmp, changed);
+ hwbuf->gamepads.Write(tmp);
// Schedule our next interval of polling.
ScheduleDoPoll();
diff --git a/content/browser/gamepad/gamepad_provider_unittest.cc b/content/browser/gamepad/gamepad_provider_unittest.cc
index d81b3f5..69cee11 100644
--- a/content/browser/gamepad/gamepad_provider_unittest.cc
+++ b/content/browser/gamepad/gamepad_provider_unittest.cc
@@ -6,6 +6,7 @@
#include "base/process_util.h"
#include "base/synchronization/waitable_event.h"
#include "base/system_monitor/system_monitor.h"
+#include "base/threading/platform_thread.h"
#include "content/browser/gamepad/data_fetcher.h"
#include "content/browser/gamepad/gamepad_provider.h"
#include "content/common/gamepad_hardware_buffer.h"
@@ -22,19 +23,14 @@ using WebKit::WebGamepads;
class MockDataFetcher : public GamepadDataFetcher {
public:
explicit MockDataFetcher(const WebGamepads& test_data)
- : test_data_(test_data),
- read_data_(false, false) {
+ : test_data_(test_data) {
}
virtual void GetGamepadData(WebGamepads* pads,
bool devices_changed_hint) OVERRIDE {
*pads = test_data_;
- read_data_.Signal();
}
- void WaitForDataRead() { return read_data_.Wait(); }
-
WebGamepads test_data_;
- base::WaitableEvent read_data_;
};
// Main test fixture
@@ -77,8 +73,6 @@ TEST_F(GamepadProviderTest, DISABLED_PollingAccess) {
main_message_loop_.RunAllPending();
- mock_data_fetcher_->WaitForDataRead();
-
// Renderer-side, pull data out of poll buffer.
base::SharedMemoryHandle handle =
provider->GetRendererSharedMemoryHandle(base::GetCurrentProcessHandle());
@@ -88,15 +82,14 @@ TEST_F(GamepadProviderTest, DISABLED_PollingAccess) {
void* mem = shared_memory->memory();
GamepadHardwareBuffer* hwbuf = static_cast<GamepadHardwareBuffer*>(mem);
- // See gamepad_hardware_buffer.h for details on the read discipline.
WebGamepads output;
-
- base::subtle::Atomic32 version;
- do {
- version = hwbuf->sequence.ReadBegin();
- memcpy(&output, &hwbuf->buffer, sizeof(output));
- } while (hwbuf->sequence.ReadRetry(version));
-
+ output.length = 0;
+ for (;;) {
+ hwbuf->gamepads.ReadTo(&output);
+ if (output.length == 1)
+ break;
+ base::PlatformThread::YieldCurrentThread();
+ }
EXPECT_EQ(1u, output.length);
EXPECT_EQ(1u, output.items[0].buttonsLength);
EXPECT_EQ(1.f, output.items[0].buttons[0]);
diff --git a/content/common/gamepad_hardware_buffer.h b/content/common/gamepad_hardware_buffer.h
index 077c782..7f4bad9 100644
--- a/content/common/gamepad_hardware_buffer.h
+++ b/content/common/gamepad_hardware_buffer.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -24,10 +24,13 @@ contention is detected by using the associated SeqLock.
*/
struct GamepadHardwareBuffer {
- GamepadSeqLock sequence;
- WebKit::WebGamepads buffer;
+ GamepadSeqLock<WebKit::WebGamepads> gamepads;
+ GamepadHardwareBuffer();
};
+inline GamepadHardwareBuffer::GamepadHardwareBuffer() {
+}
+
}
#endif // CONTENT_COMMON_GAMEPAD_HARDWARE_BUFFER_H_
diff --git a/content/common/gamepad_seqlock.cc b/content/common/gamepad_seqlock.cc
index 7f7e14f..3e49f1e 100644
--- a/content/common/gamepad_seqlock.cc
+++ b/content/common/gamepad_seqlock.cc
@@ -1,49 +1,87 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "content/common/gamepad_seqlock.h"
namespace content {
+namespace internal {
-GamepadSeqLock::GamepadSeqLock()
- : sequence_(0) {
-}
-
-base::subtle::Atomic32 GamepadSeqLock::ReadBegin() {
- base::subtle::Atomic32 version;
- for (;;) {
- version = base::subtle::NoBarrier_Load(&sequence_);
-
- // If the counter is even, then the associated data might be in a
- // consistent state, so we can try to read.
- if ((version & 1) == 0)
- break;
-
- // Otherwise, the writer is in the middle of an update. Retry the read.
- base::PlatformThread::YieldCurrentThread();
+GamepadSeqLockBase::GamepadSeqLockBase(BaseEntry* entries, size_t size)
+ : size_(size)
+ , current_(0) {
+ const size_t ent_size = sizeof(BaseEntry) + sizeof(Atomic32) * size;
+ memset(entries, 0, kEntries * ent_size);
+ for (size_t i = 0; i < kEntries; ++i) {
+ entries_[i] = reinterpret_cast<BaseEntry*>
+ (reinterpret_cast<char*>(entries) + i * ent_size);
}
- return version;
}
-bool GamepadSeqLock::ReadRetry(base::subtle::Atomic32 version) {
- // If the sequence number was updated then a read should be re-attempted.
- // -- Load fence, read membarrier
- return base::subtle::Release_Load(&sequence_) != version;
-}
+// The algorithm works as follows.
+// The SeqLock contains 2 user objects - a current and a non-current.
+// The object roles can be swapped by incrementing the current_ variable.
+// Initially both objects are consistent, that is, their sequence_%2 == 0.
+// A writer proceeds as follows. First, it marks the non-current object
+// as inconsistent by incrementing it's sequence_ (the sequence_ becomes odd).
+// Then it mutates the object. Then marks it as consistent by incrementing
+// it's sequence_ once again (the sequence_ is even now). And finally swaps
+// the object roles, that is, the object becomes current.
+// Such disipline establishes an important property - the current object
+// is always consistent and contains the most recent data.
+// Readers proceed as follows. First, determine what is the current object,
+// remember it's seqence, check that the sequence is even
+// (the object is consistent), copy out the object, verify that
+// the sequence is not changed. If any of the checks fail, a reader works
+// with a non-current object (current object is always consistent), so it
+// just retries from the beginning. Thus readers are completely lock-free,
+// that is, a reader retries iff a writer has accomplished a write operation
+// during reading (only constant sequence of writes can stall readers,
+// a stalled writer can't block readers).
-void GamepadSeqLock::WriteBegin() {
- // Increment the sequence number to odd to indicate the beginning of a write
- // update.
- base::subtle::Barrier_AtomicIncrement(&sequence_, 1);
- // -- Store fence, write membarrier
+void GamepadSeqLockBase::ReadTo(Atomic32* obj) {
+ using base::subtle::NoBarrier_Load;
+ using base::subtle::Acquire_Load;
+ using base::subtle::MemoryBarrier;
+ for (;;) {
+ // Determine the current object.
+ Atomic32 cur = Acquire_Load(&current_);
+ BaseEntry* ent = entries_[cur % kEntries];
+ Atomic32 seq = Acquire_Load(&ent->sequence_); // membar #LoadLoad
+ // If the counter is even, then the object is not already current,
+ // no need to yield, just retry (the current object is consistent).
+ if (seq % 2)
+ continue;
+ // Copy out the entry.
+ for (size_t i = 0; i < size_; ++i)
+ obj[i] = NoBarrier_Load(&ent->data_[i]);
+ MemoryBarrier(); // membar #LoadLoad
+ Atomic32 seq2 = NoBarrier_Load(&ent->sequence_);
+ // If the counter is changed, then we've read inconsistent data,
+ // no need to yield, just retry (the current object is consistent).
+ if (seq2 != seq)
+ continue;
+ break;
+ }
}
-void GamepadSeqLock::WriteEnd() {
- // Increment the sequence to an even number to indicate the completion of
- // a write update.
- // -- Store fence, write membarrier
- base::subtle::Barrier_AtomicIncrement(&sequence_, 1);
+void GamepadSeqLockBase::Write(const Atomic32* obj) {
+ using base::subtle::NoBarrier_Store;
+ using base::subtle::Release_Store;
+ using base::subtle::MemoryBarrier;
+ // Get the non current object...
+ BaseEntry* ent = entries_[(current_ + 1) % kEntries];
+ // ... and mark it as inconsistent.
+ NoBarrier_Store(&ent->sequence_, ent->sequence_ + 1);
+ MemoryBarrier(); // membar #StoreStore
+ // Copy in the entry.
+ for (size_t i = 0; i < size_; ++i)
+ NoBarrier_Store(&ent->data_[i], obj[i]);
+ // Mark the object as consistent again.
+ Release_Store(&ent->sequence_, ent->sequence_ + 1); // membar #StoreStore
+ // Switch the current object.
+ Release_Store(&current_, current_ + 1);
}
-} // namespace content
+} // namespace internal
+} // namespace content
diff --git a/content/common/gamepad_seqlock.h b/content/common/gamepad_seqlock.h
index 35f51e5..66f955d 100644
--- a/content/common/gamepad_seqlock.h
+++ b/content/common/gamepad_seqlock.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -6,38 +6,76 @@
#define CONTENT_COMMON_GAMEPAD_SEQLOCK_H_
#include "base/atomicops.h"
-#include "base/threading/platform_thread.h"
+#include "base/basictypes.h"
#include "content/common/content_export.h"
namespace content {
+namespace internal {
+
+class CONTENT_EXPORT GamepadSeqLockBase {
+ protected:
+ static const size_t kEntries = 2;
+ typedef base::subtle::Atomic32 Atomic32;
+ struct BaseEntry {
+ Atomic32 sequence_;
+ // Both writer and readers work with the array concurrently,
+ // so it must be accessed with atomic operations.
+ Atomic32 data_[1];
+ };
+
+ GamepadSeqLockBase(BaseEntry* entries, size_t size);
+ void ReadTo(Atomic32* obj);
+ void Write(const Atomic32* obj);
+
+ private:
+ const size_t size_;
+ Atomic32 current_;
+ BaseEntry* entries_[kEntries];
+};
+
+} // namespace internal
+
// This SeqLock handles only *one* writer and multiple readers. It may be
-// suitable for low-contention with relatively infrequent writes, and many
-// readers. See:
+// suitable for high read frequency scenarios and is especially useful in shared
+// memory environment. See for the basic idea:
// http://en.wikipedia.org/wiki/Seqlock
-// http://www.concurrencykit.org/doc/ck_sequence.html
-// This implementation is based on ck_sequence.h from http://concurrencykit.org.
-//
-// Currently, this is used in only one location. It may make sense to
-// generalize with a higher-level construct that owns both the lock and the
-// data buffer, if it is to be used more widely.
-//
-// You must be very careful not to operate on potentially inconsistent read
-// buffers. If the read must be retry'd, the data in the read buffer could
-// contain any random garbage. e.g., contained pointers might be
-// garbage, or indices could be out of range. Probably the only suitable thing
-// to do during the read loop is to make a copy of the data, and operate on it
-// only after the read was found to be consistent.
-class CONTENT_EXPORT GamepadSeqLock {
+// However, this implementation is an improved lock-free variant.
+// The SeqLock can hold only POD fully-embed data (no pointers
+// to satellite data), copies are done with memcpy.
+template<typename T>
+class GamepadSeqLock : public internal::GamepadSeqLockBase {
public:
- GamepadSeqLock();
- base::subtle::Atomic32 ReadBegin();
- bool ReadRetry(base::subtle::Atomic32 version);
- void WriteBegin();
- void WriteEnd();
+ GamepadSeqLock()
+ : internal::GamepadSeqLockBase(entries_, kSize) {
+ }
- private:
- base::subtle::Atomic32 sequence_;
+ // May be called concurrently with ReadTo and Write.
+ // - If ReadTo occurs in the middle of Write (on another thread),
+ // ReadTo gets the old data (not the new data being written
+ // by the other thread).
+ // - ReadTo never spins unless the writer thread is calling Write
+ // multiple times before ReadTo completes.
+ void ReadTo(T* obj) {
+ // Breaks strict-aliasing rules.
+ Base::ReadTo(reinterpret_cast<Atomic32*>(obj));
+ }
+
+ // Must be called by a single thread at a time. May be called concurrently
+ // with ReadTo.
+ void Write(const T& obj) {
+ // Breaks strict-aliasing rules.
+ Base::Write(reinterpret_cast<const Atomic32*>(&obj));
+ }
+
+private:
+ typedef internal::GamepadSeqLockBase Base;
+ COMPILE_ASSERT(sizeof(T) % sizeof(Atomic32) == 0, bad_object_size);
+ static size_t const kSize = sizeof(T) / sizeof(Atomic32);
+ struct Entry : Base::BaseEntry {
+ Atomic32 data_[kSize];
+ };
+ Entry entries_[kEntries];
DISALLOW_COPY_AND_ASSIGN(GamepadSeqLock);
};
diff --git a/content/common/gamepad_seqlock_unittest.cc b/content/common/gamepad_seqlock_unittest.cc
index 7e5da3f..cfd3b74 100644
--- a/content/common/gamepad_seqlock_unittest.cc
+++ b/content/common/gamepad_seqlock_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -8,7 +8,6 @@
#include "base/atomic_ref_count.h"
#include "base/threading/platform_thread.h"
-#include "base/third_party/dynamic_annotations/dynamic_annotations.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace base {
@@ -24,11 +23,9 @@ class BasicSeqLockTestThread : public PlatformThread::Delegate {
BasicSeqLockTestThread() {}
void Init(
- content::GamepadSeqLock* seqlock,
- TestData* data,
+ content::GamepadSeqLock<TestData>* seqlock,
base::subtle::Atomic32* ready) {
seqlock_ = seqlock;
- data_ = data;
ready_ = ready;
}
virtual void ThreadMain() {
@@ -38,12 +35,7 @@ class BasicSeqLockTestThread : public PlatformThread::Delegate {
for (unsigned i = 0; i < 1000; ++i) {
TestData copy;
- base::subtle::Atomic32 version;
- do {
- version = seqlock_->ReadBegin();
- copy = *data_;
- } while (seqlock_->ReadRetry(version));
-
+ seqlock_->ReadTo(&copy);
EXPECT_EQ(copy.a + 100, copy.b);
EXPECT_EQ(copy.c, copy.b + copy.a);
}
@@ -52,37 +44,33 @@ class BasicSeqLockTestThread : public PlatformThread::Delegate {
}
private:
- content::GamepadSeqLock* seqlock_;
- TestData* data_;
+ content::GamepadSeqLock<TestData>* seqlock_;
base::AtomicRefCount* ready_;
DISALLOW_COPY_AND_ASSIGN(BasicSeqLockTestThread);
};
TEST(GamepadSeqLockTest, ManyThreads) {
- content::GamepadSeqLock seqlock;
+ content::GamepadSeqLock<TestData> seqlock;
TestData data = { 0, 0, 0 };
base::AtomicRefCount ready = 0;
- ANNOTATE_BENIGN_RACE_SIZED(&data, sizeof(data), "Racey reads are discarded");
-
static const unsigned kNumReaderThreads = 10;
BasicSeqLockTestThread threads[kNumReaderThreads];
PlatformThreadHandle handles[kNumReaderThreads];
for (unsigned i = 0; i < kNumReaderThreads; ++i)
- threads[i].Init(&seqlock, &data, &ready);
+ threads[i].Init(&seqlock, &ready);
for (unsigned i = 0; i < kNumReaderThreads; ++i)
ASSERT_TRUE(PlatformThread::Create(0, &threads[i], &handles[i]));
// The main thread is the writer, and the spawned are readers.
unsigned counter = 0;
for (;;) {
- seqlock.WriteBegin();
data.a = counter++;
data.b = data.a + 100;
data.c = data.b + data.a;
- seqlock.WriteEnd();
+ seqlock.Write(data);
if (counter == 1)
base::AtomicRefCountIncN(&ready, kNumReaderThreads);
diff --git a/content/content_tests.gypi b/content/content_tests.gypi
index 0320e1f..6ec38e2 100644
--- a/content/content_tests.gypi
+++ b/content/content_tests.gypi
@@ -305,6 +305,7 @@
'common/android/address_parser_unittest.cc',
'common/mac/attributed_string_coder_unittest.mm',
'common/mac/font_descriptor_unittest.mm',
+ 'common/gamepad_seqlock_unittest.cc',
'common/gpu/gpu_info_unittest.cc',
'common/gpu/gpu_memory_manager_unittest.cc',
'common/gpu/media/avc_config_record_builder_unittest.cc',
diff --git a/content/renderer/gamepad_shared_memory_reader.cc b/content/renderer/gamepad_shared_memory_reader.cc
index 55c6f62..0d249a7 100644
--- a/content/renderer/gamepad_shared_memory_reader.cc
+++ b/content/renderer/gamepad_shared_memory_reader.cc
@@ -41,31 +41,7 @@ void GamepadSharedMemoryReader::SampleGamepads(WebKit::WebGamepads& gamepads) {
if (!base::SharedMemory::IsHandleValid(renderer_shared_memory_handle_))
return;
- // Only try to read this many times before failing to avoid waiting here
- // very long in case of contention with the writer. TODO(scottmg) Tune this
- // number (as low as 1?) if histogram shows distribution as mostly
- // 0-and-maximum.
- const int kMaximumContentionCount = 10;
- int contention_count = -1;
- base::subtle::Atomic32 version;
- do {
- version = gamepad_hardware_buffer_->sequence.ReadBegin();
- memcpy(&read_into, &gamepad_hardware_buffer_->buffer, sizeof(read_into));
- ++contention_count;
- if (contention_count == kMaximumContentionCount)
- break;
- } while (gamepad_hardware_buffer_->sequence.ReadRetry(version));
- UMA_HISTOGRAM_COUNTS("Gamepad.ReadContentionCount", contention_count);
-
- if (contention_count >= kMaximumContentionCount) {
- // We failed to successfully read, presumably because the hardware
- // thread was taking unusually long. Don't copy the data to the output
- // buffer, and simply leave what was there before.
- return;
- }
-
- // New data was read successfully, copy it into the output buffer.
- memcpy(&gamepads, &read_into, sizeof(gamepads));
+ gamepad_hardware_buffer_->gamepads.ReadTo(&gamepads);
// Override the "connected" with false until the user has interacted
// with the gamepad. This is to prevent fingerprinting on drive-by pages.
diff --git a/tools/valgrind/tsan/ignores.txt b/tools/valgrind/tsan/ignores.txt
index 7f28d10..ac677b8 100644
--- a/tools/valgrind/tsan/ignores.txt
+++ b/tools/valgrind/tsan/ignores.txt
@@ -60,6 +60,8 @@ src:*base/synchronization/waitable_event*
# Don't instrument code dealing with atomics (base::subtle)
fun:*base*subtle*Release_Store*
+fun:*base*subtle*NoBarrier_Store*
+fun:*base*subtle*Acquire_Load*
fun:*base*subtle*NoBarrier_CompareAndSwap*
fun:*base*subtle*NoBarrier_Load*
# Keep some mangling so we don't match NoBarrier_AtomicIncrement
generated by cgit v1.1 at 2025-02-08 09:54:42 +0000