[PATCH v7 1/1] libcamera: add method to set thread affinity

Kieran Bingham kieran.bingham at ideasonboard.com
Fri Nov 29 18:26:11 CET 2024


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

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
> >


More information about the libcamera-devel mailing list