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

21593 diff mbox series

From: Han-Lin Chen <hanlinchen@chromium.org>

Add method to set thread affinity to Thread class.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 include/libcamera/base/thread.h |  6 ++++++
 src/libcamera/base/thread.cpp   | 31 +++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

Quoting Harvey Yang (2024-10-11 18:00:48)
> From: Han-Lin Chen <hanlinchen@chromium.org>
> 
> Add method to set thread affinity to Thread class.
> 
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang@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) ?

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

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?


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.

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

Hi Kieran,

On Sat, Oct 12, 2024 at 5:39 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Harvey Yang (2024-10-11 18:00:48)
> > From: Han-Lin Chen <hanlinchen@chromium.org>
> >
> > Add method to set thread affinity to Thread class.
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > Signed-off-by: Harvey Yang <chenghaoyang@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
> >