[PATCH] Thread: Fix setThreadAffinity issue

[Laurent Pinchart]

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 ?

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