[{"id":32304,"web_url":"https://patchwork.libcamera.org/comment/32304/","msgid":"<173210175436.1605529.6067472970814854934@ping.linuxembedded.co.uk>","date":"2024-11-20T11:22:34","subject":"Re: [PATCH v7 1/1] libcamera: add method to set thread affinity","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Harvey Yang (2024-10-29 08:57:55)\n> From: Han-Lin Chen \n> \n> Add method to set thread affinity to Thread class.\n> \n> Signed-off-by: Han-Lin Chen \n> Co-developed-by: Harvey Yang \n> Signed-off-by: Harvey Yang \n\nLooks good to me.\n\n\nReviewed-by: Kieran Bingham \n\n> ---\n> include/libcamera/base/thread.h | 5 ++++\n> src/libcamera/base/thread.cpp | 47 +++++++++++++++++++++++++++++++++\n> test/threads.cpp | 40 ++++++++++++++++++++++++++++\n> 3 files changed, 92 insertions(+)\n> \n> diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h\n> index 4f33de63d..3209d4f7c 100644\n> --- a/include/libcamera/base/thread.h\n> +++ b/include/libcamera/base/thread.h\n> @@ -15,6 +15,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> \n> namespace libcamera {\n> @@ -35,6 +36,8 @@ public:\n> void exit(int code = 0);\n> bool wait(utils::duration duration = utils::duration::max());\n> \n> + int setThreadAffinity(const Span &cpus);\n> +\n> bool isRunning();\n> \n> Signal<> finished;\n> @@ -54,6 +57,8 @@ private:\n> void startThread();\n> void finishThread();\n> \n> + void setThreadAffinityInternal();\n> +\n> void postMessage(std::unique_ptr msg, Object *receiver);\n> void removeMessages(Object *receiver);\n> \n> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> index 8735670b8..f6322fe31 100644\n> --- a/src/libcamera/base/thread.cpp\n> +++ b/src/libcamera/base/thread.cpp\n> @@ -9,6 +9,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -128,6 +129,8 @@ private:\n> int exitCode_;\n> \n> MessageQueue messages_;\n> +\n> + std::optional cpuset_;\n> };\n> \n> /**\n> @@ -281,6 +284,8 @@ void Thread::startThread()\n> data_->tid_ = syscall(SYS_gettid);\n> currentThreadData = data_;\n> \n> + setThreadAffinityInternal();\n> +\n> run();\n> }\n> \n> @@ -410,6 +415,48 @@ bool Thread::wait(utils::duration duration)\n> return hasFinished;\n> }\n> \n> +/**\n> + * \\brief Set the CPU affinity mask of the thread\n> + * \\param[in] cpus The list of CPU indices that the thread is set affinity to\n> + *\n> + * The CPU indices should be within [0, std::thread::hardware_concurrency()).\n> + * If any index is invalid, this function won't modify the thread affinity and\n> + * will return an error.\n> + *\n> + * \\return 0 if all indices are valid, -EINVAL otherwise\n> + */\n> +int Thread::setThreadAffinity(const Span &cpus)\n> +{\n> + const unsigned int numCpus = std::thread::hardware_concurrency();\n> +\n> + MutexLocker locker(data_->mutex_);\n> + data_->cpuset_ = cpu_set_t();\n> + CPU_ZERO(&data_->cpuset_.value());\n> +\n> + for (const unsigned int &cpu : cpus) {\n> + if (cpu >= numCpus) {\n> + LOG(Thread, Error) << \"Invalid CPU \" << cpu << \"for thread affinity\";\n> + return -EINVAL;\n> + }\n> +\n> + CPU_SET(cpu, &data_->cpuset_.value());\n> + }\n> +\n> + if (data_->running_)\n> + setThreadAffinityInternal();\n> +\n> + return 0;\n> +}\n> +\n> +void Thread::setThreadAffinityInternal()\n> +{\n> + if (!data_->cpuset_)\n> + return;\n> +\n> + const cpu_set_t &cpuset = data_->cpuset_.value();\n> + pthread_setaffinity_np(thread_.native_handle(), sizeof(cpuset), &cpuset);\n> +}\n> +\n> /**\n> * \\brief Check if the thread is running\n> *\n> diff --git a/test/threads.cpp b/test/threads.cpp\n> index ceb4fa0f2..8d6ee1510 100644\n> --- a/test/threads.cpp\n> +++ b/test/threads.cpp\n> @@ -9,9 +9,11 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> +#include \n> #include \n> \n> #include \"test.h\"\n> @@ -66,6 +68,27 @@ private:\n> bool &cancelled_;\n> };\n> \n> +class CpuSetTester : public Object\n> +{\n> +public:\n> + CpuSetTester(unsigned int cpuset)\n> + : cpuset_(cpuset) {}\n> +\n> + bool testCpuSet()\n> + {\n> + int ret = sched_getcpu();\n> + if (static_cast(cpuset_) != ret) {\n> + cout << \"Invalid cpuset: \" << ret << \", expecting: \" << cpuset_ << endl;\n> + return false;\n> + }\n> +\n> + return true;\n> + }\n> +\n> +private:\n> + const unsigned int cpuset_;\n> +};\n> +\n> class ThreadTest : public Test\n> {\n> protected:\n> @@ -165,6 +188,23 @@ protected:\n> return TestFail;\n> }\n> \n> + const unsigned int numCpus = std::thread::hardware_concurrency();\n> + for (unsigned int i = 0; i < numCpus; ++i) {\n> + thread = std::make_unique();\n> + const std::array cpus{ i };\n> + thread->setThreadAffinity(cpus);\n> + thread->start();\n> +\n> + CpuSetTester tester(i);\n> + tester.moveToThread(thread.get());\n> +\n> + if (!tester.invokeMethod(&CpuSetTester::testCpuSet, ConnectionTypeBlocking))\n> + return TestFail;\n> +\n> + thread->exit(0);\n> + thread->wait();\n> + }\n> +\n> return TestPass;\n> }\n> \n> -- \n> 2.47.0.163.g1226f6d8fa-goog\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 02CD9C0F2A\n\tfor ;\n\tWed, 20 Nov 2024 11:22:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C16865904;\n\tWed, 20 Nov 2024 12:22:39 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B261265898\n\tfor ;\n\tWed, 20 Nov 2024 12:22:37 +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 7BC05675;\n\tWed, 20 Nov 2024 12:22:19 +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=\"he1i0Ta7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732101739;\n\tbh=8cUCmXvRbisg61gUIhhAhSoBopXnCc4eKEySZxF56A4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=he1i0Ta76SHfZM6/Ff6hs3G5PO/FhkUp+ZHiWsxfkXf8pGLHvdEnFTv1HWzx5ysac\n\tm5i6NPzWWNX42xrck6o+FpmZ9qQmNyr/FFSCIrASWOarMND9Mw9Q5FXlzr52iqCseM\n\tNNHl0b01XqbYgCW/i2gIiaxSIdwsEnR3VcP4dZgE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241029085837.3615699-2-chenghaoyang@chromium.org>","References":"<20241029085837.3615699-1-chenghaoyang@chromium.org>\n\t<20241029085837.3615699-2-chenghaoyang@chromium.org>","Subject":"Re: [PATCH v7 1/1] libcamera: add method to set thread affinity","From":"Kieran Bingham ","Cc":"Han-Lin Chen ,\n\tHarvey Yang ","To":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 20 Nov 2024 11:22:34 +0000","Message-ID":"<173210175436.1605529.6067472970814854934@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":32463,"web_url":"https://patchwork.libcamera.org/comment/32463/","msgid":"<173290117155.3135963.16725390839095338213@ping.linuxembedded.co.uk>","date":"2024-11-29T17:26:11","subject":"Re: [PATCH v7 1/1] libcamera: add method to set thread affinity","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2024-11-20 11:22:34)\n> Quoting Harvey Yang (2024-10-29 08:57:55)\n> > From: Han-Lin Chen \n> > \n> > Add method to set thread affinity to Thread class.\n> > \n> > Signed-off-by: Han-Lin Chen \n> > Co-developed-by: Harvey Yang \n> > Signed-off-by: Harvey Yang \n> \n> Looks good to me.\n> \n> \n> Reviewed-by: Kieran Bingham \n\nI have merged this patch based on the tests succeeding at :\n\n - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319468\n\nSince then the following pipelines have successfully also passed on top\nof this patch:\n\n- https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319906\n- https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319907\n\nHowever, I am now seeing failures at \n- https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1320174\n\nwith the following jobs:\n- https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67448427\n- https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67445198\n\n63/78 libcamera / threads FAIL 1.19s exit status 1\n>>> MALLOC_PERTURB_=148 LD_LIBRARY_PATH=/builds/camera/libcamera/build/src/libcamera/base:/builds/camera/libcamera/build/src/libcamera /builds/camera/libcamera/build/test/threads\n――――――――――――――――――――――――――――――――――――― ✀ ―――――――――――――――――――――――――――――――――――――\nstderr:\nAddressSanitizer:DEADLYSIGNAL\n=================================================================\n==909==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000002d0 (pc 0x7f5f6eab1670 bp 0x7f5f6cbfecf0 sp 0x7f5f6cbfecc8 T25)\n==909==The signal is caused by a READ memory access.\n==909==Hint: address points to the zero page.\n #0 0x7f5f6eab1670 in pthread_setaffinity_np (/lib/x86_64-linux-gnu/libc.so.6+0x8f670)\n #1 0x7f5f6eed73df in libcamera::Thread::setThreadAffinityInternal() ../src/libcamera/base/thread.cpp:457\n #2 0x7f5f6eed62be in libcamera::Thread::startThread() ../src/libcamera/base/thread.cpp:287\n #3 0x7f5f6eee0e96 in void std::__invoke_impl(std::__invoke_memfun_deref, void (libcamera::Thread::*&&)(), libcamera::Thread*&&) /usr/include/c++/12/bits/invoke.h:74\n #4 0x7f5f6eee0d02 in std::__invoke_result::type std::__invoke(void (libcamera::Thread::*&&)(), libcamera::Thread*&&) /usr/include/c++/12/bits/invoke.h:96\n #5 0x7f5f6eee0c72 in void std::thread::_Invoker >::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>) /usr/include/c++/12/bits/std_thread.h:252\n #6 0x7f5f6eee0c2b in std::thread::_Invoker >::operator()() /usr/include/c++/12/bits/std_thread.h:259\n #7 0x7f5f6eee0c0f in std::thread::_State_impl > >::_M_run() /usr/include/c++/12/bits/std_thread.h:210\n #8 0x7f5f6ecf74a2 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xd44a2)\n #9 0x7f5f6eaab1c3 (/lib/x86_64-linux-gnu/libc.so.6+0x891c3)\n #10 0x7f5f6eb2b85b (/lib/x86_64-linux-gnu/libc.so.6+0x10985b)\nAddressSanitizer can not provide additional info.\nSUMMARY: AddressSanitizer: SEGV (/lib/x86_64-linux-gnu/libc.so.6+0x8f670) in pthread_setaffinity_np\nThread T25 created by T0 here:\n #0 0x7f5f6ef5c726 in __interceptor_pthread_create ../../../../src/libsanitizer/asan/asan_interceptors.cpp:207\n #1 0x7f5f6ecf7578 in std::thread::_M_start_thread(std::unique_ptr >, void (*)()) (/lib/x86_64-linux-gnu/libstdc++.so.6+0xd4578)\n #2 0x7f5f6eed5ddb in libcamera::Thread::start() ../src/libcamera/base/thread.cpp:259\n #3 0x55dea5dbf519 in ThreadTest::run() ../test/threads.cpp:196\n #4 0x55dea5dc5bf4 in Test::execute() ../test/libtest/test.cpp:33\n #5 0x55dea5dbd54f in main ../test/threads.cpp:216\n #6 0x7f5f6ea49249 (/lib/x86_64-linux-gnu/libc.so.6+0x27249)\n==909==ABORTING\n\nI'm not yet sure what to do here, it might be that we need to revert the\nThreadAffinity patch.\n\n\nHarvey, could you see if you can replicate this issue - or spot any\npotential issues here please?\n\n--\nKieran\n\n\n> \n> > ---\n> > include/libcamera/base/thread.h | 5 ++++\n> > src/libcamera/base/thread.cpp | 47 +++++++++++++++++++++++++++++++++\n> > test/threads.cpp | 40 ++++++++++++++++++++++++++++\n> > 3 files changed, 92 insertions(+)\n> > \n> > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h\n> > index 4f33de63d..3209d4f7c 100644\n> > --- a/include/libcamera/base/thread.h\n> > +++ b/include/libcamera/base/thread.h\n> > @@ -15,6 +15,7 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > \n> > namespace libcamera {\n> > @@ -35,6 +36,8 @@ public:\n> > void exit(int code = 0);\n> > bool wait(utils::duration duration = utils::duration::max());\n> > \n> > + int setThreadAffinity(const Span &cpus);\n> > +\n> > bool isRunning();\n> > \n> > Signal<> finished;\n> > @@ -54,6 +57,8 @@ private:\n> > void startThread();\n> > void finishThread();\n> > \n> > + void setThreadAffinityInternal();\n> > +\n> > void postMessage(std::unique_ptr msg, Object *receiver);\n> > void removeMessages(Object *receiver);\n> > \n> > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> > index 8735670b8..f6322fe31 100644\n> > --- a/src/libcamera/base/thread.cpp\n> > +++ b/src/libcamera/base/thread.cpp\n> > @@ -9,6 +9,7 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -128,6 +129,8 @@ private:\n> > int exitCode_;\n> > \n> > MessageQueue messages_;\n> > +\n> > + std::optional cpuset_;\n> > };\n> > \n> > /**\n> > @@ -281,6 +284,8 @@ void Thread::startThread()\n> > data_->tid_ = syscall(SYS_gettid);\n> > currentThreadData = data_;\n> > \n> > + setThreadAffinityInternal();\n> > +\n> > run();\n> > }\n> > \n> > @@ -410,6 +415,48 @@ bool Thread::wait(utils::duration duration)\n> > return hasFinished;\n> > }\n> > \n> > +/**\n> > + * \\brief Set the CPU affinity mask of the thread\n> > + * \\param[in] cpus The list of CPU indices that the thread is set affinity to\n> > + *\n> > + * The CPU indices should be within [0, std::thread::hardware_concurrency()).\n> > + * If any index is invalid, this function won't modify the thread affinity and\n> > + * will return an error.\n> > + *\n> > + * \\return 0 if all indices are valid, -EINVAL otherwise\n> > + */\n> > +int Thread::setThreadAffinity(const Span &cpus)\n> > +{\n> > + const unsigned int numCpus = std::thread::hardware_concurrency();\n> > +\n> > + MutexLocker locker(data_->mutex_);\n> > + data_->cpuset_ = cpu_set_t();\n> > + CPU_ZERO(&data_->cpuset_.value());\n> > +\n> > + for (const unsigned int &cpu : cpus) {\n> > + if (cpu >= numCpus) {\n> > + LOG(Thread, Error) << \"Invalid CPU \" << cpu << \"for thread affinity\";\n> > + return -EINVAL;\n> > + }\n> > +\n> > + CPU_SET(cpu, &data_->cpuset_.value());\n> > + }\n> > +\n> > + if (data_->running_)\n> > + setThreadAffinityInternal();\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +void Thread::setThreadAffinityInternal()\n> > +{\n> > + if (!data_->cpuset_)\n> > + return;\n> > +\n> > + const cpu_set_t &cpuset = data_->cpuset_.value();\n> > + pthread_setaffinity_np(thread_.native_handle(), sizeof(cpuset), &cpuset);\n> > +}\n> > +\n> > /**\n> > * \\brief Check if the thread is running\n> > *\n> > diff --git a/test/threads.cpp b/test/threads.cpp\n> > index ceb4fa0f2..8d6ee1510 100644\n> > --- a/test/threads.cpp\n> > +++ b/test/threads.cpp\n> > @@ -9,9 +9,11 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > \n> > +#include \n> > #include \n> > \n> > #include \"test.h\"\n> > @@ -66,6 +68,27 @@ private:\n> > bool &cancelled_;\n> > };\n> > \n> > +class CpuSetTester : public Object\n> > +{\n> > +public:\n> > + CpuSetTester(unsigned int cpuset)\n> > + : cpuset_(cpuset) {}\n> > +\n> > + bool testCpuSet()\n> > + {\n> > + int ret = sched_getcpu();\n> > + if (static_cast(cpuset_) != ret) {\n> > + cout << \"Invalid cpuset: \" << ret << \", expecting: \" << cpuset_ << endl;\n> > + return false;\n> > + }\n> > +\n> > + return true;\n> > + }\n> > +\n> > +private:\n> > + const unsigned int cpuset_;\n> > +};\n> > +\n> > class ThreadTest : public Test\n> > {\n> > protected:\n> > @@ -165,6 +188,23 @@ protected:\n> > return TestFail;\n> > }\n> > \n> > + const unsigned int numCpus = std::thread::hardware_concurrency();\n> > + for (unsigned int i = 0; i < numCpus; ++i) {\n> > + thread = std::make_unique();\n> > + const std::array cpus{ i };\n> > + thread->setThreadAffinity(cpus);\n> > + thread->start();\n> > +\n> > + CpuSetTester tester(i);\n> > + tester.moveToThread(thread.get());\n> > +\n> > + if (!tester.invokeMethod(&CpuSetTester::testCpuSet, ConnectionTypeBlocking))\n> > + return TestFail;\n> > +\n> > + thread->exit(0);\n> > + thread->wait();\n> > + }\n> > +\n> > return TestPass;\n> > }\n> > \n> > -- \n> > 2.47.0.163.g1226f6d8fa-goog\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 5E84BBE175\n\tfor ;\n\tFri, 29 Nov 2024 17:26:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 79E5B6602B;\n\tFri, 29 Nov 2024 18:26:16 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A29C66601A\n\tfor ;\n\tFri, 29 Nov 2024 18:26:14 +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 DBA68A57;\n\tFri, 29 Nov 2024 18:25:49 +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=\"XJWdHeFJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732901150;\n\tbh=bx7jXet9RjZwRQEASrQmBwWMyGQZbKEQAxVbb4+RzGc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=XJWdHeFJkPfRviG8Nvwkt3VHKWhyp5+JxyQSQvJbdrWQQsBGQOyatHPsF2GVy57Vv\n\tORWqilj+OG1UNL4fjQf1bUb7x8470xVi9g98c5Sj/cw+4DArg5QBQgdtzqD5hAwdGN\n\tQmGQ/zqJm5oEjoP5cJYHB8cpr7HmhJvTQii1keQQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<173210175436.1605529.6067472970814854934@ping.linuxembedded.co.uk>","References":"<20241029085837.3615699-1-chenghaoyang@chromium.org>\n\t<20241029085837.3615699-2-chenghaoyang@chromium.org>\n\t<173210175436.1605529.6067472970814854934@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v7 1/1] libcamera: add method to set thread affinity","From":"Kieran Bingham ","Cc":"Han-Lin Chen ,\n\tHarvey Yang ","To":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 29 Nov 2024 17:26:11 +0000","Message-ID":"<173290117155.3135963.16725390839095338213@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":32465,"web_url":"https://patchwork.libcamera.org/comment/32465/","msgid":"","date":"2024-11-29T17:59:54","subject":"Re: [PATCH v7 1/1] libcamera: add method to set thread affinity","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Kieran,\n\nOn Sat, Nov 30, 2024 at 1:26 AM Kieran Bingham\n wrote:\n>\n> Quoting Kieran Bingham (2024-11-20 11:22:34)\n> > Quoting Harvey Yang (2024-10-29 08:57:55)\n> > > From: Han-Lin Chen \n> > >\n> > > Add method to set thread affinity to Thread class.\n> > >\n> > > Signed-off-by: Han-Lin Chen \n> > > Co-developed-by: Harvey Yang \n> > > Signed-off-by: Harvey Yang \n> >\n> > Looks good to me.\n> >\n> >\n> > Reviewed-by: Kieran Bingham \n>\n> I have merged this patch based on the tests succeeding at :\n>\n> - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319468\n>\n> Since then the following pipelines have successfully also passed on top\n> of this patch:\n>\n> - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319906\n> - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319907\n>\n> However, I am now seeing failures at\n> - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1320174\n\nInteresting. My gitlab pipeline on the same code base passed:\nhttps://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1319966\n\n>\n> with the following jobs:\n> - https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67448427\n> - https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67445198\n>\n> 63/78 libcamera / threads FAIL 1.19s exit status 1\n> >>> MALLOC_PERTURB_=148 LD_LIBRARY_PATH=/builds/camera/libcamera/build/src/libcamera/base:/builds/camera/libcamera/build/src/libcamera /builds/camera/libcamera/build/test/threads\n> ――――――――――――――――――――――――――――――――――――― ✀ ―――――――――――――――――――――――――――――――――――――\n> stderr:\n> AddressSanitizer:DEADLYSIGNAL\n> =================================================================\n> ==909==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000002d0 (pc 0x7f5f6eab1670 bp 0x7f5f6cbfecf0 sp 0x7f5f6cbfecc8 T25)\n> ==909==The signal is caused by a READ memory access.\n> ==909==Hint: address points to the zero page.\n> #0 0x7f5f6eab1670 in pthread_setaffinity_np (/lib/x86_64-linux-gnu/libc.so.6+0x8f670)\n> #1 0x7f5f6eed73df in libcamera::Thread::setThreadAffinityInternal() ../src/libcamera/base/thread.cpp:457\n> #2 0x7f5f6eed62be in libcamera::Thread::startThread() ../src/libcamera/base/thread.cpp:287\n> #3 0x7f5f6eee0e96 in void std::__invoke_impl(std::__invoke_memfun_deref, void (libcamera::Thread::*&&)(), libcamera::Thread*&&) /usr/include/c++/12/bits/invoke.h:74\n> #4 0x7f5f6eee0d02 in std::__invoke_result::type std::__invoke(void (libcamera::Thread::*&&)(), libcamera::Thread*&&) /usr/include/c++/12/bits/invoke.h:96\n> #5 0x7f5f6eee0c72 in void std::thread::_Invoker >::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>) /usr/include/c++/12/bits/std_thread.h:252\n> #6 0x7f5f6eee0c2b in std::thread::_Invoker >::operator()() /usr/include/c++/12/bits/std_thread.h:259\n> #7 0x7f5f6eee0c0f in std::thread::_State_impl > >::_M_run() /usr/include/c++/12/bits/std_thread.h:210\n> #8 0x7f5f6ecf74a2 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xd44a2)\n> #9 0x7f5f6eaab1c3 (/lib/x86_64-linux-gnu/libc.so.6+0x891c3)\n> #10 0x7f5f6eb2b85b (/lib/x86_64-linux-gnu/libc.so.6+0x10985b)\n> AddressSanitizer can not provide additional info.\n> SUMMARY: AddressSanitizer: SEGV (/lib/x86_64-linux-gnu/libc.so.6+0x8f670) in pthread_setaffinity_np\n> Thread T25 created by T0 here:\n> #0 0x7f5f6ef5c726 in __interceptor_pthread_create ../../../../src/libsanitizer/asan/asan_interceptors.cpp:207\n> #1 0x7f5f6ecf7578 in std::thread::_M_start_thread(std::unique_ptr >, void (*)()) (/lib/x86_64-linux-gnu/libstdc++.so.6+0xd4578)\n> #2 0x7f5f6eed5ddb in libcamera::Thread::start() ../src/libcamera/base/thread.cpp:259\n> #3 0x55dea5dbf519 in ThreadTest::run() ../test/threads.cpp:196\n> #4 0x55dea5dc5bf4 in Test::execute() ../test/libtest/test.cpp:33\n> #5 0x55dea5dbd54f in main ../test/threads.cpp:216\n> #6 0x7f5f6ea49249 (/lib/x86_64-linux-gnu/libc.so.6+0x27249)\n> ==909==ABORTING\n>\n> I'm not yet sure what to do here, it might be that we need to revert the\n> ThreadAffinity patch.\n>\n>\n> Harvey, could you see if you can replicate this issue - or spot any\n> potential issues here please?\n\nThe crash happens on the use case that it sets thread affinity before\nstarting the thread. mtkisp7 always starts the thread first though.\nLet me do some hacks and run CTS...\n(Basic usages seem fine to me currently.\n\nIn the meantime, I don't mind reverting the ThreadAffinity patch for\nnow. Sorry for the flaky issue.\n\nBR,\nHarvey\n\n>\n> --\n> Kieran\n>\n>\n> >\n> > > ---\n> > > include/libcamera/base/thread.h | 5 ++++\n> > > src/libcamera/base/thread.cpp | 47 +++++++++++++++++++++++++++++++++\n> > > test/threads.cpp | 40 ++++++++++++++++++++++++++++\n> > > 3 files changed, 92 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h\n> > > index 4f33de63d..3209d4f7c 100644\n> > > --- a/include/libcamera/base/thread.h\n> > > +++ b/include/libcamera/base/thread.h\n> > > @@ -15,6 +15,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > >\n> > > namespace libcamera {\n> > > @@ -35,6 +36,8 @@ public:\n> > > void exit(int code = 0);\n> > > bool wait(utils::duration duration = utils::duration::max());\n> > >\n> > > + int setThreadAffinity(const Span &cpus);\n> > > +\n> > > bool isRunning();\n> > >\n> > > Signal<> finished;\n> > > @@ -54,6 +57,8 @@ private:\n> > > void startThread();\n> > > void finishThread();\n> > >\n> > > + void setThreadAffinityInternal();\n> > > +\n> > > void postMessage(std::unique_ptr msg, Object *receiver);\n> > > void removeMessages(Object *receiver);\n> > >\n> > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> > > index 8735670b8..f6322fe31 100644\n> > > --- a/src/libcamera/base/thread.cpp\n> > > +++ b/src/libcamera/base/thread.cpp\n> > > @@ -9,6 +9,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -128,6 +129,8 @@ private:\n> > > int exitCode_;\n> > >\n> > > MessageQueue messages_;\n> > > +\n> > > + std::optional cpuset_;\n> > > };\n> > >\n> > > /**\n> > > @@ -281,6 +284,8 @@ void Thread::startThread()\n> > > data_->tid_ = syscall(SYS_gettid);\n> > > currentThreadData = data_;\n> > >\n> > > + setThreadAffinityInternal();\n> > > +\n> > > run();\n> > > }\n> > >\n> > > @@ -410,6 +415,48 @@ bool Thread::wait(utils::duration duration)\n> > > return hasFinished;\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Set the CPU affinity mask of the thread\n> > > + * \\param[in] cpus The list of CPU indices that the thread is set affinity to\n> > > + *\n> > > + * The CPU indices should be within [0, std::thread::hardware_concurrency()).\n> > > + * If any index is invalid, this function won't modify the thread affinity and\n> > > + * will return an error.\n> > > + *\n> > > + * \\return 0 if all indices are valid, -EINVAL otherwise\n> > > + */\n> > > +int Thread::setThreadAffinity(const Span &cpus)\n> > > +{\n> > > + const unsigned int numCpus = std::thread::hardware_concurrency();\n> > > +\n> > > + MutexLocker locker(data_->mutex_);\n> > > + data_->cpuset_ = cpu_set_t();\n> > > + CPU_ZERO(&data_->cpuset_.value());\n> > > +\n> > > + for (const unsigned int &cpu : cpus) {\n> > > + if (cpu >= numCpus) {\n> > > + LOG(Thread, Error) << \"Invalid CPU \" << cpu << \"for thread affinity\";\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + CPU_SET(cpu, &data_->cpuset_.value());\n> > > + }\n> > > +\n> > > + if (data_->running_)\n> > > + setThreadAffinityInternal();\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +void Thread::setThreadAffinityInternal()\n> > > +{\n> > > + if (!data_->cpuset_)\n> > > + return;\n> > > +\n> > > + const cpu_set_t &cpuset = data_->cpuset_.value();\n> > > + pthread_setaffinity_np(thread_.native_handle(), sizeof(cpuset), &cpuset);\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Check if the thread is running\n> > > *\n> > > diff --git a/test/threads.cpp b/test/threads.cpp\n> > > index ceb4fa0f2..8d6ee1510 100644\n> > > --- a/test/threads.cpp\n> > > +++ b/test/threads.cpp\n> > > @@ -9,9 +9,11 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > +#include \n> > > #include \n> > >\n> > > #include \"test.h\"\n> > > @@ -66,6 +68,27 @@ private:\n> > > bool &cancelled_;\n> > > };\n> > >\n> > > +class CpuSetTester : public Object\n> > > +{\n> > > +public:\n> > > + CpuSetTester(unsigned int cpuset)\n> > > + : cpuset_(cpuset) {}\n> > > +\n> > > + bool testCpuSet()\n> > > + {\n> > > + int ret = sched_getcpu();\n> > > + if (static_cast(cpuset_) != ret) {\n> > > + cout << \"Invalid cpuset: \" << ret << \", expecting: \" << cpuset_ << endl;\n> > > + return false;\n> > > + }\n> > > +\n> > > + return true;\n> > > + }\n> > > +\n> > > +private:\n> > > + const unsigned int cpuset_;\n> > > +};\n> > > +\n> > > class ThreadTest : public Test\n> > > {\n> > > protected:\n> > > @@ -165,6 +188,23 @@ protected:\n> > > return TestFail;\n> > > }\n> > >\n> > > + const unsigned int numCpus = std::thread::hardware_concurrency();\n> > > + for (unsigned int i = 0; i < numCpus; ++i) {\n> > > + thread = std::make_unique();\n> > > + const std::array cpus{ i };\n> > > + thread->setThreadAffinity(cpus);\n> > > + thread->start();\n> > > +\n> > > + CpuSetTester tester(i);\n> > > + tester.moveToThread(thread.get());\n> > > +\n> > > + if (!tester.invokeMethod(&CpuSetTester::testCpuSet, ConnectionTypeBlocking))\n> > > + return TestFail;\n> > > +\n> > > + thread->exit(0);\n> > > + thread->wait();\n> > > + }\n> > > +\n> > > return TestPass;\n> > > }\n> > >\n> > > --\n> > > 2.47.0.163.g1226f6d8fa-goog\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 312C1C31E9\n\tfor ;\n\tFri, 29 Nov 2024 18:00:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2433566031;\n\tFri, 29 Nov 2024 19:00:07 +0100 (CET)","from mail-lj1-x22b.google.com (mail-lj1-x22b.google.com\n\t[IPv6:2a00:1450:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2E8076601A\n\tfor ;\n\tFri, 29 Nov 2024 19:00:06 +0100 (CET)","by mail-lj1-x22b.google.com with SMTP id\n\t38308e7fff4ca-2ffa12ad18cso24746701fa.2\n\tfor ;\n\tFri, 29 Nov 2024 10:00:06 -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=\"Q1BYdpDk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1732903205; x=1733508005;\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=yNfB/y+mqTeP/qP15lQQ6XUaHJ5K2c2SbKfW49Spngo=;\n\tb=Q1BYdpDkJzW7ryv2NybtTK21331feWLbkB8dVBTuNFPjZbsx5GjH/UxZhhFGNQkPJT\n\tFBDd05a/eG6Oa2N1Z/uwNrsnSC0Y4ENwnTv/8nZ4vBkkJJVW3boxFDB2jFvGCjjYWfhX\n\tjvsEpeFPvMQSTf+YIAbWC4WZFEW7WG0aCNSu4=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1732903205; x=1733508005;\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=yNfB/y+mqTeP/qP15lQQ6XUaHJ5K2c2SbKfW49Spngo=;\n\tb=Cr4DsUKWyuklaRPZp2qfN3Dl+6bFoMENJYDEMr5rRAexRXLw8lvS1dF2himovN5+41\n\tjA7oQ1eCeEuHdtEbE1zXiw+/OC166t4TRyAN1SUvjkb/2CfGq/7MdQDgCkqBQKzeIKAt\n\tR9EpNU/W01uArwTQzFKY1VitzJ6RaNUbHGpPfKbAoZYkiZmOSlvz3qqmNPVL1vfcRwKd\n\tD/2g93Oapu9jK3qMDHJHthZG4DoxgV40noyAFSK9//UKqJVMAV8ni/FYUNCfNYEAzq90\n\t/G9rVuzl2Xcr0jJdjMtr98/PGARaOJ9IJDPzC1lK21QaaUiQgkmwZ+Cf7lOOiIA+qbKr\n\tCBUw==","X-Gm-Message-State":"AOJu0YwFN4PM74e+lJBG0KjqXrzWdGH31VbNNDMmRFCp5APH7WR442QR\n\te31EaZ/whTFShxt+5X92xD5xFMiUGgGOj0lHC4mUDipeJDZ4sZTq2i4e0V2LIftnZtawcJOcW2V\n\ttY2s5qsNr+3Ecge5zGk0RD5kr+y8RAaq96Tqf","X-Gm-Gg":"ASbGncsNgIgX74Ra6UpssjwrdfAKP9dRtQnG6ddSEObCbDLn0NfgBd5tnlZe+/fyqWd\n\tpPQdSHBJ91ahop2AqBt5p/crMAOFsskjcdBnRTwFyM+vMDZ8ewsy39UhXDnthC9Ir","X-Google-Smtp-Source":"AGHT+IHep6Y5txkvoSX2FVtufwZ6qqoQncDovPh8BpnyvI2/LDgybHL12ejXt8qGhPIEj0Ukjr5vraaN7GpVsTpOVfU=","X-Received":"by 2002:a2e:bc22:0:b0:2fa:d317:b777 with SMTP id\n\t38308e7fff4ca-2ffd5feb882mr83495991fa.2.1732903205017;\n\tFri, 29 Nov 2024 10:00:05 -0800 (PST)","MIME-Version":"1.0","References":"<20241029085837.3615699-1-chenghaoyang@chromium.org>\n\t<20241029085837.3615699-2-chenghaoyang@chromium.org>\n\t<173210175436.1605529.6067472970814854934@ping.linuxembedded.co.uk>\n\t<173290117155.3135963.16725390839095338213@ping.linuxembedded.co.uk>","In-Reply-To":"<173290117155.3135963.16725390839095338213@ping.linuxembedded.co.uk>","From":"Cheng-Hao Yang ","Date":"Sat, 30 Nov 2024 01:59:54 +0800","Message-ID":"","Subject":"Re: [PATCH v7 1/1] libcamera: add method to set thread affinity","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","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":32482,"web_url":"https://patchwork.libcamera.org/comment/32482/","msgid":"","date":"2024-12-02T09:07:07","subject":"Re: [PATCH v7 1/1] libcamera: add method to set thread affinity","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Kieran,\n\nOn Sat, Nov 30, 2024 at 1:59 AM Cheng-Hao Yang\n wrote:\n>\n> Hi Kieran,\n>\n> On Sat, Nov 30, 2024 at 1:26 AM Kieran Bingham\n> wrote:\n> >\n> > Quoting Kieran Bingham (2024-11-20 11:22:34)\n> > > Quoting Harvey Yang (2024-10-29 08:57:55)\n> > > > From: Han-Lin Chen \n> > > >\n> > > > Add method to set thread affinity to Thread class.\n> > > >\n> > > > Signed-off-by: Han-Lin Chen \n> > > > Co-developed-by: Harvey Yang \n> > > > Signed-off-by: Harvey Yang \n> > >\n> > > Looks good to me.\n> > >\n> > >\n> > > Reviewed-by: Kieran Bingham \n> >\n> > I have merged this patch based on the tests succeeding at :\n> >\n> > - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319468\n> >\n> > Since then the following pipelines have successfully also passed on top\n> > of this patch:\n> >\n> > - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319906\n> > - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319907\n> >\n> > However, I am now seeing failures at\n> > - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1320174\n>\n> Interesting. My gitlab pipeline on the same code base passed:\n> https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1319966\n>\n> >\n> > with the following jobs:\n> > - https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67448427\n> > - https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67445198\n> >\n> > 63/78 libcamera / threads FAIL 1.19s exit status 1\n> > >>> MALLOC_PERTURB_=148 LD_LIBRARY_PATH=/builds/camera/libcamera/build/src/libcamera/base:/builds/camera/libcamera/build/src/libcamera /builds/camera/libcamera/build/test/threads\n> > ――――――――――――――――――――――――――――――――――――― ✀ ―――――――――――――――――――――――――――――――――――――\n> > stderr:\n> > AddressSanitizer:DEADLYSIGNAL\n> > =================================================================\n> > ==909==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000002d0 (pc 0x7f5f6eab1670 bp 0x7f5f6cbfecf0 sp 0x7f5f6cbfecc8 T25)\n> > ==909==The signal is caused by a READ memory access.\n> > ==909==Hint: address points to the zero page.\n> > #0 0x7f5f6eab1670 in pthread_setaffinity_np (/lib/x86_64-linux-gnu/libc.so.6+0x8f670)\n> > #1 0x7f5f6eed73df in libcamera::Thread::setThreadAffinityInternal() ../src/libcamera/base/thread.cpp:457\n> > #2 0x7f5f6eed62be in libcamera::Thread::startThread() ../src/libcamera/base/thread.cpp:287\n> > #3 0x7f5f6eee0e96 in void std::__invoke_impl(std::__invoke_memfun_deref, void (libcamera::Thread::*&&)(), libcamera::Thread*&&) /usr/include/c++/12/bits/invoke.h:74\n> > #4 0x7f5f6eee0d02 in std::__invoke_result::type std::__invoke(void (libcamera::Thread::*&&)(), libcamera::Thread*&&) /usr/include/c++/12/bits/invoke.h:96\n> > #5 0x7f5f6eee0c72 in void std::thread::_Invoker >::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>) /usr/include/c++/12/bits/std_thread.h:252\n> > #6 0x7f5f6eee0c2b in std::thread::_Invoker >::operator()() /usr/include/c++/12/bits/std_thread.h:259\n> > #7 0x7f5f6eee0c0f in std::thread::_State_impl > >::_M_run() /usr/include/c++/12/bits/std_thread.h:210\n> > #8 0x7f5f6ecf74a2 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xd44a2)\n> > #9 0x7f5f6eaab1c3 (/lib/x86_64-linux-gnu/libc.so.6+0x891c3)\n> > #10 0x7f5f6eb2b85b (/lib/x86_64-linux-gnu/libc.so.6+0x10985b)\n> > AddressSanitizer can not provide additional info.\n> > SUMMARY: AddressSanitizer: SEGV (/lib/x86_64-linux-gnu/libc.so.6+0x8f670) in pthread_setaffinity_np\n> > Thread T25 created by T0 here:\n> > #0 0x7f5f6ef5c726 in __interceptor_pthread_create ../../../../src/libsanitizer/asan/asan_interceptors.cpp:207\n> > #1 0x7f5f6ecf7578 in std::thread::_M_start_thread(std::unique_ptr >, void (*)()) (/lib/x86_64-linux-gnu/libstdc++.so.6+0xd4578)\n> > #2 0x7f5f6eed5ddb in libcamera::Thread::start() ../src/libcamera/base/thread.cpp:259\n> > #3 0x55dea5dbf519 in ThreadTest::run() ../test/threads.cpp:196\n> > #4 0x55dea5dc5bf4 in Test::execute() ../test/libtest/test.cpp:33\n> > #5 0x55dea5dbd54f in main ../test/threads.cpp:216\n> > #6 0x7f5f6ea49249 (/lib/x86_64-linux-gnu/libc.so.6+0x27249)\n> > ==909==ABORTING\n> >\n> > I'm not yet sure what to do here, it might be that we need to revert the\n> > ThreadAffinity patch.\n> >\n> >\n> > Harvey, could you see if you can replicate this issue - or spot any\n> > potential issues here please?\n>\n> The crash happens on the use case that it sets thread affinity before\n> starting the thread. mtkisp7 always starts the thread first though.\n> Let me do some hacks and run CTS...\n> (Basic usages seem fine to me currently.\n>\n> In the meantime, I don't mind reverting the ThreadAffinity patch for\n> now. Sorry for the flaky issue.\n\nI reproduced it on mtkisp7, and I think the main difference of the\nupstream patch and the one adopted in CrOS mtkisp7 branch is\nthat the former one call setThreadAffinityInternal in the starting\nfunction of std::thread.\n\nI tried the fix `[PATCH] Thread: Fix setThreadAffinity issue`, and\nI believe it works on mtkisp7. Please help check if it makes sense.\n\nBR,\nHarvey\n\n>\n> BR,\n> Harvey\n>\n> >\n> > --\n> > Kieran\n> >\n> >\n> > >\n> > > > ---\n> > > > include/libcamera/base/thread.h | 5 ++++\n> > > > src/libcamera/base/thread.cpp | 47 +++++++++++++++++++++++++++++++++\n> > > > test/threads.cpp | 40 ++++++++++++++++++++++++++++\n> > > > 3 files changed, 92 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h\n> > > > index 4f33de63d..3209d4f7c 100644\n> > > > --- a/include/libcamera/base/thread.h\n> > > > +++ b/include/libcamera/base/thread.h\n> > > > @@ -15,6 +15,7 @@\n> > > >\n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > >\n> > > > namespace libcamera {\n> > > > @@ -35,6 +36,8 @@ public:\n> > > > void exit(int code = 0);\n> > > > bool wait(utils::duration duration = utils::duration::max());\n> > > >\n> > > > + int setThreadAffinity(const Span &cpus);\n> > > > +\n> > > > bool isRunning();\n> > > >\n> > > > Signal<> finished;\n> > > > @@ -54,6 +57,8 @@ private:\n> > > > void startThread();\n> > > > void finishThread();\n> > > >\n> > > > + void setThreadAffinityInternal();\n> > > > +\n> > > > void postMessage(std::unique_ptr msg, Object *receiver);\n> > > > void removeMessages(Object *receiver);\n> > > >\n> > > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp\n> > > > index 8735670b8..f6322fe31 100644\n> > > > --- a/src/libcamera/base/thread.cpp\n> > > > +++ b/src/libcamera/base/thread.cpp\n> > > > @@ -9,6 +9,7 @@\n> > > >\n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -128,6 +129,8 @@ private:\n> > > > int exitCode_;\n> > > >\n> > > > MessageQueue messages_;\n> > > > +\n> > > > + std::optional cpuset_;\n> > > > };\n> > > >\n> > > > /**\n> > > > @@ -281,6 +284,8 @@ void Thread::startThread()\n> > > > data_->tid_ = syscall(SYS_gettid);\n> > > > currentThreadData = data_;\n> > > >\n> > > > + setThreadAffinityInternal();\n> > > > +\n> > > > run();\n> > > > }\n> > > >\n> > > > @@ -410,6 +415,48 @@ bool Thread::wait(utils::duration duration)\n> > > > return hasFinished;\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\brief Set the CPU affinity mask of the thread\n> > > > + * \\param[in] cpus The list of CPU indices that the thread is set affinity to\n> > > > + *\n> > > > + * The CPU indices should be within [0, std::thread::hardware_concurrency()).\n> > > > + * If any index is invalid, this function won't modify the thread affinity and\n> > > > + * will return an error.\n> > > > + *\n> > > > + * \\return 0 if all indices are valid, -EINVAL otherwise\n> > > > + */\n> > > > +int Thread::setThreadAffinity(const Span &cpus)\n> > > > +{\n> > > > + const unsigned int numCpus = std::thread::hardware_concurrency();\n> > > > +\n> > > > + MutexLocker locker(data_->mutex_);\n> > > > + data_->cpuset_ = cpu_set_t();\n> > > > + CPU_ZERO(&data_->cpuset_.value());\n> > > > +\n> > > > + for (const unsigned int &cpu : cpus) {\n> > > > + if (cpu >= numCpus) {\n> > > > + LOG(Thread, Error) << \"Invalid CPU \" << cpu << \"for thread affinity\";\n> > > > + return -EINVAL;\n> > > > + }\n> > > > +\n> > > > + CPU_SET(cpu, &data_->cpuset_.value());\n> > > > + }\n> > > > +\n> > > > + if (data_->running_)\n> > > > + setThreadAffinityInternal();\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +void Thread::setThreadAffinityInternal()\n> > > > +{\n> > > > + if (!data_->cpuset_)\n> > > > + return;\n> > > > +\n> > > > + const cpu_set_t &cpuset = data_->cpuset_.value();\n> > > > + pthread_setaffinity_np(thread_.native_handle(), sizeof(cpuset), &cpuset);\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\brief Check if the thread is running\n> > > > *\n> > > > diff --git a/test/threads.cpp b/test/threads.cpp\n> > > > index ceb4fa0f2..8d6ee1510 100644\n> > > > --- a/test/threads.cpp\n> > > > +++ b/test/threads.cpp\n> > > > @@ -9,9 +9,11 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > >\n> > > > +#include \n> > > > #include \n> > > >\n> > > > #include \"test.h\"\n> > > > @@ -66,6 +68,27 @@ private:\n> > > > bool &cancelled_;\n> > > > };\n> > > >\n> > > > +class CpuSetTester : public Object\n> > > > +{\n> > > > +public:\n> > > > + CpuSetTester(unsigned int cpuset)\n> > > > + : cpuset_(cpuset) {}\n> > > > +\n> > > > + bool testCpuSet()\n> > > > + {\n> > > > + int ret = sched_getcpu();\n> > > > + if (static_cast(cpuset_) != ret) {\n> > > > + cout << \"Invalid cpuset: \" << ret << \", expecting: \" << cpuset_ << endl;\n> > > > + return false;\n> > > > + }\n> > > > +\n> > > > + return true;\n> > > > + }\n> > > > +\n> > > > +private:\n> > > > + const unsigned int cpuset_;\n> > > > +};\n> > > > +\n> > > > class ThreadTest : public Test\n> > > > {\n> > > > protected:\n> > > > @@ -165,6 +188,23 @@ protected:\n> > > > return TestFail;\n> > > > }\n> > > >\n> > > > + const unsigned int numCpus = std::thread::hardware_concurrency();\n> > > > + for (unsigned int i = 0; i < numCpus; ++i) {\n> > > > + thread = std::make_unique();\n> > > > + const std::array cpus{ i };\n> > > > + thread->setThreadAffinity(cpus);\n> > > > + thread->start();\n> > > > +\n> > > > + CpuSetTester tester(i);\n> > > > + tester.moveToThread(thread.get());\n> > > > +\n> > > > + if (!tester.invokeMethod(&CpuSetTester::testCpuSet, ConnectionTypeBlocking))\n> > > > + return TestFail;\n> > > > +\n> > > > + thread->exit(0);\n> > > > + thread->wait();\n> > > > + }\n> > > > +\n> > > > return TestPass;\n> > > > }\n> > > >\n> > > > --\n> > > > 2.47.0.163.g1226f6d8fa-goog\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 8F350BDCBF\n\tfor ;\n\tMon, 2 Dec 2024 09:07:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 59D6D6605F;\n\tMon, 2 Dec 2024 10:07:21 +0100 (CET)","from mail-lj1-x229.google.com (mail-lj1-x229.google.com\n\t[IPv6:2a00:1450:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3472166040\n\tfor ;\n\tMon, 2 Dec 2024 10:07:19 +0100 (CET)","by mail-lj1-x229.google.com with SMTP id\n\t38308e7fff4ca-2f75c56f16aso37013151fa.0\n\tfor ;\n\tMon, 02 Dec 2024 01:07:19 -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=\"fB5KiInn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1733130438; x=1733735238;\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=PXjvuLJhPJqiYyd++O3Yd0COoLrkhk5GbmNlZhn6sos=;\n\tb=fB5KiInnEumzbpbQjHq6mdgQs9BAdxt+JviF/YVtY1EekGMlBxb6YqbW6uj0RmJybY\n\tje9l7cdCuLvX0P1OxVy7WNhI9QH5eXSlAIhgf1RVc+G0GJfYCD5HrnwbrohSlCkkd5st\n\tDMdCNlUVL2NFSqoufiX2dQMX11mwtyCGieTh4=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733130438; x=1733735238;\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=PXjvuLJhPJqiYyd++O3Yd0COoLrkhk5GbmNlZhn6sos=;\n\tb=YBOj8qt7wLylV66SXdsezgpzwxPhouUFKULb2VFMxp4fK24Wd72k2O32zZiwtC2AQF\n\tnXipQtiB0ip7k+Ty2PRagyoyI2VaaRdVhxb0quzektNI1ZjJURGMX+MbYliPfioTKrOT\n\tyt22etnea0OL/1lBEvlD9M6UHlahuSnzUNMeVsNOKqbxv3rP3LzuzWmTsO+uY4SHaqVM\n\t8mYgDTIYIyx65o9/Z0qzLHNHHbmJhUbG9Y/1jIeb5FrKoUR38RhKU9YwMMIyDOa0GT9T\n\t18IXfAGcbZkrsFzXH+mT9F6//TS3MRLAk55BaQwCOo77Y4LichUQzembovAd9wGoC4yA\n\tmDAw==","X-Gm-Message-State":"AOJu0Yz4NWnmR/xifpYYQtj5q8CsmiRmIGt1QTApXEW2i1gazU60/+Ch\n\t8jtbqFrm7Th3RvwlDSLId7eu7hGmm/AgnXljGaPONMOXzEv92PqhkIOjCEetEWP/KaxasflWpNX\n\tSvP8IwD01F5wu0TfcQz1Wsx6DjEsiptTV2F3O","X-Gm-Gg":"ASbGnctpQ8BQlWLIUVaiUsDE4P7NoGCxlJLzrDrqhn4QhQ3hepSLNparAR+yWRoJiya\n\tSy4vaRvde78Oh/exGUgaqjYgvmh3qqga/T7d9tg9lNHQPsDNh3hoVcRBx7wg=","X-Google-Smtp-Source":"AGHT+IEKZ8e6AQrWnFo2fouqZwc2bUo4vlWWn6USjiBvROd1qaRe7tVAbDNLIBZyNhMg2rvJsGPU/Ffod4rX4wCBrYs=","X-Received":"by 2002:a2e:bccd:0:b0:2ff:c62c:15d0 with SMTP id\n\t38308e7fff4ca-2ffd6142a3dmr111355541fa.35.1733130438154;\n\tMon, 02 Dec 2024 01:07:18 -0800 (PST)","MIME-Version":"1.0","References":"<20241029085837.3615699-1-chenghaoyang@chromium.org>\n\t<20241029085837.3615699-2-chenghaoyang@chromium.org>\n\t<173210175436.1605529.6067472970814854934@ping.linuxembedded.co.uk>\n\t<173290117155.3135963.16725390839095338213@ping.linuxembedded.co.uk>\n\t","In-Reply-To":"","From":"Cheng-Hao Yang ","Date":"Mon, 2 Dec 2024 17:07:07 +0800","Message-ID":"","Subject":"Re: [PATCH v7 1/1] libcamera: add method to set thread affinity","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","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\" "}}]