| Message ID | 20241202090457.584020-1-chenghaoyang@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi 2024. december 2., hétfő 10:04 keltezéssel, Harvey Yang <chenghaoyang@chromium.org> írta: > This fixes commit 4d9db06d669044c0c461a2aed79c85c7fe32a502. > > Previously we call Thread::setThreadAffinityInternal in > Thread::startThread, which seems to lead to a SEGV in > pthread_setaffinity_np. I believe the patch should describe what the cause is. As far as I can see it's a data race on `Thread::thread_` where it is concurrently set and read from `Thread::start()` and `Thread::startThread()`, respectively. > > This patch moves the call after the construction of std::thread. > Should probably have: Fixes: 4d9db06d66 ("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(-) > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp > index f6322fe31..319bfda9a 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(); > } > > -- > 2.47.0.338.g60cca15819-goog > Regards, Barnabás Pőcze
On Tue, Dec 03, 2024 at 12:35:38PM +0000, Barnabás Pőcze wrote: > 2024. december 2., hétfő 10:04 keltezéssel, Harvey Yang írta: > > > This fixes commit 4d9db06d669044c0c461a2aed79c85c7fe32a502. > > > > Previously we call Thread::setThreadAffinityInternal in > > Thread::startThread, which seems to lead to a SEGV in > > pthread_setaffinity_np. > > I believe the patch should describe what the cause is. As far as I can Yes, all patches should do so. Saying there seems to be a problem and it seems to be fixed by a change without further explanation creates very little confidence, and it shouldn't be up to the reviewer to try and understand the fix. > see it's a data race on `Thread::thread_` where it is concurrently set > and read from `Thread::start()` and `Thread::startThread()`, respectively. > > > This patch moves the call after the construction of std::thread. If I recall correctly, setThreadAffinityInternal() was moved to Thread::startThread() based on review discussions, to pin the thread to CPUs before starting its main function. This patch drops this. The commit message needs to explain why this is considered fine. > Should probably have: > > Fixes: 4d9db06d66 ("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(-) > > > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp > > index f6322fe31..319bfda9a 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 f6322fe31..319bfda9a 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(); }
This fixes commit 4d9db06d669044c0c461a2aed79c85c7fe32a502. Previously we call Thread::setThreadAffinityInternal in Thread::startThread, which seems to lead to a SEGV in pthread_setaffinity_np. This patch moves the call after the construction of std::thread. Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> --- src/libcamera/base/thread.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)