From patchwork Tue Jan 23 01:12:37 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 19448 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id CEA42C323E for ; Tue, 23 Jan 2024 01:12:48 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id B9CF762936; Tue, 23 Jan 2024 02:12:47 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="EgyOMzaf"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 622D861D46 for ; Tue, 23 Jan 2024 02:12:46 +0100 (CET) Received: from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi [89.27.53.110]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id C868BE4; Tue, 23 Jan 2024 02:11:32 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1705972293; bh=QXS2LLJyme/qlJQczTtL6Qm7dcm6cXkEvzIe304Xo6I=; h=From:To:Cc:Subject:Date:From; b=EgyOMzafpS1b9zcdQpvZvg3ZcBKs4rEkZ5qhiVd+w+DA+vwBreK8HSSvMViXFZu8S 6JrlMDRXVcnxoTQpBkeKOblPvBNfeky5nAtoMTpyaUxLN6L/61d/RXDoSM64WUQphH QocfEJJiCojdTsWtEQFkQfhKgCIC7OFC1Dlgb/C4= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Subject: [PATCH v2 00/12] libcamera: Hardening against thread race conditions Date: Tue, 23 Jan 2024 03:12:37 +0200 Message-ID: <20240123011249.22716-1-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.43.0 MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" 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). Compared to v1, small issues reported during review have been addressed. Please see individual patches for detailed changelogs. 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 | 55 ++++++++- 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 | 109 ++++++++++++++++++ test/timer-thread.cpp | 37 ++---- .../module_ipa_proxy.h.tmpl | 1 + 17 files changed, 281 insertions(+), 107 deletions(-) create mode 100644 test/timer-fail.cpp base-commit: 6fb55e1e70426207e538a88b39081bdc644b6d87