[PATCH v7 1/1] libcamera: add method to set thread affinity
Cheng-Hao Yang
chenghaoyang at chromium.org
Mon Dec 2 10:07:07 CET 2024
Hi Kieran,
On Sat, Nov 30, 2024 at 1:59 AM Cheng-Hao Yang
<chenghaoyang at chromium.org> wrote:
>
> Hi Kieran,
>
> On Sat, Nov 30, 2024 at 1:26 AM Kieran Bingham
> <kieran.bingham at 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 at chromium.org>
> > > >
> > > > Add method to set thread affinity to Thread class.
> > > >
> > > > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > > > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > >
> > > Looks good to me.
> > >
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at 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
> > > >
More information about the libcamera-devel
mailing list