[PATCH v3 05/16] libcamera: shared_mem_object: reorganize the code and document the SharedMemObject class
Andrei Konovalov
andrey.konovalov.ynk at gmail.com
Sun Feb 18 20:22:54 CET 2024
Hi Milan,
Thank you for the review!
On 15.02.2024 16:30, Milan Zamazal wrote:
> 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?
Right (if the memory is allocated and truncated OK, but mmap fails).
> It would be good to clarify this and refer to the bool operator here.
I will.
I'll also modify the SoftwareIsp constructor to use 'if (!sharedParams_)' instead of
'if (!sharedParams_.fd().isValid())'.
>> + *
>> + * 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
OK, I'll fix these misprints (they are two).
Thanks,
Andrei
>> + * \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
>
More information about the libcamera-devel
mailing list