[PATCH v6 05/18] libcamera: shared_mem_object: reorganize the code and document the SharedMemObject class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Mar 27 13:57:01 CET 2024
Hi Milan and Andrey,
Thank you for the patch.
On Tue, Mar 19, 2024 at 01:35:52PM +0100, Milan Zamazal wrote:
> 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.
The commit message should explain why.
> 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 | 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
>
> diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h
> index 98636b44..43b07c9d 100644
> --- a/include/libcamera/internal/shared_mem_object.h
> +++ b/include/libcamera/internal/shared_mem_object.h
> @@ -6,12 +6,9 @@
> */
> #pragma once
>
> -#include <fcntl.h>
> #include <stddef.h>
> #include <string>
> #include <sys/mman.h>
> -#include <sys/stat.h>
> -#include <unistd.h>
> #include <utility>
>
> #include <libcamera/base/class.h>
> @@ -19,58 +16,92 @@
>
> 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;
> + }
> +
> + virtual ~SharedMem()
> + {
> + if (mem_)
> + munmap(mem_, size_);
> + }
I think neither of these need to be inline. Same for operator=().
> +
> + /* Make SharedMem non-copyable for now. */
Record the reason in the commit message, and drop this comment.
> + LIBCAMERA_DISABLE_COPY(SharedMem)
This goes in the private: section.
> +
> + 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
Make this return a Span<uint8_t>. Naked pointers lead to out-of-bound
access.
> + {
> + return mem_;
> + }
> +
> +private:
> + std::string name_;
name_ is set in the constructor and then never used. I would drop it.
> + SharedFD fd_;
> + size_t size_;
Missing blank line.
> +protected:
> + void *mem_;
We put protected before private, but it seems mem_ can be private as
it's exposed by the mem() function, and classes that inherit SharedMem
shouldn't have a need to set mem_. You can then store mem_ and size_ as
a Span instead of separate members.
> +};
> +
> template<class T>
Should we limit this to standard-layout types
(https://en.cppreference.com/w/cpp/types/is_standard_layout) ? Ideally
we should forbid usage of types that contain pointers, as they can't be
shared between processes, but there's no corresponding C++ type trait as
far as I can tell.
> -class SharedMemObject
> +class SharedMemObject : public SharedMem
> {
> public:
> static constexpr std::size_t SIZE = sizeof(T);
s/SIZE/kSize/
It is probably best done in a separate patch (before or after this one,
probably before is better).
>
> 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)
> - return;
> -
> - fd_ = SharedFD(std::move(ret));
> - if (!fd_.isValid())
> - return;
> -
> - ret = ftruncate(fd_.get(), SIZE);
> - if (ret < 0)
> + if (mem_ == nullptr)
> 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)...);
> }
>
> 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. */
This also goes to the private: section.
> @@ -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
This seems to be a candidate for the base class, it doesn't depend on
the type T.
> {
> return !!obj_;
> }
>
> private:
> - std::string name_;
> - SharedFD fd_;
> T *obj_;
Do we need to store obj_, or could we always cast the
> };
>
> 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..44fe74c2
> --- /dev/null
> +++ b/src/libcamera/shared_mem_object.cpp
> @@ -0,0 +1,190 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Raspberry Pi Ltd
The documentation is Dennis and Andrei's work, I don't think this should
be owned by Raspberry Pi.
> + *
> + * 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
s/class/classes/, or maybe better, just "Helpers for shared memory
allocations".
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class SharedMem
> + * \brief Helper class for allocating shared memory
* \brief Helper class to allocate and manage memory shareable between processes
> + *
> + * 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.
memfd comes a bit out of the blue here. Let me try to improve the
documentation:
* 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.
*
* 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.
> + */
> +
> +/**
> + * \fn SharedMem::SharedMem(const std::string &name, std::size_t size)
> + * \brief Constructor for the SharedMem
* \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.
> + */
> +
> +/**
> + * \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()
* \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.
*/
> + */
> +
> +/**
> + * \fn SharedMem &SharedMem::operator=(SharedMem &&rhs)
> + * \brief Move constructor for SharedMem
s/constructor/assignment operator/
> + * \param[in] rhs The object to move
> + */
> +
> +/**
> + * \fn const SharedFD &SharedMem::fd() const
> + * \brief Gets the file descriptor for the underlying shared memory
s/Gets/Retrieve/
> + * \return The file descriptor
* \return The file descriptor, or an invalid SharedFD if allocation failed
> + */
> +
> +/**
> + * \fn void *SharedMem::mem() const
> + * \brief Gets the pointer to the underlying shared memory
> + * \return The pointer to the shared memory
* \fn Span<uint8_t> SharedMem::mem() const
* \brief Retrieve the underlying shared memory
* \return The memory buffer, or an empty span if allocation failed
> + */
> +
> +SharedMem::SharedMem(const std::string &name, std::size_t size)
> + : name_(name), size_(size), mem_(nullptr)
This should come right after the corresponding documentation block, and
you can then delete the \fn line of that block.
> +{
> + 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;
Should you clear fd_ here ? It seems pointless to keep it open, and the
class would expose consistent information.
> +
Should we set the GROW and SHRINK seals (in a separate patch) ?
> + mem_ = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED,
> + fd_.get(), 0);
> + if (mem_ == MAP_FAILED)
> + mem_ = nullptr;
Same here, should you clear fd_ ?
> +}
> +
> +/**
> + * \var SharedMem::mem_
> + * \brief Pointer to the shared memory allocated
> + */
> +
> +/**
> + * \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 ensure
> + * that the SharedMemObject was created successfully, one needs to verify that the
> + * overloaded bool() operator returns true. The object created in the shared memory
The part about the bool() operator should be moved to SharedMem.
> + * can be accessed using the SharedMemObject::operator*() indirection operator. Its
> + * members can be accessed with the SharedMemObject::operator->() member of pointer
> + * operator.
> + *
> + * 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.
* \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 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 that is going to be stored here
* \brief The size of the object stored in shared memory
> + */
> +
> +/**
> + * \fn SharedMemObject< T >::SharedMemObject(const std::string &name, Args &&...args)
I think you can drop < T > here (or it's missing everywhere below).
> + * \brief Constructor for the SharedMemObject
* \brief Construct a SharedMemObject
> + * \param[in] name Name of the SharedMemObject
> + * \param[in] args Args to pass to the constructor of the object in shared memory
s/Args/Arguments/
s/in shared memory/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 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::~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 Operator= for SharedMemObject
* \brief Move assignment operator for SharedMemObject
> + * \param[in] rhs The SharedMemObject object to take the data from
*
* Moving a SharedMemObject does not affect in stored object.
> + */
> +
> +/**
> + * \fn SharedMemObject::operator->()
> + * \brief Operator-> for SharedMemObject
> + * \return The pointer to the object
* \brief Dereference the stored object
* \return Pointer to the stored object
> + */
> +
> +/**
> + * \fn const T *SharedMemObject::operator->() const
> + * \brief Operator-> for SharedMemObject
> + * \return The pointer to the const object
Same here. You use \copydoc to avoid duplicating the documentation,
there are a few examples in the code base. You may need to use copydoc
in the non-const version instead of here though, no sure.
> + */
> +
> +/**
> + * \fn SharedMemObject::operator*()
> + * \brief Operator* for SharedMemObject
> + * \return The reference to the object
* \brief Dereference the stored object
* \return Reference to the stored object
Same comment regarding copydoc.
> + */
> +
> +/**
> + * \fn const T &SharedMemObject::operator*() const
> + * \brief Operator* for SharedMemObject
> + * \return Const reference to the object
> + */
> +
> +/**
> + * \fn SharedMemObject::operator bool()
> + * \brief Operator bool() for SharedMemObject
> + * \return True if the object was created OK in the shared memory, false otherwise
Assuming you'll move this to SharedMem,
* \brief Check if the shared memory allocation succeeded
* \return True if allocation of the shared memorysucceeded, false otherwise
> + */
> +
> +} // namespace libcamera
/* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list