Mir

BufferSwapperMulti::in_use_by_client counter drops below zero

Bug #1199717 reported by Daniel van Vugt
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Mir
Fix Released
High
Daniel van Vugt
Mir 0.0.9
mir (Ubuntu)
Fix Released
Undecided
Unassigned

Bug Description

In my debugging I discovered that BufferSwapperMulti::in_use_by_client frequently drops below zero. It looks like a logic error. See attached a patch which tests for this and results in integration tests hanging.

Related branches

lp://staging/~vanvugt/mir/switch
PS Jenkins bot (community): Approve (continuous-integration)
Alan Griffiths: Approve
Kevin DuBois (community): Abstain
Alexandros Frantzis (community): Approve
Diff: 5340 lines (+1332/-3186)
55 files modified
debian/control (+2/-2)
debian/libmirserver1.install (+1/-1)
examples/buffer_render_target.cpp (+8/-0)
examples/buffer_render_target.h (+1/-0)
include/server/mir/compositor/back_buffer_strategy.h (+0/-47)
include/server/mir/compositor/buffer_allocation_strategy.h (+0/-63)
include/server/mir/compositor/buffer_stream_factory.h (+6/-6)
include/server/mir/compositor/buffer_stream_surfaces.h (+0/-1)
include/server/mir/compositor/buffer_swapper.h (+0/-131)
include/server/mir/compositor/buffer_swapper_exceptions.h (+0/-42)
include/server/mir/compositor/buffer_swapper_multi.h (+0/-66)
include/server/mir/compositor/buffer_swapper_spin.h (+0/-73)
include/server/mir/compositor/multi_acquisition_back_buffer_strategy.h (+0/-66)
include/server/mir/compositor/swapper_factory.h (+0/-55)
include/server/mir/default_server_configuration.h (+0/-2)
include/test/mir_test_doubles/mock_buffer_bundle.h (+2/-0)
include/test/mir_test_doubles/mock_swapper.h (+0/-66)
include/test/mir_test_doubles/mock_swapper_factory.h (+0/-47)
src/server/CMakeLists.txt (+1/-1)
src/server/compositor/CMakeLists.txt (+0/-5)
src/server/compositor/buffer_bundle.h (+2/-0)
src/server/compositor/buffer_stream_factory.cpp (+5/-5)
src/server/compositor/buffer_stream_surfaces.cpp (+4/-7)
src/server/compositor/buffer_swapper_multi.cpp (+0/-174)
src/server/compositor/buffer_swapper_spin.cpp (+0/-114)
src/server/compositor/multi_acquisition_back_buffer_strategy.cpp (+0/-119)
src/server/compositor/rw_lock.cpp (+0/-71)
src/server/compositor/rw_lock.h (+0/-88)
src/server/compositor/swapper_factory.cpp (+0/-118)
src/server/compositor/switching_bundle.cpp (+352/-51)
src/server/compositor/switching_bundle.h (+55/-11)
src/server/compositor/temporary_buffers.cpp (+16/-7)
src/server/compositor/temporary_buffers.h (+13/-2)
src/server/default_server_configuration.cpp (+1/-14)
tests/integration-tests/compositor/CMakeLists.txt (+0/-1)
tests/integration-tests/compositor/test_buffer_stream.cpp (+132/-130)
tests/integration-tests/compositor/test_stress_buffer_swapper.cpp (+0/-308)
tests/integration-tests/compositor/test_swapping_swappers.cpp (+3/-6)
tests/integration-tests/graphics/android/test_buffer_integration.cpp (+5/-6)
tests/integration-tests/graphics/android/test_internal_client.cpp (+1/-4)
tests/integration-tests/shell/test_session.cpp (+1/-0)
tests/integration-tests/shell/test_session_manager.cpp (+0/-1)
tests/integration-tests/test_surfaceloop.cpp (+8/-39)
tests/unit-tests/compositor/CMakeLists.txt (+0/-6)
tests/unit-tests/compositor/test_buffer_stream.cpp (+14/-2)
tests/unit-tests/compositor/test_buffer_swapper.cpp (+0/-156)
tests/unit-tests/compositor/test_buffer_swapper_double.cpp (+0/-207)
tests/unit-tests/compositor/test_buffer_swapper_spin_triple.cpp (+0/-213)
tests/unit-tests/compositor/test_buffer_swapper_triple.cpp (+0/-125)
tests/unit-tests/compositor/test_rw_lock.cpp (+0/-173)
tests/unit-tests/compositor/test_swapper_factory.cpp (+0/-221)
tests/unit-tests/compositor/test_switching_bundle.cpp (+688/-111)
tests/unit-tests/compositor/test_temporary_buffers.cpp (+9/-21)
tests/unit-tests/graphics/android/test_internal_client_interpreter.cpp (+0/-1)
tests/unit-tests/surfaces/test_surface_stack.cpp (+2/-0)
lp://staging/ubuntu/saucy/mir
Revision history for this message
Daniel van Vugt (vanvugt) wrote :
Revision history for this message
kevin gunn (kgunn72) wrote :

should be fixed for 13.10

Changed in mir:
importance: Undecided → High
status: New → Triaged
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I think I see the problem. When the switching bundle switches, it only assigns free buffers to the new swapper. Non-free buffers are those still in use by a client/compositor.

So it's quite possible and correct that there will be more release calls than acquires after a switch. Either we should remove the client counter (because it's plain wrong), or we should augment it with a second counter to distinguish client acquires which happened before/after the latest switch.

Changed in mir:
assignee: nobody → Daniel van Vugt (vanvugt)
Daniel van Vugt (vanvugt)
Changed in mir:
status: Triaged → In Progress
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

Fix committed into lp:mir at revision 948, scheduled for release in mir, milestone 0.0.9

Changed in mir:
status: In Progress → Fix Committed
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package mir - 0.0.8+13.10.20130809.3-0ubuntu1

---------------
mir (0.0.8+13.10.20130809.3-0ubuntu1) saucy; urgency=low

  [ Daniel van Vugt ]
  * Simplify and generalize buffer swapping to support arbitrary numbers
    of: - buffers - compositors - clients This is a unified N-buffers
    algorithm which supports any positive number of buffers, as well as
    dynamically switching between synchronous and asynchronous
    behaviour. So it does everything the existing code does and more.
    The key requirement is to support an arbitrary number of (different)
    simultaneous compositor acquisitions, as is needed for bypass
    support (coming soon). This then leads to the requirement that a
    compositor and a snapshot buffer should be acquired differently.
    Because a snapshot should never consume frames, only observe them.
    On the other hand, a compositior acquire must consume a frame (if
    available) so as to guarantee correct ordering when more than one
    are acquired simultaneously in bypass mode. Also fixes LP: #1199717
    and is half the fix for LP: #1199450. (LP: #1199450, #1199717)

  [ Alan Griffiths ]
  * doc, tools: update instructions for "flipped" image.

  [ Christopher James Halse Rogers ]
  * Split out unversioned Mir platform package to fix upgrades across
    libmirserver SONAME bumps.

  [ Ubuntu daily release ]
  * Automatic snapshot from revision 950
 -- Ubuntu daily release <email address hidden> Fri, 09 Aug 2013 11:14:41 +0000

Changed in mir (Ubuntu):
status: New → Fix Released
Robert Ancell (robert-ancell)
Changed in mir:
milestone: none → 0.0.9
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Bug attachments

Remote bug watches

Bug watches keep track of this bug in other bug trackers.