From patchwork Sun Jan 21 03:59:36 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 19421 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 696B2C323E for ; Sun, 21 Jan 2024 03:59:48 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 3CAA662931; Sun, 21 Jan 2024 04:59:47 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org; s=mail; t=1705809587; bh=zZlxAAlvTAv7EwqxUZb1d/Hd1ai0KT5IedM0gVFsbUc=; h=To:Date:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=amMg+3Ark5rh0cPjJrBAc6WJ+9H0JA09Q/U1qbiaJkVtaLo/ISBf+YWmUOwa0UU7S PhjwCH3pnJkNBLE8SGxLoXze9MMfWLHj2F1pkSuuZ2xPt64Ktf76x0lPU4fKQCWW/c 1RBQcQgLRBT6yjZTI4TH0ayCyPyz9UcDL4HrerCdd5WW06VrwgPodZqxvNerx28aJq tGQXg6QiUn7aHuyeYw3eNuuGfzlb3PNe+IR/QgxM3xA7EvpQQqPhSDSzJx+kAIJy+d +o/o+wECB50Rc1E9mN9VxcP8Ly1tjFYLLLp4QRbl7K8DGEzwhiHSYSFHS9VdBEB5aQ FSsw77O7aj1Jw== Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B3F461D30 for ; Sun, 21 Jan 2024 04:59:45 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="ZjOIH90D"; dkim-atps=neutral Received: from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi [89.27.53.110]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 524C58D4; Sun, 21 Jan 2024 04:58:33 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1705809513; bh=zZlxAAlvTAv7EwqxUZb1d/Hd1ai0KT5IedM0gVFsbUc=; h=From:To:Cc:Subject:Date:From; b=ZjOIH90DxfYyNwynazwwvF6gwdp9/im03eJ3zOiDDmjauM6IS5g11QCx00OOf5IGl izuf9v7m46A6AwQuIDd8nTwx3HJ25XB4eMHK+elvW3+P+WtQRp0/QY7/9MOJqT1B8S O+vkFZIOUEo+LcsRRrWcNAJH3SOB96aHKipIwldY= To: libcamera-devel@lists.libcamera.org Date: Sun, 21 Jan 2024 05:59:36 +0200 Message-ID: <20240121035948.4226-1-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.43.0 MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 00/12] libcamera: Hardening against thread race conditions 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: , X-Patchwork-Original-From: Laurent Pinchart via libcamera-devel From: Laurent Pinchart Reply-To: Laurent Pinchart 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). 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 ? 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