Unity System Compositor

Merge lp:~alan-griffiths/unity-system-compositor/rework-DeadlockLP1491566 into lp:unity-system-compositor

  1. rework-DeadlockLP1491566
  2. Merge into trunk
Proposed by Alan Griffiths on 2016-02-19
Status: Merged
Approved by: Alan Griffiths on 2016-02-25
Approved revision: 285
Merged at revision: 282
Proposed branch: lp:~alan-griffiths/unity-system-compositor/rework-DeadlockLP1491566
Merge into: lp:unity-system-compositor
Diff against target: 119 lines (+34/-33)
1 file modified
tests/integration-tests/test_deadlock_lp1491566.cpp (+34/-33)
To merge this branch: bzr merge lp:~alan-griffiths/unity-system-compositor/rework-DeadlockLP1491566
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Approve on 2016-02-25
Alexandros Frantzis (community) 2016-02-19 Approve on 2016-02-25
Mir CI Bot continuous-integration Approve on 2016-02-25
Review via email: mp+286698@code.launchpad.net

Commit Message

Rework DeadlockLP1491566 tests to avoid spinlocks

Description of the Change

Rework DeadlockLP1491566 tests to avoid spinlocks

This test has failed intermittently on ARM64 during release. (But I've not been able to reproduce the failure,)

I hope that any sensitivity to machine load or scheduling will be reduced by changing the synchronization mechanism.

To post a comment you must log in.
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:282
https://mir-jenkins.ubuntu.com/job/usc-ci/7/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-usc/7
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/149
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/145
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/145
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=amd64,release=xenial/11
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=amd64,release=xenial/11/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=armhf,release=vivid+overlay/11
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=armhf,release=vivid+overlay/11/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=i386,release=xenial/11
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=i386,release=xenial/11/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/usc-ci/7/rebuild

review: Approve (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Alexandros Frantzis (afrantzis) wrote :

I don't know what the problem on arm64 is, so is hard to tell if the code changes will help. The old and the new code are equally correctly, so, unless there is arm64 libstdc++ bug the new code is working around, I think the change that has the most potential to help is the timeout increase.

+ std::unique_lock<decltype(waiting_guard)> lock(waiting_guard);
+ waiting_cv.wait_for(lock, timeout, [this] { return waiting_for_future; });

This wait doesn't gain us anything since it doesn't guarantee that set_power_mode will have started running and acquired any locks.

// Wait a bit for async operation to get processed
- std::this_thread::sleep_for(
- std::chrono::milliseconds{500});

The wait is used to (drastically) increase the probability of reproducing the deadlock scenario. If we don't wait enough, and depending on thread scheduling, the dimmer/power_off callback may finish before the async invocation of set_power_mode gets a chance to run and acquire the locks.

review: Needs Fixing
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:283
https://mir-jenkins.ubuntu.com/job/usc-ci/8/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-usc/8
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/195
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/191
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/191
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=amd64,release=xenial/12
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=amd64,release=xenial/12/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=armhf,release=vivid+overlay/12
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=armhf,release=vivid+overlay/12/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=i386,release=xenial/12
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=i386,release=xenial/12/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/usc-ci/8/rebuild

review: Approve (continuous-integration)
lp:~alan-griffiths/unity-system-compositor/rework-DeadlockLP1491566 updated on 2016-02-24
283. By Alan Griffiths on 2016-02-24

Rework again following review feedback

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
lp:~alan-griffiths/unity-system-compositor/rework-DeadlockLP1491566 updated on 2016-02-24
284. By Alan Griffiths on 2016-02-24

Reinstate abort() failure mode

Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:284
https://mir-jenkins.ubuntu.com/job/usc-ci/9/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-usc/9
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/197
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/193
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/193
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=amd64,release=xenial/13
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=amd64,release=xenial/13/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=armhf,release=vivid+overlay/13
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=armhf,release=vivid+overlay/13/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=i386,release=xenial/13
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=i386,release=xenial/13/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/usc-ci/9/rebuild

review: Approve (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
lp:~alan-griffiths/unity-system-compositor/rework-DeadlockLP1491566 updated on 2016-02-25
285. By Alan Griffiths on 2016-02-25

merge lp:unity-system-compositor

Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:285
https://mir-jenkins.ubuntu.com/job/usc-ci/11/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-usc/12
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/214
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/209
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/209
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=amd64,release=xenial/16
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=amd64,release=xenial/16/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=armhf,release=vivid+overlay/16
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=armhf,release=vivid+overlay/16/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=i386,release=xenial/16
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-usc/arch=i386,release=xenial/16/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/usc-ci/11/rebuild

review: Approve (continuous-integration)
Alexandros Frantzis (afrantzis) wrote :

Looks good.

review: Approve
PS Jenkins bot (ps-jenkins) wrote :
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/test_deadlock_lp1491566.cpp'
2--- tests/integration-tests/test_deadlock_lp1491566.cpp 2016-02-02 09:18:24 +0000
3+++ tests/integration-tests/test_deadlock_lp1491566.cpp 2016-02-25 09:37:25 +0000
4@@ -21,7 +21,6 @@
5 #include "src/performance_booster.h"
6 #include "src/screen_hardware.h"
7 #include "src/power_state_change_reason.h"
8-#include "spin_wait.h"
9 #include "usc/test/stub_display_configuration.h"
10 #include "usc/test/mock_display.h"
11
12@@ -35,12 +34,14 @@
13
14 #include <thread>
15 #include <future>
16+#include <condition_variable>
17
18 namespace mg = mir::graphics;
19 namespace ut = usc::test;
20
21 namespace
22 {
23+auto constexpr timeout = std::chrono::seconds{9};
24
25 struct NullCompositor : mir::compositor::Compositor
26 {
27@@ -136,16 +137,28 @@
28 PowerStateChangeReason::power_key);
29 }
30
31- void wait_for_async_operation(std::future<void>& future)
32- {
33- if (!usc::test::spin_wait_for_condition_or_timeout(
34- [&future] { return future.valid(); },
35- std::chrono::seconds{3}))
36- {
37- throw std::runtime_error{"Future is not valid!"};
38- }
39-
40- if (future.wait_for(std::chrono::seconds{3}) != std::future_status::ready)
41+ void launch_async_set_screen_power_mode_then_try_to_deadlock()
42+ {
43+ {
44+ std::lock_guard<decltype(future_guard)> lock(future_guard);
45+ future = async_set_screen_power_mode();
46+ future_init.notify_one();
47+ }
48+
49+ // We're still holding a lock, so set_screen_power_mode should get scheduled and block
50+ // This is not deterministic, but helped reproduce the deadlock scenario
51+ std::this_thread::sleep_for(std::chrono::milliseconds{500});
52+ };
53+
54+ void wait_for_async_set_screen_power_mode()
55+ {
56+ {
57+ std::unique_lock<decltype(future_guard)> lock(future_guard);
58+ if (!future_init.wait_for(lock, timeout, [this] { return future.valid(); }))
59+ throw std::runtime_error{"Future is not valid!"};
60+ }
61+
62+ if (future.wait_for(timeout) != std::future_status::ready)
63 {
64 std::cerr << "Deadlock detected. Aborting." << std::endl;
65 abort();
66@@ -171,6 +184,10 @@
67 {power_off_timeout, dimmer_timeout}};
68
69 std::thread main_loop_thread;
70+
71+ std::mutex future_guard;
72+ std::condition_variable future_init;
73+ std::future<void> future;
74 };
75
76 }
77@@ -192,36 +209,20 @@
78
79 TEST_F(DeadlockLP1491566, between_dimmer_handler_and_screen_power_mode_request_is_averted)
80 {
81- std::future<void> future;
82-
83- mir_screen.before_dimmer_alarm_func =
84- [this, &future]
85- {
86- future = async_set_screen_power_mode();
87- // Wait a bit for async operation to get processed
88- std::this_thread::sleep_for(
89- std::chrono::milliseconds{500});
90- };
91+ mir_screen.before_dimmer_alarm_func = [this]
92+ { launch_async_set_screen_power_mode_then_try_to_deadlock(); };
93
94 schedule_inactivity_handlers();
95
96- wait_for_async_operation(future);
97+ wait_for_async_set_screen_power_mode();
98 }
99
100 TEST_F(DeadlockLP1491566, between_power_off_handler_and_screen_power_mode_request_is_averted)
101 {
102- std::future<void> future;
103-
104- mir_screen.before_power_off_alarm_func =
105- [this, &future]
106- {
107- future = async_set_screen_power_mode();
108- // Wait a bit for async operation to get processed
109- std::this_thread::sleep_for(
110- std::chrono::milliseconds{500});
111- };
112+ mir_screen.before_power_off_alarm_func = [this]
113+ { launch_async_set_screen_power_mode_then_try_to_deadlock(); };
114
115 schedule_inactivity_handlers();
116
117- wait_for_async_operation(future);
118+ wait_for_async_set_screen_power_mode();
119 }

Subscribers

People subscribed via source and target branches