[PATCH] Thread: Fix setThreadAffinity issue

[Kieran Bingham]

Quoting Laurent Pinchart (2025-01-07 09:40:54)
> Hi Harvey,
> 
> On Tue, Dec 03, 2024 at 02:47:06PM +0200, Laurent Pinchart wrote:
> > 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.
> 
> Could you please send a new version with an improved commit message ?

I saw this issue in CI today again too:

https://gitlab.freedesktop.org/camera/libcamera/-/jobs/68964947

So I'd be keen to merge this once the commit message is cleaned up.

--
Kieran


> 
> > > 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 at 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();
> > > >  }
> > > > 
> 
> -- 
> Regards,
> 
> Laurent Pinchart