[{"id":23252,"web_url":"https://patchwork.libcamera.org/comment/23252/","msgid":"","date":"2022-05-30T15:53:49","subject":"Re: [libcamera-devel] [PATCH v4 11/16] py: merge read_event() and\n\tget_ready_requests()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nThank you for the patch.\n\nOn Mon, May 30, 2022 at 05:27:17PM +0300, Tomi Valkeinen wrote:\n> We always call CameraManager.read_event() and\n> CameraManager.get_ready_requests(), so to simplify the use merge the\n> read_event() into the get_ready_requests().\n> \n> This has the side effect that get_ready_requests() will now block if\n> there is no event ready. If we ever need to call get_ready_requests() in\n> a polling manner we will need a new function which behaves differently.\n> \n> However, afaics the only sensible way to manage the event loop is to use\n> select/poll on the eventfd and then call get_ready_requests() once,\n> which is the use case what the current merged function supports.\n> \n> Signed-off-by: Tomi Valkeinen \n> ---\n> src/py/cam/cam.py | 2 --\n> src/py/libcamera/py_main.cpp | 7 ++-----\n> test/py/unittests.py | 4 ----\n> 3 files changed, 2 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py\n> index bf8529d9..2ae89fa8 100755\n> --- a/src/py/cam/cam.py\n> +++ b/src/py/cam/cam.py\n> @@ -243,8 +243,6 @@ class CaptureState:\n> # Called from renderer when there is a libcamera event\n> def event_handler(self):\n> try:\n> - self.cm.read_event()\n> -\n> reqs = self.cm.get_ready_requests()\n> \n> for req in reqs:\n> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> index fcf009f0..505cc3dc 100644\n> --- a/src/py/libcamera/py_main.cpp\n> +++ b/src/py/libcamera/py_main.cpp\n> @@ -213,15 +213,12 @@ PYBIND11_MODULE(_libcamera, m)\n> \t\t\treturn gEventfd;\n> \t\t})\n> \n> -\t\t.def(\"read_event\", [](CameraManager &) {\n> +\t\t.def(\"get_ready_requests\", [](CameraManager &) {\n> \t\t\tuint8_t buf[8];\n> \n> -\t\t\tint ret = read(gEventfd, buf, 8);\n> -\t\t\tif (ret != 8)\n> +\t\t\tif (read(gEventfd, buf, 8) != 8)\n> \t\t\t\tthrow std::system_error(errno, std::generic_category());\n\nWe need a memory barrier here to prevent reordering. I'm quite sure\nthere's one somewhere due to the read() call and the lock below, but I\nhaven't been able to figure out the C++ rule that guarantees this. Does\nanyone know ?\n\nReviewed-by: Laurent Pinchart \n\n> -\t\t})\n> \n> -\t\t.def(\"get_ready_requests\", [](CameraManager &) {\n> \t\t\tstd::vector v;\n> \n> \t\t\t{\n> diff --git a/test/py/unittests.py b/test/py/unittests.py\n> index 33b35a0a..9adc4337 100755\n> --- a/test/py/unittests.py\n> +++ b/test/py/unittests.py\n> @@ -210,8 +210,6 @@ class SimpleCaptureMethods(CameraTesterBase):\n> reqs = []\n> \n> while True:\n> - cm.read_event()\n> -\n> ready_reqs = cm.get_ready_requests()\n> \n> reqs += ready_reqs\n> @@ -283,8 +281,6 @@ class SimpleCaptureMethods(CameraTesterBase):\n> while running:\n> events = sel.select()\n> for key, _ in events:\n> - cm.read_event()\n> -\n> ready_reqs = cm.get_ready_requests()\n> \n> reqs += ready_reqs","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 4C28BBD161\n\tfor ;\n\tMon, 30 May 2022 15:53:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8366E65633;\n\tMon, 30 May 2022 17:53: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 174CB60411\n\tfor ;\n\tMon, 30 May 2022 17:53:53 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(lmontsouris-659-1-41-236.w92-154.abo.wanadoo.fr [92.154.76.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 84DE525B;\n\tMon, 30 May 2022 17:53:52 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653926034;\n\tbh=qrFCq5+VmSwD3/LirHpxJ9rqg8I+qAcPXTwwlahqGF4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=nbTUdQnu/qoZSbGiJCSJG1rIfJNo8abXVqgyn5gPZhgMe30fKurGR8Kct46srpUz3\n\tHZi8zvQT8IQ660tXWIgZJ5KELUCYo3BaxBkzxawxawB/MbNHI+qXtDY0CSPF/UGu52\n\tJdKYrD40CSdwi3I335fEC80qvlvR4/4KQJkgsj8nSkv+ZM3jIiiwOnKn1GxvOxZ3wa\n\tmrNGoW8heHmuTENpAIKH5qsk3gKXTj5KeHLNwOAJESCr6PquA2GrgKi+L5oUE+RMFQ\n\tdQ6w2Q8018wyFW1gh9IPBissMMGlhD/pvBLR/Fc8rK4U18C//KW11TQBBjpuq4QVyu\n\tnM5Koj4j9Yz1g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653926032;\n\tbh=qrFCq5+VmSwD3/LirHpxJ9rqg8I+qAcPXTwwlahqGF4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=la3RdvvkowC2XVBTlv9w3L7N6GIwK6MacLO6Ajyl837vCtKFWHdTh5idtPP74gSi7\n\tTaIYOGI+Lnt5Fue4WwCcNyhCZxTVpPKFmOsyT92WS+YPFB5diEC3NDckq8VzQNKR/C\n\thvj/B0ZfwQ5M2wZoaW7X4gFqYtoBFDV+5IYi4MiA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"la3Rdvvk\"; dkim-atps=neutral","Date":"Mon, 30 May 2022 18:53:49 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220530142722.57618-1-tomi.valkeinen@ideasonboard.com>\n\t<20220530142722.57618-12-tomi.valkeinen@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220530142722.57618-12-tomi.valkeinen@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 11/16] py: merge read_event() and\n\tget_ready_requests()","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23253,"web_url":"https://patchwork.libcamera.org/comment/23253/","msgid":"","date":"2022-05-30T16:57:02","subject":"Re: [libcamera-devel] [PATCH v4 11/16] py: merge read_event() and\n\tget_ready_requests()","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 30/05/2022 18:53, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> Thank you for the patch.\n> \n> On Mon, May 30, 2022 at 05:27:17PM +0300, Tomi Valkeinen wrote:\n>> We always call CameraManager.read_event() and\n>> CameraManager.get_ready_requests(), so to simplify the use merge the\n>> read_event() into the get_ready_requests().\n>>\n>> This has the side effect that get_ready_requests() will now block if\n>> there is no event ready. If we ever need to call get_ready_requests() in\n>> a polling manner we will need a new function which behaves differently.\n>>\n>> However, afaics the only sensible way to manage the event loop is to use\n>> select/poll on the eventfd and then call get_ready_requests() once,\n>> which is the use case what the current merged function supports.\n>>\n>> Signed-off-by: Tomi Valkeinen \n>> ---\n>> src/py/cam/cam.py | 2 --\n>> src/py/libcamera/py_main.cpp | 7 ++-----\n>> test/py/unittests.py | 4 ----\n>> 3 files changed, 2 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py\n>> index bf8529d9..2ae89fa8 100755\n>> --- a/src/py/cam/cam.py\n>> +++ b/src/py/cam/cam.py\n>> @@ -243,8 +243,6 @@ class CaptureState:\n>> # Called from renderer when there is a libcamera event\n>> def event_handler(self):\n>> try:\n>> - self.cm.read_event()\n>> -\n>> reqs = self.cm.get_ready_requests()\n>> \n>> for req in reqs:\n>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n>> index fcf009f0..505cc3dc 100644\n>> --- a/src/py/libcamera/py_main.cpp\n>> +++ b/src/py/libcamera/py_main.cpp\n>> @@ -213,15 +213,12 @@ PYBIND11_MODULE(_libcamera, m)\n>> \t\t\treturn gEventfd;\n>> \t\t})\n>> \n>> -\t\t.def(\"read_event\", [](CameraManager &) {\n>> +\t\t.def(\"get_ready_requests\", [](CameraManager &) {\n>> \t\t\tuint8_t buf[8];\n>> \n>> -\t\t\tint ret = read(gEventfd, buf, 8);\n>> -\t\t\tif (ret != 8)\n>> +\t\t\tif (read(gEventfd, buf, 8) != 8)\n>> \t\t\t\tthrow std::system_error(errno, std::generic_category());\n> \n> We need a memory barrier here to prevent reordering. I'm quite sure\n\nWe then need the same in handleRequestCompleted().\n\n> there's one somewhere due to the read() call and the lock below, but I\n> haven't been able to figure out the C++ rule that guarantees this. Does\n> anyone know ?\n\nhttps://en.cppreference.com/w/cpp/atomic/memory_order\n\nSo the mutex brings the normal acquire-release ordering. I would think a \nsyscall includes a barrier, but I can't find any details right away.\n\n Tomi","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 31398BD160\n\tfor ;\n\tMon, 30 May 2022 16:57:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 19DD065633;\n\tMon, 30 May 2022 18:57:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 586C960411\n\tfor ;\n\tMon, 30 May 2022 18:57:07 +0200 (CEST)","from [192.168.1.111] (91-156-85-209.elisa-laajakaista.fi\n\t[91.156.85.209])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 400686DF;\n\tMon, 30 May 2022 18:57:06 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653929829;\n\tbh=yAN5EokeYPWZoHjB/6Bam/8Ci0ZWC+IH7VnkQVMswyk=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ILdjUv8iJxQvX9Xj/mzZYof7ofoXrGbRS3sGTGeZ19n7ZW0deoDhNXLXR9vqNd1eo\n\txCj9IS1kFXq2z9txJlPSzQIc9QO/o5wMZNAoasNV7sxYOvpGao5BbLmw2VIm08zYgN\n\tipOpuXTEivHrvpr2/1XB+ryrTWtekEo/AETlMMfNbLgwlIWuPi90uwNkO6s/gMi9d6\n\t8gtV6FHlscYfA7VVtm3+iEQgwNgLukiTe35qqITaVbZf7IYNksIz5p76J0LiDizHG5\n\teffdQftRIHcIFuW0WVIwu2eNOBmeaGYh2hAKIedK1k3gzOSPWfCOOkYKL1mVAXnc7r\n\tHuPcSmjd23hPQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653929826;\n\tbh=yAN5EokeYPWZoHjB/6Bam/8Ci0ZWC+IH7VnkQVMswyk=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=EGyIRj+Sx7qF97qPOy1xQN9gG150DqGAxEMtTsn1Lk0y8dDlMNeEzKBQGixpEsZ4p\n\tayvkNuIUzao+BYMyLsdRAvL60kiNWH0/5Es1Zt2uLT8GqavgvQffjOckPeYwWZ8bAX\n\tYMmfrMSdwvfFgcO0d/v/znl3HyYfZmEgkwO4MI2U="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"EGyIRj+S\"; dkim-atps=neutral","Message-ID":"","Date":"Mon, 30 May 2022 19:57:02 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220530142722.57618-1-tomi.valkeinen@ideasonboard.com>\n\t<20220530142722.57618-12-tomi.valkeinen@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v4 11/16] py: merge read_event() and\n\tget_ready_requests()","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23264,"web_url":"https://patchwork.libcamera.org/comment/23264/","msgid":"<9d1e128a-22ca-87e8-540e-a1a02e7d07fa@ideasonboard.com>","date":"2022-05-31T05:51:52","subject":"Re: [libcamera-devel] [PATCH v4 11/16] py: merge read_event() and\n\tget_ready_requests()","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 30/05/2022 19:57, Tomi Valkeinen wrote:\n\n>>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n>>> index fcf009f0..505cc3dc 100644\n>>> --- a/src/py/libcamera/py_main.cpp\n>>> +++ b/src/py/libcamera/py_main.cpp\n>>> @@ -213,15 +213,12 @@ PYBIND11_MODULE(_libcamera, m)\n>>>               return gEventfd;\n>>>           })\n>>> -        .def(\"read_event\", [](CameraManager &) {\n>>> +        .def(\"get_ready_requests\", [](CameraManager &) {\n>>>               uint8_t buf[8];\n>>> -            int ret = read(gEventfd, buf, 8);\n>>> -            if (ret != 8)\n>>> +            if (read(gEventfd, buf, 8) != 8)\n>>>                   throw std::system_error(errno, \n>>> std::generic_category());\n>>\n>> We need a memory barrier here to prevent reordering. I'm quite sure\n> \n> We then need the same in handleRequestCompleted().\n> \n>> there's one somewhere due to the read() call and the lock below, but I\n>> haven't been able to figure out the C++ rule that guarantees this. Does\n>> anyone know ?\n> \n> https://en.cppreference.com/w/cpp/atomic/memory_order\n> \n> So the mutex brings the normal acquire-release ordering. I would think a \n> syscall includes a barrier, but I can't find any details right away.\n\nPerhaps something like this? I don't know if the fences are needed, but maybe\nbetter safe than sorry.\n\ndiff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\nindex 505cc3dc..b035a101 100644\n--- a/src/py/libcamera/py_main.cpp\n+++ b/src/py/libcamera/py_main.cpp\n@@ -5,6 +5,7 @@\n * Python bindings\n */\n \n+#include \n #include \n #include \n #include \n@@ -116,6 +117,12 @@ static void handleRequestCompleted(Request *req)\n \t\tgReqList.push_back(req);\n \t}\n \n+\t/*\n+\t * Prevent the write below from possibly being reordered to happen\n+\t * before the push_back() above.\n+\t */\n+\tstd::atomic_thread_fence(std::memory_order_acquire);\n+\n \tuint64_t v = 1;\n \tsize_t s = write(gEventfd, &v, 8);\n \t/*\n@@ -219,6 +226,12 @@ PYBIND11_MODULE(_libcamera, m)\n \t\t\tif (read(gEventfd, buf, 8) != 8)\n \t\t\t\tthrow std::system_error(errno, std::generic_category());\n \n+\t\t\t/*\n+\t\t\t * Prevent the read above from possibly being reordered\n+\t\t\t * to happen after the swap() below.\n+\t\t\t */\n+\t\t\tstd::atomic_thread_fence(std::memory_order_release);\n+\n \t\t\tstd::vector v;\n \n \t\t\t{","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 CC27BBD160\n\tfor ;\n\tTue, 31 May 2022 05:51:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1F0BD65633;\n\tTue, 31 May 2022 07:51:58 +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 E42EB6040A\n\tfor ;\n\tTue, 31 May 2022 07:51:56 +0200 (CEST)","from [192.168.1.111] (91-156-85-209.elisa-laajakaista.fi\n\t[91.156.85.209])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0F62D6F0;\n\tTue, 31 May 2022 07:51:56 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653976318;\n\tbh=o8hsJ6xJoyP6/tR88gPZ1TC8+TUyj04TatytgIHr1Sk=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=FB/oyoRj6efngbwH7TiTfKJiWP4HFiilT3neXgcNWH9MZVC2aNFJJOCMT4Yohk0hE\n\tbH/j8Qx/qLPZxDm2gubMgMVWEWG7OovTsCWXWMpROwlQ5I+tZlv7NMCGGvcB7qaTmj\n\tMd9gju6q+auvGFq94ItEVWxseJ52HWQylFEmiDHNUZUZQWVs9qO+97g5SpErylkduf\n\twOo/jjdOc2+6Ka4PjgjXFdY54fRb6I9Dks5jvkYlwAMrfoyMOFJWAyO+gIx1QtYiCF\n\tafK9Sg4jHzTCHxmOC6Zt41fGcLgjkNRhnjMrsCyUc0QVAT5HDOVn0rjTfJ/vBITVUE\n\tAfvMHAcThdH0Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653976316;\n\tbh=o8hsJ6xJoyP6/tR88gPZ1TC8+TUyj04TatytgIHr1Sk=;\n\th=Date:Subject:From:To:Cc:References:In-Reply-To:From;\n\tb=CWB550Pcr+8/yUUWOPrc00nZnuvcunEICcjI9Yz9IsMXukC4J0Ur4t1apyKanqOfT\n\tBPCTez1haZ1ufKzfKkwqH2adbTSd37ZOyobcuzcscC3hoExg7jZUaEwzyS5YrvgOdB\n\t9MLDLl+Wfbfu6b/EhzZJtMKJr981EL7/cBPPZPDs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"CWB550Pc\"; dkim-atps=neutral","Message-ID":"<9d1e128a-22ca-87e8-540e-a1a02e7d07fa@ideasonboard.com>","Date":"Tue, 31 May 2022 08:51:52 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220530142722.57618-1-tomi.valkeinen@ideasonboard.com>\n\t<20220530142722.57618-12-tomi.valkeinen@ideasonboard.com>\n\t\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v4 11/16] py: merge read_event() and\n\tget_ready_requests()","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23289,"web_url":"https://patchwork.libcamera.org/comment/23289/","msgid":"<87a0dfa0-dedf-8b77-7b47-d9ff7acdbdbc@ideasonboard.com>","date":"2022-06-02T08:27:35","subject":"Re: [libcamera-devel] [PATCH v4 11/16] py: merge read_event() and\n\tget_ready_requests()","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 31/05/2022 08:51, Tomi Valkeinen wrote:\n> On 30/05/2022 19:57, Tomi Valkeinen wrote:\n> \n>>>> diff --git a/src/py/libcamera/py_main.cpp \n>>>> b/src/py/libcamera/py_main.cpp\n>>>> index fcf009f0..505cc3dc 100644\n>>>> --- a/src/py/libcamera/py_main.cpp\n>>>> +++ b/src/py/libcamera/py_main.cpp\n>>>> @@ -213,15 +213,12 @@ PYBIND11_MODULE(_libcamera, m)\n>>>>               return gEventfd;\n>>>>           })\n>>>> -        .def(\"read_event\", [](CameraManager &) {\n>>>> +        .def(\"get_ready_requests\", [](CameraManager &) {\n>>>>               uint8_t buf[8];\n>>>> -            int ret = read(gEventfd, buf, 8);\n>>>> -            if (ret != 8)\n>>>> +            if (read(gEventfd, buf, 8) != 8)\n>>>>                   throw std::system_error(errno, \n>>>> std::generic_category());\n>>>\n>>> We need a memory barrier here to prevent reordering. I'm quite sure\n>>\n>> We then need the same in handleRequestCompleted().\n>>\n>>> there's one somewhere due to the read() call and the lock below, but I\n>>> haven't been able to figure out the C++ rule that guarantees this. Does\n>>> anyone know ?\n>>\n>> https://en.cppreference.com/w/cpp/atomic/memory_order\n>>\n>> So the mutex brings the normal acquire-release ordering. I would think \n>> a syscall includes a barrier, but I can't find any details right away.\n> \n> Perhaps something like this? I don't know if the fences are needed, but \n> maybe\n> better safe than sorry.\n> \n> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> index 505cc3dc..b035a101 100644\n> --- a/src/py/libcamera/py_main.cpp\n> +++ b/src/py/libcamera/py_main.cpp\n> @@ -5,6 +5,7 @@\n>   * Python bindings\n>   */\n> \n> +#include \n>  #include \n>  #include \n>  #include \n> @@ -116,6 +117,12 @@ static void handleRequestCompleted(Request *req)\n>          gReqList.push_back(req);\n>      }\n> \n> +    /*\n> +     * Prevent the write below from possibly being reordered to happen\n> +     * before the push_back() above.\n> +     */\n> +    std::atomic_thread_fence(std::memory_order_acquire);\n> +\n>      uint64_t v = 1;\n>      size_t s = write(gEventfd, &v, 8);\n>      /*\n> @@ -219,6 +226,12 @@ PYBIND11_MODULE(_libcamera, m)\n>              if (read(gEventfd, buf, 8) != 8)\n>                  throw std::system_error(errno, std::generic_category());\n> \n> +            /*\n> +             * Prevent the read above from possibly being reordered\n> +             * to happen after the swap() below.\n> +             */\n> +            std::atomic_thread_fence(std::memory_order_release);\n> +\n>              std::vector v;\n> \n>              {\n\nI think this could use std::atomic_signal_fence, but I'm pretty sure \nthere's no real life performance difference.\n\n Tomi","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 3C105BD160\n\tfor ;\n\tThu, 2 Jun 2022 08:27:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 865D16563A;\n\tThu, 2 Jun 2022 10:27:41 +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 EEC9365633\n\tfor ;\n\tThu, 2 Jun 2022 10:27:39 +0200 (CEST)","from [192.168.1.111] (91-156-85-209.elisa-laajakaista.fi\n\t[91.156.85.209])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0F4C56BD;\n\tThu, 2 Jun 2022 10:27:39 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654158461;\n\tbh=wKNNYD8wreDqc4+CeokVBG84ws1x321QJUx//MNEKkM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=nGwz2oi1TrGyc3j+RTijNYgPFkEAMid3r/LCfB96Q5SyCOctgqL1PsuoUielh/max\n\tI/cdrhUy4lGkIm1FP5/UPk9Y9VRFHUObR/LtTrOhV1FMEA59H/rSvqgX+3LOn9sQcu\n\tN4fGth35xoP1oZ4zI4jYmMry6hWwiEXDcyUx4XBfEcYvZGufVphN++pKiq1qO+Y03u\n\tPfjCy1xdJxErWWmpjcFhfXccREtvp9C6c4VbOMhHmlEOKqAMtU5ofzfR0L2bjuADnp\n\tk69mEx+OnkPWDqb9XLz7MDFMMobfHK5p+xrXwq5w8Oc0hS7PuHWI/6HwG/vhLKrm9O\n\t6XmdYmmjqZAFA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654158459;\n\tbh=wKNNYD8wreDqc4+CeokVBG84ws1x321QJUx//MNEKkM=;\n\th=Date:Subject:From:To:Cc:References:In-Reply-To:From;\n\tb=fmgEpqTpcZuSTJakt7YU+k6UfPxMVFWlKQ5hcDV1pARXSg+5Hcp5eJnIhuNYHMYn9\n\toujZbVJJAHcsOPqOZBM6PTOTKlmkIucViRog2In+BNWbL2sRT3Bbc7VgKXRiuEf9ks\n\tLB6yC4xVeL4Hlyp+bLkoow/tiz6VU3GQFbbEryag="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"fmgEpqTp\"; dkim-atps=neutral","Message-ID":"<87a0dfa0-dedf-8b77-7b47-d9ff7acdbdbc@ideasonboard.com>","Date":"Thu, 2 Jun 2022 11:27:35 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220530142722.57618-1-tomi.valkeinen@ideasonboard.com>\n\t<20220530142722.57618-12-tomi.valkeinen@ideasonboard.com>\n\t\n\t\n\t<9d1e128a-22ca-87e8-540e-a1a02e7d07fa@ideasonboard.com>","In-Reply-To":"<9d1e128a-22ca-87e8-540e-a1a02e7d07fa@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v4 11/16] py: merge read_event() and\n\tget_ready_requests()","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23313,"web_url":"https://patchwork.libcamera.org/comment/23313/","msgid":"","date":"2022-06-03T23:16:28","subject":"Re: [libcamera-devel] [PATCH v4 11/16] py: merge read_event() and\n\tget_ready_requests()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomi,\n\nOn Thu, Jun 02, 2022 at 11:27:35AM +0300, Tomi Valkeinen wrote:\n> On 31/05/2022 08:51, Tomi Valkeinen wrote:\n> > On 30/05/2022 19:57, Tomi Valkeinen wrote:\n> > \n> >>>> diff --git a/src/py/libcamera/py_main.cpp \n> >>>> b/src/py/libcamera/py_main.cpp\n> >>>> index fcf009f0..505cc3dc 100644\n> >>>> --- a/src/py/libcamera/py_main.cpp\n> >>>> +++ b/src/py/libcamera/py_main.cpp\n> >>>> @@ -213,15 +213,12 @@ PYBIND11_MODULE(_libcamera, m)\n> >>>>               return gEventfd;\n> >>>>           })\n> >>>> -        .def(\"read_event\", [](CameraManager &) {\n> >>>> +        .def(\"get_ready_requests\", [](CameraManager &) {\n> >>>>               uint8_t buf[8];\n> >>>> -            int ret = read(gEventfd, buf, 8);\n> >>>> -            if (ret != 8)\n> >>>> +            if (read(gEventfd, buf, 8) != 8)\n> >>>>                   throw std::system_error(errno, std::generic_category());\n> >>>\n> >>> We need a memory barrier here to prevent reordering. I'm quite sure\n> >>\n> >> We then need the same in handleRequestCompleted().\n> >>\n> >>> there's one somewhere due to the read() call and the lock below, but I\n> >>> haven't been able to figure out the C++ rule that guarantees this. Does\n> >>> anyone know ?\n> >>\n> >> https://en.cppreference.com/w/cpp/atomic/memory_order\n> >>\n> >> So the mutex brings the normal acquire-release ordering. I would think \n> >> a syscall includes a barrier, but I can't find any details right away.\n> > \n> > Perhaps something like this? I don't know if the fences are needed, but maybe\n> > better safe than sorry.\n\nThanks for all the information. If I understand things correctly, the\nmutex should be enough. See below (where I'm sure I'll say many stupid,\nor at least incorrect, things).\n\n> > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> > index 505cc3dc..b035a101 100644\n> > --- a/src/py/libcamera/py_main.cpp\n> > +++ b/src/py/libcamera/py_main.cpp\n> > @@ -5,6 +5,7 @@\n> >   * Python bindings\n> >   */\n> > \n> > +#include \n> >  #include \n> >  #include \n> >  #include \n> > @@ -116,6 +117,12 @@ static void handleRequestCompleted(Request *req)\n\nAdding some more context:\n\n {\n std::lock_guard guard(gReqlistMutex);\n\t ^---- [1]\n> >          gReqList.push_back(req);\n> >      }\n ^---- [2]\n\n[1] locks the mutex, which is an acquire operation. It means that no\nread or write in the same thread after the lock() can be reordered\nbefore it.\n\n[2] unlocks the mutex, which is a release operation. It means that no\nread or write in the same thread before the unlock() can be reordered\nafter it.\n\nOn the reader side, we could thus observe the write() below before the\npush_back, but never before the lock(). Effectively, what we could see\non the reader thread, assuming that the write() doesn't contain any\nbarrier (which is likely incorrect), is\n\n\t{\n\t\tstd::lock_guard guard(gReqlistMutex);\n\t\twrite(gEventfd, &v, 8);\n\t\tgReqList.push_back(req);\n\t}\n\n> > \n> > +    /*\n> > +     * Prevent the write below from possibly being reordered to happen\n> > +     * before the push_back() above.\n> > +     */\n> > +    std::atomic_thread_fence(std::memory_order_acquire);\n> > +\n> >      uint64_t v = 1;\n> >      size_t s = write(gEventfd, &v, 8);\n> >      /*\n> > @@ -219,6 +226,12 @@ PYBIND11_MODULE(_libcamera, m)\n> >              if (read(gEventfd, buf, 8) != 8)\n> >                  throw std::system_error(errno, std::generic_category());\n> > \n> > +            /*\n> > +             * Prevent the read above from possibly being reordered\n> > +             * to happen after the swap() below.\n> > +             */\n> > +            std::atomic_thread_fence(std::memory_order_release);\n> > +\n> >              std::vector v;\n> > \n> >              {\n std::lock_guard guard(gReqlistMutex);\n\t\t\t ^---- [3]\n swap(v, gReqList);\n }\n\t\t ^---- [4]\n\nSimilarly, [3] is an acquire operation, [3] is a release operation. What\ncould thus be observed by the writer is\n\n\t{\n\t\tstd::lock_guard guard(qReglistMutex);\n\t\tswap(v, gReqList);\n\t\tread(gEventfd, buf, 8);\n\t}\n\nIf get_ready_requests() gets called because select() wakes up due to an\nevent on the eventfd, it means the write() has been performed. While it\nmay be seen by the reader before the .push_back(), the lock will ensure\nsequential consistency between the reader and the writer, the reader\nwon't be able to acquire the lock before the writer releases it, so\nswap() will not occur before the .push_back() is observed.\n\nIf get_ready_requests() gets called in blocking mode, read() would\nblock, so there's no way it can get reordered after the lock() by the\nCPU as the latter won't be executed yet. I'm pretty sure the \"as-is\"\nrule (https://en.cppreference.com/w/cpp/language/as_if) prevents the\ncompiler from reordering the read() after the lock() :-) This should\nthus also be safe.\n\nTl;dr:\n\nReviewed-by: Laurent Pinchart \n\nNow the real question is: how many mistakes or inaccuracies does the\nabove explanation contain ? :-)\n\n> I think this could use std::atomic_signal_fence, but I'm pretty sure \n> there's no real life performance difference.","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 CBF56BD161\n\tfor ;\n\tFri, 3 Jun 2022 23:16:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EAFC665638;\n\tSat, 4 Jun 2022 01:16:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 030406040E\n\tfor ;\n\tSat, 4 Jun 2022 01:16:34 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(lmontsouris-659-1-41-236.w92-154.abo.wanadoo.fr [92.154.76.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BE248496;\n\tSat, 4 Jun 2022 01:16:33 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654298197;\n\tbh=CBJvx32ry8lLeJJ5xCCNxqe3WAExVfEr+wsgyhjUyB0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=PSJIrnEt/UGO/YX2P0rC7CJvx5XjigYvK4BUI1WQw+16hBZbTgRWheyY4iWQUR/qs\n\taUXPXYN8gC2h52u5XftlTRyTo1dtr45ARmUtpUK0KC5msqRme60NYE5UuH1lmWDdqJ\n\tmbvh9mggBcazqiglXS5rt5Tlwne1R8PzPl/P9QJlw4IGJDc8MwUo2JuC/h52ytmwdK\n\tYt/IYJBVeWX1lqhF7IwIL59TOGFW555O4f+2J21jQEdAEuoUpj5DLJwXzLXs8OQ3eT\n\tBOVG1c5KifaVKGZUoEUJLSubtGMIsiDn+2r8iOn+baMXsiC6++aR36LuDH8DzCjiQj\n\tJI2g2gLuR5ndw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654298193;\n\tbh=CBJvx32ry8lLeJJ5xCCNxqe3WAExVfEr+wsgyhjUyB0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XYQXt4ewoEQC+1RwgL9eOobHEEcvff8Nf2LegCfhbbZ/RQcIDJyZfHPRngScKKahS\n\tB3BPgp6sYUXMmvUfnvoFuEFr/mPpUzx/6pNCsSifn5lJa8qR321PnZ5apq3iXpZhNJ\n\t6u218m68de39b7PTg/8L8zovtiSOTcCBaOGxypoU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"XYQXt4ew\"; dkim-atps=neutral","Date":"Sat, 4 Jun 2022 02:16:28 +0300","To":"Tomi Valkeinen ","Message-ID":"","References":"<20220530142722.57618-1-tomi.valkeinen@ideasonboard.com>\n\t<20220530142722.57618-12-tomi.valkeinen@ideasonboard.com>\n\t\n\t\n\t<9d1e128a-22ca-87e8-540e-a1a02e7d07fa@ideasonboard.com>\n\t<87a0dfa0-dedf-8b77-7b47-d9ff7acdbdbc@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<87a0dfa0-dedf-8b77-7b47-d9ff7acdbdbc@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 11/16] py: merge read_event() and\n\tget_ready_requests()","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23330,"web_url":"https://patchwork.libcamera.org/comment/23330/","msgid":"<00d48c11-d4e5-8b07-ef2a-bc9489112c35@ideasonboard.com>","date":"2022-06-06T06:36:12","subject":"Re: [libcamera-devel] [PATCH v4 11/16] py: merge read_event() and\n\tget_ready_requests()","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 04/06/2022 02:16, Laurent Pinchart wrote:\n> Hi Tomi,\n> \n> On Thu, Jun 02, 2022 at 11:27:35AM +0300, Tomi Valkeinen wrote:\n>> On 31/05/2022 08:51, Tomi Valkeinen wrote:\n>>> On 30/05/2022 19:57, Tomi Valkeinen wrote:\n>>>\n>>>>>> diff --git a/src/py/libcamera/py_main.cpp\n>>>>>> b/src/py/libcamera/py_main.cpp\n>>>>>> index fcf009f0..505cc3dc 100644\n>>>>>> --- a/src/py/libcamera/py_main.cpp\n>>>>>> +++ b/src/py/libcamera/py_main.cpp\n>>>>>> @@ -213,15 +213,12 @@ PYBIND11_MODULE(_libcamera, m)\n>>>>>>               return gEventfd;\n>>>>>>           })\n>>>>>> -        .def(\"read_event\", [](CameraManager &) {\n>>>>>> +        .def(\"get_ready_requests\", [](CameraManager &) {\n>>>>>>               uint8_t buf[8];\n>>>>>> -            int ret = read(gEventfd, buf, 8);\n>>>>>> -            if (ret != 8)\n>>>>>> +            if (read(gEventfd, buf, 8) != 8)\n>>>>>>                   throw std::system_error(errno, std::generic_category());\n>>>>>\n>>>>> We need a memory barrier here to prevent reordering. I'm quite sure\n>>>>\n>>>> We then need the same in handleRequestCompleted().\n>>>>\n>>>>> there's one somewhere due to the read() call and the lock below, but I\n>>>>> haven't been able to figure out the C++ rule that guarantees this. Does\n>>>>> anyone know ?\n>>>>\n>>>> https://en.cppreference.com/w/cpp/atomic/memory_order\n>>>>\n>>>> So the mutex brings the normal acquire-release ordering. I would think\n>>>> a syscall includes a barrier, but I can't find any details right away.\n>>>\n>>> Perhaps something like this? I don't know if the fences are needed, but maybe\n>>> better safe than sorry.\n> \n> Thanks for all the information. If I understand things correctly, the\n> mutex should be enough. See below (where I'm sure I'll say many stupid,\n> or at least incorrect, things).\n> \n>>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n>>> index 505cc3dc..b035a101 100644\n>>> --- a/src/py/libcamera/py_main.cpp\n>>> +++ b/src/py/libcamera/py_main.cpp\n>>> @@ -5,6 +5,7 @@\n>>>   * Python bindings\n>>>   */\n>>>\n>>> +#include \n>>>  #include \n>>>  #include \n>>>  #include \n>>> @@ -116,6 +117,12 @@ static void handleRequestCompleted(Request *req)\n> \n> Adding some more context:\n> \n> {\n> std::lock_guard guard(gReqlistMutex);\n> \t ^---- [1]\n>>>          gReqList.push_back(req);\n>>>      }\n> ^---- [2]\n> \n> [1] locks the mutex, which is an acquire operation. It means that no\n> read or write in the same thread after the lock() can be reordered\n> before it.\n> \n> [2] unlocks the mutex, which is a release operation. It means that no\n> read or write in the same thread before the unlock() can be reordered\n> after it.\n> \n> On the reader side, we could thus observe the write() below before the\n> push_back, but never before the lock(). Effectively, what we could see\n> on the reader thread, assuming that the write() doesn't contain any\n> barrier (which is likely incorrect), is\n> \n> \t{\n> \t\tstd::lock_guard guard(gReqlistMutex);\n> \t\twrite(gEventfd, &v, 8);\n> \t\tgReqList.push_back(req);\n> \t}\n> \n>>>\n>>> +    /*\n>>> +     * Prevent the write below from possibly being reordered to happen\n>>> +     * before the push_back() above.\n>>> +     */\n>>> +    std::atomic_thread_fence(std::memory_order_acquire);\n>>> +\n>>>      uint64_t v = 1;\n>>>      size_t s = write(gEventfd, &v, 8);\n>>>      /*\n>>> @@ -219,6 +226,12 @@ PYBIND11_MODULE(_libcamera, m)\n>>>              if (read(gEventfd, buf, 8) != 8)\n>>>                  throw std::system_error(errno, std::generic_category());\n>>>\n>>> +            /*\n>>> +             * Prevent the read above from possibly being reordered\n>>> +             * to happen after the swap() below.\n>>> +             */\n>>> +            std::atomic_thread_fence(std::memory_order_release);\n>>> +\n>>>              std::vector v;\n>>>\n>>>              {\n> std::lock_guard guard(gReqlistMutex);\n> \t\t\t ^---- [3]\n> swap(v, gReqList);\n> }\n> \t\t ^---- [4]\n> \n> Similarly, [3] is an acquire operation, [3] is a release operation. What\n> could thus be observed by the writer is\n> \n> \t{\n> \t\tstd::lock_guard guard(qReglistMutex);\n> \t\tswap(v, gReqList);\n> \t\tread(gEventfd, buf, 8);\n> \t}\n> \n> If get_ready_requests() gets called because select() wakes up due to an\n> event on the eventfd, it means the write() has been performed. While it\n> may be seen by the reader before the .push_back(), the lock will ensure\n> sequential consistency between the reader and the writer, the reader\n> won't be able to acquire the lock before the writer releases it, so\n> swap() will not occur before the .push_back() is observed.\n> \n> If get_ready_requests() gets called in blocking mode, read() would\n> block, so there's no way it can get reordered after the lock() by the\n> CPU as the latter won't be executed yet. I'm pretty sure the \"as-is\"\n> rule (https://en.cppreference.com/w/cpp/language/as_if) prevents the\n> compiler from reordering the read() after the lock() :-) This should\n> thus also be safe.\n\nI agree. I also studied all possible ordering combinations, and I cannot \nfind a case which wouldn't work (without any fences).\n\nAnd I also find it difficult to believe the read/write could happen \ninside the locks, although I still don't know why.\n\nConsider two threads, each of which runs a function which does lock(); \naccess-shared-variable; unlock(). If you then add a read() or write() \ncall to one of those functions, and the call blocks for a longer time, \nand that call is moved to happen inside the lock, it would cause the \nother thread to be blocked too.\n\nI don't think I would ever figure out that kind of bug, if it would \nhappen. But why it would not happen?\n\nWords of wisdom: Single threaded programming is hard. Multi-threaded \nprogramming is impossible.\n\n> Tl;dr:\n> \n> Reviewed-by: Laurent Pinchart \n> \n> Now the real question is: how many mistakes or inaccuracies does the\n> above explanation contain ? :-)\n\nIn ten years, we happen to encounter this mail via some random googling, \nand, oh man, the shame of writing all this nonsense when we were young \nand ignorant...\n\n Tomi","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 AF10BBD161\n\tfor ;\n\tMon, 6 Jun 2022 06:36:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E605265634;\n\tMon, 6 Jun 2022 08:36:16 +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 63F7A600F4\n\tfor ;\n\tMon, 6 Jun 2022 08:36:15 +0200 (CEST)","from [192.168.1.111] (91-156-85-209.elisa-laajakaista.fi\n\t[91.156.85.209])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8FDF930A;\n\tMon, 6 Jun 2022 08:36:14 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654497376;\n\tbh=4rwDepyktAL+C/UWnI6xKmjXpqBB0wURQrj9y3pkp1E=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=jmsb1gm2GdbtlzFIQpqx3Yk0dG5WCLQrGJYu04s6PLNmnt91ZYzNo3kWN0RyVjRGh\n\txdMty8/cl0CBUunG9JNonHDK7pSXwWIQzuxJWAKJ/vsa0Y0HlDJgU1wHooorkuDJQF\n\tF/s0zpQDrTOzBfPeugDcTXhFYVnqnyuDxKBZFpY+nkn5/eBSa4ProSDNu1I9vL7gqx\n\tJQOzB1jCrxvMXl5pi+SyJV6EbyAb7rm+WhkDlN+m9XWLg23Crll2cjqw2TitGm+WF0\n\tdRvhzbptI6bz+jje5bzmI35jaPBLfDUohQrEbwlfl6JNA8wzsc2GLiAb9tJdEu/NF/\n\tsTfjVDiqgO/bA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654497374;\n\tbh=4rwDepyktAL+C/UWnI6xKmjXpqBB0wURQrj9y3pkp1E=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=lfVtSzbE+9keCC/lm5mBRMv7qvGGWJo0y3U+aqOtpnRiFaAzEaivffBfplUkb4t4k\n\tgzFr3Vc7NoNFwiP+fBqU7dhlz+OFFkvadKA+LfYSBvYClhO28EkUdd+cV5HbIqWlyh\n\tjOvyT/7lHevefRdm8OmAsDwcaqW8dHKDKaWxYl10="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lfVtSzbE\"; dkim-atps=neutral","Message-ID":"<00d48c11-d4e5-8b07-ef2a-bc9489112c35@ideasonboard.com>","Date":"Mon, 6 Jun 2022 09:36:12 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.9.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220530142722.57618-1-tomi.valkeinen@ideasonboard.com>\n\t<20220530142722.57618-12-tomi.valkeinen@ideasonboard.com>\n\t\n\t\n\t<9d1e128a-22ca-87e8-540e-a1a02e7d07fa@ideasonboard.com>\n\t<87a0dfa0-dedf-8b77-7b47-d9ff7acdbdbc@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v4 11/16] py: merge read_event() and\n\tget_ready_requests()","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","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]