| Message ID | 20250107144141.3416094-1-chenghaoyang@chromium.org |
|---|---|
| State | Accepted |
| Commit | d49a84a4f3aa63efc900564ff32558e4f5d85b04 |
| Headers | show |
| Series |
|
| Related | show |
Hi Harvey, Thank you for the patch. On Tue, Jan 07, 2025 at 02:41:37PM +0000, Harvey Yang wrote: > Previously we call Thread::setThreadAffinityInternal in > Thread::startThread. The purpose was to avoid the main workload being > run on incorrect CPUs. This leads to a race condition of setting > `Thread::thread_` in `Thread::start()` and accessing > `Thread::setThreadAffinityInternal` though. > > This patch moves the call after the construction of std::thread to avoid > the race condition. The downside is that the first tasks, if any, upon > starting a thread might be run on incorrect CPUs. > > Fixes: 4d9db06d6690 ("libcamera: add method to set thread affinity") > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/base/thread.cpp | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp > index f6322fe3108b..319bfda9ae98 100644 > --- a/src/libcamera/base/thread.cpp > +++ b/src/libcamera/base/thread.cpp > @@ -257,6 +257,8 @@ void Thread::start() > data_->exit_.store(false, std::memory_order_relaxed); > > thread_ = std::thread(&Thread::startThread, this); > + > + setThreadAffinityInternal(); > } > > void Thread::startThread() > @@ -284,8 +286,6 @@ void Thread::startThread() > data_->tid_ = syscall(SYS_gettid); > currentThreadData = data_; > > - setThreadAffinityInternal(); > - > run(); > } >
diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp index f6322fe3108b..319bfda9ae98 100644 --- a/src/libcamera/base/thread.cpp +++ b/src/libcamera/base/thread.cpp @@ -257,6 +257,8 @@ void Thread::start() data_->exit_.store(false, std::memory_order_relaxed); thread_ = std::thread(&Thread::startThread, this); + + setThreadAffinityInternal(); } void Thread::startThread() @@ -284,8 +286,6 @@ void Thread::startThread() data_->tid_ = syscall(SYS_gettid); currentThreadData = data_; - setThreadAffinityInternal(); - run(); }
Previously we call Thread::setThreadAffinityInternal in Thread::startThread. The purpose was to avoid the main workload being run on incorrect CPUs. This leads to a race condition of setting `Thread::thread_` in `Thread::start()` and accessing `Thread::setThreadAffinityInternal` though. This patch moves the call after the construction of std::thread to avoid the race condition. The downside is that the first tasks, if any, upon starting a thread might be run on incorrect CPUs. Fixes: 4d9db06d6690 ("libcamera: add method to set thread affinity") Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> --- src/libcamera/base/thread.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)