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

21748 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 |  5 ++++
 src/libcamera/base/thread.cpp   | 47 +++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

Quoting Harvey Yang (2024-10-24 09:13:53)
> From: Han-Lin Chen <hanlinchen@chromium.org>
> 
> Add method to set thread affinity to Thread class.

As it stands, this is still adding unused code.

Can't we get 'something' into test/threads.cpp so that at least we know
the code is both compiled and /run/ by the CI runs ?

Perhaps even as simple as setting the threadAffinity to CPU 0 and making
sure that's the CPU the thread is on!?


#include <sched.h>  // For sched_getcpu()
int cpu = sched_getcpu();

And make sure 'cpu' is the value we expect ?

It could launch a thread on 'each' CPU for instance? Or even just only
validate it gets tied to CPU 0 ?

Are any specific permissions required to be able to use
Thread::setThreadAffinity()?


> 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 |  5 ++++
>  src/libcamera/base/thread.cpp   | 47 +++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h
> index 4f33de63d..3209d4f7c 100644
> --- a/include/libcamera/base/thread.h
> +++ b/include/libcamera/base/thread.h
> @@ -15,6 +15,7 @@
>  
>  #include <libcamera/base/message.h>
>  #include <libcamera/base/signal.h>
> +#include <libcamera/base/span.h>
>  #include <libcamera/base/utils.h>
>  
>  namespace libcamera {
> @@ -35,6 +36,8 @@ public:
>         void exit(int code = 0);
>         bool wait(utils::duration duration = utils::duration::max());
>  
> +       int setThreadAffinity(const Span<const unsigned int> &cpus);
> +
>         bool isRunning();
>  
>         Signal<> finished;
> @@ -54,6 +57,8 @@ private:
>         void startThread();
>         void finishThread();
>  
> +       void setThreadAffinityInternal();
> +
>         void postMessage(std::unique_ptr<Message> msg, Object *receiver);
>         void removeMessages(Object *receiver);
>  
> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
> index 8735670b8..f6322fe31 100644
> --- a/src/libcamera/base/thread.cpp
> +++ b/src/libcamera/base/thread.cpp
> @@ -9,6 +9,7 @@
>  
>  #include <atomic>
>  #include <list>
> +#include <optional>
>  #include <sys/syscall.h>
>  #include <sys/types.h>
>  #include <unistd.h>
> @@ -128,6 +129,8 @@ private:
>         int exitCode_;
>  
>         MessageQueue messages_;
> +
> +       std::optional<cpu_set_t> cpuset_;
>  };
>  
>  /**
> @@ -281,6 +284,8 @@ void Thread::startThread()
>         data_->tid_ = syscall(SYS_gettid);
>         currentThreadData = data_;
>  
> +       setThreadAffinityInternal();
> +
>         run();
>  }
>  
> @@ -410,6 +415,48 @@ 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
> + *
> + * The CPU indices should be within [0, std::thread::hardware_concurrency()).
> + * If any index is invalid, this function won't modify the thread affinity and
> + * will return an error.
> + *
> + * \return 0 if all indices are valid, -EINVAL otherwise
> + */
> +int Thread::setThreadAffinity(const Span<const unsigned int> &cpus)
> +{
> +       const unsigned int numCpus = std::thread::hardware_concurrency();
> +
> +       MutexLocker locker(data_->mutex_);
> +       data_->cpuset_ = cpu_set_t();
> +       CPU_ZERO(&data_->cpuset_.value());
> +
> +       for (const unsigned int &cpu : cpus) {
> +               if (cpu >= numCpus) {
> +                       LOG(Thread, Error) << "Invalid CPU " << cpu << "for thread affinity";
> +                       return -EINVAL;
> +               }
> +
> +               CPU_SET(cpu, &data_->cpuset_.value());
> +       }
> +
> +       if (data_->running_)
> +               setThreadAffinityInternal();
> +
> +       return 0;
> +}
> +
> +void Thread::setThreadAffinityInternal()
> +{
> +       if (!data_->cpuset_)
> +               return;
> +
> +       const cpu_set_t &cpuset = data_->cpuset_.value();
> +       pthread_setaffinity_np(thread_.native_handle(), sizeof(cpuset), &cpuset);
> +}
> +
>  /**
>   * \brief Check if the thread is running
>   *
> -- 
> 2.47.0.163.g1226f6d8fa-goog
>

Hi Kieran,

On Fri, Oct 25, 2024 at 8:48 AM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Harvey Yang (2024-10-24 09:13:53)
> > From: Han-Lin Chen <hanlinchen@chromium.org>
> >
> > Add method to set thread affinity to Thread class.
>
> As it stands, this is still adding unused code.
>
> Can't we get 'something' into test/threads.cpp so that at least we know
> the code is both compiled and /run/ by the CI runs ?
>
> Perhaps even as simple as setting the threadAffinity to CPU 0 and making
> sure that's the CPU the thread is on!?
>
>
> #include <sched.h>  // For sched_getcpu()
> int cpu = sched_getcpu();
>
> And make sure 'cpu' is the value we expect ?
>
> It could launch a thread on 'each' CPU for instance? Or even just only
> validate it gets tied to CPU 0 ?

Added unit tests to set affinity to every available cpu core.
I'm not sure how to run a function on a thread elegantly, so I ended up
declaring a class derived from Object. Please let me know how to make
it cleaner.

>
> Are any specific permissions required to be able to use
> Thread::setThreadAffinity()?

IIUC, a user can call pthread_setaffinity_np on a thread that it owns.
The syscall under the hood is sched_setaffinity, which is already included
in CrOS' allowed syscalls.
Should I add this info in the commit message?

BR,
Harvey




>
>
> > 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 |  5 ++++
> >  src/libcamera/base/thread.cpp   | 47 +++++++++++++++++++++++++++++++++
> >  2 files changed, 52 insertions(+)
> >
> > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h
> > index 4f33de63d..3209d4f7c 100644
> > --- a/include/libcamera/base/thread.h
> > +++ b/include/libcamera/base/thread.h
> > @@ -15,6 +15,7 @@
> >
> >  #include <libcamera/base/message.h>
> >  #include <libcamera/base/signal.h>
> > +#include <libcamera/base/span.h>
> >  #include <libcamera/base/utils.h>
> >
> >  namespace libcamera {
> > @@ -35,6 +36,8 @@ public:
> >         void exit(int code = 0);
> >         bool wait(utils::duration duration = utils::duration::max());
> >
> > +       int setThreadAffinity(const Span<const unsigned int> &cpus);
> > +
> >         bool isRunning();
> >
> >         Signal<> finished;
> > @@ -54,6 +57,8 @@ private:
> >         void startThread();
> >         void finishThread();
> >
> > +       void setThreadAffinityInternal();
> > +
> >         void postMessage(std::unique_ptr<Message> msg, Object *receiver);
> >         void removeMessages(Object *receiver);
> >
> > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
> > index 8735670b8..f6322fe31 100644
> > --- a/src/libcamera/base/thread.cpp
> > +++ b/src/libcamera/base/thread.cpp
> > @@ -9,6 +9,7 @@
> >
> >  #include <atomic>
> >  #include <list>
> > +#include <optional>
> >  #include <sys/syscall.h>
> >  #include <sys/types.h>
> >  #include <unistd.h>
> > @@ -128,6 +129,8 @@ private:
> >         int exitCode_;
> >
> >         MessageQueue messages_;
> > +
> > +       std::optional<cpu_set_t> cpuset_;
> >  };
> >
> >  /**
> > @@ -281,6 +284,8 @@ void Thread::startThread()
> >         data_->tid_ = syscall(SYS_gettid);
> >         currentThreadData = data_;
> >
> > +       setThreadAffinityInternal();
> > +
> >         run();
> >  }
> >
> > @@ -410,6 +415,48 @@ 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
> > + *
> > + * The CPU indices should be within [0, std::thread::hardware_concurrency()).
> > + * If any index is invalid, this function won't modify the thread affinity and
> > + * will return an error.
> > + *
> > + * \return 0 if all indices are valid, -EINVAL otherwise
> > + */
> > +int Thread::setThreadAffinity(const Span<const unsigned int> &cpus)
> > +{
> > +       const unsigned int numCpus = std::thread::hardware_concurrency();
> > +
> > +       MutexLocker locker(data_->mutex_);
> > +       data_->cpuset_ = cpu_set_t();
> > +       CPU_ZERO(&data_->cpuset_.value());
> > +
> > +       for (const unsigned int &cpu : cpus) {
> > +               if (cpu >= numCpus) {
> > +                       LOG(Thread, Error) << "Invalid CPU " << cpu << "for thread affinity";
> > +                       return -EINVAL;
> > +               }
> > +
> > +               CPU_SET(cpu, &data_->cpuset_.value());
> > +       }
> > +
> > +       if (data_->running_)
> > +               setThreadAffinityInternal();
> > +
> > +       return 0;
> > +}
> > +
> > +void Thread::setThreadAffinityInternal()
> > +{
> > +       if (!data_->cpuset_)
> > +               return;
> > +
> > +       const cpu_set_t &cpuset = data_->cpuset_.value();
> > +       pthread_setaffinity_np(thread_.native_handle(), sizeof(cpuset), &cpuset);
> > +}
> > +
> >  /**
> >   * \brief Check if the thread is running
> >   *
> > --
> > 2.47.0.163.g1226f6d8fa-goog
> >