[{"id":35410,"web_url":"https://patchwork.libcamera.org/comment/35410/","msgid":"<20250814135623.GB8360@pendragon.ideasonboard.com>","date":"2025-08-14T13:56:23","subject":"Re: [RFC PATCH v1] libcamera: thread: Fix event dispatcher creation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Tue, Jan 28, 2025 at 01:03:14PM +0000, Barnabás Pőcze wrote:\n> Use an appropriate compare-and-exchange atomic instruction\n> to set the event dispatcher pointer. This ensures that there\n> is only ever a single event dispatcher associated with a\n> given thread.\n> \n> Consider e.g. the following series of events:\n> \n> 1. thread 0 :: if (!data_->dispatcher_.load(std::memory_order_relaxed))\n> // reads nullptr and proceeds into the `if`'s block\n> \n> 2. thread 1 :: if (!data_->dispatcher_.load(std::memory_order_relaxed))\n> // reads nullptr and proceeds into the `if`'s block\n> \n> 3. thread 0 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release);\n> // creates dispatcher and sets pointer\n> \n> 4. thread 1 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release);\n> // creates dispatcher and sets pointer\n> \n> At the end, one of the event dispatchers is leaked, and\n> furthermore the `eventDispatcher()` calls might return\n> different values in the calling threads.\n\nYes, the current code is clearly broken if called from different threads\nconcurrently.\n\nThe question is, do we want to call this function from different threads\n? As far as I can see, it's only called for the current thread in all\nlocations. By definition that means the thread is started, with its\nrun() function called. By default run() calls exec(), which creates the\nevent dispatcher. Even if a thread reimplements run() and either doesn't\ncall exec(), or call eventDispatcher() before calling exec(), we should\nbe safe.\n\nThe only unsafe situation would be calling eventDispatcher() from a\nseparate thread. The eventDispatcher() function is documented as\nthread-safe, so its API allows that, even if the implementation doesn't.\nDo we want to allow that ? Is there a use case for accessing the event\ndispatcher from other threads (other than through the Thread class API,\nsuch as Thread::exit()) ?\n\nWe currently need to access the event dispatcher to manage event\nnotifiers and timers. In particular, the following functions access the\nevent dispatcher *of the current thread*:\n\n- EventNotifier::setEnabled()\n- Timer::registerTimer()\n- Timer::unregisterTimer()\n- IPCPipeUnixSocket::call()\n- IPAProxy*Worker::run()\n\nThe first three functions can only be called from the thread the event\nnotifier or timer belong to.\n\nUsage of the event dispatcher in IPCPipeUnixSocket::call() is a bit of a\nhack that I'd like to fix, but in any case the function only accesses\nthe event dispatcher of the current thread.\n\nIPAProxy*Worker::run() also accesses the event dispatcher of the current\nthread, but in this case the Thread is not running (as in having his\nrun() function called) as it's a MainThread instance that represents a\nmain application thread, outside of the control of libcamera.\n\nIt should therefore be safe to consider Thread::eventDispatcher() as\ncallable only from its Thread.\n\nWe could even take it one step further, and create the event dispatcher\nin Thread::exec() instead of in Thread::eventDispatcher(). We would have\nto consider some corner cases, such as moving a timer or event notifier\nto a thread that has not been started yet. Callers of\nThread::eventDispatcher() may need to perform null checks. We would also\nneed to figure out how to handle IPAProxy*Worker::run().\n\nWhat do you think ?\n\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/libcamera/base/thread.cpp | 13 +++++++++----\n> 1 file changed, 9 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> index de60567f6..93ec7b5a0 100644\n> --- a/src/libcamera/base/thread.cpp\n> +++ b/src/libcamera/base/thread.cpp\n> @@ -518,11 +518,16 @@ pid_t Thread::currentId()\n> */\n> EventDispatcher *Thread::eventDispatcher()\n> {\n> -\tif (!data_->dispatcher_.load(std::memory_order_relaxed))\n> -\t\tdata_->dispatcher_.store(new EventDispatcherPoll(),\n> -\t\t\t\t\t std::memory_order_release);\n> +\tif (EventDispatcher *dispatcher = data_->dispatcher_.load(std::memory_order_acquire))\n> +\t\treturn dispatcher;\n> \n> -\treturn data_->dispatcher_.load(std::memory_order_relaxed);\n> +\tauto dispatcher = std::make_unique();\n> +\tEventDispatcher *expected = nullptr;\n> +\n> +\tif (data_->dispatcher_.compare_exchange_strong(expected, dispatcher.get(), std::memory_order_acq_rel))\n> +\t\treturn dispatcher.release();\n> +\n> +\treturn expected;\n> }\n> \n> /**","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 24477BDCC1\n\tfor ;\n\tThu, 14 Aug 2025 13:56:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F04C469254;\n\tThu, 14 Aug 2025 15:56:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D6B6661444\n\tfor ;\n\tThu, 14 Aug 2025 15:56:43 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 611B36DF;\n\tThu, 14 Aug 2025 15:55:49 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OikRrDb2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755179749;\n\tbh=75TvAo5TqlC/Mk63532yJ0GBoXBD0y4/X4UX2nXoHbY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OikRrDb2zIMC6gstVzM5Vngx1V72vanUZh/PKxAGCuECah+9XWUmtGEI33eWILbpO\n\tZaWaG78P2sIgc7ki811YE4Laqy5k5mhicBkz1204Ept+ZA7e3uDdIl58X6hQ8MntnM\n\tEihYzrwuvXPYEEDnOWbJ3PiI63BEw6UeuG73esM8=","Date":"Thu, 14 Aug 2025 16:56:23 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1] libcamera: thread: Fix event dispatcher creation","Message-ID":"<20250814135623.GB8360@pendragon.ideasonboard.com>","References":"<20250128130308.509297-1-pobrn@protonmail.com>\n\t<20250128130308.509297-2-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250128130308.509297-2-pobrn@protonmail.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35411,"web_url":"https://patchwork.libcamera.org/comment/35411/","msgid":"","date":"2025-08-14T14:30:14","subject":"Re: [RFC PATCH v1] libcamera: thread: Fix event dispatcher creation","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 08. 14. 15:56 keltezéssel, Laurent Pinchart írta:\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> On Tue, Jan 28, 2025 at 01:03:14PM +0000, Barnabás Pőcze wrote:\n>> Use an appropriate compare-and-exchange atomic instruction\n>> to set the event dispatcher pointer. This ensures that there\n>> is only ever a single event dispatcher associated with a\n>> given thread.\n>>\n>> Consider e.g. the following series of events:\n>>\n>> 1. thread 0 :: if (!data_->dispatcher_.load(std::memory_order_relaxed))\n>> // reads nullptr and proceeds into the `if`'s block\n>>\n>> 2. thread 1 :: if (!data_->dispatcher_.load(std::memory_order_relaxed))\n>> // reads nullptr and proceeds into the `if`'s block\n>>\n>> 3. thread 0 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release);\n>> // creates dispatcher and sets pointer\n>>\n>> 4. thread 1 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release);\n>> // creates dispatcher and sets pointer\n>>\n>> At the end, one of the event dispatchers is leaked, and\n>> furthermore the `eventDispatcher()` calls might return\n>> different values in the calling threads.\n> \n> Yes, the current code is clearly broken if called from different threads\n> concurrently.\n> \n> The question is, do we want to call this function from different threads\n> ? As far as I can see, it's only called for the current thread in all\n> locations. By definition that means the thread is started, with its\n> run() function called. By default run() calls exec(), which creates the\n> event dispatcher. Even if a thread reimplements run() and either doesn't\n> call exec(), or call eventDispatcher() before calling exec(), we should\n> be safe.\n> \n> The only unsafe situation would be calling eventDispatcher() from a\n> separate thread. The eventDispatcher() function is documented as\n> thread-safe, so its API allows that, even if the implementation doesn't.\n> Do we want to allow that ? Is there a use case for accessing the event\n> dispatcher from other threads (other than through the Thread class API,\n> such as Thread::exit()) ?\n> \n> We currently need to access the event dispatcher to manage event\n> notifiers and timers. In particular, the following functions access the\n> event dispatcher *of the current thread*:\n> \n> - EventNotifier::setEnabled()\n> - Timer::registerTimer()\n> - Timer::unregisterTimer()\n> - IPCPipeUnixSocket::call()\n> - IPAProxy*Worker::run()\n> \n> The first three functions can only be called from the thread the event\n> notifier or timer belong to.\n> \n> Usage of the event dispatcher in IPCPipeUnixSocket::call() is a bit of a\n> hack that I'd like to fix, but in any case the function only accesses\n> the event dispatcher of the current thread.\n> \n> IPAProxy*Worker::run() also accesses the event dispatcher of the current\n> thread, but in this case the Thread is not running (as in having his\n> run() function called) as it's a MainThread instance that represents a\n> main application thread, outside of the control of libcamera.\n> \n> It should therefore be safe to consider Thread::eventDispatcher() as\n> callable only from its Thread.\n> \n> We could even take it one step further, and create the event dispatcher\n> in Thread::exec() instead of in Thread::eventDispatcher(). We would have\n> to consider some corner cases, such as moving a timer or event notifier\n> to a thread that has not been started yet. Callers of\n> Thread::eventDispatcher() may need to perform null checks. We would also\n> need to figure out how to handle IPAProxy*Worker::run().\n> \n> What do you think ?\n\nThe motivation for this change, as far as I remember, was that the documentation\nstates that this function is thread safe. The variable also has an atomic type,\nso I did not do more investigations because the the intent seemed clear.\n\nIf the conclusion is that thread safety is not needed, then I believe the mention of\nthread safety should be dropped from the documentation and the type modified to be non-atomic.\n\nI don't have a concrete opinion on creating the event loop just in exec(). I suppose\nit could make sense, but I'd probably decouple the two types first by removing\n`Thread::current()->dispatchMessages()` from `EventDispatcherPoll::processEvents`\nand instead registering an eventfd for the messages.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/libcamera/base/thread.cpp | 13 +++++++++----\n>> 1 file changed, 9 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n>> index de60567f6..93ec7b5a0 100644\n>> --- a/src/libcamera/base/thread.cpp\n>> +++ b/src/libcamera/base/thread.cpp\n>> @@ -518,11 +518,16 @@ pid_t Thread::currentId()\n>> */\n>> EventDispatcher *Thread::eventDispatcher()\n>> {\n>> -\tif (!data_->dispatcher_.load(std::memory_order_relaxed))\n>> -\t\tdata_->dispatcher_.store(new EventDispatcherPoll(),\n>> -\t\t\t\t\t std::memory_order_release);\n>> +\tif (EventDispatcher *dispatcher = data_->dispatcher_.load(std::memory_order_acquire))\n>> +\t\treturn dispatcher;\n>>\n>> -\treturn data_->dispatcher_.load(std::memory_order_relaxed);\n>> +\tauto dispatcher = std::make_unique();\n>> +\tEventDispatcher *expected = nullptr;\n>> +\n>> +\tif (data_->dispatcher_.compare_exchange_strong(expected, dispatcher.get(), std::memory_order_acq_rel))\n>> +\t\treturn dispatcher.release();\n>> +\n>> +\treturn expected;\n>> }\n>>\n>> /**\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 429ABBEFBE\n\tfor ;\n\tThu, 14 Aug 2025 14:30:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 596C269254;\n\tThu, 14 Aug 2025 16:30:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C66B661444\n\tfor ;\n\tThu, 14 Aug 2025 16:30:18 +0200 (CEST)","from [192.168.33.15] (185.221.141.188.nat.pool.zt.hu\n\t[185.221.141.188])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 454757F0;\n\tThu, 14 Aug 2025 16:29:24 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"CpXMYnjj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755181764;\n\tbh=sS6FFluYqYDEXgGV2zHEkcTyhqs0IHJyOgO7KT7TPaQ=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=CpXMYnjj1krqkjZO2KL11CnYs3O/h7s8IbprdoOyPZg6WVz013lgagEiQclHM1bF0\n\tXzSNatL1XLKWDS8y0YNxSLiOsq8M5Xq6jTxW1VmDOoWnPUJySlmZ+xOs0VrTuJ8JTB\n\tgTgMd46fSvkGV5NXF3kjuCKThdZ2RSoYzrbIH9zY=","Message-ID":"","Date":"Thu, 14 Aug 2025 16:30:14 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1] libcamera: thread: Fix event dispatcher creation","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250128130308.509297-1-pobrn@protonmail.com>\n\t<20250128130308.509297-2-pobrn@protonmail.com>\n\t<20250814135623.GB8360@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250814135623.GB8360@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35412,"web_url":"https://patchwork.libcamera.org/comment/35412/","msgid":"<20250814144100.GC8360@pendragon.ideasonboard.com>","date":"2025-08-14T14:41:00","subject":"Re: [RFC PATCH v1] libcamera: thread: Fix event dispatcher creation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Aug 14, 2025 at 04:30:14PM +0200, Barnabás Pőcze wrote:\n> 2025. 08. 14. 15:56 keltezéssel, Laurent Pinchart írta:\n> > On Tue, Jan 28, 2025 at 01:03:14PM +0000, Barnabás Pőcze wrote:\n> >> Use an appropriate compare-and-exchange atomic instruction\n> >> to set the event dispatcher pointer. This ensures that there\n> >> is only ever a single event dispatcher associated with a\n> >> given thread.\n> >>\n> >> Consider e.g. the following series of events:\n> >>\n> >> 1. thread 0 :: if (!data_->dispatcher_.load(std::memory_order_relaxed))\n> >> // reads nullptr and proceeds into the `if`'s block\n> >>\n> >> 2. thread 1 :: if (!data_->dispatcher_.load(std::memory_order_relaxed))\n> >> // reads nullptr and proceeds into the `if`'s block\n> >>\n> >> 3. thread 0 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release);\n> >> // creates dispatcher and sets pointer\n> >>\n> >> 4. thread 1 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release);\n> >> // creates dispatcher and sets pointer\n> >>\n> >> At the end, one of the event dispatchers is leaked, and\n> >> furthermore the `eventDispatcher()` calls might return\n> >> different values in the calling threads.\n> > \n> > Yes, the current code is clearly broken if called from different threads\n> > concurrently.\n> > \n> > The question is, do we want to call this function from different threads\n> > ? As far as I can see, it's only called for the current thread in all\n> > locations. By definition that means the thread is started, with its\n> > run() function called. By default run() calls exec(), which creates the\n> > event dispatcher. Even if a thread reimplements run() and either doesn't\n> > call exec(), or call eventDispatcher() before calling exec(), we should\n> > be safe.\n> > \n> > The only unsafe situation would be calling eventDispatcher() from a\n> > separate thread. The eventDispatcher() function is documented as\n> > thread-safe, so its API allows that, even if the implementation doesn't.\n> > Do we want to allow that ? Is there a use case for accessing the event\n> > dispatcher from other threads (other than through the Thread class API,\n> > such as Thread::exit()) ?\n> > \n> > We currently need to access the event dispatcher to manage event\n> > notifiers and timers. In particular, the following functions access the\n> > event dispatcher *of the current thread*:\n> > \n> > - EventNotifier::setEnabled()\n> > - Timer::registerTimer()\n> > - Timer::unregisterTimer()\n> > - IPCPipeUnixSocket::call()\n> > - IPAProxy*Worker::run()\n> > \n> > The first three functions can only be called from the thread the event\n> > notifier or timer belong to.\n> > \n> > Usage of the event dispatcher in IPCPipeUnixSocket::call() is a bit of a\n> > hack that I'd like to fix, but in any case the function only accesses\n> > the event dispatcher of the current thread.\n> > \n> > IPAProxy*Worker::run() also accesses the event dispatcher of the current\n> > thread, but in this case the Thread is not running (as in having his\n> > run() function called) as it's a MainThread instance that represents a\n> > main application thread, outside of the control of libcamera.\n> > \n> > It should therefore be safe to consider Thread::eventDispatcher() as\n> > callable only from its Thread.\n> > \n> > We could even take it one step further, and create the event dispatcher\n> > in Thread::exec() instead of in Thread::eventDispatcher(). We would have\n> > to consider some corner cases, such as moving a timer or event notifier\n> > to a thread that has not been started yet. Callers of\n> > Thread::eventDispatcher() may need to perform null checks. We would also\n> > need to figure out how to handle IPAProxy*Worker::run().\n> > \n> > What do you think ?\n> \n> The motivation for this change, as far as I remember, was that the documentation\n> states that this function is thread safe. The variable also has an atomic type,\n> so I did not do more investigations because the the intent seemed clear.\n> \n> If the conclusion is that thread safety is not needed, then I believe the mention of\n> thread safety should be dropped from the documentation and the type modified to be non-atomic.\n\nNote that eventDispatcher_ still needs to be accessed from other threads\nthrough Thread::exit() and Thread::postMessage(). I haven't checked if\nthis requires (or could benefit from) an atomic type.\n\n> I don't have a concrete opinion on creating the event loop just in exec(). I suppose\n> it could make sense, but I'd probably decouple the two types first by removing\n> `Thread::current()->dispatchMessages()` from `EventDispatcherPoll::processEvents`\n> and instead registering an eventfd for the messages.\n\nWouldn't that impact performance ? Messages are completely internal to\nlibcamera, I'd keep them that way if possible.\n\nIn any case, we can decouple the thread safety issue from the event\ndispatcher creation issue. Let's address the former for now.\n\n> >> Signed-off-by: Barnabás Pőcze \n> >> ---\n> >> src/libcamera/base/thread.cpp | 13 +++++++++----\n> >> 1 file changed, 9 insertions(+), 4 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> >> index de60567f6..93ec7b5a0 100644\n> >> --- a/src/libcamera/base/thread.cpp\n> >> +++ b/src/libcamera/base/thread.cpp\n> >> @@ -518,11 +518,16 @@ pid_t Thread::currentId()\n> >> */\n> >> EventDispatcher *Thread::eventDispatcher()\n> >> {\n> >> -\tif (!data_->dispatcher_.load(std::memory_order_relaxed))\n> >> -\t\tdata_->dispatcher_.store(new EventDispatcherPoll(),\n> >> -\t\t\t\t\t std::memory_order_release);\n> >> +\tif (EventDispatcher *dispatcher = data_->dispatcher_.load(std::memory_order_acquire))\n> >> +\t\treturn dispatcher;\n> >>\n> >> -\treturn data_->dispatcher_.load(std::memory_order_relaxed);\n> >> +\tauto dispatcher = std::make_unique();\n> >> +\tEventDispatcher *expected = nullptr;\n> >> +\n> >> +\tif (data_->dispatcher_.compare_exchange_strong(expected, dispatcher.get(), std::memory_order_acq_rel))\n> >> +\t\treturn dispatcher.release();\n> >> +\n> >> +\treturn expected;\n> >> }\n> >>\n> >> /**\n> > \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 923ADBDCC1\n\tfor ;\n\tThu, 14 Aug 2025 14:41:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BAB6169251;\n\tThu, 14 Aug 2025 16:41:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CD8B469247\n\tfor ;\n\tThu, 14 Aug 2025 16:41:20 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 52D597E6;\n\tThu, 14 Aug 2025 16:40:26 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"CfZZYO/E\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755182426;\n\tbh=61Ha1jw7tPCkrlKlUZDCwd+CehrW0KZpNwlhlaF+tOU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CfZZYO/EzC9wIoHUAsMVxUCDXgfwgvCSrdgp3uMySNSNFSZNx/2N0gJzMjEtKteEx\n\t5OjPI5+8mwNAM4L1lYEDIQOKTM+SU6L7sCT7asVtxVJhSmaFVjEfa2FaN8v2m+GuWr\n\tcU2qRIgJC+Xzk0RGW8LUTO7F/8Wh0Ph7zPdm9jrk=","Date":"Thu, 14 Aug 2025 17:41:00 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1] libcamera: thread: Fix event dispatcher creation","Message-ID":"<20250814144100.GC8360@pendragon.ideasonboard.com>","References":"<20250128130308.509297-1-pobrn@protonmail.com>\n\t<20250128130308.509297-2-pobrn@protonmail.com>\n\t<20250814135623.GB8360@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35413,"web_url":"https://patchwork.libcamera.org/comment/35413/","msgid":"<2444af88-4a8d-4be4-9ebc-480c90c878da@ideasonboard.com>","date":"2025-08-14T15:03:39","subject":"Re: [RFC PATCH v1] libcamera: thread: Fix event dispatcher creation","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 08. 14. 16:41 keltezéssel, Laurent Pinchart írta:\n> On Thu, Aug 14, 2025 at 04:30:14PM +0200, Barnabás Pőcze wrote:\n>> 2025. 08. 14. 15:56 keltezéssel, Laurent Pinchart írta:\n>>> On Tue, Jan 28, 2025 at 01:03:14PM +0000, Barnabás Pőcze wrote:\n>>>> Use an appropriate compare-and-exchange atomic instruction\n>>>> to set the event dispatcher pointer. This ensures that there\n>>>> is only ever a single event dispatcher associated with a\n>>>> given thread.\n>>>>\n>>>> Consider e.g. the following series of events:\n>>>>\n>>>> 1. thread 0 :: if (!data_->dispatcher_.load(std::memory_order_relaxed))\n>>>> // reads nullptr and proceeds into the `if`'s block\n>>>>\n>>>> 2. thread 1 :: if (!data_->dispatcher_.load(std::memory_order_relaxed))\n>>>> // reads nullptr and proceeds into the `if`'s block\n>>>>\n>>>> 3. thread 0 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release);\n>>>> // creates dispatcher and sets pointer\n>>>>\n>>>> 4. thread 1 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release);\n>>>> // creates dispatcher and sets pointer\n>>>>\n>>>> At the end, one of the event dispatchers is leaked, and\n>>>> furthermore the `eventDispatcher()` calls might return\n>>>> different values in the calling threads.\n>>>\n>>> Yes, the current code is clearly broken if called from different threads\n>>> concurrently.\n>>>\n>>> The question is, do we want to call this function from different threads\n>>> ? As far as I can see, it's only called for the current thread in all\n>>> locations. By definition that means the thread is started, with its\n>>> run() function called. By default run() calls exec(), which creates the\n>>> event dispatcher. Even if a thread reimplements run() and either doesn't\n>>> call exec(), or call eventDispatcher() before calling exec(), we should\n>>> be safe.\n>>>\n>>> The only unsafe situation would be calling eventDispatcher() from a\n>>> separate thread. The eventDispatcher() function is documented as\n>>> thread-safe, so its API allows that, even if the implementation doesn't.\n>>> Do we want to allow that ? Is there a use case for accessing the event\n>>> dispatcher from other threads (other than through the Thread class API,\n>>> such as Thread::exit()) ?\n>>>\n>>> We currently need to access the event dispatcher to manage event\n>>> notifiers and timers. In particular, the following functions access the\n>>> event dispatcher *of the current thread*:\n>>>\n>>> - EventNotifier::setEnabled()\n>>> - Timer::registerTimer()\n>>> - Timer::unregisterTimer()\n>>> - IPCPipeUnixSocket::call()\n>>> - IPAProxy*Worker::run()\n>>>\n>>> The first three functions can only be called from the thread the event\n>>> notifier or timer belong to.\n>>>\n>>> Usage of the event dispatcher in IPCPipeUnixSocket::call() is a bit of a\n>>> hack that I'd like to fix, but in any case the function only accesses\n>>> the event dispatcher of the current thread.\n>>>\n>>> IPAProxy*Worker::run() also accesses the event dispatcher of the current\n>>> thread, but in this case the Thread is not running (as in having his\n>>> run() function called) as it's a MainThread instance that represents a\n>>> main application thread, outside of the control of libcamera.\n>>>\n>>> It should therefore be safe to consider Thread::eventDispatcher() as\n>>> callable only from its Thread.\n>>>\n>>> We could even take it one step further, and create the event dispatcher\n>>> in Thread::exec() instead of in Thread::eventDispatcher(). We would have\n>>> to consider some corner cases, such as moving a timer or event notifier\n>>> to a thread that has not been started yet. Callers of\n>>> Thread::eventDispatcher() may need to perform null checks. We would also\n>>> need to figure out how to handle IPAProxy*Worker::run().\n>>>\n>>> What do you think ?\n>>\n>> The motivation for this change, as far as I remember, was that the documentation\n>> states that this function is thread safe. The variable also has an atomic type,\n>> so I did not do more investigations because the the intent seemed clear.\n>>\n>> If the conclusion is that thread safety is not needed, then I believe the mention of\n>> thread safety should be dropped from the documentation and the type modified to be non-atomic.\n> \n> Note that eventDispatcher_ still needs to be accessed from other threads\n> through Thread::exit() and Thread::postMessage(). I haven't checked if\n> this requires (or could benefit from) an atomic type.\n\nAhh, that is true. `Thread::postMessage()` most certainly needs atomic access to\nthe ptr. Same with `Thread::exit()`.\n\n\n> \n>> I don't have a concrete opinion on creating the event loop just in exec(). I suppose\n>> it could make sense, but I'd probably decouple the two types first by removing\n>> `Thread::current()->dispatchMessages()` from `EventDispatcherPoll::processEvents`\n>> and instead registering an eventfd for the messages.\n> \n> Wouldn't that impact performance ? Messages are completely internal to\n> libcamera, I'd keep them that way if possible.\n\nI don't see a substantial difference. Now the message passing mechanism\nuses `EventDispatcher::interrupt()`, which will still just signal an\neventfd. The main difference is where that eventfd is. I would argue\nthat by keeping it in `Thread`, and letting `EventDispatcher` not to\nhave to deal with message passing, things become more separated.\n\n\n> \n> In any case, we can decouple the thread safety issue from the event\n> dispatcher creation issue. Let's address the former for now.\n\nSo I suppose the question is whether `Thread::eventDispatcher()` should be\nthread safe or not. I don't have a concrete opinion on that. I believe the\nproposed changes here shouldn't incur significant performance penalty, so\nwe might want to make it thread safe. Although arguably `EventDispatcher::interrupt()`\nis the only thread safe function, so it might not have a lot of utility.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>>>> Signed-off-by: Barnabás Pőcze \n>>>> ---\n>>>> src/libcamera/base/thread.cpp | 13 +++++++++----\n>>>> 1 file changed, 9 insertions(+), 4 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n>>>> index de60567f6..93ec7b5a0 100644\n>>>> --- a/src/libcamera/base/thread.cpp\n>>>> +++ b/src/libcamera/base/thread.cpp\n>>>> @@ -518,11 +518,16 @@ pid_t Thread::currentId()\n>>>> */\n>>>> EventDispatcher *Thread::eventDispatcher()\n>>>> {\n>>>> -\tif (!data_->dispatcher_.load(std::memory_order_relaxed))\n>>>> -\t\tdata_->dispatcher_.store(new EventDispatcherPoll(),\n>>>> -\t\t\t\t\t std::memory_order_release);\n>>>> +\tif (EventDispatcher *dispatcher = data_->dispatcher_.load(std::memory_order_acquire))\n>>>> +\t\treturn dispatcher;\n>>>>\n>>>> -\treturn data_->dispatcher_.load(std::memory_order_relaxed);\n>>>> +\tauto dispatcher = std::make_unique();\n>>>> +\tEventDispatcher *expected = nullptr;\n>>>> +\n>>>> +\tif (data_->dispatcher_.compare_exchange_strong(expected, dispatcher.get(), std::memory_order_acq_rel))\n>>>> +\t\treturn dispatcher.release();\n>>>> +\n>>>> +\treturn expected;\n>>>> }\n>>>>\n>>>> /**\n>>>\n>>\n> \n> --\n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D33B4BDCC1\n\tfor ;\n\tThu, 14 Aug 2025 15:03:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E5C3361444;\n\tThu, 14 Aug 2025 17:03:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9868D61444\n\tfor ;\n\tThu, 14 Aug 2025 17:03:42 +0200 (CEST)","from [192.168.33.15] (185.221.141.188.nat.pool.zt.hu\n\t[185.221.141.188])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4AB4E6DF;\n\tThu, 14 Aug 2025 17:02:48 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"eVyB+lzm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755183768;\n\tbh=wHg2E0P9acZqUX8Tsck1vsZSrp1gG7IS4RKtneoD9b8=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=eVyB+lzmUIUcsgAZzutwNz4YEpL8M6EMRtMOYEo83r55Tt3UYD/xiZXuAv6DULddl\n\t69a1fH/na9IkN+gSCejiOh23wTIvzZ+e4gOydHb/f8yFFJNamnA8BTlRTOGKL4KdPS\n\tY/k2rJRfF9/v8m08PDCzLqd2rganK7zeCqdsxU2o=","Message-ID":"<2444af88-4a8d-4be4-9ebc-480c90c878da@ideasonboard.com>","Date":"Thu, 14 Aug 2025 17:03:39 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1] libcamera: thread: Fix event dispatcher creation","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250128130308.509297-1-pobrn@protonmail.com>\n\t<20250128130308.509297-2-pobrn@protonmail.com>\n\t<20250814135623.GB8360@pendragon.ideasonboard.com>\n\t\n\t<3Isvs6qN9a1edD5-zdKYCpHRFURjViUsDZt1paZcQjJ28ICnrouA-m7nCdSN9G2QkTmXNcl0HKySYn9QxKcYgw==@protonmail.internalid>\n\t<20250814144100.GC8360@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250814144100.GC8360@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35418,"web_url":"https://patchwork.libcamera.org/comment/35418/","msgid":"<20250814220245.GC6201@pendragon.ideasonboard.com>","date":"2025-08-14T22:02:45","subject":"Re: [RFC PATCH v1] libcamera: thread: Fix event dispatcher creation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Aug 14, 2025 at 05:03:39PM +0200, Barnabás Pőcze wrote:\n> 2025. 08. 14. 16:41 keltezéssel, Laurent Pinchart írta:\n> > On Thu, Aug 14, 2025 at 04:30:14PM +0200, Barnabás Pőcze wrote:\n> >> 2025. 08. 14. 15:56 keltezéssel, Laurent Pinchart írta:\n> >>> On Tue, Jan 28, 2025 at 01:03:14PM +0000, Barnabás Pőcze wrote:\n> >>>> Use an appropriate compare-and-exchange atomic instruction\n> >>>> to set the event dispatcher pointer. This ensures that there\n> >>>> is only ever a single event dispatcher associated with a\n> >>>> given thread.\n> >>>>\n> >>>> Consider e.g. the following series of events:\n> >>>>\n> >>>> 1. thread 0 :: if (!data_->dispatcher_.load(std::memory_order_relaxed))\n> >>>> // reads nullptr and proceeds into the `if`'s block\n> >>>>\n> >>>> 2. thread 1 :: if (!data_->dispatcher_.load(std::memory_order_relaxed))\n> >>>> // reads nullptr and proceeds into the `if`'s block\n> >>>>\n> >>>> 3. thread 0 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release);\n> >>>> // creates dispatcher and sets pointer\n> >>>>\n> >>>> 4. thread 1 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release);\n> >>>> // creates dispatcher and sets pointer\n> >>>>\n> >>>> At the end, one of the event dispatchers is leaked, and\n> >>>> furthermore the `eventDispatcher()` calls might return\n> >>>> different values in the calling threads.\n> >>>\n> >>> Yes, the current code is clearly broken if called from different threads\n> >>> concurrently.\n> >>>\n> >>> The question is, do we want to call this function from different threads\n> >>> ? As far as I can see, it's only called for the current thread in all\n> >>> locations. By definition that means the thread is started, with its\n> >>> run() function called. By default run() calls exec(), which creates the\n> >>> event dispatcher. Even if a thread reimplements run() and either doesn't\n> >>> call exec(), or call eventDispatcher() before calling exec(), we should\n> >>> be safe.\n> >>>\n> >>> The only unsafe situation would be calling eventDispatcher() from a\n> >>> separate thread. The eventDispatcher() function is documented as\n> >>> thread-safe, so its API allows that, even if the implementation doesn't.\n> >>> Do we want to allow that ? Is there a use case for accessing the event\n> >>> dispatcher from other threads (other than through the Thread class API,\n> >>> such as Thread::exit()) ?\n> >>>\n> >>> We currently need to access the event dispatcher to manage event\n> >>> notifiers and timers. In particular, the following functions access the\n> >>> event dispatcher *of the current thread*:\n> >>>\n> >>> - EventNotifier::setEnabled()\n> >>> - Timer::registerTimer()\n> >>> - Timer::unregisterTimer()\n> >>> - IPCPipeUnixSocket::call()\n> >>> - IPAProxy*Worker::run()\n> >>>\n> >>> The first three functions can only be called from the thread the event\n> >>> notifier or timer belong to.\n> >>>\n> >>> Usage of the event dispatcher in IPCPipeUnixSocket::call() is a bit of a\n> >>> hack that I'd like to fix, but in any case the function only accesses\n> >>> the event dispatcher of the current thread.\n> >>>\n> >>> IPAProxy*Worker::run() also accesses the event dispatcher of the current\n> >>> thread, but in this case the Thread is not running (as in having his\n> >>> run() function called) as it's a MainThread instance that represents a\n> >>> main application thread, outside of the control of libcamera.\n> >>>\n> >>> It should therefore be safe to consider Thread::eventDispatcher() as\n> >>> callable only from its Thread.\n> >>>\n> >>> We could even take it one step further, and create the event dispatcher\n> >>> in Thread::exec() instead of in Thread::eventDispatcher(). We would have\n> >>> to consider some corner cases, such as moving a timer or event notifier\n> >>> to a thread that has not been started yet. Callers of\n> >>> Thread::eventDispatcher() may need to perform null checks. We would also\n> >>> need to figure out how to handle IPAProxy*Worker::run().\n> >>>\n> >>> What do you think ?\n> >>\n> >> The motivation for this change, as far as I remember, was that the documentation\n> >> states that this function is thread safe. The variable also has an atomic type,\n> >> so I did not do more investigations because the the intent seemed clear.\n> >>\n> >> If the conclusion is that thread safety is not needed, then I believe the mention of\n> >> thread safety should be dropped from the documentation and the type modified to be non-atomic.\n> > \n> > Note that eventDispatcher_ still needs to be accessed from other threads\n> > through Thread::exit() and Thread::postMessage(). I haven't checked if\n> > this requires (or could benefit from) an atomic type.\n> \n> Ahh, that is true. `Thread::postMessage()` most certainly needs atomic access to\n> the ptr. Same with `Thread::exit()`.\n> \n> >> I don't have a concrete opinion on creating the event loop just in exec(). I suppose\n> >> it could make sense, but I'd probably decouple the two types first by removing\n> >> `Thread::current()->dispatchMessages()` from `EventDispatcherPoll::processEvents`\n> >> and instead registering an eventfd for the messages.\n> > \n> > Wouldn't that impact performance ? Messages are completely internal to\n> > libcamera, I'd keep them that way if possible.\n> \n> I don't see a substantial difference. Now the message passing mechanism\n> uses `EventDispatcher::interrupt()`, which will still just signal an\n> eventfd. The main difference is where that eventfd is. I would argue\n> that by keeping it in `Thread`, and letting `EventDispatcher` not to\n> have to deal with message passing, things become more separated.\n\nI see messages as a kind of event, but that's debatable :-) We would\nstill need to be able to interrupt the event dispatcher, so an eventfd\nwould still be needed there. Maybe I don't understand your proposal\ncorrectly, but it also feels like there isn't really anything to fix\nthere, so refactoring may be low priority.\n\n> > In any case, we can decouple the thread safety issue from the event\n> > dispatcher creation issue. Let's address the former for now.\n> \n> So I suppose the question is whether `Thread::eventDispatcher()` should be\n> thread safe or not. I don't have a concrete opinion on that. I believe the\n> proposed changes here shouldn't incur significant performance penalty, so\n> we might want to make it thread safe. Although arguably `EventDispatcher::interrupt()`\n> is the only thread safe function, so it might not have a lot of utility.\n\nAt the moment I'm leaning slightly towards the \"keep is simple\"\napproach, given we have no use case for calling\nThread::eventDispatcher() from a different thread. Even if your patch\ndoes not cause significant performance issues, it makes the code more\ncomplicated, and therefore has a maintenance cost.\n\n> >>>> Signed-off-by: Barnabás Pőcze \n> >>>> ---\n> >>>> src/libcamera/base/thread.cpp | 13 +++++++++----\n> >>>> 1 file changed, 9 insertions(+), 4 deletions(-)\n> >>>>\n> >>>> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> >>>> index de60567f6..93ec7b5a0 100644\n> >>>> --- a/src/libcamera/base/thread.cpp\n> >>>> +++ b/src/libcamera/base/thread.cpp\n> >>>> @@ -518,11 +518,16 @@ pid_t Thread::currentId()\n> >>>> */\n> >>>> EventDispatcher *Thread::eventDispatcher()\n> >>>> {\n> >>>> -\tif (!data_->dispatcher_.load(std::memory_order_relaxed))\n> >>>> -\t\tdata_->dispatcher_.store(new EventDispatcherPoll(),\n> >>>> -\t\t\t\t\t std::memory_order_release);\n> >>>> +\tif (EventDispatcher *dispatcher = data_->dispatcher_.load(std::memory_order_acquire))\n> >>>> +\t\treturn dispatcher;\n> >>>>\n> >>>> -\treturn data_->dispatcher_.load(std::memory_order_relaxed);\n> >>>> +\tauto dispatcher = std::make_unique();\n> >>>> +\tEventDispatcher *expected = nullptr;\n> >>>> +\n> >>>> +\tif (data_->dispatcher_.compare_exchange_strong(expected, dispatcher.get(), std::memory_order_acq_rel))\n> >>>> +\t\treturn dispatcher.release();\n> >>>> +\n> >>>> +\treturn expected;\n> >>>> }\n> >>>>\n> >>>> /**","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6160ABDCC1\n\tfor ;\n\tThu, 14 Aug 2025 22:03:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 63E7A69257;\n\tFri, 15 Aug 2025 00:03:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 69FAD6922C\n\tfor ;\n\tFri, 15 Aug 2025 00:03:05 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 99325605;\n\tFri, 15 Aug 2025 00:02:10 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"j8r8DJ84\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755208930;\n\tbh=lxpaI4X4xwRalytKOfT713J34NvW/r0Dt7MowP4Npjk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=j8r8DJ84pEJmgv9QUa1p0quV1cAgOv2bxxH6yojlhko7XvEdYf/r65ZXNrzCiHgH/\n\tARSeYNXfhKL6lAFDbFHUXKzISyysE9P4tqJpl6Jrrt9ZYKnkUhJHU/a+bFshRlgk2r\n\tpNK3PBLCbNyToDYPtLcP1EihiV228yZRWk8OAfXI=","Date":"Fri, 15 Aug 2025 01:02:45 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1] libcamera: thread: Fix event dispatcher creation","Message-ID":"<20250814220245.GC6201@pendragon.ideasonboard.com>","References":"<20250128130308.509297-1-pobrn@protonmail.com>\n\t<20250128130308.509297-2-pobrn@protonmail.com>\n\t<20250814135623.GB8360@pendragon.ideasonboard.com>\n\t\n\t<3Isvs6qN9a1edD5-zdKYCpHRFURjViUsDZt1paZcQjJ28ICnrouA-m7nCdSN9G2QkTmXNcl0HKySYn9QxKcYgw==@protonmail.internalid>\n\t<20250814144100.GC8360@pendragon.ideasonboard.com>\n\t<2444af88-4a8d-4be4-9ebc-480c90c878da@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<2444af88-4a8d-4be4-9ebc-480c90c878da@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35431,"web_url":"https://patchwork.libcamera.org/comment/35431/","msgid":"<6809d0e6-ad50-4e34-bb87-c125720928e9@ideasonboard.com>","date":"2025-08-15T10:09:19","subject":"Re: [RFC PATCH v1] libcamera: thread: Fix event dispatcher creation","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 08. 15. 0:02 keltezéssel, Laurent Pinchart írta:\n> On Thu, Aug 14, 2025 at 05:03:39PM +0200, Barnabás Pőcze wrote:\n>> 2025. 08. 14. 16:41 keltezéssel, Laurent Pinchart írta:\n>>> On Thu, Aug 14, 2025 at 04:30:14PM +0200, Barnabás Pőcze wrote:\n>>>> 2025. 08. 14. 15:56 keltezéssel, Laurent Pinchart írta:\n>>>>> On Tue, Jan 28, 2025 at 01:03:14PM +0000, Barnabás Pőcze wrote:\n>>>>>> Use an appropriate compare-and-exchange atomic instruction\n>>>>>> to set the event dispatcher pointer. This ensures that there\n>>>>>> is only ever a single event dispatcher associated with a\n>>>>>> given thread.\n>>>>>>\n>>>>>> Consider e.g. the following series of events:\n>>>>>>\n>>>>>> 1. thread 0 :: if (!data_->dispatcher_.load(std::memory_order_relaxed))\n>>>>>> // reads nullptr and proceeds into the `if`'s block\n>>>>>>\n>>>>>> 2. thread 1 :: if (!data_->dispatcher_.load(std::memory_order_relaxed))\n>>>>>> // reads nullptr and proceeds into the `if`'s block\n>>>>>>\n>>>>>> 3. thread 0 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release);\n>>>>>> // creates dispatcher and sets pointer\n>>>>>>\n>>>>>> 4. thread 1 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release);\n>>>>>> // creates dispatcher and sets pointer\n>>>>>>\n>>>>>> At the end, one of the event dispatchers is leaked, and\n>>>>>> furthermore the `eventDispatcher()` calls might return\n>>>>>> different values in the calling threads.\n>>>>>\n>>>>> Yes, the current code is clearly broken if called from different threads\n>>>>> concurrently.\n>>>>>\n>>>>> The question is, do we want to call this function from different threads\n>>>>> ? As far as I can see, it's only called for the current thread in all\n>>>>> locations. By definition that means the thread is started, with its\n>>>>> run() function called. By default run() calls exec(), which creates the\n>>>>> event dispatcher. Even if a thread reimplements run() and either doesn't\n>>>>> call exec(), or call eventDispatcher() before calling exec(), we should\n>>>>> be safe.\n>>>>>\n>>>>> The only unsafe situation would be calling eventDispatcher() from a\n>>>>> separate thread. The eventDispatcher() function is documented as\n>>>>> thread-safe, so its API allows that, even if the implementation doesn't.\n>>>>> Do we want to allow that ? Is there a use case for accessing the event\n>>>>> dispatcher from other threads (other than through the Thread class API,\n>>>>> such as Thread::exit()) ?\n>>>>>\n>>>>> We currently need to access the event dispatcher to manage event\n>>>>> notifiers and timers. In particular, the following functions access the\n>>>>> event dispatcher *of the current thread*:\n>>>>>\n>>>>> - EventNotifier::setEnabled()\n>>>>> - Timer::registerTimer()\n>>>>> - Timer::unregisterTimer()\n>>>>> - IPCPipeUnixSocket::call()\n>>>>> - IPAProxy*Worker::run()\n>>>>>\n>>>>> The first three functions can only be called from the thread the event\n>>>>> notifier or timer belong to.\n>>>>>\n>>>>> Usage of the event dispatcher in IPCPipeUnixSocket::call() is a bit of a\n>>>>> hack that I'd like to fix, but in any case the function only accesses\n>>>>> the event dispatcher of the current thread.\n>>>>>\n>>>>> IPAProxy*Worker::run() also accesses the event dispatcher of the current\n>>>>> thread, but in this case the Thread is not running (as in having his\n>>>>> run() function called) as it's a MainThread instance that represents a\n>>>>> main application thread, outside of the control of libcamera.\n>>>>>\n>>>>> It should therefore be safe to consider Thread::eventDispatcher() as\n>>>>> callable only from its Thread.\n>>>>>\n>>>>> We could even take it one step further, and create the event dispatcher\n>>>>> in Thread::exec() instead of in Thread::eventDispatcher(). We would have\n>>>>> to consider some corner cases, such as moving a timer or event notifier\n>>>>> to a thread that has not been started yet. Callers of\n>>>>> Thread::eventDispatcher() may need to perform null checks. We would also\n>>>>> need to figure out how to handle IPAProxy*Worker::run().\n>>>>>\n>>>>> What do you think ?\n>>>>\n>>>> The motivation for this change, as far as I remember, was that the documentation\n>>>> states that this function is thread safe. The variable also has an atomic type,\n>>>> so I did not do more investigations because the the intent seemed clear.\n>>>>\n>>>> If the conclusion is that thread safety is not needed, then I believe the mention of\n>>>> thread safety should be dropped from the documentation and the type modified to be non-atomic.\n>>>\n>>> Note that eventDispatcher_ still needs to be accessed from other threads\n>>> through Thread::exit() and Thread::postMessage(). I haven't checked if\n>>> this requires (or could benefit from) an atomic type.\n>>\n>> Ahh, that is true. `Thread::postMessage()` most certainly needs atomic access to\n>> the ptr. Same with `Thread::exit()`.\n>>\n>>>> I don't have a concrete opinion on creating the event loop just in exec(). I suppose\n>>>> it could make sense, but I'd probably decouple the two types first by removing\n>>>> `Thread::current()->dispatchMessages()` from `EventDispatcherPoll::processEvents`\n>>>> and instead registering an eventfd for the messages.\n>>>\n>>> Wouldn't that impact performance ? Messages are completely internal to\n>>> libcamera, I'd keep them that way if possible.\n>>\n>> I don't see a substantial difference. Now the message passing mechanism\n>> uses `EventDispatcher::interrupt()`, which will still just signal an\n>> eventfd. The main difference is where that eventfd is. I would argue\n>> that by keeping it in `Thread`, and letting `EventDispatcher` not to\n>> have to deal with message passing, things become more separated.\n> \n> I see messages as a kind of event, but that's debatable :-) We would\n> still need to be able to interrupt the event dispatcher, so an eventfd\n> would still be needed there. Maybe I don't understand your proposal\n> correctly, but it also feels like there isn't really anything to fix\n> there, so refactoring may be low priority.\n> \n>>> In any case, we can decouple the thread safety issue from the event\n>>> dispatcher creation issue. Let's address the former for now.\n>>\n>> So I suppose the question is whether `Thread::eventDispatcher()` should be\n>> thread safe or not. I don't have a concrete opinion on that. I believe the\n>> proposed changes here shouldn't incur significant performance penalty, so\n>> we might want to make it thread safe. Although arguably `EventDispatcher::interrupt()`\n>> is the only thread safe function, so it might not have a lot of utility.\n> \n> At the moment I'm leaning slightly towards the \"keep is simple\"\n> approach, given we have no use case for calling\n> Thread::eventDispatcher() from a different thread. Even if your patch\n> does not cause significant performance issues, it makes the code more\n> complicated, and therefore has a maintenance cost.\n\nOkay, then I'll send a patch removing the mention of thread safety\nfrom the documentation.\n\n\n> \n>>>>>> Signed-off-by: Barnabás Pőcze \n>>>>>> ---\n>>>>>> src/libcamera/base/thread.cpp | 13 +++++++++----\n>>>>>> 1 file changed, 9 insertions(+), 4 deletions(-)\n>>>>>>\n>>>>>> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n>>>>>> index de60567f6..93ec7b5a0 100644\n>>>>>> --- a/src/libcamera/base/thread.cpp\n>>>>>> +++ b/src/libcamera/base/thread.cpp\n>>>>>> @@ -518,11 +518,16 @@ pid_t Thread::currentId()\n>>>>>> */\n>>>>>> EventDispatcher *Thread::eventDispatcher()\n>>>>>> {\n>>>>>> -\tif (!data_->dispatcher_.load(std::memory_order_relaxed))\n>>>>>> -\t\tdata_->dispatcher_.store(new EventDispatcherPoll(),\n>>>>>> -\t\t\t\t\t std::memory_order_release);\n>>>>>> +\tif (EventDispatcher *dispatcher = data_->dispatcher_.load(std::memory_order_acquire))\n>>>>>> +\t\treturn dispatcher;\n>>>>>>\n>>>>>> -\treturn data_->dispatcher_.load(std::memory_order_relaxed);\n>>>>>> +\tauto dispatcher = std::make_unique();\n>>>>>> +\tEventDispatcher *expected = nullptr;\n>>>>>> +\n>>>>>> +\tif (data_->dispatcher_.compare_exchange_strong(expected, dispatcher.get(), std::memory_order_acq_rel))\n>>>>>> +\t\treturn dispatcher.release();\n>>>>>> +\n>>>>>> +\treturn expected;\n>>>>>> }\n>>>>>>\n>>>>>> /**\n> \n> --\n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AA41BBDCC1\n\tfor ;\n\tFri, 15 Aug 2025 10:09:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B466969257;\n\tFri, 15 Aug 2025 12:09:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 982CD61443\n\tfor ;\n\tFri, 15 Aug 2025 12:09:23 +0200 (CEST)","from [192.168.33.21] (185.221.141.188.nat.pool.zt.hu\n\t[185.221.141.188])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 876C163B;\n\tFri, 15 Aug 2025 12:08:28 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DVZ/xhrV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755252508;\n\tbh=YQll25OYYMnqQ14gM/Mm2e2dvsZbp9sYMYKop/oQ4R8=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=DVZ/xhrVML7VIwhBmhqklGRkQsaP8ysWxlm4+GYMFUNXQhGYAThvWKaFDCn6fGYKW\n\tQY+54F4TvLHg6NPfnluntqEYU3ZI7LoMjnuQ5ICbGLUFivbHdVuuuDP37rpmI2DgXO\n\tTaPxW+yW5KoRa5it+4kuEvWkMgWuHSgq+MVct7rQ=","Message-ID":"<6809d0e6-ad50-4e34-bb87-c125720928e9@ideasonboard.com>","Date":"Fri, 15 Aug 2025 12:09:19 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1] libcamera: thread: Fix event dispatcher creation","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250128130308.509297-1-pobrn@protonmail.com>\n\t<20250128130308.509297-2-pobrn@protonmail.com>\n\t<20250814135623.GB8360@pendragon.ideasonboard.com>\n\t\n\t<3Isvs6qN9a1edD5-zdKYCpHRFURjViUsDZt1paZcQjJ28ICnrouA-m7nCdSN9G2QkTmXNcl0HKySYn9QxKcYgw==@protonmail.internalid>\n\t<20250814144100.GC8360@pendragon.ideasonboard.com>\n\t<2444af88-4a8d-4be4-9ebc-480c90c878da@ideasonboard.com>\n\t<6m_YlIR9_bnvV3JzHUMz3h8V7TJ79BHNYZq9UJhpcOmF4LoRLSAi4p4Fbkb1MBaMwiuwFPqjX1xv0xnQIA9PHw==@protonmail.internalid>\n\t<20250814220245.GC6201@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250814220245.GC6201@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]