[{"id":35420,"web_url":"https://patchwork.libcamera.org/comment/35420/","msgid":"<20250814223647.GF6201@pendragon.ideasonboard.com>","date":"2025-08-14T22:36:47","subject":"Re: [PATCH v1] libcamera: base: semaphore: Do not unlock prematurely","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 Thu, Aug 14, 2025 at 11:31:38AM +0200, Barnabás Pőcze wrote:\n> In `Semaphore::release()`, unlocking the mutex before signalling the condition\n> variable can be problematic, especially with \"temporary\" objects such as the\n> ones `BoundMethodBase::activatePack()` uses to handle `ConnectionTypeBlocking`.\n> \n> Specifically, `Semaphore::acquire()` might lock the mutex after `Semaphore::release()`\n> has unlocked it, but before it had the chance to notify the condition variable.\n> In that case `Semaphore::acquire()` can succeed, and execution may proceed to\n> destroy the `Semaphore` object while the other thread is in the process of\n> running `std::condition_variable::notify_all()`.\n> \n> See [0] for essentially the same issue in libcxx and its fix at [1].\n> \n> [0]: https://github.com/llvm/llvm-project/issues/23667\n> [1]: https://github.com/llvm/llvm-project/commit/b3ec43d78aa2599e1592392844a7ec638fab7902\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=225\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/libcamera/base/semaphore.cpp | 6 ++----\n> 1 file changed, 2 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/base/semaphore.cpp b/src/libcamera/base/semaphore.cpp\n> index 862f3b313..6aec8b92d 100644\n> --- a/src/libcamera/base/semaphore.cpp\n> +++ b/src/libcamera/base/semaphore.cpp\n> @@ -93,11 +93,9 @@ bool Semaphore::tryAcquire(unsigned int n)\n> */\n> void Semaphore::release(unsigned int n)\n> {\n> -\t{\n> -\t\tMutexLocker locker(mutex_);\n> -\t\tavailable_ += n;\n> -\t}\n> +\tMutexLocker locker(mutex_);\n> \n> +\tavailable_ += n;\n> \tcv_.notify_all();\n\nIt's a bit of a shame, as this degrades performance. The notify_all()\ncall can cause the thread being woken up to be scheduled on the same CPU\nbefore the mutex is unlocked, and it will then immediately go back to\nsleep when trying to acquire the mutex :-/\n\nThis fixes a real bug though, so\n\nReviewed-by: Laurent Pinchart \n\nbut I wonder if there could be a way to improve this.\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 54408BDCC1\n\tfor ;\n\tThu, 14 Aug 2025 22:37:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4194C69254;\n\tFri, 15 Aug 2025 00:37:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3CFB46922C\n\tfor ;\n\tFri, 15 Aug 2025 00:37:07 +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 800C46DF;\n\tFri, 15 Aug 2025 00:36:12 +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=\"JsfEJM8V\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755210972;\n\tbh=rBZaltBdL0AqAeL3KTrIEX+9I7IwHJ2PN77GSCHIii4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JsfEJM8VJEq3/zj9OT3f6jEAVsJmGNfAMAa61Fo5kbqcLmEz+HoaIPKpo/ESl/c1+\n\tOdbhgIo8Jw7/3r+V2NgKJLXRvUC9+SNEISUOm+ujYIqVbByENmpF1f/MATY190/6bM\n\tQ+uAO3au2xJ57hP0dmuMPimThcpDXsJf15v4KUTQ=","Date":"Fri, 15 Aug 2025 01:36:47 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: base: semaphore: Do not unlock prematurely","Message-ID":"<20250814223647.GF6201@pendragon.ideasonboard.com>","References":"<20250814093138.2075098-1-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250814093138.2075098-1-barnabas.pocze@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":35424,"web_url":"https://patchwork.libcamera.org/comment/35424/","msgid":"<41148987-33f4-4f4c-a5fc-8f91c4a5924f@ideasonboard.com>","date":"2025-08-15T07:41:43","subject":"Re: [PATCH v1] libcamera: base: semaphore: Do not unlock prematurely","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:36 keltezéssel, Laurent Pinchart írta:\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> On Thu, Aug 14, 2025 at 11:31:38AM +0200, Barnabás Pőcze wrote:\n>> In `Semaphore::release()`, unlocking the mutex before signalling the condition\n>> variable can be problematic, especially with \"temporary\" objects such as the\n>> ones `BoundMethodBase::activatePack()` uses to handle `ConnectionTypeBlocking`.\n>>\n>> Specifically, `Semaphore::acquire()` might lock the mutex after `Semaphore::release()`\n>> has unlocked it, but before it had the chance to notify the condition variable.\n>> In that case `Semaphore::acquire()` can succeed, and execution may proceed to\n>> destroy the `Semaphore` object while the other thread is in the process of\n>> running `std::condition_variable::notify_all()`.\n>>\n>> See [0] for essentially the same issue in libcxx and its fix at [1].\n>>\n>> [0]: https://github.com/llvm/llvm-project/issues/23667\n>> [1]: https://github.com/llvm/llvm-project/commit/b3ec43d78aa2599e1592392844a7ec638fab7902\n>>\n>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=225\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/libcamera/base/semaphore.cpp | 6 ++----\n>> 1 file changed, 2 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/src/libcamera/base/semaphore.cpp b/src/libcamera/base/semaphore.cpp\n>> index 862f3b313..6aec8b92d 100644\n>> --- a/src/libcamera/base/semaphore.cpp\n>> +++ b/src/libcamera/base/semaphore.cpp\n>> @@ -93,11 +93,9 @@ bool Semaphore::tryAcquire(unsigned int n)\n>> */\n>> void Semaphore::release(unsigned int n)\n>> {\n>> -\t{\n>> -\t\tMutexLocker locker(mutex_);\n>> -\t\tavailable_ += n;\n>> -\t}\n>> +\tMutexLocker locker(mutex_);\n>> \n>> +\tavailable_ += n;\n>> \tcv_.notify_all();\n> \n> It's a bit of a shame, as this degrades performance. The notify_all()\n> call can cause the thread being woken up to be scheduled on the same CPU\n> before the mutex is unlocked, and it will then immediately go back to\n> sleep when trying to acquire the mutex :-/\n> \n> This fixes a real bug though, so\n> \n> Reviewed-by: Laurent Pinchart \n> \n> but I wonder if there could be a way to improve this.\n\nFor example, in the case of message passing, in C++20 one possible solution would\nbe to use one `std::atomic` and its `wait()` and `notify_one()` methods. Or\nthere is std::latch or std::binary_semaphore, but both are more complex than the\nsimple \"event\" mechanism that message passing requires.\n\n\nRegards,\nBarnabás Pőcze\n\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 430BEBEFBE\n\tfor ;\n\tFri, 15 Aug 2025 07:41:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 22EFA69259;\n\tFri, 15 Aug 2025 09:41:48 +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 83A9969244\n\tfor ;\n\tFri, 15 Aug 2025 09:41:46 +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 7E99F56D;\n\tFri, 15 Aug 2025 09:40:51 +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=\"KgThtwlI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755243651;\n\tbh=nMr38aZrjLIAiIbKruhlIdhbeXbL4xu7P/mEuGTVzZI=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=KgThtwlIQIyTvEJ2ilnoSEdWWsn9WGSJkU2zaMQ4pUKkDMMkAzOWJ0sjINtd1h+ax\n\tXXzs053m0ZsR0a7+Pft11k4dxFBQbToip5hrKBdCCmPfdC729JOkATAqe7PM35c7KW\n\tQnRJ08FrUbq5zmBqqV4tgIX/Vfi8oc62vA8ItVFg=","Message-ID":"<41148987-33f4-4f4c-a5fc-8f91c4a5924f@ideasonboard.com>","Date":"Fri, 15 Aug 2025 09:41:43 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1] libcamera: base: semaphore: Do not unlock prematurely","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250814093138.2075098-1-barnabas.pocze@ideasonboard.com>\n\t<20250814223647.GF6201@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250814223647.GF6201@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":35479,"web_url":"https://patchwork.libcamera.org/comment/35479/","msgid":"<175551724081.560048.13772012757066147144@ping.linuxembedded.co.uk>","date":"2025-08-18T11:40:40","subject":"Re: [PATCH v1] libcamera: base: semaphore: Do not unlock prematurely","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-08-15 08:41:43)\n> 2025. 08. 15. 0:36 keltezéssel, Laurent Pinchart írta:\n> > Hi Barnabás,\n> > \n> > Thank you for the patch.\n> > \n> > On Thu, Aug 14, 2025 at 11:31:38AM +0200, Barnabás Pőcze wrote:\n> >> In `Semaphore::release()`, unlocking the mutex before signalling the condition\n> >> variable can be problematic, especially with \"temporary\" objects such as the\n> >> ones `BoundMethodBase::activatePack()` uses to handle `ConnectionTypeBlocking`.\n> >>\n> >> Specifically, `Semaphore::acquire()` might lock the mutex after `Semaphore::release()`\n> >> has unlocked it, but before it had the chance to notify the condition variable.\n> >> In that case `Semaphore::acquire()` can succeed, and execution may proceed to\n> >> destroy the `Semaphore` object while the other thread is in the process of\n> >> running `std::condition_variable::notify_all()`.\n> >>\n> >> See [0] for essentially the same issue in libcxx and its fix at [1].\n> >>\n> >> [0]: https://github.com/llvm/llvm-project/issues/23667\n> >> [1]: https://github.com/llvm/llvm-project/commit/b3ec43d78aa2599e1592392844a7ec638fab7902\n> >>\n> >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=225\n\nIf this fixes a bug in the code - is there value in a Fixes: tag ?\n\n> >> Signed-off-by: Barnabás Pőcze \n> >> ---\n> >> src/libcamera/base/semaphore.cpp | 6 ++----\n> >> 1 file changed, 2 insertions(+), 4 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/base/semaphore.cpp b/src/libcamera/base/semaphore.cpp\n> >> index 862f3b313..6aec8b92d 100644\n> >> --- a/src/libcamera/base/semaphore.cpp\n> >> +++ b/src/libcamera/base/semaphore.cpp\n> >> @@ -93,11 +93,9 @@ bool Semaphore::tryAcquire(unsigned int n)\n> >> */\n> >> void Semaphore::release(unsigned int n)\n> >> {\n> >> - {\n> >> - MutexLocker locker(mutex_);\n> >> - available_ += n;\n> >> - }\n> >> + MutexLocker locker(mutex_);\n> >> \n> >> + available_ += n;\n> >> cv_.notify_all();\n> > \n> > It's a bit of a shame, as this degrades performance. The notify_all()\n> > call can cause the thread being woken up to be scheduled on the same CPU\n> > before the mutex is unlocked, and it will then immediately go back to\n> > sleep when trying to acquire the mutex :-/\n\nThat sounds annoying - but how often will it happen or what would the\ncost actually be ? I guess that's hard to measure.\n\n> > \n> > This fixes a real bug though, so\n> > \n> > Reviewed-by: Laurent Pinchart \n\nReviewed-by: Kieran Bingham \n\n> > \n> > but I wonder if there could be a way to improve this.\n> \n> For example, in the case of message passing, in C++20 one possible solution would\n> be to use one `std::atomic` and its `wait()` and `notify_one()` methods. Or\n> there is std::latch or std::binary_semaphore, but both are more complex than the\n> simple \"event\" mechanism that message passing requires.\n> \n> \n> Regards,\n> Barnabás Pőcze\n> \n> \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 C8C17BEFBE\n\tfor ;\n\tMon, 18 Aug 2025 11:40:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F287B69259;\n\tMon, 18 Aug 2025 13:40:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DA81869257\n\tfor ;\n\tMon, 18 Aug 2025 13:40:43 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BDB3617D1;\n\tMon, 18 Aug 2025 13:39:46 +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=\"eKqYsRp9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755517186;\n\tbh=mrdxdWL2ldFG0SHxfj8WjLsk55QIj/eou8HE+HI6vTw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=eKqYsRp9iHBXdXf9UatsxXoMtt78WP2MsjuQ9uriVcyidTn2pV6Dl2up2vaIEdLTg\n\ttENFWP+v0Tk3aC/n9Y299Qtuc1fOqdEYPL52BOVk+Z1f7fot6qK3Ks5f9QBhes8m+A\n\tTW4kzUuaEm2a18lS62o1R0jCkg//hlH6Ip+/yU40=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<41148987-33f4-4f4c-a5fc-8f91c4a5924f@ideasonboard.com>","References":"<20250814093138.2075098-1-barnabas.pocze@ideasonboard.com>\n\t<20250814223647.GF6201@pendragon.ideasonboard.com>\n\t<41148987-33f4-4f4c-a5fc-8f91c4a5924f@ideasonboard.com>","Subject":"Re: [PATCH v1] libcamera: base: semaphore: Do not unlock prematurely","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tLaurent Pinchart ","Date":"Mon, 18 Aug 2025 12:40:40 +0100","Message-ID":"<175551724081.560048.13772012757066147144@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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":35488,"web_url":"https://patchwork.libcamera.org/comment/35488/","msgid":"<018b8bb2-ee99-401a-a2c5-1b47bec73562@ideasonboard.com>","date":"2025-08-18T15:03:48","subject":"Re: [PATCH v1] libcamera: base: semaphore: Do not unlock prematurely","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 08. 18. 13:40 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-08-15 08:41:43)\n>> 2025. 08. 15. 0:36 keltezéssel, Laurent Pinchart írta:\n>>> Hi Barnabás,\n>>>\n>>> Thank you for the patch.\n>>>\n>>> On Thu, Aug 14, 2025 at 11:31:38AM +0200, Barnabás Pőcze wrote:\n>>>> In `Semaphore::release()`, unlocking the mutex before signalling the condition\n>>>> variable can be problematic, especially with \"temporary\" objects such as the\n>>>> ones `BoundMethodBase::activatePack()` uses to handle `ConnectionTypeBlocking`.\n>>>>\n>>>> Specifically, `Semaphore::acquire()` might lock the mutex after `Semaphore::release()`\n>>>> has unlocked it, but before it had the chance to notify the condition variable.\n>>>> In that case `Semaphore::acquire()` can succeed, and execution may proceed to\n>>>> destroy the `Semaphore` object while the other thread is in the process of\n>>>> running `std::condition_variable::notify_all()`.\n>>>>\n>>>> See [0] for essentially the same issue in libcxx and its fix at [1].\n>>>>\n>>>> [0]: https://github.com/llvm/llvm-project/issues/23667\n>>>> [1]: https://github.com/llvm/llvm-project/commit/b3ec43d78aa2599e1592392844a7ec638fab7902\n>>>>\n>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=225\n> \n> If this fixes a bug in the code - is there value in a Fixes: tag ?\n\nIt's been like this since this class had been added:\n66e7c5b774e288faa3a9b413861d6a77723db3ad (\"libcamera: Add Semaphore class\")\n\nI'll add it now.\n\n\n> \n>>>> Signed-off-by: Barnabás Pőcze \n>>>> ---\n>>>> src/libcamera/base/semaphore.cpp | 6 ++----\n>>>> 1 file changed, 2 insertions(+), 4 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/base/semaphore.cpp b/src/libcamera/base/semaphore.cpp\n>>>> index 862f3b313..6aec8b92d 100644\n>>>> --- a/src/libcamera/base/semaphore.cpp\n>>>> +++ b/src/libcamera/base/semaphore.cpp\n>>>> @@ -93,11 +93,9 @@ bool Semaphore::tryAcquire(unsigned int n)\n>>>> */\n>>>> void Semaphore::release(unsigned int n)\n>>>> {\n>>>> - {\n>>>> - MutexLocker locker(mutex_);\n>>>> - available_ += n;\n>>>> - }\n>>>> + MutexLocker locker(mutex_);\n>>>> \n>>>> + available_ += n;\n>>>> cv_.notify_all();\n>>>\n>>> It's a bit of a shame, as this degrades performance. The notify_all()\n>>> call can cause the thread being woken up to be scheduled on the same CPU\n>>> before the mutex is unlocked, and it will then immediately go back to\n>>> sleep when trying to acquire the mutex :-/\n> \n> That sounds annoying - but how often will it happen or what would the\n> cost actually be ? I guess that's hard to measure.\n> \n>>>\n>>> This fixes a real bug though, so\n>>>\n>>> Reviewed-by: Laurent Pinchart \n> \n> Reviewed-by: Kieran Bingham \n> \n>>>\n>>> but I wonder if there could be a way to improve this.\n>>\n>> For example, in the case of message passing, in C++20 one possible solution would\n>> be to use one `std::atomic` and its `wait()` and `notify_one()` methods. Or\n>> there is std::latch or std::binary_semaphore, but both are more complex than the\n>> simple \"event\" mechanism that message passing requires.\n>>\n>>\n>> Regards,\n>> Barnabás Pőcze\n>>\n>>\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 C4C10BDCC1\n\tfor ;\n\tMon, 18 Aug 2025 15:03:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C84B169261;\n\tMon, 18 Aug 2025 17:03:54 +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 3DD9D69257\n\tfor ;\n\tMon, 18 Aug 2025 17:03:53 +0200 (CEST)","from [192.168.33.19] (185.221.141.188.nat.pool.zt.hu\n\t[185.221.141.188])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B7DC411EB;\n\tMon, 18 Aug 2025 17:02:55 +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=\"BFHNHPbd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755529375;\n\tbh=2vjvUSJ2SBB1ig1GvkAtLqluQ3Mws0bFefnUD4r+8ww=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=BFHNHPbduMAEC/s/7ZDOmSq+MgKpJAyxyGkMkEgAXpuZ3Rq03flxG5pq9Tyjr1oXl\n\t8A+Xfap5fniXf0HX8nijW71GdCQloQnarufbFSouGuj4zYseQa98G6rtVBNyQSQPeR\n\tjQF0fEOsX2SeE+CBLuaFZBZjd2iLh3x7VIc9s204=","Message-ID":"<018b8bb2-ee99-401a-a2c5-1b47bec73562@ideasonboard.com>","Date":"Mon, 18 Aug 2025 17:03:48 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1] libcamera: base: semaphore: Do not unlock prematurely","To":"Kieran Bingham ,\n\tLaurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250814093138.2075098-1-barnabas.pocze@ideasonboard.com>\n\t<20250814223647.GF6201@pendragon.ideasonboard.com>\n\t<41148987-33f4-4f4c-a5fc-8f91c4a5924f@ideasonboard.com>\n\t<175551724081.560048.13772012757066147144@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<175551724081.560048.13772012757066147144@ping.linuxembedded.co.uk>","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":35492,"web_url":"https://patchwork.libcamera.org/comment/35492/","msgid":"<175555742414.3797197.11220089435658751959@ping.linuxembedded.co.uk>","date":"2025-08-18T22:50:24","subject":"Re: [PATCH v1] libcamera: base: semaphore: Do not unlock prematurely","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-08-18 16:03:48)\n> 2025. 08. 18. 13:40 keltezéssel, Kieran Bingham írta:\n> > Quoting Barnabás Pőcze (2025-08-15 08:41:43)\n> >> 2025. 08. 15. 0:36 keltezéssel, Laurent Pinchart írta:\n> >>> Hi Barnabás,\n> >>>\n> >>> Thank you for the patch.\n> >>>\n> >>> On Thu, Aug 14, 2025 at 11:31:38AM +0200, Barnabás Pőcze wrote:\n> >>>> In `Semaphore::release()`, unlocking the mutex before signalling the condition\n> >>>> variable can be problematic, especially with \"temporary\" objects such as the\n> >>>> ones `BoundMethodBase::activatePack()` uses to handle `ConnectionTypeBlocking`.\n> >>>>\n> >>>> Specifically, `Semaphore::acquire()` might lock the mutex after `Semaphore::release()`\n> >>>> has unlocked it, but before it had the chance to notify the condition variable.\n> >>>> In that case `Semaphore::acquire()` can succeed, and execution may proceed to\n> >>>> destroy the `Semaphore` object while the other thread is in the process of\n> >>>> running `std::condition_variable::notify_all()`.\n> >>>>\n> >>>> See [0] for essentially the same issue in libcxx and its fix at [1].\n> >>>>\n> >>>> [0]: https://github.com/llvm/llvm-project/issues/23667\n> >>>> [1]: https://github.com/llvm/llvm-project/commit/b3ec43d78aa2599e1592392844a7ec638fab7902\n> >>>>\n> >>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=225\n> > \n> > If this fixes a bug in the code - is there value in a Fixes: tag ?\n> \n> It's been like this since this class had been added:\n> 66e7c5b774e288faa3a9b413861d6a77723db3ad (\"libcamera: Add Semaphore class\")\n> \n> I'll add it now.\n\n\nThanks - that will help the code archeologists :D (but it also helps my\nrelease notes!)\n\n\n--\nKieran\n> \n> \n> > \n> >>>> Signed-off-by: Barnabás Pőcze \n> >>>> ---\n> >>>> src/libcamera/base/semaphore.cpp | 6 ++----\n> >>>> 1 file changed, 2 insertions(+), 4 deletions(-)\n> >>>>\n> >>>> diff --git a/src/libcamera/base/semaphore.cpp b/src/libcamera/base/semaphore.cpp\n> >>>> index 862f3b313..6aec8b92d 100644\n> >>>> --- a/src/libcamera/base/semaphore.cpp\n> >>>> +++ b/src/libcamera/base/semaphore.cpp\n> >>>> @@ -93,11 +93,9 @@ bool Semaphore::tryAcquire(unsigned int n)\n> >>>> */\n> >>>> void Semaphore::release(unsigned int n)\n> >>>> {\n> >>>> - {\n> >>>> - MutexLocker locker(mutex_);\n> >>>> - available_ += n;\n> >>>> - }\n> >>>> + MutexLocker locker(mutex_);\n> >>>> \n> >>>> + available_ += n;\n> >>>> cv_.notify_all();\n> >>>\n> >>> It's a bit of a shame, as this degrades performance. The notify_all()\n> >>> call can cause the thread being woken up to be scheduled on the same CPU\n> >>> before the mutex is unlocked, and it will then immediately go back to\n> >>> sleep when trying to acquire the mutex :-/\n> > \n> > That sounds annoying - but how often will it happen or what would the\n> > cost actually be ? I guess that's hard to measure.\n> > \n> >>>\n> >>> This fixes a real bug though, so\n> >>>\n> >>> Reviewed-by: Laurent Pinchart \n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> >>>\n> >>> but I wonder if there could be a way to improve this.\n> >>\n> >> For example, in the case of message passing, in C++20 one possible solution would\n> >> be to use one `std::atomic` and its `wait()` and `notify_one()` methods. Or\n> >> there is std::latch or std::binary_semaphore, but both are more complex than the\n> >> simple \"event\" mechanism that message passing requires.\n> >>\n> >>\n> >> Regards,\n> >> Barnabás Pőcze\n> >>\n> >>\n> >>>\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 9D76ABEFBE\n\tfor ;\n\tMon, 18 Aug 2025 22:50:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 47EF769265;\n\tTue, 19 Aug 2025 00:50:29 +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 E3BE9613C5\n\tfor ;\n\tTue, 19 Aug 2025 00:50:26 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 71082446;\n\tTue, 19 Aug 2025 00:49:29 +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=\"GokOErV3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755557369;\n\tbh=Mswy7+OzSe62dfQwSBlMenZHVHHZ4+dXyfrPlORPqpY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=GokOErV3+lqka9+1+OcrNc7e9Tqmbg4NtRbgV4OVH97k//dSqHjdv9RifajXvg5rs\n\tyldxDRyL3YRNvB3vzJ/sF9CyZzzlQYJhiJcsYDDVIIi8eeSyY3QKyjK78G9D092+Ly\n\tkv93OIEKKhkM6V6dppfBAPIYpUp2JpjHw/AxyK90=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<018b8bb2-ee99-401a-a2c5-1b47bec73562@ideasonboard.com>","References":"<20250814093138.2075098-1-barnabas.pocze@ideasonboard.com>\n\t<20250814223647.GF6201@pendragon.ideasonboard.com>\n\t<41148987-33f4-4f4c-a5fc-8f91c4a5924f@ideasonboard.com>\n\t<175551724081.560048.13772012757066147144@ping.linuxembedded.co.uk>\n\t<018b8bb2-ee99-401a-a2c5-1b47bec73562@ideasonboard.com>","Subject":"Re: [PATCH v1] libcamera: base: semaphore: Do not unlock prematurely","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tLaurent Pinchart ","Date":"Mon, 18 Aug 2025 23:50:24 +0100","Message-ID":"<175555742414.3797197.11220089435658751959@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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\" "}}]