[PATCH v7 06/18] libcamera: shared_mem_object: reorganize the code and document the SharedMemObject class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Apr 14 23:47:55 CEST 2024
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 at 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 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>
> Reviewed-by: Milan Zamazal <mzamazal at 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 at 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 */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list