Mir

Merge lp://qastaging/~raof/mir/release-rendersurface-may-require-RPC into lp://qastaging/mir

  1. release-rendersurface-may-require-RPC
  2. Merge into development-branch
Proposed by Chris Halse Rogers
Status: Work in progress
Proposed branch: lp://qastaging/~raof/mir/release-rendersurface-may-require-RPC
Merge into: lp://qastaging/mir
Diff against target: 601 lines (+262/-40)
14 files modified
include/test/mir/test/validity_matchers.h (+8/-0)
playground/egldiamond_render_surface.c (+1/-1)
playground/mir_demo_client_chain_jumping_buffers.c (+2/-1)
playground/mir_demo_client_prerendered_frames.c (+1/-1)
playground/render_surface.cpp (+1/-1)
src/client/mir_connection.cpp (+14/-12)
src/client/mir_connection.h (+3/-1)
src/client/mir_render_surface_api.cpp (+31/-2)
src/client/symbols.map (+1/-0)
src/include/client/mir_toolkit/mir_render_surface.h (+13/-1)
tests/acceptance-tests/staging/test_render_surface.cpp (+147/-16)
tests/mir_test/CMakeLists.txt (+4/-0)
tests/mir_test/validity_matchers.cpp (+31/-0)
tests/unit-tests/client/test_mir_render_surface.cpp (+5/-4)
To merge this branch: bzr merge lp://qastaging/~raof/mir/release-rendersurface-may-require-RPC
Reviewer Review Type Date Requested Status
Cemil Azizoglu (community) Needs Fixing
Mir CI Bot continuous-integration Approve
Review via email: mp+312761@code.qastaging.launchpad.net

Commit message

Rework mir_render_surface_release() to explicitly identify the RPC wait involved.

In the case that the MirRenderSurface is backed by a MirBufferStream, releasing the MirRenderSurface
implies releasing the MirBufferStream, and releasing the MirBufferStream requires an RPC wait.

Other, future, MirRenderSurface backing objects might require RPC waits to release, too.

Split into the traditional mir_render_surface_release(rs, callback, ctx)/_sync() pairs.

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

FAILED: Continuous integration, rev:3871
https://mir-jenkins.ubuntu.com/job/mir-ci/2336/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/3050/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/3116
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/3108
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/3108
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/3108
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3079/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3079/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3079/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3079/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3079/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3079/console

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

review: Needs Fixing (continuous-integration)
Reply
lp://qastaging/~raof/mir/release-rendersurface-may-require-RPC updated
3872. By Chris Halse Rogers

Fix MirRenderSurfaceTest that make was mysteriously failing to rebuild

Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3872
https://mir-jenkins.ubuntu.com/job/mir-ci/2337/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/3051/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/3117
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/3109
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/3109
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/3109
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3080
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3080/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3080
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3080/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3080
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3080/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3080
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3080/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3080
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3080/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3080/console

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

review: Needs Fixing (continuous-integration)
Reply
Revision history for this message
Chris Halse Rogers (raof) wrote :

Bug #1647573, retriggering.

Reply
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3872
https://mir-jenkins.ubuntu.com/job/mir-ci/2342/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/3056/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/3122
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/3114
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/3114
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/3114
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3085
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3085/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3085
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3085/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3085
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3085/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3085
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3085/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3085
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3085/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3085/console

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

review: Needs Fixing (continuous-integration)
Reply
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3872
https://mir-jenkins.ubuntu.com/job/mir-ci/2348/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/3063
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/3129
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/3121
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/3121
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/3121
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3092
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3092/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3092
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3092/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3092
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3092/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3092
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3092/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3092
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3092/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3092
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3092/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Reply
Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

123 + StreamRelease stream_release{reinterpret_cast<MirBufferStream*>(render_surface),
124 + nullptr,
125 + reinterpret_cast<mir_buffer_stream_callback>(callback),
126 + ctx,
127 rs->stream_id().as_value(),
128 render_surface};

This is actually executed for PCs as well.

review: Needs Fixing
Reply
Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

I guess I don't understand why releasing of streams requires an RPC wait while that of chains doesn't, when the operation is the same (i.e. server.release_buffer_stream(...)).

Reply
Revision history for this message
Chris Halse Rogers (raof) wrote :

Releasing streams requires an RPC wait for two reasons:
1) It's a necessary client synchronisation point: clients almost always want to have some data associated with a Mir object, and they can't free that associated data until we can guarantee they'll never get a callback on that Mir object again.
2) The code
mir_render_surface_release(rs)
mir_connection_release(connection)
is currently racy, and will frequently lead to client deadlocks.

Like so:
  Both the rs_release and connection_release client requests will get queued up
  to the socket, the server sends the responses and then closes the connection.
  .
  The rs_release response results in erasing the bufferstream from the connection
  map.
  .
  The bufferstream destructor calls release_buffer on all of its allocated buffers.
  (This is why the current acceptance tests don't notice the problem - we don't
   wait for the bufferstream to actually be valid, so its allocation requests
   are unfulfilled at destruction time, so it never tries to release them.)
  .
  The buffer_release call hits the RPC layer, which notices that the socket has
  been closed and fires on_disconnected.
  .
  on_disconnected runs all the pending completions. The first one is...
  release_connection, which immediately tries to grab the connection_map
  lock, which we already hold way up in rs_release, and deadlock.

I guess both of these could be solved instead by
a) Ensuring that mir_render_surface_release does not return until it's guaranteed that no more callbacks will be fired on the underlying bufferstream, and
b) Ensuring that we don't deadlock in this scenario.

I'll see how difficult this is to do.

Reply
Revision history for this message
Chris Halse Rogers (raof) wrote :

Hm. (2) is actually a flaw in my follow-up code, so it's only (1) that we're concerned about...

Reply

Unmerged revisions

3872. By Chris Halse Rogers

Fix MirRenderSurfaceTest that make was mysteriously failing to rebuild

3871. By Chris Halse Rogers

Rework mir_render_surface_release() to explicitly identify the RPC wait involved.

In the case that the MirRenderSurface is backed by a MirBufferStream, releasing the MirRenderSurface
implies releasing the MirBufferStream, and releasing the MirBufferStream requires an RPC wait.

Other, future, MirRenderSurface backing objects might require RPC waits to release, too.

Split into the traditional mir_render_surface_release(rs, callback, ctx)/_sync() pairs.

3870. By Chris Halse Rogers

Add IsValid() implementations for MirBufferStream and MirRenderSurface

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches