[PATCH] Thread: Fix setThreadAffinity issue

[Laurent Pinchart]

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