[PATCH v5 05/18] libcamera: shared_mem_object: reorganize the code and document the SharedMemObject class

[Milan Zamazal]

Hi,

Barnabás Pőcze <pobrn at protonmail.com> writes:

> Hi
>
>
> 2024. március 11., hétfő 15:15 keltezéssel, Hans de Goede <hdegoede at redhat.com> írta:
>
>> From: Andrei Konovalov <andrey.konovalov.ynk at gmail.com>
>> 
>> Split the parts which doesn't otherwise depend on the type T or
>> arguments Args out of the SharedMemObject class into a new
>> SharedMem class.
>> 
>> Doxygen documentation by Dennis Bonke and Andrei Konovalov.
>> 
>> Reviewed-by: Pavel Machek <pavel at ucw.cz>
>> Co-developed-by: Dennis Bonke <admin at dennisbonke.com>
>> Signed-off-by: Dennis Bonke <admin at dennisbonke.com>
>> Signed-off-by: Andrei Konovalov <andrey.konovalov.ynk at gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>>  .../libcamera/internal/shared_mem_object.h    | 101 ++++++----
>>  src/libcamera/meson.build                     |   1 +
>>  src/libcamera/shared_mem_object.cpp           | 190 ++++++++++++++++++
>>  3 files changed, 253 insertions(+), 39 deletions(-)
>>  create mode 100644 src/libcamera/shared_mem_object.cpp
>> 
>> [...]
>>  template<class T>
>> -class SharedMemObject
>> +class SharedMemObject : public SharedMem
>>  {
>>  public:
>>  	static constexpr std::size_t SIZE = sizeof(T);
>> 
>>  	SharedMemObject()
>> -		: obj_(nullptr)
>> +		: SharedMem(), obj_(nullptr)
>>  	{
>>  	}
>> 
>>  	template<class... Args>
>>  	SharedMemObject(const std::string &name, Args &&...args)
>> -		: name_(name), obj_(nullptr)
>> +		: SharedMem(name, SIZE), obj_(nullptr)
>>  	{
>> -		void *mem;
>> -		int ret;
>> -
>> -		ret = memfd_create(name_.c_str(), MFD_CLOEXEC);
>> -		if (ret < 0)
>> +		if (mem_ == nullptr)
>>  			return;
>> 
>> -		fd_ = SharedFD(std::move(ret));
>> -		if (!fd_.isValid())
>> -			return;
>> -
>> -		ret = ftruncate(fd_.get(), SIZE);
>> -		if (ret < 0)
>> -			return;
>> -
>> -		mem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
>> -			   fd_.get(), 0);
>> -		if (mem == MAP_FAILED)
>> -			return;
>> -
>> -		obj_ = new (mem) T(std::forward<Args>(args)...);
>> +		obj_ = new (mem_) T(std::forward<Args>(args)...);
>
> If I see it correctly, every `SharedMemObject` stores the same pointer twice
> (`obj_` and `mem_` in the base class). I think this can be avoided.

Perhaps but I think it's better not to touch this code as we approach merging
this initial software ISP support, thus no change here in v6.  If we feel an
urge for improvement here, a patch can be created afterwards.

Regards,
Milan

>>  	}
>> 
>>  	SharedMemObject(SharedMemObject<T> &&rhs)
>> +		: SharedMem(std::move(rhs))
>>  	{
>> -		this->name_ = std::move(rhs.name_);
>> -		this->fd_ = std::move(rhs.fd_);
>>  		this->obj_ = rhs.obj_;
>>  		rhs.obj_ = nullptr;
>>  	}
>> 
>>  	~SharedMemObject()
>>  	{
>> -		if (obj_) {
>> +		if (obj_)
>>  			obj_->~T();
>> -			munmap(obj_, SIZE);
>> -		}
>>  	}
>> 
>>  	/* Make SharedMemObject non-copyable for now. */
>> @@ -78,8 +109,7 @@ public:
>> 
>>  	SharedMemObject<T> &operator=(SharedMemObject<T> &&rhs)
>>  	{
>> -		this->name_ = std::move(rhs.name_);
>> -		this->fd_ = std::move(rhs.fd_);
>> +		SharedMem::operator=(std::move(rhs));
>>  		this->obj_ = rhs.obj_;
>>  		rhs.obj_ = nullptr;
>>  		return *this;
>> @@ -105,19 +135,12 @@ public:
>>  		return *obj_;
>>  	}
>> 
>> -	const SharedFD &fd() const
>> -	{
>> -		return fd_;
>> -	}
>> -
>>  	explicit operator bool() const
>>  	{
>>  		return !!obj_;
>>  	}
>> 
>>  private:
>> -	std::string name_;
>> -	SharedFD fd_;
>>  	T *obj_;
>>  };
>> 
>> [...]
>
>
> Regards,
> Barnabás Pőcze