[v7,06/18] libcamera: shared_mem_object: reorganize the code and document the SharedMemObject class

19825 diff mbox series

From: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>

The SharedMemObject class template contains a fair amount of inline code that
does not depend on the template types T. To avoid duplicating it in every
template specialization, split that code to a separate base SharedMem class.

We don't define copy semantics for the classes (we don't need one at the moment)
and we make them non-copyable since the default copy constructor would lead to
use-after-unmap.

Doxygen documentation by Dennis Bonke and Andrei Konovalov.

Reviewed-by: Pavel Machek <pavel@ucw.cz>
Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
Signed-off-by: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
---
 .../libcamera/internal/shared_mem_object.h    | 108 ++++----
 src/libcamera/meson.build                     |   1 +
 src/libcamera/shared_mem_object.cpp           | 244 ++++++++++++++++++
 3 files changed, 303 insertions(+), 50 deletions(-)
 create mode 100644 src/libcamera/shared_mem_object.cpp

Hi Milan and Andrei,

Thank you for the patch.

On Thu, Apr 04, 2024 at 10:46:43AM +0200, Milan Zamazal wrote:
> From: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
> 
> The SharedMemObject class template contains a fair amount of inline code that

The standard line length for commit messages is 72. Out of curiosity,
are you using an editor that has a different setting by default ?

> does not depend on the template types T. To avoid duplicating it in every
> template specialization, split that code to a separate base SharedMem class.
> 
> We don't define copy semantics for the classes (we don't need one at the moment)
> and we make them non-copyable since the default copy constructor would lead to
> use-after-unmap.
> 
> Doxygen documentation by Dennis Bonke and Andrei Konovalov.
> 
> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  .../libcamera/internal/shared_mem_object.h    | 108 ++++----
>  src/libcamera/meson.build                     |   1 +
>  src/libcamera/shared_mem_object.cpp           | 244 ++++++++++++++++++
>  3 files changed, 303 insertions(+), 50 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 8f2e7120..b2ab9ad1 100644
> --- a/include/libcamera/internal/shared_mem_object.h
> +++ b/include/libcamera/internal/shared_mem_object.h
> @@ -1,85 +1,103 @@
>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
>  /*
> - * Copyright (C) 2023, Raspberry Pi Ltd
> + * Copyright (C) 2023 Raspberry Pi Ltd
> + * Copyright (C) 2024 Andrei Konovalov
> + * Copyright (C) 2024 Dennis Bonke
>   *
> - * shared_mem_object.h - Helper class for shared memory allocations
> + * shared_mem_object.h - Helpers for shared memory allocations
>   */
>  #pragma once
>  
> -#include <fcntl.h>
>  #include <stddef.h>
> +#include <stdint.h>
>  #include <string>
>  #include <sys/mman.h>
> -#include <sys/stat.h>
> -#include <unistd.h>
> +#include <type_traits>
>  #include <utility>
>  
>  #include <libcamera/base/class.h>
>  #include <libcamera/base/shared_fd.h>
> +#include <libcamera/base/span.h>
>  
>  namespace libcamera {
>  
> -template<class T>
> -class SharedMemObject
> +class SharedMem
>  {
>  public:
> -	static constexpr std::size_t size = sizeof(T);
> +	using span = Span<uint8_t>;

The type would need to be named Span, but I think it should be dropped,
it doesn't seem to increase readability to me.

>  
> -	SharedMemObject()
> -		: obj_(nullptr)
> +	SharedMem()
> +		: mem_(span())

There's no need for an explicit initializer for mem_. You're
initializing it to a default-constructed value, which is exactly what
the default constructor would do. You can write

	SharedMem();

here, and

SharedMem::SharedMem() = default;

in the .cpp file. This is one less user of the span type. The other two
in this file will need to spell out Span<uint8_t> explicitly.

>  	{
>  	}
>  
> -	template<class... Args>
> -	SharedMemObject(const std::string &name, Args &&...args)
> -		: name_(name), obj_(nullptr)
> +	SharedMem(const std::string &name, std::size_t size);
> +	SharedMem(SharedMem &&rhs);
> +
> +	virtual ~SharedMem();
> +
> +	SharedMem &operator=(SharedMem &&rhs);
> +
> +	const SharedFD &fd() const
>  	{
> -		void *mem;
> -		int ret;
> +		return fd_;
> +	}
>  
> -		ret = memfd_create(name_.c_str(), MFD_CLOEXEC);
> -		if (ret < 0)
> -			return;
> +	span mem() const
> +	{
> +		return mem_;
> +	}
>  
> -		fd_ = SharedFD(std::move(ret));
> -		if (!fd_.isValid())
> -			return;
> +	explicit operator bool() const
> +	{
> +		return !mem_.empty();
> +	}
>  
> -		ret = ftruncate(fd_.get(), size);
> -		if (ret < 0)
> -			return;
> +private:
> +	LIBCAMERA_DISABLE_COPY(SharedMem)
> +
> +	SharedFD fd_;
> +
> +	span mem_;
> +};
> +
> +template<class T, typename = std::enable_if_t<std::is_standard_layout<T>::value>>
> +class SharedMemObject : public SharedMem
> +{
> +public:
> +	static constexpr std::size_t size = sizeof(T);

s/size/kSize/

> +
> +	SharedMemObject()
> +		: SharedMem(), obj_(nullptr)
> +	{
> +	}
>  
> -		mem = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED,
> -			   fd_.get(), 0);
> -		if (mem == MAP_FAILED)
> +	template<class... Args>
> +	SharedMemObject(const std::string &name, Args &&...args)
> +		: SharedMem(name, size), obj_(nullptr)
> +	{
> +		if (mem().empty())
>  			return;
>  
> -		obj_ = new (mem) T(std::forward<Args>(args)...);
> +		obj_ = new (mem().data()) T(std::forward<Args>(args)...);
>  	}
>  
>  	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. */
> -	LIBCAMERA_DISABLE_COPY(SharedMemObject)
> -
>  	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 +123,9 @@ public:
>  		return *obj_;
>  	}
>  
> -	const SharedFD &fd() const
> -	{
> -		return fd_;
> -	}
> -
> -	explicit operator bool() const
> -	{
> -		return !!obj_;
> -	}
> -
>  private:
> -	std::string name_;
> -	SharedFD fd_;
> +	LIBCAMERA_DISABLE_COPY(SharedMemObject)
> +
>  	T *obj_;
>  };
>  
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index dd8107fa..ce31180b 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -39,6 +39,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..210a7a37
> --- /dev/null
> +++ b/src/libcamera/shared_mem_object.cpp
> @@ -0,0 +1,244 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023 Raspberry Pi Ltd
> + * Copyright (C) 2024 Andrei Konovalov
> + * Copyright (C) 2024 Dennis Bonke
> + * Copyright (C) 2024 Ideas on Board Oy
> + *
> + * shared_mem_object.cpp - Helpers for shared memory allocations
> + */
> +
> +#include "libcamera/internal/shared_mem_object.h"
> +
> +#include <stddef.h>
> +#include <stdint.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include <libcamera/base/shared_fd.h>

No need to include shared_fd.h, the header file does so.

> +
> +/**
> + * \file shared_mem_object.cpp
> + * \brief Helpers for shared memory allocations
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class SharedMem
> + * \brief Helper class to allocate and manage memory shareable between processes
> + *
> + * SharedMem manages memory suitable for sharing between processes. When an
> + * instance is constructed, it allocates a memory buffer of the requested size
> + * backed by an anonymous file, using the memfd API.
> + *
> + * The allocated memory is exposed by the mem() function. If memory allocation
> + * fails, the function returns an empty Span. This can be also checked using the
> + * bool() operator.
> + *
> + * The file descriptor for the backing file is exposed as a SharedFD by the fd()
> + * function. It can be shared with other processes across IPC boundaries, which
> + * can then map the memory with 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.
> + */
> +
> +/**
> + * \typedef SharedMem::span
> + * \brief Type of the Span wrapping the memory pointer
> + */
> +
> +/**
> + * \brief Construct a SharedMem with memory of the given \a size
> + * \param[in] name Name of the SharedMem
> + * \param[in] size Size of the shared memory to allocate and map
> + *
> + * The \a name is used for debugging purpose only. Multiple SharedMem instances
> + * can have the same name.
> + */
> +SharedMem::SharedMem(const std::string &name, std::size_t size)
> +	: mem_(span())

You can drop the explicit initialization here too.

> +{
> +	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) {
> +		fd_ = SharedFD();
> +		return;
> +	}
> +
> +	mem_ = span{
> +		static_cast<uint8_t *>(
> +			mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED,
> +			     fd_.get(), 0)),
> +		size
> +	};

That's hard to read.

	void *mem = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED,
			 fd_.get(), 0);
	if (mem == MAP_FAILED) {
		fd_ = SharedFD();
		return;
	}

	mem_ = { static_cast<uint8_t *>(mem), size };

One less user of the span type :-)

> +	if (mem_.data() == MAP_FAILED) {
> +		mem_ = span();
> +		fd_ = SharedFD();
> +	}
> +}
> +
> +/**
> + * \brief Move constructor for SharedMem
> + * \param[in] rhs The object to move
> + */
> +SharedMem::SharedMem(SharedMem &&rhs)
> +{
> +	this->fd_ = std::move(rhs.fd_);
> +	this->mem_ = rhs.mem_;
> +	rhs.mem_ = span();

	rhs.mem_ = {};

and similarly for all the span type users below.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +}
> +
> +/**
> + * \brief Destroy the SharedMem instance
> + *
> + * Destroying an instance invalidates the memory mapping exposed with mem().
> + * Other mappings of the backing file, created in this or other processes with
> + * mmap(), remain valid.
> + *
> + * Similarly, other references to the backing file descriptor created by copying
> + * the SharedFD returned by fd() remain valid. The underlying memory will be
> + * freed only when all file descriptors that reference the anonymous file get
> + * closed.
> + */
> +SharedMem::~SharedMem()
> +{
> +	if (!mem_.empty())
> +		munmap(mem_.data(), mem_.size_bytes());
> +}
> +
> +/**
> + * \brief Move assignment operator for SharedMem
> + * \param[in] rhs The object to move
> + */
> +SharedMem &SharedMem::operator=(SharedMem &&rhs)
> +{
> +	this->fd_ = std::move(rhs.fd_);
> +	this->mem_ = rhs.mem_;
> +	rhs.mem_ = span();
> +	return *this;
> +}
> +
> +/**
> + * \fn const SharedFD &SharedMem::fd() const
> + * \brief Retrieve the file descriptor for the underlying shared memory
> + * \return The file descriptor, or an invalid SharedFD if allocation failed
> + */
> +
> +/**
> + * \fn span SharedMem::mem() const
> + * \brief Retrieve the underlying shared memory
> + * \return The memory buffer, or an empty span if allocation failed
> + */
> +
> +/**
> + * \fn SharedMem::operator bool()
> + * \brief Check if the shared memory allocation succeeded
> + * \return True if allocation of the shared memory succeeded, false otherwise
> + */
> +
> +/**
> + * \class SharedMemObject
> + * \brief Helper class to allocate an object in shareable memory
> + * \tparam The object type
> + *
> + * The SharedMemObject class is a specialization of the SharedMem class that
> + * wraps an object of type \a T and constructs it in shareable memory. It uses
> + * the same underlying memory allocation and sharing mechanism as the SharedMem
> + * class.
> + *
> + * The wrapped object is constructed at the same time as the SharedMemObject
> + * instance, by forwarding the arguments passed to the SharedMemObject
> + * constructor. The underlying memory allocation is sized to the object \a T
> + * size. The bool() operator should be used to check the allocation was
> + * successful. The object can be accessed using the dereference operators
> + * operator*() and operator->().
> + *
> + * While no restriction on the type \a T is enforced, not all types are suitable
> + * for sharing between multiple processes. Most notably, any object type that
> + * contains pointer or reference members will likely cause issues. Even if those
> + * members refer to other members of the same object, the shared memory will be
> + * mapped at different addresses in different processes, and the pointers will
> + * not be valid.
> + *
> + * A new anonymous file is created for every SharedMemObject instance. If there
> + * is a need to share a large number of small objects, these objects should be
> + * grouped into a single larger object to limit the number of file descriptors.
> + *
> + * To share the object with other processes, see the SharedMem documentation.
> + */
> +
> +/**
> + * \var SharedMemObject::size
> + * \brief The size of the object stored in shared memory
> + */
> +
> +/**
> + * \fn SharedMemObject::SharedMemObject(const std::string &name, Args &&...args)
> + * \brief Construct a SharedMemObject
> + * \param[in] name Name of the SharedMemObject
> + * \param[in] args Arguments to pass to the constructor of the object T
> + *
> + * The \a name is used for debugging purpose only. Multiple SharedMem instances
> + * can have the same name.
> + */
> +
> +/**
> + * \fn SharedMemObject::SharedMemObject(SharedMemObject<T> &&rhs)
> + * \brief Move constructor for SharedMemObject
> + * \param[in] rhs The object to move
> + */
> +
> +/**
> + * \fn SharedMemObject::~SharedMemObject()
> + * \brief Destroy the SharedMemObject instance
> + *
> + * Destroying a SharedMemObject calls the wrapped T object's destructor. While
> + * the underlying memory may not be freed immediately if other mappings have
> + * been created manually (see SharedMem::~SharedMem() for more information), the
> + * stored object may be modified. Depending on the ~T() destructor, accessing
> + * the object after destruction of the SharedMemObject causes undefined
> + * behaviour. It is the responsibility of the user of this class to synchronize
> + * with other users who have access to the shared object.
> + */
> +
> +/**
> + * \fn SharedMemObject::operator=(SharedMemObject<T> &&rhs)
> + * \brief Move assignment operator for SharedMemObject
> + * \param[in] rhs The SharedMemObject object to take the data from
> + *
> + * Moving a SharedMemObject does not affect the stored object.
> + */
> +
> +/**
> + * \fn SharedMemObject::operator->()
> + * \brief Dereference the stored object
> + * \return Pointer to the stored object
> + */
> +
> +/**
> + * \fn const T *SharedMemObject::operator->() const
> + * \copydoc SharedMemObject::operator->
> + */
> +
> +/**
> + * \fn SharedMemObject::operator*()
> + * \brief Dereference the stored object
> + * \return Reference to the stored object
> + */
> +
> +/**
> + * \fn const T &SharedMemObject::operator*() const
> + * \copydoc SharedMemObject::operator*
> + */
> +
> +} /* namespace libcamera */

Hi Laurent,

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> Hi Milan and Andrei,
>
> Thank you for the patch.
>
> On Thu, Apr 04, 2024 at 10:46:43AM +0200, Milan Zamazal wrote:
>> From: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
>> 
>> The SharedMemObject class template contains a fair amount of inline code that
>
> The standard line length for commit messages is 72. Out of curiosity,
> are you using an editor that has a different setting by default ?

Yes.  Magit used to set it to 72 but due to complaints from users who have
different project policies, it stopped doing so and leaves it at whatever value
is set.  I'll change it in my environment to 72 for libcamera commit messages.

>> does not depend on the template types T. To avoid duplicating it in every
>> template specialization, split that code to a separate base SharedMem class.
>> 
>> We don't define copy semantics for the classes (we don't need one at the moment)
>> and we make them non-copyable since the default copy constructor would lead to
>> use-after-unmap.
>> 
>> Doxygen documentation by Dennis Bonke and Andrei Konovalov.
>> 
>> Reviewed-by: Pavel Machek <pavel@ucw.cz>
>> Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
>> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
>> Signed-off-by: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  .../libcamera/internal/shared_mem_object.h    | 108 ++++----
>>  src/libcamera/meson.build                     |   1 +
>>  src/libcamera/shared_mem_object.cpp           | 244 ++++++++++++++++++
>>  3 files changed, 303 insertions(+), 50 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 8f2e7120..b2ab9ad1 100644
>> --- a/include/libcamera/internal/shared_mem_object.h
>> +++ b/include/libcamera/internal/shared_mem_object.h
>> @@ -1,85 +1,103 @@
>>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
>>  /*
>> - * Copyright (C) 2023, Raspberry Pi Ltd
>> + * Copyright (C) 2023 Raspberry Pi Ltd
>> + * Copyright (C) 2024 Andrei Konovalov
>> + * Copyright (C) 2024 Dennis Bonke
>>   *
>> - * shared_mem_object.h - Helper class for shared memory allocations
>> + * shared_mem_object.h - Helpers for shared memory allocations
>>   */
>>  #pragma once
>>  
>> -#include <fcntl.h>
>>  #include <stddef.h>
>> +#include <stdint.h>
>>  #include <string>
>>  #include <sys/mman.h>
>> -#include <sys/stat.h>
>> -#include <unistd.h>
>> +#include <type_traits>
>>  #include <utility>
>>  
>>  #include <libcamera/base/class.h>
>>  #include <libcamera/base/shared_fd.h>
>> +#include <libcamera/base/span.h>
>>  
>>  namespace libcamera {
>>  
>> -template<class T>
>> -class SharedMemObject
>> +class SharedMem
>>  {
>>  public:
>> -	static constexpr std::size_t size = sizeof(T);
>> +	using span = Span<uint8_t>;
>
> The type would need to be named Span, but I think it should be dropped,
> it doesn't seem to increase readability to me.

I haven't done this for readability but to have the type specification in a
single place.  But yes, in this case the type is given and if we decide to
change it to something else (e.g. C++20 std::span) then it'll be handled by a
mass update.  So I can drop the definition.