[PATCH v3 05/16] libcamera: shared_mem_object: reorganize the code and document the SharedMemObject class

[Milan Zamazal]

Hans de Goede <hdegoede at redhat.com> writes:

> 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>
> ---
>  .../libcamera/internal/shared_mem_object.h    |  92 ++++++---
>  src/libcamera/meson.build                     |   1 +
>  src/libcamera/shared_mem_object.cpp           | 191 ++++++++++++++++++
>  3 files changed, 253 insertions(+), 31 deletions(-)
>  create mode 100644 src/libcamera/shared_mem_object.cpp
>
> diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h
> index bfb639ee..a4de6500 100644
> --- a/include/libcamera/internal/shared_mem_object.h
> +++ b/include/libcamera/internal/shared_mem_object.h
> @@ -7,11 +7,8 @@
>  #pragma once
>  
>  #include <cstddef>
> -#include <fcntl.h>
>  #include <string>
>  #include <sys/mman.h>
> -#include <sys/stat.h>
> -#include <unistd.h>
>  #include <utility>
>  
>  #include <libcamera/base/class.h>
> @@ -19,6 +16,59 @@
>  
>  namespace libcamera {
>  
> +class SharedMem
> +{
> +public:
> +	SharedMem()
> +		: mem_(nullptr)
> +	{
> +	}
> +
> +	SharedMem(const std::string &name, std::size_t size);
> +
> +	SharedMem(SharedMem &&rhs)
> +	{
> +		this->name_ = std::move(rhs.name_);
> +		this->fd_ = std::move(rhs.fd_);
> +		this->mem_ = rhs.mem_;
> +		rhs.mem_ = nullptr;
> +	}
> +
> +	~SharedMem()
> +	{
> +		if (mem_)
> +			munmap(mem_, size_);
> +	}
> +
> +	/* Make SharedMem non-copyable for now. */
> +	LIBCAMERA_DISABLE_COPY(SharedMem)
> +
> +	SharedMem &operator=(SharedMem &&rhs)
> +	{
> +		this->name_ = std::move(rhs.name_);
> +		this->fd_ = std::move(rhs.fd_);
> +		this->mem_ = rhs.mem_;
> +		rhs.mem_ = nullptr;
> +		return *this;
> +	}
> +
> +	const SharedFD &fd() const
> +	{
> +		return fd_;
> +	}
> +
> +	void *mem() const
> +	{
> +		return mem_;
> +	}
> +
> +private:
> +	std::string name_;
> +	SharedFD fd_;
> +	size_t size_;
> +	void *mem_;
> +};
> +
>  template<class T>
>  class SharedMemObject
>  {
> @@ -32,26 +82,11 @@ public:
>  
>  	template<class... Args>
>  	SharedMemObject(const std::string &name, Args &&...args)
> -		: name_(name), obj_(nullptr)
> +		: shMem_(name, SIZE), obj_(nullptr)
>  	{
> -		void *mem;
> -		int ret;
> +		void *mem = shMem_.mem();
>  
> -		ret = memfd_create(name_.c_str(), MFD_CLOEXEC);
> -		if (ret < 0)
> -			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)
> +		if (mem == nullptr)
>  			return;
>  
>  		obj_ = new (mem) T(std::forward<Args>(args)...);
> @@ -59,18 +94,15 @@ public:
>  
>  	SharedMemObject(SharedMemObject<T> &&rhs)
>  	{
> -		this->name_ = std::move(rhs.name_);
> -		this->fd_ = std::move(rhs.fd_);
> +		this->shMem_ = std::move(rhs.shMem_);
>  		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 +110,7 @@ public:
>  
>  	SharedMemObject<T> &operator=(SharedMemObject<T> &&rhs)
>  	{
> -		this->name_ = std::move(rhs.name_);
> -		this->fd_ = std::move(rhs.fd_);
> +		this->shMem_ = std::move(rhs.shMem_);
>  		this->obj_ = rhs.obj_;
>  		rhs.obj_ = nullptr;
>  		return *this;
> @@ -107,7 +138,7 @@ public:
>  
>  	const SharedFD &fd() const
>  	{
> -		return fd_;
> +		return shMem_.fd();
>  	}
>  
>  	explicit operator bool() const
> @@ -116,8 +147,7 @@ public:
>  	}
>  
>  private:
> -	std::string name_;
> -	SharedFD fd_;
> +	SharedMem shMem_;
>  	T *obj_;
>  };
>  
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 3c5e43df..94a95ae3 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -41,6 +41,7 @@ libcamera_sources = files([
>      'process.cpp',
>      'pub_key.cpp',
>      'request.cpp',
> +    'shared_mem_object.cpp',
>      'source_paths.cpp',
>      'stream.cpp',
>      'sysfs.cpp',
> diff --git a/src/libcamera/shared_mem_object.cpp b/src/libcamera/shared_mem_object.cpp
> new file mode 100644
> index 00000000..06bbee38
> --- /dev/null
> +++ b/src/libcamera/shared_mem_object.cpp
> @@ -0,0 +1,191 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Raspberry Pi Ltd
> + *
> + * shared_mem_object.cpp - Helper class for shared memory allocations
> + */
> +
> +#include "libcamera/internal/shared_mem_object.h"
> +
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +/**
> + * \file shared_mem_object.cpp
> + * \brief Helper class for shared memory allocations
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class SharedMem
> + * \brief Helper class for allocating shared memory
> + *
> + * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.
> + *
> + * SharedMem allocates the shared memory of the given size and maps it.
> + * To check that the shared memory was allocated and mapped successfully, one
> + * needs to verify that the pointer to the shared memory returned by SharedMem::mem()
> + * is not nullptr.
> + *
> + * To access the shared memory from another process the SharedFD should be passed
> + * to that process, and then the shared memory should be mapped into that process
> + * address space by calling mmap().
> + *
> + * A single memfd is created for every SharedMem. If there is a need to allocate
> + * a large number of objects in shared memory, these objects should be grouped
> + * together and use the shared memory allocated by a single SharedMem object if
> + * possible. This will help to minimize the number of created memfd's.
> + */
> +
> +/**
> + * \fn SharedMem::SharedMem(const std::string &name, std::size_t size)
> + * \brief Contstructor for the SharedMem
> + * \param[in] name Name of the SharedMem
> + * \param[in] size Size of the shared memory to allocate and map
> + */
> +
> +/**
> + * \fn SharedMem::SharedMem(SharedMem &&rhs)
> + * \brief Move constructor for SharedMem
> + * \param[in] rhs The object to move
> + */
> +
> +/**
> + * \fn SharedMem::~SharedMem()
> + * \brief SharedMem destructor
> + *
> + * Unmaps the allocated shared memory. Decrements the shared memory descriptor use
> + * count.
> + */
> +
> +/**
> + * \fn SharedMem &SharedMem::operator=(SharedMem &&rhs)
> + * \brief Move constructor for SharedMem
> + * \param[in] rhs The object to move
> + */
> +
> +/**
> + * \fn const SharedFD &SharedMem::fd() const
> + * \brief Gets the file descriptor for the underlaying shared memory
> + * \return The file descriptor
> + */
> +
> +/**
> + * \fn void *SharedMem::mem() const
> + * \brief Gets the pointer to the underlaying shared memory
> + * \return The pointer to the shared memory
> + */
> +
> +SharedMem::SharedMem(const std::string &name, std::size_t size)
> +	: name_(name), size_(size), mem_(nullptr)
> +{
> +	int fd = memfd_create(name_.c_str(), MFD_CLOEXEC);
> +	if (fd < 0)
> +		return;
> +
> +	fd_ = SharedFD(std::move(fd));
> +	if (!fd_.isValid())
> +		return;
> +
> +	if (ftruncate(fd_.get(), size_) < 0)
> +		return;
> +
> +	mem_ = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED,
> +		    fd_.get(), 0);
> +	if (mem_ == MAP_FAILED)
> +		mem_ = nullptr;
> +}
> +
> +/**
> + * \class SharedMemObject
> + * \brief Helper class for allocating objects in shared memory
> + *
> + * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.
> + *
> + * Given the type of the object to be created in shared memory and the arguments
> + * to pass to this object's constructor, SharedMemObject allocates the shared memory
> + * of the size of the object and constructs the object in this memory. To check that
> + * the SharedMemObject was created successfully, one needs to verify that the
> + * underlying SharedFD (the reference to it is returned by SharedMemObject::fd() member
> + * function) is valid. The object created in the shared memory can be accessed using
> + * the SharedMemObject::operator*() indirection operator. Its members can be accessed
> + * with the SharedMemObject::operator->() member of pointer operator.

But it can still happen that SharedMemObject::fd() is all right but the object
returned from the operators is nullptr, right?  It would be good to clarify this
and refer to the bool operator here.

> 
> + *
> + * To access the object from another process the SharedFD should be passed to that
> + * process, and the shared memory should be mapped by calling mmap().
> + *
> + * A single memfd is created for every SharedMemObject. If there is a need to allocate
> + * a large number of objects in shared memory, these objects should be grouped into a
> + * single large object to keep the number of created memfd's reasonably small.
> + */
> +
> +/**
> + * \var SharedMemObject::SIZE
> + * \brief The size of the object that is going to be stored here
> + */
> +
> +/**
> + * \fn SharedMemObject< T >::SharedMemObject(const std::string &name, Args &&...args)
> + * \brief Contstructor for the SharedMemObject
             ^^^^^^^^^^^^

Constructor

> + * \param[in] name Name of the SharedMemObject
> + * \param[in] args Args to pass to the constructor of the object in shared memory
> + */
> +
> +/**
> + * \fn SharedMemObject::SharedMemObject(SharedMemObject<T> &&rhs)
> + * \brief Move constructor for SharedMemObject
> + * \param[in] rhs The object to move
> + */
> +
> +/**
> + * \fn SharedMemObject::~SharedMemObject()
> + * \brief SharedMemObject destructor
> + *
> + * Destroys the object created in the shared memory and then unmaps the shared memory.
> + * Decrements the shared memory descriptor use count.
> + */
> +
> +/**
> + * \fn SharedMemObject::operator=(SharedMemObject<T> &&rhs)
> + * \brief Operator= for SharedMemObject
> + * \param[in] rhs The SharedMemObject object to take the data from
> + */
> +
> +/**
> + * \fn SharedMemObject::operator->()
> + * \brief Operator-> for SharedMemObject
> + * \return The pointer to the object
> + */
> +
> +/**
> + * \fn const T *SharedMemObject::operator->() const
> + * \brief Operator-> for SharedMemObject
> + * \return The pointer to the const object
> + */
> +
> +/**
> + * \fn SharedMemObject::operator*()
> + * \brief Operator* for SharedMemObject
> + * \return The reference to the object
> + */
> +
> +/**
> + * \fn const T &SharedMemObject::operator*() const
> + * \brief Operator* for SharedMemObject
> + * \return Const reference to the object
> + */
> +
> +/**
> + * \fn SharedMemObject::fd()
> + * \brief Gets the file descriptor for the underlaying storage file
> + * \return The shared memory file descriptor (as SharedFD)
> + */
> +
> +/**
> + * \fn SharedMemObject::operator bool()
> + * \brief Operator bool() for SharedMemObject
> + * \return True if the object was created OK in the shared memory, false otherwise
> + */
> +
> +} // namespace libcamera