Mir

Merge lp://qastaging/~alan-griffiths/mir/doodle-in-TimeoutFrameDroppingPolicy-code into lp://qastaging/mir

doodle-in-TimeoutFrameDroppingPolicy-code
Merge into development-branch

Proposed by Alan Griffiths on 2014-07-10

Status:	Rejected
Rejected by:	Alan Griffiths on 2014-07-15
Proposed branch:	lp://qastaging/~alan-griffiths/mir/doodle-in-TimeoutFrameDroppingPolicy-code
Merge into:	lp://qastaging/mir
Diff against target:	79 lines (+29/-13) 1 file modified src/server/compositor/timeout_frame_dropping_policy_factory.cpp (+29/-13)
To merge this branch:	bzr merge lp://qastaging/~alan-griffiths/mir/doodle-in-TimeoutFrameDroppingPolicy-code
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Alberto Aguirre (community)		2014-07-10	Needs Fixing on 2014-07-14
PS Jenkins bot (community)	continuous-integration		Approve on 2014-07-11
Review via email: mp+226341@code.qastaging.launchpad.net

Commit message

compositor: Use lock to avoid races in TimeoutFrameDroppingPolicy

Description of the change

compositor: use lock to avoid races in TimeoutFrameDroppingPolicy

The old implementation was accidentally reliant on the alarm implementation synchronizing alarm calls and invocation of the lambda. This revision aims to make TimeoutFrameDroppingPolicy thread safe.

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-07-10:

This:
64+ if ([&]{ std::lock_guard<std::mutex> lock{mutex}; return --pending_swaps > 0; }())

should be:

bool update_alarm{false};
bool reschedule{false};
{
    std::lock_guard<std::mutex> lock{mutex};
    update_alarm = pending_swaps > 0;
    reschedule = update_alarm && --pending_swaps > 0;
}

if (update_alarm)
{
    if (reschedule)
        alarm->reschedule_in(timeout);
    else
        alarm->cancel();
}

This should address the following scenario:

Assume pending_swaps = 1

timeout occurs and Thread A executes alarm handler,

since pending_swaps = 1, after

37+ call_drop = pending_swaps > 0;
38+ reshedule = call_drop && --pending_swaps > 0;

pending_swaps = 0
call_drop = true
reshedule = false

Let's say Thread A gets pre-empted before invoking drop_frame()

Thread B calls swap_unblocked, now pending_swaps = -1

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-11:

FAILED: Continuous integration, rev:1764
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2134/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/916
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/922/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/915
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/655
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/655/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/653
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/653/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2938
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2938/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2056
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/9679

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-07-11:

Failure looks like lp:1336671 (assuming fixed umockdev package is yet to land in archive: it shows as "committed" not "released")

Rebuilding

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-11:

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

review: Approve (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-07-11:

LGTM

review: Approve

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2014-07-14:

Well given that alarm cancel is back to the original semantics (like in 0.3.0) then swap_unblocked should attempt to cancel alarm first so that if it returns false, then it means the callback handler is running or about to run, so pending swaps should not be touched or the alarm rescheduled (since it will be done by the callback handler)

review: Needs Fixing

Unmerged revisions

1764. By Alan Griffiths on 2014-07-11: merge lp:~mir-team/mir/development-branch
1763. By Alan Griffiths on 2014-07-11: Alberto's fix for race decrementing pending_swaps
1762. By Alan Griffiths on 2014-07-10: Avoid some raciness in TimeoutFrameDroppingPolicy