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

21608 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   | 52 +++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

Hi Harvey, Han-Lin,

Thank you for the patch.

On Mon, Oct 14, 2024 at 06:58:56AM +0000, Harvey Yang wrote:
> 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   | 52 +++++++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h
> index 4f33de63d..58cae341b 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());
>  
> +	void 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..f40c7d8e7 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_ = std::nullopt;
>  };
>  
>  /**
> @@ -254,6 +257,8 @@ void Thread::start()
>  	data_->exit_.store(false, std::memory_order_relaxed);
>  
>  	thread_ = std::thread(&Thread::startThread, this);
> +
> +	setThreadAffinityInternal();
>  }
>  
>  void Thread::startThread()
> @@ -410,6 +415,53 @@ 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()),

s/cpu/CPU/

> + * and the total number should not exceed `CPU_SETSIZE`. Invalid indices and
> + * extra indices will be ignored.
> + */
> +void 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());
> +
> +	unsigned int count = 0;
> +	for (const unsigned int &cpu : cpus) {
> +		if (count == CPU_SETSIZE) {
> +			LOG(Thread, Error) << "cpus_ already contains " << CPU_SETSIZE
> +					   << " cpus. Ignoring the rest.";
> +			break;
> +		}

I don't think this check is needed anymore. You should adapt the
documentation accordingly.

> +
> +		if (cpu >= numCpus) {
> +			LOG(Thread, Error) << "Ignore an invalid cpu index: " << cpu;
> +			continue;

Why is it better to ignore the value instead of returning an error ?

> +		}
> +
> +		++count;
> +		CPU_SET(cpu, &data_->cpuset_.value());
> +	}
> +
> +	if (data_->running_)
> +		setThreadAffinityInternal();
> +}
> +
> +void Thread::setThreadAffinityInternal()
> +{
> +	if (!data_->cpuset_)
> +		return;
> +
> +	auto handle = thread_.native_handle();
> +
> +	pthread_setaffinity_np(handle, sizeof(cpu_set_t), &data_->cpuset_.value());

	const cpu_set_t &cpuset_ = data_->cpuset_.value();
	pthread_setaffinity_np(thread_.native_handle(), sizeof(cpuset), &cpuset);

and you can drop the handle variable.

> +}
> +
>  /**
>   * \brief Check if the thread is running
>   *

Hi Laurent,

On Tue, Oct 15, 2024 at 6:33 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Harvey, Han-Lin,
>
> Thank you for the patch.
>
> On Mon, Oct 14, 2024 at 06:58:56AM +0000, Harvey Yang wrote:
> > 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   | 52 +++++++++++++++++++++++++++++++++
> >  2 files changed, 57 insertions(+)
> >
> > diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h
> > index 4f33de63d..58cae341b 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());
> >
> > +     void 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..f40c7d8e7 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_ = std::nullopt;
> >  };
> >
> >  /**
> > @@ -254,6 +257,8 @@ void Thread::start()
> >       data_->exit_.store(false, std::memory_order_relaxed);
> >
> >       thread_ = std::thread(&Thread::startThread, this);
> > +
> > +     setThreadAffinityInternal();
> >  }
> >
> >  void Thread::startThread()
> > @@ -410,6 +415,53 @@ 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()),
>
> s/cpu/CPU/

Done

>
> > + * and the total number should not exceed `CPU_SETSIZE`. Invalid indices and
> > + * extra indices will be ignored.
> > + */
> > +void 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());
> > +
> > +     unsigned int count = 0;
> > +     for (const unsigned int &cpu : cpus) {
> > +             if (count == CPU_SETSIZE) {
> > +                     LOG(Thread, Error) << "cpus_ already contains " << CPU_SETSIZE
> > +                                        << " cpus. Ignoring the rest.";
> > +                     break;
> > +             }
>
> I don't think this check is needed anymore. You should adapt the
> documentation accordingly.

Done

>
> > +
> > +             if (cpu >= numCpus) {
> > +                     LOG(Thread, Error) << "Ignore an invalid cpu index: " << cpu;
> > +                     continue;
>
> Why is it better to ignore the value instead of returning an error ?

Yeah that's also an option. Updated.

>
> > +             }
> > +
> > +             ++count;
> > +             CPU_SET(cpu, &data_->cpuset_.value());
> > +     }
> > +
> > +     if (data_->running_)
> > +             setThreadAffinityInternal();
> > +}
> > +
> > +void Thread::setThreadAffinityInternal()
> > +{
> > +     if (!data_->cpuset_)
> > +             return;
> > +
> > +     auto handle = thread_.native_handle();
> > +
> > +     pthread_setaffinity_np(handle, sizeof(cpu_set_t), &data_->cpuset_.value());
>
>         const cpu_set_t &cpuset_ = data_->cpuset_.value();
>         pthread_setaffinity_np(thread_.native_handle(), sizeof(cpuset), &cpuset);
>
> and you can drop the handle variable.

Done

BR,
Harvey



>
> > +}
> > +
> >  /**
> >   * \brief Check if the thread is running
> >   *
>
> --
> Regards,
>
> Laurent Pinchart