[PATCH 1/1] libcamera: add method to set thread affinity

[Cheng-Hao Yang]

Hi Kieran,

On Sat, Oct 12, 2024 at 5:39 PM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting Harvey Yang (2024-10-11 18:00:48)
> > From: Han-Lin Chen <hanlinchen at chromium.org>
> >
> > Add method to set thread affinity to Thread class.
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > ---
> >  include/libcamera/base/thread.h |  6 ++++++
> >  src/libcamera/base/thread.cpp   | 31 +++++++++++++++++++++++++++++++
> >  2 files changed, 37 insertions(+)
> >
> > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h
> > index 4f33de63d..c74fe102b 100644
> > --- a/include/libcamera/base/thread.h
> > +++ b/include/libcamera/base/thread.h
> > @@ -35,6 +35,8 @@ public:
> >         void exit(int code = 0);
> >         bool wait(utils::duration duration = utils::duration::max());
> >
> > +       void setThreadAffinity(const std::vector<int> &cpus);
> > +
> >         bool isRunning();
> >
> >         Signal<> finished;
> > @@ -54,6 +56,8 @@ private:
> >         void startThread();
> >         void finishThread();
> >
> > +       void setThreadAffinityInternal();
> > +
> >         void postMessage(std::unique_ptr<Message> msg, Object *receiver);
> >         void removeMessages(Object *receiver);
> >
> > @@ -67,6 +71,8 @@ private:
> >
> >         std::thread thread_;
> >         ThreadData *data_;
> > +
> > +       std::vector<int> cpus_;
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
> > index 8735670b8..2417043a3 100644
> > --- a/src/libcamera/base/thread.cpp
> > +++ b/src/libcamera/base/thread.cpp
> > @@ -254,6 +254,8 @@ void Thread::start()
> >         data_->exit_.store(false, std::memory_order_relaxed);
> >
> >         thread_ = std::thread(&Thread::startThread, this);
> > +
> > +       setThreadAffinityInternal();
> >  }
> >
> >  void Thread::startThread()
> > @@ -410,6 +412,35 @@ bool Thread::wait(utils::duration duration)
> >         return hasFinished;
> >  }
> >
> > +/**
> > + * \brief Set the CPU affinity mask of the thread
> > + * \param[in] cpus The list of CPU indices that the thread is set affinity to
> > + */
> > +void Thread::setThreadAffinity(const std::vector<int> &cpus)
> > +{
> > +       cpus_ = cpus;
> > +
>
> Should there be any checks here to verify the CPU list is valid? What
> happens if I try to call Thread::setThreadAffinity(-1, 1000000) ?

Added checks to ensure the value is within [0,
`std::thread::hardware_concurrency()`).

>
> Perhaps it should also ensure cpus.size() is never bigger than
> CPU_SETSIZE?

Added logic to ignore the rest of cpu indices when `cpus_` is full.

>
> The asynchronous nature here means we won't know if the call to
> pthread_setaffinity_np() is successful until the thread is started, but
> is there anything we can do to validate the input as this is extending
> the Thread API?
>
> I'm not sure if there is anything worthwhile yet, but is there anything
> around this that should then be added to the unit tests?

Should we add return values or getter functions in Thread class to
allow checking the results in unit tests?

>
>
> I know all of the above is really unlikely to be called incorrectly in
> the code you'll add to use these - but we should treat libcamera-base as
> if it were an entirely separate helper library so we shouldn't skimp on
> layer checks and validation.
>
>
> > +       MutexLocker locker(data_->mutex_);
> > +       if (data_->running_)
>
> I'ts equivalent, so it's fine like the above, but there's also the
> helper which could make this:
>
>         if (isRunning())
>
> But both are fine with me so either way.
>


Actually it's a bit different, as we call
`Thread::setThreadAffinityInternal` within the lock. Using
`Thread::isRunning` is more complicated IIUC to achieve the
same logic. So let's keep it this way :)

BR,
Harvey




> --
> Kieran
>
> > +               setThreadAffinityInternal();
> > +}
> > +
> > +void Thread::setThreadAffinityInternal()
> > +{
> > +       if (cpus_.empty())
> > +               return;
> > +
> > +       auto handle = thread_.native_handle();
> > +
> > +       cpu_set_t cpuset;
> > +       CPU_ZERO(&cpuset);
> > +
> > +       for (auto cpu : cpus_)
> > +               CPU_SET(cpu, &cpuset);
> > +
> > +       pthread_setaffinity_np(handle, sizeof(cpu_set_t), &cpuset);
> > +}
> > +
> >  /**
> >   * \brief Check if the thread is running
> >   *
> > --
> > 2.47.0.rc1.288.g06298d1525-goog
> >