[{"id":32497,"web_url":"https://patchwork.libcamera.org/comment/32497/","msgid":"","date":"2024-12-03T12:35:38","subject":"Re: [PATCH] Thread: Fix setThreadAffinity issue","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. december 2., hétfő 10:04 keltezéssel, Harvey Yang írta:\n\n> This fixes commit 4d9db06d669044c0c461a2aed79c85c7fe32a502.\n> \n> Previously we call Thread::setThreadAffinityInternal in\n> Thread::startThread, which seems to lead to a SEGV in\n> pthread_setaffinity_np.\n\nI believe the patch should describe what the cause is. As far as I can\nsee it's a data race on `Thread::thread_` where it is concurrently set\nand read from `Thread::start()` and `Thread::startThread()`, respectively.\n\n\n> \n> This patch moves the call after the construction of std::thread.\n> \n\nShould probably have:\n\n Fixes: 4d9db06d66 (\"libcamera: add method to set thread affinity\")\n\n\n> Signed-off-by: Harvey Yang \n> ---\n> src/libcamera/base/thread.cpp | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> index f6322fe31..319bfda9a 100644\n> --- a/src/libcamera/base/thread.cpp\n> +++ b/src/libcamera/base/thread.cpp\n> @@ -257,6 +257,8 @@ void Thread::start()\n> \tdata_->exit_.store(false, std::memory_order_relaxed);\n> \n> \tthread_ = std::thread(&Thread::startThread, this);\n> +\n> +\tsetThreadAffinityInternal();\n> }\n> \n> void Thread::startThread()\n> @@ -284,8 +286,6 @@ void Thread::startThread()\n> \tdata_->tid_ = syscall(SYS_gettid);\n> \tcurrentThreadData = data_;\n> \n> -\tsetThreadAffinityInternal();\n> -\n> \trun();\n> }\n> \n> --\n> 2.47.0.338.g60cca15819-goog\n> \n\n\nRegards,\nBarnabás Pőcze","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 36283BDC71\n\tfor ;\n\tTue, 3 Dec 2024 12:35:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6A4A16608E;\n\tTue, 3 Dec 2024 13:35:46 +0100 (CET)","from mail-4322.protonmail.ch (mail-4322.protonmail.ch\n\t[185.70.43.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 78B0365FD6\n\tfor ;\n\tTue, 3 Dec 2024 13:35:44 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"fuLQT0xL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1733229343; x=1733488543;\n\tbh=00ErRUxXCJ9iRvB/HZKZ/Brje7Y1T38nvXL0D4q7888=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=fuLQT0xLVixbRLAaCc7Y/pqt2ZrWLruvROG8bVgdnxMbnD9IcsCjyT2HDF0cgRC55\n\tcHJfoc0OV3l0DCmvtmz4KGGSxMSq3FYGk0BehVj8+tjXXwuXdYriNCxM3ShKkZVDXZ\n\tDiI218ycUv8AMu4dxDpQFleOVsKBQjoKYld+G7EPiDiNDReaKJMZTjLjV70ZBTNJIk\n\tgTpPanc9Ii2HtEzErU5bG+yxoKbWaZJGQWMXgaEc0VDnRDCQ6MSWtPsQyMCnHPCwQc\n\taACqZwb6b1mhr+/63RPYtdVuKKfJNgOScJsXjUmfMsEhF28F4fli6G5lmbp8OTLqDm\n\t693x34Jkl5ROg==","Date":"Tue, 03 Dec 2024 12:35:38 +0000","To":"Harvey Yang ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] Thread: Fix setThreadAffinity issue","Message-ID":"","In-Reply-To":"<20241202090457.584020-1-chenghaoyang@chromium.org>","References":"<20241202090457.584020-1-chenghaoyang@chromium.org>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"42eda84f19c8e685127d11b57506506443a9b83e","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":32498,"web_url":"https://patchwork.libcamera.org/comment/32498/","msgid":"<20241203124706.GA24222@pendragon.ideasonboard.com>","date":"2024-12-03T12:47:06","subject":"Re: [PATCH] Thread: Fix setThreadAffinity issue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Dec 03, 2024 at 12:35:38PM +0000, Barnabás Pőcze wrote:\n> 2024. december 2., hétfő 10:04 keltezéssel, Harvey Yang írta:\n> \n> > This fixes commit 4d9db06d669044c0c461a2aed79c85c7fe32a502.\n> > \n> > Previously we call Thread::setThreadAffinityInternal in\n> > Thread::startThread, which seems to lead to a SEGV in\n> > pthread_setaffinity_np.\n> \n> I believe the patch should describe what the cause is. As far as I can\n\nYes, all patches should do so. Saying there seems to be a problem and it\nseems to be fixed by a change without further explanation creates very\nlittle confidence, and it shouldn't be up to the reviewer to try and\nunderstand the fix.\n\n> see it's a data race on `Thread::thread_` where it is concurrently set\n> and read from `Thread::start()` and `Thread::startThread()`, respectively.\n> \n> > This patch moves the call after the construction of std::thread.\n\nIf I recall correctly, setThreadAffinityInternal() was moved to\nThread::startThread() based on review discussions, to pin the thread to\nCPUs before starting its main function. This patch drops this. The\ncommit message needs to explain why this is considered fine.\n\n> Should probably have:\n> \n> Fixes: 4d9db06d66 (\"libcamera: add method to set thread affinity\")\n> \n> > Signed-off-by: Harvey Yang \n> > ---\n> > src/libcamera/base/thread.cpp | 4 ++--\n> > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> > index f6322fe31..319bfda9a 100644\n> > --- a/src/libcamera/base/thread.cpp\n> > +++ b/src/libcamera/base/thread.cpp\n> > @@ -257,6 +257,8 @@ void Thread::start()\n> > \tdata_->exit_.store(false, std::memory_order_relaxed);\n> > \n> > \tthread_ = std::thread(&Thread::startThread, this);\n> > +\n> > +\tsetThreadAffinityInternal();\n> > }\n> > \n> > void Thread::startThread()\n> > @@ -284,8 +286,6 @@ void Thread::startThread()\n> > \tdata_->tid_ = syscall(SYS_gettid);\n> > \tcurrentThreadData = data_;\n> > \n> > -\tsetThreadAffinityInternal();\n> > -\n> > \trun();\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 F04A1BDCBF\n\tfor ;\n\tTue, 3 Dec 2024 12:47:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D12C26608E;\n\tTue, 3 Dec 2024 13:47:19 +0100 (CET)","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 38A3365FD6\n\tfor ;\n\tTue, 3 Dec 2024 13:47:18 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ADCB5E1;\n\tTue, 3 Dec 2024 13:46:50 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"kc1sjWii\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733230010;\n\tbh=f8vyWY8Ai8eD0TpDZXGUGP55vmh040aJt0wjxj4zwms=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kc1sjWiiGvZUj3QEhHa0pvMvasK0QVLWN/dKyTYGtsNAa/C3hoo+Z6aAN03f2RXtV\n\tEjICtaJgCsBR8VGTMRF6N4uEixyBqnM8bTi/oIHznwCj9xsaWM1BfNHbKNUtOQpSiU\n\tYXlDXVLVb55set/h+AegYxuZKZtTU44Ph4q+/mYc=","Date":"Tue, 3 Dec 2024 14:47:06 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] Thread: Fix setThreadAffinity issue","Message-ID":"<20241203124706.GA24222@pendragon.ideasonboard.com>","References":"<20241202090457.584020-1-chenghaoyang@chromium.org>\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":32937,"web_url":"https://patchwork.libcamera.org/comment/32937/","msgid":"<20250107094054.GA15717@pendragon.ideasonboard.com>","date":"2025-01-07T09:40:54","subject":"Re: [PATCH] Thread: Fix setThreadAffinity issue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Harvey,\n\nOn Tue, Dec 03, 2024 at 02:47:06PM +0200, Laurent Pinchart wrote:\n> On Tue, Dec 03, 2024 at 12:35:38PM +0000, Barnabás Pőcze wrote:\n> > 2024. december 2., hétfő 10:04 keltezéssel, Harvey Yang írta:\n> > \n> > > This fixes commit 4d9db06d669044c0c461a2aed79c85c7fe32a502.\n> > > \n> > > Previously we call Thread::setThreadAffinityInternal in\n> > > Thread::startThread, which seems to lead to a SEGV in\n> > > pthread_setaffinity_np.\n> > \n> > I believe the patch should describe what the cause is. As far as I can\n> \n> Yes, all patches should do so. Saying there seems to be a problem and it\n> seems to be fixed by a change without further explanation creates very\n> little confidence, and it shouldn't be up to the reviewer to try and\n> understand the fix.\n\nCould you please send a new version with an improved commit message ?\n\n> > see it's a data race on `Thread::thread_` where it is concurrently set\n> > and read from `Thread::start()` and `Thread::startThread()`, respectively.\n> > \n> > > This patch moves the call after the construction of std::thread.\n> \n> If I recall correctly, setThreadAffinityInternal() was moved to\n> Thread::startThread() based on review discussions, to pin the thread to\n> CPUs before starting its main function. This patch drops this. The\n> commit message needs to explain why this is considered fine.\n> \n> > Should probably have:\n> > \n> > Fixes: 4d9db06d66 (\"libcamera: add method to set thread affinity\")\n> > \n> > > Signed-off-by: Harvey Yang \n> > > ---\n> > > src/libcamera/base/thread.cpp | 4 ++--\n> > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> > > index f6322fe31..319bfda9a 100644\n> > > --- a/src/libcamera/base/thread.cpp\n> > > +++ b/src/libcamera/base/thread.cpp\n> > > @@ -257,6 +257,8 @@ void Thread::start()\n> > > \tdata_->exit_.store(false, std::memory_order_relaxed);\n> > > \n> > > \tthread_ = std::thread(&Thread::startThread, this);\n> > > +\n> > > +\tsetThreadAffinityInternal();\n> > > }\n> > > \n> > > void Thread::startThread()\n> > > @@ -284,8 +286,6 @@ void Thread::startThread()\n> > > \tdata_->tid_ = syscall(SYS_gettid);\n> > > \tcurrentThreadData = data_;\n> > > \n> > > -\tsetThreadAffinityInternal();\n> > > -\n> > > \trun();\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 36CC8C32DC\n\tfor ;\n\tTue, 7 Jan 2025 09:41:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 36D20684E2;\n\tTue, 7 Jan 2025 10:41:01 +0100 (CET)","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 7C32F684D0\n\tfor ;\n\tTue, 7 Jan 2025 10:40:58 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B10169FF;\n\tTue, 7 Jan 2025 10:40:05 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"FrGUijg4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736242805;\n\tbh=kz0wEEEPX8Ir60QJ4II7avBwg7jdFgwCKHX3GEwaELE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FrGUijg4qBc/WSPbNsmVo/TSQh1fmr7/I8eYm79qHnTHMi7LY28uKxOS3ZXeGRLNx\n\tASQtGoLdFu4AmQpqkGuKZq5ASreZBCoRaQkEdVeQK7f/rqa9PNdhbSDRnwWSDqD8Yw\n\tF/N4Q5ZysRzjGYu8migplw0aKKfwc6dF5j3Z3YMI=","Date":"Tue, 7 Jan 2025 11:40:54 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] Thread: Fix setThreadAffinity issue","Message-ID":"<20250107094054.GA15717@pendragon.ideasonboard.com>","References":"<20241202090457.584020-1-chenghaoyang@chromium.org>\n\t\n\t<20241203124706.GA24222@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20241203124706.GA24222@pendragon.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":32938,"web_url":"https://patchwork.libcamera.org/comment/32938/","msgid":"<173624898420.2992722.12255017991954329922@ping.linuxembedded.co.uk>","date":"2025-01-07T11:23:04","subject":"Re: [PATCH] Thread: Fix setThreadAffinity issue","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2025-01-07 09:40:54)\n> Hi Harvey,\n> \n> On Tue, Dec 03, 2024 at 02:47:06PM +0200, Laurent Pinchart wrote:\n> > On Tue, Dec 03, 2024 at 12:35:38PM +0000, Barnabás Pőcze wrote:\n> > > 2024. december 2., hétfő 10:04 keltezéssel, Harvey Yang írta:\n> > > \n> > > > This fixes commit 4d9db06d669044c0c461a2aed79c85c7fe32a502.\n> > > > \n> > > > Previously we call Thread::setThreadAffinityInternal in\n> > > > Thread::startThread, which seems to lead to a SEGV in\n> > > > pthread_setaffinity_np.\n> > > \n> > > I believe the patch should describe what the cause is. As far as I can\n> > \n> > Yes, all patches should do so. Saying there seems to be a problem and it\n> > seems to be fixed by a change without further explanation creates very\n> > little confidence, and it shouldn't be up to the reviewer to try and\n> > understand the fix.\n> \n> Could you please send a new version with an improved commit message ?\n\nI saw this issue in CI today again too:\n\nhttps://gitlab.freedesktop.org/camera/libcamera/-/jobs/68964947\n\nSo I'd be keen to merge this once the commit message is cleaned up.\n\n--\nKieran\n\n\n> \n> > > see it's a data race on `Thread::thread_` where it is concurrently set\n> > > and read from `Thread::start()` and `Thread::startThread()`, respectively.\n> > > \n> > > > This patch moves the call after the construction of std::thread.\n> > \n> > If I recall correctly, setThreadAffinityInternal() was moved to\n> > Thread::startThread() based on review discussions, to pin the thread to\n> > CPUs before starting its main function. This patch drops this. The\n> > commit message needs to explain why this is considered fine.\n> > \n> > > Should probably have:\n> > > \n> > > Fixes: 4d9db06d66 (\"libcamera: add method to set thread affinity\")\n> > > \n> > > > Signed-off-by: Harvey Yang \n> > > > ---\n> > > > src/libcamera/base/thread.cpp | 4 ++--\n> > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > \n> > > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> > > > index f6322fe31..319bfda9a 100644\n> > > > --- a/src/libcamera/base/thread.cpp\n> > > > +++ b/src/libcamera/base/thread.cpp\n> > > > @@ -257,6 +257,8 @@ void Thread::start()\n> > > > data_->exit_.store(false, std::memory_order_relaxed);\n> > > > \n> > > > thread_ = std::thread(&Thread::startThread, this);\n> > > > +\n> > > > + setThreadAffinityInternal();\n> > > > }\n> > > > \n> > > > void Thread::startThread()\n> > > > @@ -284,8 +286,6 @@ void Thread::startThread()\n> > > > data_->tid_ = syscall(SYS_gettid);\n> > > > currentThreadData = data_;\n> > > > \n> > > > - setThreadAffinityInternal();\n> > > > -\n> > > > run();\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 DF420BD80A\n\tfor ;\n\tTue, 7 Jan 2025 11:23:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 450AD684E2;\n\tTue, 7 Jan 2025 12:23:09 +0100 (CET)","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 C29BF684CD\n\tfor ;\n\tTue, 7 Jan 2025 12:23:07 +0100 (CET)","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 0730F74A;\n\tTue, 7 Jan 2025 12:22:14 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PaxRnQRB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736248935;\n\tbh=fVwdgnyCcjTidtoTQJYUOUrm9m6hG/1iUtCvqw5MlQo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=PaxRnQRBgcne0LBUIglYwMnMFkT8u4Hrf8aXTGON4O7p5QChiilWYIuhe6lZhBKOe\n\t1SWGDY1rTaD7MeTztbOwwd3JkgvX+M275tfthVLcawodoG7q8qLl7I1wL8HMvEmaTX\n\tQUW7VDDpQx8KpGkusj/rG9quWWYmlpoTz/37C+Fc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250107094054.GA15717@pendragon.ideasonboard.com>","References":"<20241202090457.584020-1-chenghaoyang@chromium.org>\n\t\n\t<20241203124706.GA24222@pendragon.ideasonboard.com>\n\t<20250107094054.GA15717@pendragon.ideasonboard.com>","Subject":"Re: [PATCH] Thread: Fix setThreadAffinity issue","From":"Kieran Bingham ","Cc":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tLaurent Pinchart ","Date":"Tue, 07 Jan 2025 11:23:04 +0000","Message-ID":"<173624898420.2992722.12255017991954329922@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":32942,"web_url":"https://patchwork.libcamera.org/comment/32942/","msgid":"","date":"2025-01-07T14:43:18","subject":"Re: [PATCH] Thread: Fix setThreadAffinity issue","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi all,\n\nUploaded v2.\n\nAs it was Barnabás who found the root cause, I wouldn't mind if\nBarnabás takes the credit and uploads new versions of the fix. JFYI.\n\nBR,\nHarvey\n\nOn Tue, Jan 7, 2025 at 12:23 PM Kieran Bingham\n wrote:\n>\n> Quoting Laurent Pinchart (2025-01-07 09:40:54)\n> > Hi Harvey,\n> >\n> > On Tue, Dec 03, 2024 at 02:47:06PM +0200, Laurent Pinchart wrote:\n> > > On Tue, Dec 03, 2024 at 12:35:38PM +0000, Barnabás Pőcze wrote:\n> > > > 2024. december 2., hétfő 10:04 keltezéssel, Harvey Yang írta:\n> > > >\n> > > > > This fixes commit 4d9db06d669044c0c461a2aed79c85c7fe32a502.\n> > > > >\n> > > > > Previously we call Thread::setThreadAffinityInternal in\n> > > > > Thread::startThread, which seems to lead to a SEGV in\n> > > > > pthread_setaffinity_np.\n> > > >\n> > > > I believe the patch should describe what the cause is. As far as I can\n> > >\n> > > Yes, all patches should do so. Saying there seems to be a problem and it\n> > > seems to be fixed by a change without further explanation creates very\n> > > little confidence, and it shouldn't be up to the reviewer to try and\n> > > understand the fix.\n> >\n> > Could you please send a new version with an improved commit message ?\n>\n> I saw this issue in CI today again too:\n>\n> https://gitlab.freedesktop.org/camera/libcamera/-/jobs/68964947\n>\n> So I'd be keen to merge this once the commit message is cleaned up.\n>\n> --\n> Kieran\n>\n>\n> >\n> > > > see it's a data race on `Thread::thread_` where it is concurrently set\n> > > > and read from `Thread::start()` and `Thread::startThread()`, respectively.\n> > > >\n> > > > > This patch moves the call after the construction of std::thread.\n> > >\n> > > If I recall correctly, setThreadAffinityInternal() was moved to\n> > > Thread::startThread() based on review discussions, to pin the thread to\n> > > CPUs before starting its main function. This patch drops this. The\n> > > commit message needs to explain why this is considered fine.\n> > >\n> > > > Should probably have:\n> > > >\n> > > > Fixes: 4d9db06d66 (\"libcamera: add method to set thread affinity\")\n> > > >\n> > > > > Signed-off-by: Harvey Yang \n> > > > > ---\n> > > > > src/libcamera/base/thread.cpp | 4 ++--\n> > > > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> > > > > index f6322fe31..319bfda9a 100644\n> > > > > --- a/src/libcamera/base/thread.cpp\n> > > > > +++ b/src/libcamera/base/thread.cpp\n> > > > > @@ -257,6 +257,8 @@ void Thread::start()\n> > > > > data_->exit_.store(false, std::memory_order_relaxed);\n> > > > >\n> > > > > thread_ = std::thread(&Thread::startThread, this);\n> > > > > +\n> > > > > + setThreadAffinityInternal();\n> > > > > }\n> > > > >\n> > > > > void Thread::startThread()\n> > > > > @@ -284,8 +286,6 @@ void Thread::startThread()\n> > > > > data_->tid_ = syscall(SYS_gettid);\n> > > > > currentThreadData = data_;\n> > > > >\n> > > > > - setThreadAffinityInternal();\n> > > > > -\n> > > > > run();\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 5C6BBC32DC\n\tfor ;\n\tTue, 7 Jan 2025 14:43:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1C53F684D6;\n\tTue, 7 Jan 2025 15:43:32 +0100 (CET)","from mail-lj1-x231.google.com (mail-lj1-x231.google.com\n\t[IPv6:2a00:1450:4864:20::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F32AD684CD\n\tfor ;\n\tTue, 7 Jan 2025 15:43:29 +0100 (CET)","by mail-lj1-x231.google.com with SMTP id\n\t38308e7fff4ca-3011c7b39c7so182323921fa.1\n\tfor ;\n\tTue, 07 Jan 2025 06:43:29 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"SyaGjECe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1736261009; x=1736865809;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=qWlmqKTLmt/b03FmWcnYXclNc1gNP94AaDZUN/PoVp8=;\n\tb=SyaGjECe+EHGCJkG0IZ1y+ZnAetogWeRt9odxEySDv1QAvXeYts2N/FqtrysDvMEJX\n\t7mnc1qd9+H7jdocr1MFVZDB1UHxADBIZoG5UlzknQFtssXHBfZpx1gRSC+vwMM6oYOuM\n\tNovVduUPU6rxtTTa1A0V4+SGB3UDRNbXAt6us=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1736261009; x=1736865809;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=qWlmqKTLmt/b03FmWcnYXclNc1gNP94AaDZUN/PoVp8=;\n\tb=c8UOIZ31xvDiBR2eP2Qy92xiJjq9+POOcrhfCLlmRksvjeLzqchop4uQg0XETxuovW\n\t76JKbfJBUBLV/ot+vRqL++zbTvsEbW4hHNtifWVTBQUDX+8Kc2SyA87IxEdu82+MxlIY\n\tIc3Aa/Fr6bm112DEcniz6VRlYcIyx8jVpL3+tqMINNs6jt323293TXsnBhtqG/ukLy4J\n\txB8FkpB/jM4SpjCUB2dv9xCFlZYzMoAkUm79ZULmB/+qLoRlajQmPNhw6ezFPfnQlC0v\n\tPu6/yaE4jh9BUmBO4icb9frntiZUsCbl6bnoi+eEL0ahKIQOey9w/sIsPWu9bceH3yMp\n\tSa0Q==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXeTfkez/JiRYe55X2oHYArNQRcjQAcNeb+tQcvI/XUuIIJhUsO9D7n2hD4pi+BpUIkuX2FO3wUAFberIdwcjA=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YwWw3zTk+YNeOhtnXKvYLNd2UOo3lXOiQYd7zW8vgH6jsKzYEdX\n\t6Rb6Wm1CGJ9Nf4lF8wZ3cL+8ohsObQb5jR6Nl3I/kYxP9T9VCgOcMl17qwWuvmwJqUOnPZgsEBz\n\tqN7ZDE4yekcNB8mOSDkc1MccZr258MkMkd2wv","X-Gm-Gg":"ASbGncujC1sw+uUhsEk1sRWFQ5t2veIKvg9+86pmwSi6oomEJm+kz7yUNs/Xl7yCktf\n\tfywosHl6qFqK3CvLm/X7MOGosbJpZDZrQdHZZ","X-Google-Smtp-Source":"AGHT+IHIt1FO1qurcr72oaynkAI7Kx02cbKPW1qOyhlYVaCoZtA5yptDbHWBYH8ZDSzspBmvLkAJz5zrGiC8rBQXlIA=","X-Received":"by 2002:a05:651c:4cb:b0:300:41a8:125b with SMTP id\n\t38308e7fff4ca-30468623ab5mr174950951fa.37.1736261009144;\n\tTue, 07 Jan 2025 06:43:29 -0800 (PST)","MIME-Version":"1.0","References":"<20241202090457.584020-1-chenghaoyang@chromium.org>\n\t\n\t<20241203124706.GA24222@pendragon.ideasonboard.com>\n\t<20250107094054.GA15717@pendragon.ideasonboard.com>\n\t<173624898420.2992722.12255017991954329922@ping.linuxembedded.co.uk>","In-Reply-To":"<173624898420.2992722.12255017991954329922@ping.linuxembedded.co.uk>","From":"Cheng-Hao Yang ","Date":"Tue, 7 Jan 2025 15:43:18 +0100","X-Gm-Features":"AbW1kvbxM4lUhxfiS6-2ZjDa1pAP0NP9FayLh_gywpwxwnmuRlVmfB3D1BbfFFM","Message-ID":"","Subject":"Re: [PATCH] Thread: Fix setThreadAffinity issue","To":"Kieran Bingham ","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tLaurent Pinchart , \n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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\" "}}]