Mir

Merge lp:~vanvugt/mir/fix-1282886 into lp:mir

  1. fix-1282886
  2. Merge into development-branch
Proposed by Daniel van Vugt
Status: Merged
Approved by: Daniel van Vugt
Approved revision: no longer in the source branch.
Merged at revision: 1425
Proposed branch: lp:~vanvugt/mir/fix-1282886
Merge into: lp:mir
Diff against target: 131 lines (+38/-36)
1 file modified
tests/integration-tests/compositor/test_swapping_swappers.cpp (+38/-36)
To merge this branch: bzr merge lp:~vanvugt/mir/fix-1282886
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Kevin DuBois (community) Approve
Andreas Pokorny (community) Approve
Alan Griffiths Approve
Review via email: mp+207596@code.launchpad.net

Commit message

Fix mutex data race reported by helgrind in integration test:
SwapperSwappingStress (LP: #1282886)

Description of the change

.

To post a comment you must log in.
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Admittedly solving the std::atomic races is ugly. We really should create a class for that or get helgrind itself fixed, somehow.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1419
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/915/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1005
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1003
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/597
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/645
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/645/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/652
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/652/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/598
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/598/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/572
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4168

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/915/rebuild

review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

There are two parts to this fix:

the change to client_acquire_blocking has much the same effect as https://code.launchpad.net/~alan-griffiths/mir/keep-helgrind-happy/+merge/207389 and I'm happy with it.

But the other part is writing ugly code to get around the std::atomic issue with helgrind. I don't feel this is an adequate justification for such code (and doesn't solve similar cases elsewhere).

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1421
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/917/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1008
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1006
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/600
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/647
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/647/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/654
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/654/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/601
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/601/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/574
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4172

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/917/rebuild

review: Approve (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Alan, you're right. They're two separate issues which really require separate fixes. I've reverted the bespoke Atomic template and just propose fixing the original mutex/lock race now.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:1423
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/925/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1019
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1017
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/611/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/655
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/655/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/662
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/662/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/612
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/612/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/586/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4216

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/925/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> Alan, you're right. They're two separate issues which really require separate
> fixes. I've reverted the bespoke Atomic template and just propose fixing the
> original mutex/lock race now.

You've changed a member atomic<bool> to a local atomic_bool instead of reverting, but OK.

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

So long as the C++ library provides std::atomic_bool, it should be faster to compile and eliminates any risk of template bloat (since it's not a template but a class implemented in the C++ library).

Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

ok

review: Approve
Revision history for this message
Kevin DuBois (kdub) wrote :

looks good to me

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-autolanding/411/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1032
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1030
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/624
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-autolanding/316/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-autolanding/317
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-autolanding/317/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/625
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/625/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/599
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4262

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'tests/integration-tests/compositor/test_swapping_swappers.cpp'
2--- tests/integration-tests/compositor/test_swapping_swappers.cpp 2014-02-04 16:38:36 +0000
3+++ tests/integration-tests/compositor/test_swapping_swappers.cpp 2014-02-24 03:43:56 +0000
4@@ -27,6 +27,8 @@
5 #include <future>
6 #include <thread>
7 #include <chrono>
8+#include <mutex>
9+#include <atomic>
10
11 namespace mc = mir::compositor;
12 namespace mg = mir::graphics;
13@@ -49,41 +51,41 @@
14 }
15
16 std::shared_ptr<mc::SwitchingBundle> switching_bundle;
17- std::atomic<bool> client_thread_done;
18+ std::mutex acquire_mutex; // must live longer than our callback/lambda
19+
20+ mg::Buffer* client_acquire_blocking(
21+ std::shared_ptr<mc::SwitchingBundle> const& switching_bundle)
22+ {
23+ std::condition_variable cv;
24+ bool acquired = false;
25+
26+ mg::Buffer* result;
27+ switching_bundle->client_acquire(
28+ [&](mg::Buffer* new_buffer)
29+ {
30+ std::unique_lock<decltype(acquire_mutex)> lock(acquire_mutex);
31+
32+ result = new_buffer;
33+ acquired = true;
34+ cv.notify_one();
35+ });
36+
37+ std::unique_lock<decltype(acquire_mutex)> lock(acquire_mutex);
38+
39+ cv.wait(lock, [&]{ return acquired; });
40+
41+ return result;
42+ }
43 };
44
45-auto client_acquire_blocking(std::shared_ptr<mc::SwitchingBundle> const& switching_bundle)
46--> mg::Buffer*
47-{
48- std::mutex mutex;
49- std::condition_variable cv;
50- bool done = false;
51-
52- mg::Buffer* result;
53- switching_bundle->client_acquire(
54- [&](mg::Buffer* new_buffer)
55- {
56- std::unique_lock<decltype(mutex)> lock(mutex);
57-
58- result = new_buffer;
59- done = true;
60- cv.notify_one();
61- });
62-
63- std::unique_lock<decltype(mutex)> lock(mutex);
64-
65- cv.wait(lock, [&]{ return done; });
66-
67- return result;
68-}
69-}
70+} // namespace
71
72 TEST_F(SwapperSwappingStress, swapper)
73 {
74- client_thread_done = false;
75+ std::atomic_bool done(false);
76
77 auto f = std::async(std::launch::async,
78- [this]
79+ [&]
80 {
81 for(auto i=0u; i < 400; i++)
82 {
83@@ -91,14 +93,14 @@
84 std::this_thread::yield();
85 switching_bundle->client_release(b);
86 }
87- client_thread_done = true;
88+ done = true;
89 });
90
91 auto g = std::async(std::launch::async,
92- [this]
93+ [&]
94 {
95 unsigned long count = 0;
96- while(!client_thread_done)
97+ while (!done)
98 {
99 auto b = switching_bundle->compositor_acquire(++count);
100 std::this_thread::yield();
101@@ -125,10 +127,10 @@
102
103 TEST_F(SwapperSwappingStress, different_swapper_types)
104 {
105- client_thread_done = false;
106+ std::atomic_bool done(false);
107
108 auto f = std::async(std::launch::async,
109- [this]
110+ [&]
111 {
112 for(auto i=0u; i < 400; i++)
113 {
114@@ -136,14 +138,14 @@
115 std::this_thread::yield();
116 switching_bundle->client_release(b);
117 }
118- client_thread_done = true;
119+ done = true;
120 });
121
122 auto g = std::async(std::launch::async,
123- [this]
124+ [&]
125 {
126 unsigned long count = 0;
127- while(!client_thread_done)
128+ while (!done)
129 {
130 auto b = switching_bundle->compositor_acquire(++count);
131 std::this_thread::yield();

Subscribers

People subscribed via source and target branches