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

[Laurent Pinchart]

Hi Harvey, Han-Lin,

Thank you for the patch.

On Sat, Oct 12, 2024 at 12:47:57PM +0000, Harvey Yang wrote:
> 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   | 47 +++++++++++++++++++++++++++++++++
>  2 files changed, 53 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_;

You can move this to ThreadData so it won't affect the API.

>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
> index 8735670b8..de5423eda 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,51 @@ 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

As there's error handling in the function, you should describe what
happens in case of errors.

> + */
> +void Thread::setThreadAffinity(const std::vector<int> &cpus)

Make it a span so the caller can also use an array instead of a vector.

> +{
> +	cpus_.clear();
> +
> +	const unsigned int num_cpus = std::thread::hardware_concurrency();
> +	for (const int &cpu : cpus) {
> +		if (cpus_.size() == CPU_SETSIZE) {
> +			LOG(Thread, Error) << "cpus_ already contains " << CPU_SETSIZE
> +					   << " cpus. Ignoring the rest.";
> +			break;
> +		}
> +
> +		if (cpu >= static_cast<int>(num_cpus) || cpu < 0) {

Why is the argument to the function a vector of int if entries can't be
negative ? Can't it be a vector of unsigned int instead ? You'll be able
to drop the cast here and drop the second check.

> +			LOG(Thread, Error) << "Ignore an invalid cpu index: " << cpu;
> +			continue;
> +		}
> +
> +		cpus_.push_back(cpu);
> +	}
> +
> +	MutexLocker locker(data_->mutex_);
> +	if (data_->running_)
> +		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);

Can't you compute the cpu_set_t and store it in the ThreadData class,
instead of storing a vector ? This can be done without holding the lock.

> +
> +	pthread_setaffinity_np(handle, sizeof(cpu_set_t), &cpuset);

sizeof(cpu_set_t)

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

-- 
Regards,

Laurent Pinchart