| Message ID | 20241029085837.3615699-2-chenghaoyang@chromium.org |
|---|---|
| State | Accepted |
| Commit | 4d9db06d669044c0c461a2aed79c85c7fe32a502 |
| Headers | show |
| Series |
|
| Related | show |
Quoting Harvey Yang (2024-10-29 08:57:55) > From: Han-Lin Chen <hanlinchen@chromium.org> > > Add method to set thread affinity to Thread class. > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> Looks good to me. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/base/thread.h | 5 ++++ > src/libcamera/base/thread.cpp | 47 +++++++++++++++++++++++++++++++++ > test/threads.cpp | 40 ++++++++++++++++++++++++++++ > 3 files changed, 92 insertions(+) > > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h > index 4f33de63d..3209d4f7c 100644 > --- a/include/libcamera/base/thread.h > +++ b/include/libcamera/base/thread.h > @@ -15,6 +15,7 @@ > > #include <libcamera/base/message.h> > #include <libcamera/base/signal.h> > +#include <libcamera/base/span.h> > #include <libcamera/base/utils.h> > > namespace libcamera { > @@ -35,6 +36,8 @@ public: > void exit(int code = 0); > bool wait(utils::duration duration = utils::duration::max()); > > + int setThreadAffinity(const Span<const unsigned int> &cpus); > + > bool isRunning(); > > Signal<> finished; > @@ -54,6 +57,8 @@ private: > void startThread(); > void finishThread(); > > + void setThreadAffinityInternal(); > + > void postMessage(std::unique_ptr<Message> msg, Object *receiver); > void removeMessages(Object *receiver); > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp > index 8735670b8..f6322fe31 100644 > --- a/src/libcamera/base/thread.cpp > +++ b/src/libcamera/base/thread.cpp > @@ -9,6 +9,7 @@ > > #include <atomic> > #include <list> > +#include <optional> > #include <sys/syscall.h> > #include <sys/types.h> > #include <unistd.h> > @@ -128,6 +129,8 @@ private: > int exitCode_; > > MessageQueue messages_; > + > + std::optional<cpu_set_t> cpuset_; > }; > > /** > @@ -281,6 +284,8 @@ void Thread::startThread() > data_->tid_ = syscall(SYS_gettid); > currentThreadData = data_; > > + setThreadAffinityInternal(); > + > run(); > } > > @@ -410,6 +415,48 @@ bool Thread::wait(utils::duration duration) > return hasFinished; > } > > +/** > + * \brief Set the CPU affinity mask of the thread > + * \param[in] cpus The list of CPU indices that the thread is set affinity to > + * > + * The CPU indices should be within [0, std::thread::hardware_concurrency()). > + * If any index is invalid, this function won't modify the thread affinity and > + * will return an error. > + * > + * \return 0 if all indices are valid, -EINVAL otherwise > + */ > +int Thread::setThreadAffinity(const Span<const unsigned int> &cpus) > +{ > + const unsigned int numCpus = std::thread::hardware_concurrency(); > + > + MutexLocker locker(data_->mutex_); > + data_->cpuset_ = cpu_set_t(); > + CPU_ZERO(&data_->cpuset_.value()); > + > + for (const unsigned int &cpu : cpus) { > + if (cpu >= numCpus) { > + LOG(Thread, Error) << "Invalid CPU " << cpu << "for thread affinity"; > + return -EINVAL; > + } > + > + CPU_SET(cpu, &data_->cpuset_.value()); > + } > + > + if (data_->running_) > + setThreadAffinityInternal(); > + > + return 0; > +} > + > +void Thread::setThreadAffinityInternal() > +{ > + if (!data_->cpuset_) > + return; > + > + const cpu_set_t &cpuset = data_->cpuset_.value(); > + pthread_setaffinity_np(thread_.native_handle(), sizeof(cpuset), &cpuset); > +} > + > /** > * \brief Check if the thread is running > * > diff --git a/test/threads.cpp b/test/threads.cpp > index ceb4fa0f2..8d6ee1510 100644 > --- a/test/threads.cpp > +++ b/test/threads.cpp > @@ -9,9 +9,11 @@ > #include <iostream> > #include <memory> > #include <pthread.h> > +#include <sched.h> > #include <thread> > #include <time.h> > > +#include <libcamera/base/object.h> > #include <libcamera/base/thread.h> > > #include "test.h" > @@ -66,6 +68,27 @@ private: > bool &cancelled_; > }; > > +class CpuSetTester : public Object > +{ > +public: > + CpuSetTester(unsigned int cpuset) > + : cpuset_(cpuset) {} > + > + bool testCpuSet() > + { > + int ret = sched_getcpu(); > + if (static_cast<int>(cpuset_) != ret) { > + cout << "Invalid cpuset: " << ret << ", expecting: " << cpuset_ << endl; > + return false; > + } > + > + return true; > + } > + > +private: > + const unsigned int cpuset_; > +}; > + > class ThreadTest : public Test > { > protected: > @@ -165,6 +188,23 @@ protected: > return TestFail; > } > > + const unsigned int numCpus = std::thread::hardware_concurrency(); > + for (unsigned int i = 0; i < numCpus; ++i) { > + thread = std::make_unique<Thread>(); > + const std::array<const unsigned int, 1> cpus{ i }; > + thread->setThreadAffinity(cpus); > + thread->start(); > + > + CpuSetTester tester(i); > + tester.moveToThread(thread.get()); > + > + if (!tester.invokeMethod(&CpuSetTester::testCpuSet, ConnectionTypeBlocking)) > + return TestFail; > + > + thread->exit(0); > + thread->wait(); > + } > + > return TestPass; > } > > -- > 2.47.0.163.g1226f6d8fa-goog >
Quoting Kieran Bingham (2024-11-20 11:22:34) > Quoting Harvey Yang (2024-10-29 08:57:55) > > From: Han-Lin Chen <hanlinchen@chromium.org> > > > > Add method to set thread affinity to Thread class. > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > Looks good to me. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> I have merged this patch based on the tests succeeding at : - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319468 Since then the following pipelines have successfully also passed on top of this patch: - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319906 - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319907 However, I am now seeing failures at - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1320174 with the following jobs: - https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67448427 - https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67445198 63/78 libcamera / threads FAIL 1.19s exit status 1 >>> 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 ――――――――――――――――――――――――――――――――――――― ✀ ――――――――――――――――――――――――――――――――――――― stderr: AddressSanitizer:DEADLYSIGNAL ================================================================= ==909==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000002d0 (pc 0x7f5f6eab1670 bp 0x7f5f6cbfecf0 sp 0x7f5f6cbfecc8 T25) ==909==The signal is caused by a READ memory access. ==909==Hint: address points to the zero page. #0 0x7f5f6eab1670 in pthread_setaffinity_np (/lib/x86_64-linux-gnu/libc.so.6+0x8f670) #1 0x7f5f6eed73df in libcamera::Thread::setThreadAffinityInternal() ../src/libcamera/base/thread.cpp:457 #2 0x7f5f6eed62be in libcamera::Thread::startThread() ../src/libcamera/base/thread.cpp:287 #3 0x7f5f6eee0e96 in void std::__invoke_impl<void, void (libcamera::Thread::*)(), libcamera::Thread*>(std::__invoke_memfun_deref, void (libcamera::Thread::*&&)(), libcamera::Thread*&&) /usr/include/c++/12/bits/invoke.h:74 #4 0x7f5f6eee0d02 in std::__invoke_result<void (libcamera::Thread::*)(), libcamera::Thread*>::type std::__invoke<void (libcamera::Thread::*)(), libcamera::Thread*>(void (libcamera::Thread::*&&)(), libcamera::Thread*&&) /usr/include/c++/12/bits/invoke.h:96 #5 0x7f5f6eee0c72 in void std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> >::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>) /usr/include/c++/12/bits/std_thread.h:252 #6 0x7f5f6eee0c2b in std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> >::operator()() /usr/include/c++/12/bits/std_thread.h:259 #7 0x7f5f6eee0c0f in std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> > >::_M_run() /usr/include/c++/12/bits/std_thread.h:210 #8 0x7f5f6ecf74a2 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xd44a2) #9 0x7f5f6eaab1c3 (/lib/x86_64-linux-gnu/libc.so.6+0x891c3) #10 0x7f5f6eb2b85b (/lib/x86_64-linux-gnu/libc.so.6+0x10985b) AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV (/lib/x86_64-linux-gnu/libc.so.6+0x8f670) in pthread_setaffinity_np Thread T25 created by T0 here: #0 0x7f5f6ef5c726 in __interceptor_pthread_create ../../../../src/libsanitizer/asan/asan_interceptors.cpp:207 #1 0x7f5f6ecf7578 in std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) (/lib/x86_64-linux-gnu/libstdc++.so.6+0xd4578) #2 0x7f5f6eed5ddb in libcamera::Thread::start() ../src/libcamera/base/thread.cpp:259 #3 0x55dea5dbf519 in ThreadTest::run() ../test/threads.cpp:196 #4 0x55dea5dc5bf4 in Test::execute() ../test/libtest/test.cpp:33 #5 0x55dea5dbd54f in main ../test/threads.cpp:216 #6 0x7f5f6ea49249 (/lib/x86_64-linux-gnu/libc.so.6+0x27249) ==909==ABORTING I'm not yet sure what to do here, it might be that we need to revert the ThreadAffinity patch. Harvey, could you see if you can replicate this issue - or spot any potential issues here please? -- Kieran > > > --- > > include/libcamera/base/thread.h | 5 ++++ > > src/libcamera/base/thread.cpp | 47 +++++++++++++++++++++++++++++++++ > > test/threads.cpp | 40 ++++++++++++++++++++++++++++ > > 3 files changed, 92 insertions(+) > > > > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h > > index 4f33de63d..3209d4f7c 100644 > > --- a/include/libcamera/base/thread.h > > +++ b/include/libcamera/base/thread.h > > @@ -15,6 +15,7 @@ > > > > #include <libcamera/base/message.h> > > #include <libcamera/base/signal.h> > > +#include <libcamera/base/span.h> > > #include <libcamera/base/utils.h> > > > > namespace libcamera { > > @@ -35,6 +36,8 @@ public: > > void exit(int code = 0); > > bool wait(utils::duration duration = utils::duration::max()); > > > > + int setThreadAffinity(const Span<const unsigned int> &cpus); > > + > > bool isRunning(); > > > > Signal<> finished; > > @@ -54,6 +57,8 @@ private: > > void startThread(); > > void finishThread(); > > > > + void setThreadAffinityInternal(); > > + > > void postMessage(std::unique_ptr<Message> msg, Object *receiver); > > void removeMessages(Object *receiver); > > > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp > > index 8735670b8..f6322fe31 100644 > > --- a/src/libcamera/base/thread.cpp > > +++ b/src/libcamera/base/thread.cpp > > @@ -9,6 +9,7 @@ > > > > #include <atomic> > > #include <list> > > +#include <optional> > > #include <sys/syscall.h> > > #include <sys/types.h> > > #include <unistd.h> > > @@ -128,6 +129,8 @@ private: > > int exitCode_; > > > > MessageQueue messages_; > > + > > + std::optional<cpu_set_t> cpuset_; > > }; > > > > /** > > @@ -281,6 +284,8 @@ void Thread::startThread() > > data_->tid_ = syscall(SYS_gettid); > > currentThreadData = data_; > > > > + setThreadAffinityInternal(); > > + > > run(); > > } > > > > @@ -410,6 +415,48 @@ bool Thread::wait(utils::duration duration) > > return hasFinished; > > } > > > > +/** > > + * \brief Set the CPU affinity mask of the thread > > + * \param[in] cpus The list of CPU indices that the thread is set affinity to > > + * > > + * The CPU indices should be within [0, std::thread::hardware_concurrency()). > > + * If any index is invalid, this function won't modify the thread affinity and > > + * will return an error. > > + * > > + * \return 0 if all indices are valid, -EINVAL otherwise > > + */ > > +int Thread::setThreadAffinity(const Span<const unsigned int> &cpus) > > +{ > > + const unsigned int numCpus = std::thread::hardware_concurrency(); > > + > > + MutexLocker locker(data_->mutex_); > > + data_->cpuset_ = cpu_set_t(); > > + CPU_ZERO(&data_->cpuset_.value()); > > + > > + for (const unsigned int &cpu : cpus) { > > + if (cpu >= numCpus) { > > + LOG(Thread, Error) << "Invalid CPU " << cpu << "for thread affinity"; > > + return -EINVAL; > > + } > > + > > + CPU_SET(cpu, &data_->cpuset_.value()); > > + } > > + > > + if (data_->running_) > > + setThreadAffinityInternal(); > > + > > + return 0; > > +} > > + > > +void Thread::setThreadAffinityInternal() > > +{ > > + if (!data_->cpuset_) > > + return; > > + > > + const cpu_set_t &cpuset = data_->cpuset_.value(); > > + pthread_setaffinity_np(thread_.native_handle(), sizeof(cpuset), &cpuset); > > +} > > + > > /** > > * \brief Check if the thread is running > > * > > diff --git a/test/threads.cpp b/test/threads.cpp > > index ceb4fa0f2..8d6ee1510 100644 > > --- a/test/threads.cpp > > +++ b/test/threads.cpp > > @@ -9,9 +9,11 @@ > > #include <iostream> > > #include <memory> > > #include <pthread.h> > > +#include <sched.h> > > #include <thread> > > #include <time.h> > > > > +#include <libcamera/base/object.h> > > #include <libcamera/base/thread.h> > > > > #include "test.h" > > @@ -66,6 +68,27 @@ private: > > bool &cancelled_; > > }; > > > > +class CpuSetTester : public Object > > +{ > > +public: > > + CpuSetTester(unsigned int cpuset) > > + : cpuset_(cpuset) {} > > + > > + bool testCpuSet() > > + { > > + int ret = sched_getcpu(); > > + if (static_cast<int>(cpuset_) != ret) { > > + cout << "Invalid cpuset: " << ret << ", expecting: " << cpuset_ << endl; > > + return false; > > + } > > + > > + return true; > > + } > > + > > +private: > > + const unsigned int cpuset_; > > +}; > > + > > class ThreadTest : public Test > > { > > protected: > > @@ -165,6 +188,23 @@ protected: > > return TestFail; > > } > > > > + const unsigned int numCpus = std::thread::hardware_concurrency(); > > + for (unsigned int i = 0; i < numCpus; ++i) { > > + thread = std::make_unique<Thread>(); > > + const std::array<const unsigned int, 1> cpus{ i }; > > + thread->setThreadAffinity(cpus); > > + thread->start(); > > + > > + CpuSetTester tester(i); > > + tester.moveToThread(thread.get()); > > + > > + if (!tester.invokeMethod(&CpuSetTester::testCpuSet, ConnectionTypeBlocking)) > > + return TestFail; > > + > > + thread->exit(0); > > + thread->wait(); > > + } > > + > > return TestPass; > > } > > > > -- > > 2.47.0.163.g1226f6d8fa-goog > >
Hi Kieran, On Sat, Nov 30, 2024 at 1:26 AM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Kieran Bingham (2024-11-20 11:22:34) > > Quoting Harvey Yang (2024-10-29 08:57:55) > > > From: Han-Lin Chen <hanlinchen@chromium.org> > > > > > > Add method to set thread affinity to Thread class. > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > > Looks good to me. > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > I have merged this patch based on the tests succeeding at : > > - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319468 > > Since then the following pipelines have successfully also passed on top > of this patch: > > - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319906 > - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319907 > > However, I am now seeing failures at > - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1320174 Interesting. My gitlab pipeline on the same code base passed: https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1319966 > > with the following jobs: > - https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67448427 > - https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67445198 > > 63/78 libcamera / threads FAIL 1.19s exit status 1 > >>> 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 > ――――――――――――――――――――――――――――――――――――― ✀ ――――――――――――――――――――――――――――――――――――― > stderr: > AddressSanitizer:DEADLYSIGNAL > ================================================================= > ==909==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000002d0 (pc 0x7f5f6eab1670 bp 0x7f5f6cbfecf0 sp 0x7f5f6cbfecc8 T25) > ==909==The signal is caused by a READ memory access. > ==909==Hint: address points to the zero page. > #0 0x7f5f6eab1670 in pthread_setaffinity_np (/lib/x86_64-linux-gnu/libc.so.6+0x8f670) > #1 0x7f5f6eed73df in libcamera::Thread::setThreadAffinityInternal() ../src/libcamera/base/thread.cpp:457 > #2 0x7f5f6eed62be in libcamera::Thread::startThread() ../src/libcamera/base/thread.cpp:287 > #3 0x7f5f6eee0e96 in void std::__invoke_impl<void, void (libcamera::Thread::*)(), libcamera::Thread*>(std::__invoke_memfun_deref, void (libcamera::Thread::*&&)(), libcamera::Thread*&&) /usr/include/c++/12/bits/invoke.h:74 > #4 0x7f5f6eee0d02 in std::__invoke_result<void (libcamera::Thread::*)(), libcamera::Thread*>::type std::__invoke<void (libcamera::Thread::*)(), libcamera::Thread*>(void (libcamera::Thread::*&&)(), libcamera::Thread*&&) /usr/include/c++/12/bits/invoke.h:96 > #5 0x7f5f6eee0c72 in void std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> >::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>) /usr/include/c++/12/bits/std_thread.h:252 > #6 0x7f5f6eee0c2b in std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> >::operator()() /usr/include/c++/12/bits/std_thread.h:259 > #7 0x7f5f6eee0c0f in std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> > >::_M_run() /usr/include/c++/12/bits/std_thread.h:210 > #8 0x7f5f6ecf74a2 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xd44a2) > #9 0x7f5f6eaab1c3 (/lib/x86_64-linux-gnu/libc.so.6+0x891c3) > #10 0x7f5f6eb2b85b (/lib/x86_64-linux-gnu/libc.so.6+0x10985b) > AddressSanitizer can not provide additional info. > SUMMARY: AddressSanitizer: SEGV (/lib/x86_64-linux-gnu/libc.so.6+0x8f670) in pthread_setaffinity_np > Thread T25 created by T0 here: > #0 0x7f5f6ef5c726 in __interceptor_pthread_create ../../../../src/libsanitizer/asan/asan_interceptors.cpp:207 > #1 0x7f5f6ecf7578 in std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) (/lib/x86_64-linux-gnu/libstdc++.so.6+0xd4578) > #2 0x7f5f6eed5ddb in libcamera::Thread::start() ../src/libcamera/base/thread.cpp:259 > #3 0x55dea5dbf519 in ThreadTest::run() ../test/threads.cpp:196 > #4 0x55dea5dc5bf4 in Test::execute() ../test/libtest/test.cpp:33 > #5 0x55dea5dbd54f in main ../test/threads.cpp:216 > #6 0x7f5f6ea49249 (/lib/x86_64-linux-gnu/libc.so.6+0x27249) > ==909==ABORTING > > I'm not yet sure what to do here, it might be that we need to revert the > ThreadAffinity patch. > > > Harvey, could you see if you can replicate this issue - or spot any > potential issues here please? The crash happens on the use case that it sets thread affinity before starting the thread. mtkisp7 always starts the thread first though. Let me do some hacks and run CTS... (Basic usages seem fine to me currently. In the meantime, I don't mind reverting the ThreadAffinity patch for now. Sorry for the flaky issue. BR, Harvey > > -- > Kieran > > > > > > > --- > > > include/libcamera/base/thread.h | 5 ++++ > > > src/libcamera/base/thread.cpp | 47 +++++++++++++++++++++++++++++++++ > > > test/threads.cpp | 40 ++++++++++++++++++++++++++++ > > > 3 files changed, 92 insertions(+) > > > > > > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h > > > index 4f33de63d..3209d4f7c 100644 > > > --- a/include/libcamera/base/thread.h > > > +++ b/include/libcamera/base/thread.h > > > @@ -15,6 +15,7 @@ > > > > > > #include <libcamera/base/message.h> > > > #include <libcamera/base/signal.h> > > > +#include <libcamera/base/span.h> > > > #include <libcamera/base/utils.h> > > > > > > namespace libcamera { > > > @@ -35,6 +36,8 @@ public: > > > void exit(int code = 0); > > > bool wait(utils::duration duration = utils::duration::max()); > > > > > > + int setThreadAffinity(const Span<const unsigned int> &cpus); > > > + > > > bool isRunning(); > > > > > > Signal<> finished; > > > @@ -54,6 +57,8 @@ private: > > > void startThread(); > > > void finishThread(); > > > > > > + void setThreadAffinityInternal(); > > > + > > > void postMessage(std::unique_ptr<Message> msg, Object *receiver); > > > void removeMessages(Object *receiver); > > > > > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp > > > index 8735670b8..f6322fe31 100644 > > > --- a/src/libcamera/base/thread.cpp > > > +++ b/src/libcamera/base/thread.cpp > > > @@ -9,6 +9,7 @@ > > > > > > #include <atomic> > > > #include <list> > > > +#include <optional> > > > #include <sys/syscall.h> > > > #include <sys/types.h> > > > #include <unistd.h> > > > @@ -128,6 +129,8 @@ private: > > > int exitCode_; > > > > > > MessageQueue messages_; > > > + > > > + std::optional<cpu_set_t> cpuset_; > > > }; > > > > > > /** > > > @@ -281,6 +284,8 @@ void Thread::startThread() > > > data_->tid_ = syscall(SYS_gettid); > > > currentThreadData = data_; > > > > > > + setThreadAffinityInternal(); > > > + > > > run(); > > > } > > > > > > @@ -410,6 +415,48 @@ bool Thread::wait(utils::duration duration) > > > return hasFinished; > > > } > > > > > > +/** > > > + * \brief Set the CPU affinity mask of the thread > > > + * \param[in] cpus The list of CPU indices that the thread is set affinity to > > > + * > > > + * The CPU indices should be within [0, std::thread::hardware_concurrency()). > > > + * If any index is invalid, this function won't modify the thread affinity and > > > + * will return an error. > > > + * > > > + * \return 0 if all indices are valid, -EINVAL otherwise > > > + */ > > > +int Thread::setThreadAffinity(const Span<const unsigned int> &cpus) > > > +{ > > > + const unsigned int numCpus = std::thread::hardware_concurrency(); > > > + > > > + MutexLocker locker(data_->mutex_); > > > + data_->cpuset_ = cpu_set_t(); > > > + CPU_ZERO(&data_->cpuset_.value()); > > > + > > > + for (const unsigned int &cpu : cpus) { > > > + if (cpu >= numCpus) { > > > + LOG(Thread, Error) << "Invalid CPU " << cpu << "for thread affinity"; > > > + return -EINVAL; > > > + } > > > + > > > + CPU_SET(cpu, &data_->cpuset_.value()); > > > + } > > > + > > > + if (data_->running_) > > > + setThreadAffinityInternal(); > > > + > > > + return 0; > > > +} > > > + > > > +void Thread::setThreadAffinityInternal() > > > +{ > > > + if (!data_->cpuset_) > > > + return; > > > + > > > + const cpu_set_t &cpuset = data_->cpuset_.value(); > > > + pthread_setaffinity_np(thread_.native_handle(), sizeof(cpuset), &cpuset); > > > +} > > > + > > > /** > > > * \brief Check if the thread is running > > > * > > > diff --git a/test/threads.cpp b/test/threads.cpp > > > index ceb4fa0f2..8d6ee1510 100644 > > > --- a/test/threads.cpp > > > +++ b/test/threads.cpp > > > @@ -9,9 +9,11 @@ > > > #include <iostream> > > > #include <memory> > > > #include <pthread.h> > > > +#include <sched.h> > > > #include <thread> > > > #include <time.h> > > > > > > +#include <libcamera/base/object.h> > > > #include <libcamera/base/thread.h> > > > > > > #include "test.h" > > > @@ -66,6 +68,27 @@ private: > > > bool &cancelled_; > > > }; > > > > > > +class CpuSetTester : public Object > > > +{ > > > +public: > > > + CpuSetTester(unsigned int cpuset) > > > + : cpuset_(cpuset) {} > > > + > > > + bool testCpuSet() > > > + { > > > + int ret = sched_getcpu(); > > > + if (static_cast<int>(cpuset_) != ret) { > > > + cout << "Invalid cpuset: " << ret << ", expecting: " << cpuset_ << endl; > > > + return false; > > > + } > > > + > > > + return true; > > > + } > > > + > > > +private: > > > + const unsigned int cpuset_; > > > +}; > > > + > > > class ThreadTest : public Test > > > { > > > protected: > > > @@ -165,6 +188,23 @@ protected: > > > return TestFail; > > > } > > > > > > + const unsigned int numCpus = std::thread::hardware_concurrency(); > > > + for (unsigned int i = 0; i < numCpus; ++i) { > > > + thread = std::make_unique<Thread>(); > > > + const std::array<const unsigned int, 1> cpus{ i }; > > > + thread->setThreadAffinity(cpus); > > > + thread->start(); > > > + > > > + CpuSetTester tester(i); > > > + tester.moveToThread(thread.get()); > > > + > > > + if (!tester.invokeMethod(&CpuSetTester::testCpuSet, ConnectionTypeBlocking)) > > > + return TestFail; > > > + > > > + thread->exit(0); > > > + thread->wait(); > > > + } > > > + > > > return TestPass; > > > } > > > > > > -- > > > 2.47.0.163.g1226f6d8fa-goog > > >
Hi Kieran, On Sat, Nov 30, 2024 at 1:59 AM Cheng-Hao Yang <chenghaoyang@chromium.org> wrote: > > Hi Kieran, > > On Sat, Nov 30, 2024 at 1:26 AM Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: > > > > Quoting Kieran Bingham (2024-11-20 11:22:34) > > > Quoting Harvey Yang (2024-10-29 08:57:55) > > > > From: Han-Lin Chen <hanlinchen@chromium.org> > > > > > > > > Add method to set thread affinity to Thread class. > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > > > > Looks good to me. > > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > I have merged this patch based on the tests succeeding at : > > > > - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319468 > > > > Since then the following pipelines have successfully also passed on top > > of this patch: > > > > - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319906 > > - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1319907 > > > > However, I am now seeing failures at > > - https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1320174 > > Interesting. My gitlab pipeline on the same code base passed: > https://gitlab.freedesktop.org/chenghaoyang/libcamera/-/pipelines/1319966 > > > > > with the following jobs: > > - https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67448427 > > - https://gitlab.freedesktop.org/camera/libcamera/-/jobs/67445198 > > > > 63/78 libcamera / threads FAIL 1.19s exit status 1 > > >>> 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 > > ――――――――――――――――――――――――――――――――――――― ✀ ――――――――――――――――――――――――――――――――――――― > > stderr: > > AddressSanitizer:DEADLYSIGNAL > > ================================================================= > > ==909==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000002d0 (pc 0x7f5f6eab1670 bp 0x7f5f6cbfecf0 sp 0x7f5f6cbfecc8 T25) > > ==909==The signal is caused by a READ memory access. > > ==909==Hint: address points to the zero page. > > #0 0x7f5f6eab1670 in pthread_setaffinity_np (/lib/x86_64-linux-gnu/libc.so.6+0x8f670) > > #1 0x7f5f6eed73df in libcamera::Thread::setThreadAffinityInternal() ../src/libcamera/base/thread.cpp:457 > > #2 0x7f5f6eed62be in libcamera::Thread::startThread() ../src/libcamera/base/thread.cpp:287 > > #3 0x7f5f6eee0e96 in void std::__invoke_impl<void, void (libcamera::Thread::*)(), libcamera::Thread*>(std::__invoke_memfun_deref, void (libcamera::Thread::*&&)(), libcamera::Thread*&&) /usr/include/c++/12/bits/invoke.h:74 > > #4 0x7f5f6eee0d02 in std::__invoke_result<void (libcamera::Thread::*)(), libcamera::Thread*>::type std::__invoke<void (libcamera::Thread::*)(), libcamera::Thread*>(void (libcamera::Thread::*&&)(), libcamera::Thread*&&) /usr/include/c++/12/bits/invoke.h:96 > > #5 0x7f5f6eee0c72 in void std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> >::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>) /usr/include/c++/12/bits/std_thread.h:252 > > #6 0x7f5f6eee0c2b in std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> >::operator()() /usr/include/c++/12/bits/std_thread.h:259 > > #7 0x7f5f6eee0c0f in std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> > >::_M_run() /usr/include/c++/12/bits/std_thread.h:210 > > #8 0x7f5f6ecf74a2 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xd44a2) > > #9 0x7f5f6eaab1c3 (/lib/x86_64-linux-gnu/libc.so.6+0x891c3) > > #10 0x7f5f6eb2b85b (/lib/x86_64-linux-gnu/libc.so.6+0x10985b) > > AddressSanitizer can not provide additional info. > > SUMMARY: AddressSanitizer: SEGV (/lib/x86_64-linux-gnu/libc.so.6+0x8f670) in pthread_setaffinity_np > > Thread T25 created by T0 here: > > #0 0x7f5f6ef5c726 in __interceptor_pthread_create ../../../../src/libsanitizer/asan/asan_interceptors.cpp:207 > > #1 0x7f5f6ecf7578 in std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) (/lib/x86_64-linux-gnu/libstdc++.so.6+0xd4578) > > #2 0x7f5f6eed5ddb in libcamera::Thread::start() ../src/libcamera/base/thread.cpp:259 > > #3 0x55dea5dbf519 in ThreadTest::run() ../test/threads.cpp:196 > > #4 0x55dea5dc5bf4 in Test::execute() ../test/libtest/test.cpp:33 > > #5 0x55dea5dbd54f in main ../test/threads.cpp:216 > > #6 0x7f5f6ea49249 (/lib/x86_64-linux-gnu/libc.so.6+0x27249) > > ==909==ABORTING > > > > I'm not yet sure what to do here, it might be that we need to revert the > > ThreadAffinity patch. > > > > > > Harvey, could you see if you can replicate this issue - or spot any > > potential issues here please? > > The crash happens on the use case that it sets thread affinity before > starting the thread. mtkisp7 always starts the thread first though. > Let me do some hacks and run CTS... > (Basic usages seem fine to me currently. > > In the meantime, I don't mind reverting the ThreadAffinity patch for > now. Sorry for the flaky issue. I reproduced it on mtkisp7, and I think the main difference of the upstream patch and the one adopted in CrOS mtkisp7 branch is that the former one call setThreadAffinityInternal in the starting function of std::thread. I tried the fix `[PATCH] Thread: Fix setThreadAffinity issue`, and I believe it works on mtkisp7. Please help check if it makes sense. BR, Harvey > > BR, > Harvey > > > > > -- > > Kieran > > > > > > > > > > > --- > > > > include/libcamera/base/thread.h | 5 ++++ > > > > src/libcamera/base/thread.cpp | 47 +++++++++++++++++++++++++++++++++ > > > > test/threads.cpp | 40 ++++++++++++++++++++++++++++ > > > > 3 files changed, 92 insertions(+) > > > > > > > > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h > > > > index 4f33de63d..3209d4f7c 100644 > > > > --- a/include/libcamera/base/thread.h > > > > +++ b/include/libcamera/base/thread.h > > > > @@ -15,6 +15,7 @@ > > > > > > > > #include <libcamera/base/message.h> > > > > #include <libcamera/base/signal.h> > > > > +#include <libcamera/base/span.h> > > > > #include <libcamera/base/utils.h> > > > > > > > > namespace libcamera { > > > > @@ -35,6 +36,8 @@ public: > > > > void exit(int code = 0); > > > > bool wait(utils::duration duration = utils::duration::max()); > > > > > > > > + int setThreadAffinity(const Span<const unsigned int> &cpus); > > > > + > > > > bool isRunning(); > > > > > > > > Signal<> finished; > > > > @@ -54,6 +57,8 @@ private: > > > > void startThread(); > > > > void finishThread(); > > > > > > > > + void setThreadAffinityInternal(); > > > > + > > > > void postMessage(std::unique_ptr<Message> msg, Object *receiver); > > > > void removeMessages(Object *receiver); > > > > > > > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp > > > > index 8735670b8..f6322fe31 100644 > > > > --- a/src/libcamera/base/thread.cpp > > > > +++ b/src/libcamera/base/thread.cpp > > > > @@ -9,6 +9,7 @@ > > > > > > > > #include <atomic> > > > > #include <list> > > > > +#include <optional> > > > > #include <sys/syscall.h> > > > > #include <sys/types.h> > > > > #include <unistd.h> > > > > @@ -128,6 +129,8 @@ private: > > > > int exitCode_; > > > > > > > > MessageQueue messages_; > > > > + > > > > + std::optional<cpu_set_t> cpuset_; > > > > }; > > > > > > > > /** > > > > @@ -281,6 +284,8 @@ void Thread::startThread() > > > > data_->tid_ = syscall(SYS_gettid); > > > > currentThreadData = data_; > > > > > > > > + setThreadAffinityInternal(); > > > > + > > > > run(); > > > > } > > > > > > > > @@ -410,6 +415,48 @@ bool Thread::wait(utils::duration duration) > > > > return hasFinished; > > > > } > > > > > > > > +/** > > > > + * \brief Set the CPU affinity mask of the thread > > > > + * \param[in] cpus The list of CPU indices that the thread is set affinity to > > > > + * > > > > + * The CPU indices should be within [0, std::thread::hardware_concurrency()). > > > > + * If any index is invalid, this function won't modify the thread affinity and > > > > + * will return an error. > > > > + * > > > > + * \return 0 if all indices are valid, -EINVAL otherwise > > > > + */ > > > > +int Thread::setThreadAffinity(const Span<const unsigned int> &cpus) > > > > +{ > > > > + const unsigned int numCpus = std::thread::hardware_concurrency(); > > > > + > > > > + MutexLocker locker(data_->mutex_); > > > > + data_->cpuset_ = cpu_set_t(); > > > > + CPU_ZERO(&data_->cpuset_.value()); > > > > + > > > > + for (const unsigned int &cpu : cpus) { > > > > + if (cpu >= numCpus) { > > > > + LOG(Thread, Error) << "Invalid CPU " << cpu << "for thread affinity"; > > > > + return -EINVAL; > > > > + } > > > > + > > > > + CPU_SET(cpu, &data_->cpuset_.value()); > > > > + } > > > > + > > > > + if (data_->running_) > > > > + setThreadAffinityInternal(); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +void Thread::setThreadAffinityInternal() > > > > +{ > > > > + if (!data_->cpuset_) > > > > + return; > > > > + > > > > + const cpu_set_t &cpuset = data_->cpuset_.value(); > > > > + pthread_setaffinity_np(thread_.native_handle(), sizeof(cpuset), &cpuset); > > > > +} > > > > + > > > > /** > > > > * \brief Check if the thread is running > > > > * > > > > diff --git a/test/threads.cpp b/test/threads.cpp > > > > index ceb4fa0f2..8d6ee1510 100644 > > > > --- a/test/threads.cpp > > > > +++ b/test/threads.cpp > > > > @@ -9,9 +9,11 @@ > > > > #include <iostream> > > > > #include <memory> > > > > #include <pthread.h> > > > > +#include <sched.h> > > > > #include <thread> > > > > #include <time.h> > > > > > > > > +#include <libcamera/base/object.h> > > > > #include <libcamera/base/thread.h> > > > > > > > > #include "test.h" > > > > @@ -66,6 +68,27 @@ private: > > > > bool &cancelled_; > > > > }; > > > > > > > > +class CpuSetTester : public Object > > > > +{ > > > > +public: > > > > + CpuSetTester(unsigned int cpuset) > > > > + : cpuset_(cpuset) {} > > > > + > > > > + bool testCpuSet() > > > > + { > > > > + int ret = sched_getcpu(); > > > > + if (static_cast<int>(cpuset_) != ret) { > > > > + cout << "Invalid cpuset: " << ret << ", expecting: " << cpuset_ << endl; > > > > + return false; > > > > + } > > > > + > > > > + return true; > > > > + } > > > > + > > > > +private: > > > > + const unsigned int cpuset_; > > > > +}; > > > > + > > > > class ThreadTest : public Test > > > > { > > > > protected: > > > > @@ -165,6 +188,23 @@ protected: > > > > return TestFail; > > > > } > > > > > > > > + const unsigned int numCpus = std::thread::hardware_concurrency(); > > > > + for (unsigned int i = 0; i < numCpus; ++i) { > > > > + thread = std::make_unique<Thread>(); > > > > + const std::array<const unsigned int, 1> cpus{ i }; > > > > + thread->setThreadAffinity(cpus); > > > > + thread->start(); > > > > + > > > > + CpuSetTester tester(i); > > > > + tester.moveToThread(thread.get()); > > > > + > > > > + if (!tester.invokeMethod(&CpuSetTester::testCpuSet, ConnectionTypeBlocking)) > > > > + return TestFail; > > > > + > > > > + thread->exit(0); > > > > + thread->wait(); > > > > + } > > > > + > > > > return TestPass; > > > > } > > > > > > > > -- > > > > 2.47.0.163.g1226f6d8fa-goog > > > >
diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h index 4f33de63d..3209d4f7c 100644 --- a/include/libcamera/base/thread.h +++ b/include/libcamera/base/thread.h @@ -15,6 +15,7 @@ #include <libcamera/base/message.h> #include <libcamera/base/signal.h> +#include <libcamera/base/span.h> #include <libcamera/base/utils.h> namespace libcamera { @@ -35,6 +36,8 @@ public: void exit(int code = 0); bool wait(utils::duration duration = utils::duration::max()); + int setThreadAffinity(const Span<const unsigned int> &cpus); + bool isRunning(); Signal<> finished; @@ -54,6 +57,8 @@ private: void startThread(); void finishThread(); + void setThreadAffinityInternal(); + void postMessage(std::unique_ptr<Message> msg, Object *receiver); void removeMessages(Object *receiver); diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp index 8735670b8..f6322fe31 100644 --- a/src/libcamera/base/thread.cpp +++ b/src/libcamera/base/thread.cpp @@ -9,6 +9,7 @@ #include <atomic> #include <list> +#include <optional> #include <sys/syscall.h> #include <sys/types.h> #include <unistd.h> @@ -128,6 +129,8 @@ private: int exitCode_; MessageQueue messages_; + + std::optional<cpu_set_t> cpuset_; }; /** @@ -281,6 +284,8 @@ void Thread::startThread() data_->tid_ = syscall(SYS_gettid); currentThreadData = data_; + setThreadAffinityInternal(); + run(); } @@ -410,6 +415,48 @@ bool Thread::wait(utils::duration duration) return hasFinished; } +/** + * \brief Set the CPU affinity mask of the thread + * \param[in] cpus The list of CPU indices that the thread is set affinity to + * + * The CPU indices should be within [0, std::thread::hardware_concurrency()). + * If any index is invalid, this function won't modify the thread affinity and + * will return an error. + * + * \return 0 if all indices are valid, -EINVAL otherwise + */ +int Thread::setThreadAffinity(const Span<const unsigned int> &cpus) +{ + const unsigned int numCpus = std::thread::hardware_concurrency(); + + MutexLocker locker(data_->mutex_); + data_->cpuset_ = cpu_set_t(); + CPU_ZERO(&data_->cpuset_.value()); + + for (const unsigned int &cpu : cpus) { + if (cpu >= numCpus) { + LOG(Thread, Error) << "Invalid CPU " << cpu << "for thread affinity"; + return -EINVAL; + } + + CPU_SET(cpu, &data_->cpuset_.value()); + } + + if (data_->running_) + setThreadAffinityInternal(); + + return 0; +} + +void Thread::setThreadAffinityInternal() +{ + if (!data_->cpuset_) + return; + + const cpu_set_t &cpuset = data_->cpuset_.value(); + pthread_setaffinity_np(thread_.native_handle(), sizeof(cpuset), &cpuset); +} + /** * \brief Check if the thread is running * diff --git a/test/threads.cpp b/test/threads.cpp index ceb4fa0f2..8d6ee1510 100644 --- a/test/threads.cpp +++ b/test/threads.cpp @@ -9,9 +9,11 @@ #include <iostream> #include <memory> #include <pthread.h> +#include <sched.h> #include <thread> #include <time.h> +#include <libcamera/base/object.h> #include <libcamera/base/thread.h> #include "test.h" @@ -66,6 +68,27 @@ private: bool &cancelled_; }; +class CpuSetTester : public Object +{ +public: + CpuSetTester(unsigned int cpuset) + : cpuset_(cpuset) {} + + bool testCpuSet() + { + int ret = sched_getcpu(); + if (static_cast<int>(cpuset_) != ret) { + cout << "Invalid cpuset: " << ret << ", expecting: " << cpuset_ << endl; + return false; + } + + return true; + } + +private: + const unsigned int cpuset_; +}; + class ThreadTest : public Test { protected: @@ -165,6 +188,23 @@ protected: return TestFail; } + const unsigned int numCpus = std::thread::hardware_concurrency(); + for (unsigned int i = 0; i < numCpus; ++i) { + thread = std::make_unique<Thread>(); + const std::array<const unsigned int, 1> cpus{ i }; + thread->setThreadAffinity(cpus); + thread->start(); + + CpuSetTester tester(i); + tester.moveToThread(thread.get()); + + if (!tester.invokeMethod(&CpuSetTester::testCpuSet, ConnectionTypeBlocking)) + return TestFail; + + thread->exit(0); + thread->wait(); + } + return TestPass; }