[PATCH] Thread: Fix setThreadAffinity issue

[Cheng-Hao Yang]

Hi all,

Uploaded v2.

As it was Barnabás who found the root cause, I wouldn't mind if
Barnabás takes the credit and uploads new versions of the fix. JFYI.

BR,
Harvey

On Tue, Jan 7, 2025 at 12:23 PM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> 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