[libcamera-devel] [PATCH 00/12] libcamera: Hardening against thread race conditions

[Laurent Pinchart]

On Sun, Jan 21, 2024 at 05:59:36AM +0200, Laurent Pinchart via libcamera-devel wrote:
> Hello,
> 
> Bug https://bugs.libcamera.org/show_bug.cgi?id=208 reports a race
> condition that is ultimately due to incorrect usage of the libcamera
> multi-threading infrastructure by the soft ISP code under development.
> Instead of blaming the author of that code, I believe it shows we're not
> doing well enough at communicating the threading requirements. This
> patch series is an attempt to improve the situation.
> 
> libcamera has a threading model that is documented. In particular, the
> documentation classifies functions as thread-safe, thread-bound and
> reentrant. Patch 01/12 starts by fixing a minor issue in the
> documentation of a thread-bound function that does not contain the
> correct reference.
> 
> Patch 02/12 is a drive-by improvement that is, strictly speaking,
> unrelated to this series, but was developed at the same time.
> 
> The next two patches fix a first thread-related issues with the
> Object::deleteLater() function. Patch 03/12 extends a unit test to
> display the issue, and patch 04/12 fixes it.
> 
> Patches 05/12 to 10/12 continue with fixing various kinds of incorrect
> deletion of objects in unit tests. It turned out that all the race
> conditions related to this patch series are in unit tests, libcamera
> itself isn't (as far as I could see) affected. This is good news, even
> if it means we haven't been careful enough when writing unit tests,
> which calls for improvements in that area in the future.
> 
> Patches 11/12 and 12/12 finally add assertions to complain loudly about
> incorrect deletion of Object instances (11/12) and incorrect calling
> contexts of thread-bound functions (12/12).
> 
> Only the thread-bound members of Object subclasses have been hardened,
> more work is needed to extend this to individual members of other
> classes (currently only DeviceEnumerator::enumerate()), and to classes
> that are marked as thread-bound at the class level (IPCUnixSocket and
> V4L2VideoDevice).
> 
> Even if V4L2VideoDevice isn't addressed directly by this series, and the
> bug report is related to usage of the V4L2VideoDevice class, I believe
> the issue in the soft ISP code will be caught by the assertions added to
> the EventNotifier class. Milan, would you be able to test this, and
> confirm that libcamera now complains loudly ?

Also, I would like more extensive testing on Raspberry Pi to make sure
this doesn't cause any regression. Naush, David, would you be able to
give this series a try ?

> Laurent Pinchart (12):
>   libcamera: object: Fix thread-bound reference in documentation
>   libcamera: signal: Replace object.h inclusion with forward declatation
>   test: object-delete: Test deferred delete just before thread stops
>   libcamera: thread: Ensure deferred deletion of all objects before
>     stopping
>   test: event-thread: Destroy Object from correct thread context
>   test: message: Remove incorrect slow receiver test
>   test: message: Destroy Object from correct thread context
>   test: signal-threads: Destroy Object from correct thread context
>   test: timer-thread: Move timer start from wrong thread to separate
>     test
>   test: timer-thread: Destroy Object from correct thread context
>   libcamera: object: Document and ensure Object deletion constraints
>   libcamera: object: Add and use thread-bound assertion
> 
>  include/libcamera/base/object.h               |   2 +
>  include/libcamera/base/signal.h               |   3 +-
>  src/libcamera/base/bound_method.cpp           |   1 +
>  src/libcamera/base/event_notifier.cpp         |   6 +
>  src/libcamera/base/object.cpp                 |  50 +++++++-
>  src/libcamera/base/signal.cpp                 |   1 +
>  src/libcamera/base/thread.cpp                 |   7 ++
>  src/libcamera/base/timer.cpp                  |  10 +-
>  test/event-thread.cpp                         |  38 +++++--
>  test/ipa/ipa_interface_test.cpp               |   1 +
>  test/meson.build                              |   9 +-
>  test/message.cpp                              |  54 +++------
>  test/object-delete.cpp                        |  30 ++++-
>  test/signal-threads.cpp                       |  24 ++--
>  test/timer-fail.cpp                           | 107 ++++++++++++++++++
>  test/timer-thread.cpp                         |  37 ++----
>  .../module_ipa_proxy.h.tmpl                   |   1 +
>  17 files changed, 276 insertions(+), 105 deletions(-)
>  create mode 100644 test/timer-fail.cpp
> 
> 
> base-commit: 0d99f2de13faccf199e6c9e806702cb21d0b6c24

-- 
Regards,

Laurent Pinchart