[{"id":28930,"web_url":"https://patchwork.libcamera.org/comment/28930/","msgid":"<87cyrz4nbl.fsf@redhat.com>","date":"2024-03-12T13:00:30","subject":"Re: [PATCH v5 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hans de Goede writes:\n\n> From: Andrei Konovalov \n>\n> Split the parts which doesn't otherwise depend on the type T or\n> arguments Args out of the SharedMemObject class into a new\n> SharedMem class.\n>\n> Doxygen documentation by Dennis Bonke and Andrei Konovalov.\n>\n> Reviewed-by: Pavel Machek \n> Co-developed-by: Dennis Bonke \n> Signed-off-by: Dennis Bonke \n> Signed-off-by: Andrei Konovalov \n> Signed-off-by: Hans de Goede \n\nReviewed-by: Milan Zamazal \n\n> ---\n> .../libcamera/internal/shared_mem_object.h | 101 ++++++----\n> src/libcamera/meson.build | 1 +\n> src/libcamera/shared_mem_object.cpp | 190 ++++++++++++++++++\n> 3 files changed, 253 insertions(+), 39 deletions(-)\n> create mode 100644 src/libcamera/shared_mem_object.cpp\n>\n> diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h\n> index 98636b44..43b07c9d 100644\n> --- a/include/libcamera/internal/shared_mem_object.h\n> +++ b/include/libcamera/internal/shared_mem_object.h\n> @@ -6,12 +6,9 @@\n> */\n> #pragma once\n> \n> -#include \n> #include \n> #include \n> #include \n> -#include \n> -#include \n> #include \n> \n> #include \n> @@ -19,58 +16,92 @@\n> \n> namespace libcamera {\n> \n> +class SharedMem\n> +{\n> +public:\n> +\tSharedMem()\n> +\t\t: mem_(nullptr)\n> +\t{\n> +\t}\n> +\n> +\tSharedMem(const std::string &name, std::size_t size);\n> +\n> +\tSharedMem(SharedMem &&rhs)\n> +\t{\n> +\t\tthis->name_ = std::move(rhs.name_);\n> +\t\tthis->fd_ = std::move(rhs.fd_);\n> +\t\tthis->mem_ = rhs.mem_;\n> +\t\trhs.mem_ = nullptr;\n> +\t}\n> +\n> +\tvirtual ~SharedMem()\n> +\t{\n> +\t\tif (mem_)\n> +\t\t\tmunmap(mem_, size_);\n> +\t}\n> +\n> +\t/* Make SharedMem non-copyable for now. */\n> +\tLIBCAMERA_DISABLE_COPY(SharedMem)\n> +\n> +\tSharedMem &operator=(SharedMem &&rhs)\n> +\t{\n> +\t\tthis->name_ = std::move(rhs.name_);\n> +\t\tthis->fd_ = std::move(rhs.fd_);\n> +\t\tthis->mem_ = rhs.mem_;\n> +\t\trhs.mem_ = nullptr;\n> +\t\treturn *this;\n> +\t}\n> +\n> +\tconst SharedFD &fd() const\n> +\t{\n> +\t\treturn fd_;\n> +\t}\n> +\n> +\tvoid *mem() const\n> +\t{\n> +\t\treturn mem_;\n> +\t}\n> +\n> +private:\n> +\tstd::string name_;\n> +\tSharedFD fd_;\n> +\tsize_t size_;\n> +protected:\n> +\tvoid *mem_;\n> +};\n> +\n> template\n> -class SharedMemObject\n> +class SharedMemObject : public SharedMem\n> {\n> public:\n> \tstatic constexpr std::size_t SIZE = sizeof(T);\n> \n> \tSharedMemObject()\n> -\t\t: obj_(nullptr)\n> +\t\t: SharedMem(), obj_(nullptr)\n> \t{\n> \t}\n> \n> \ttemplate\n> \tSharedMemObject(const std::string &name, Args &&...args)\n> -\t\t: name_(name), obj_(nullptr)\n> +\t\t: SharedMem(name, SIZE), obj_(nullptr)\n> \t{\n> -\t\tvoid *mem;\n> -\t\tint ret;\n> -\n> -\t\tret = memfd_create(name_.c_str(), MFD_CLOEXEC);\n> -\t\tif (ret < 0)\n> +\t\tif (mem_ == nullptr)\n> \t\t\treturn;\n> \n> -\t\tfd_ = SharedFD(std::move(ret));\n> -\t\tif (!fd_.isValid())\n> -\t\t\treturn;\n> -\n> -\t\tret = ftruncate(fd_.get(), SIZE);\n> -\t\tif (ret < 0)\n> -\t\t\treturn;\n> -\n> -\t\tmem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,\n> -\t\t\t fd_.get(), 0);\n> -\t\tif (mem == MAP_FAILED)\n> -\t\t\treturn;\n> -\n> -\t\tobj_ = new (mem) T(std::forward(args)...);\n> +\t\tobj_ = new (mem_) T(std::forward(args)...);\n> \t}\n> \n> \tSharedMemObject(SharedMemObject &&rhs)\n> +\t\t: SharedMem(std::move(rhs))\n> \t{\n> -\t\tthis->name_ = std::move(rhs.name_);\n> -\t\tthis->fd_ = std::move(rhs.fd_);\n> \t\tthis->obj_ = rhs.obj_;\n> \t\trhs.obj_ = nullptr;\n> \t}\n> \n> \t~SharedMemObject()\n> \t{\n> -\t\tif (obj_) {\n> +\t\tif (obj_)\n> \t\t\tobj_->~T();\n> -\t\t\tmunmap(obj_, SIZE);\n> -\t\t}\n> \t}\n> \n> \t/* Make SharedMemObject non-copyable for now. */\n> @@ -78,8 +109,7 @@ public:\n> \n> \tSharedMemObject &operator=(SharedMemObject &&rhs)\n> \t{\n> -\t\tthis->name_ = std::move(rhs.name_);\n> -\t\tthis->fd_ = std::move(rhs.fd_);\n> +\t\tSharedMem::operator=(std::move(rhs));\n> \t\tthis->obj_ = rhs.obj_;\n> \t\trhs.obj_ = nullptr;\n> \t\treturn *this;\n> @@ -105,19 +135,12 @@ public:\n> \t\treturn *obj_;\n> \t}\n> \n> -\tconst SharedFD &fd() const\n> -\t{\n> -\t\treturn fd_;\n> -\t}\n> -\n> \texplicit operator bool() const\n> \t{\n> \t\treturn !!obj_;\n> \t}\n> \n> private:\n> -\tstd::string name_;\n> -\tSharedFD fd_;\n> \tT *obj_;\n> };\n> \n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 3c5e43df..94a95ae3 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -41,6 +41,7 @@ libcamera_sources = files([\n> 'process.cpp',\n> 'pub_key.cpp',\n> 'request.cpp',\n> + 'shared_mem_object.cpp',\n> 'source_paths.cpp',\n> 'stream.cpp',\n> 'sysfs.cpp',\n> diff --git a/src/libcamera/shared_mem_object.cpp b/src/libcamera/shared_mem_object.cpp\n> new file mode 100644\n> index 00000000..44fe74c2\n> --- /dev/null\n> +++ b/src/libcamera/shared_mem_object.cpp\n> @@ -0,0 +1,190 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Raspberry Pi Ltd\n> + *\n> + * shared_mem_object.cpp - Helper class for shared memory allocations\n> + */\n> +\n> +#include \"libcamera/internal/shared_mem_object.h\"\n> +\n> +#include \n> +#include \n> +\n> +/**\n> + * \\file shared_mem_object.cpp\n> + * \\brief Helper class for shared memory allocations\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class SharedMem\n> + * \\brief Helper class for allocating shared memory\n> + *\n> + * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.\n> + *\n> + * SharedMem allocates the shared memory of the given size and maps it.\n> + * To check that the shared memory was allocated and mapped successfully, one\n> + * needs to verify that the pointer to the shared memory returned by SharedMem::mem()\n> + * is not nullptr.\n> + *\n> + * To access the shared memory from another process the SharedFD should be passed\n> + * to that process, and then the shared memory should be mapped into that process\n> + * address space by calling mmap().\n> + *\n> + * A single memfd is created for every SharedMem. If there is a need to allocate\n> + * a large number of objects in shared memory, these objects should be grouped\n> + * together and use the shared memory allocated by a single SharedMem object if\n> + * possible. This will help to minimize the number of created memfd's.\n> + */\n> +\n> +/**\n> + * \\fn SharedMem::SharedMem(const std::string &name, std::size_t size)\n> + * \\brief Constructor for the SharedMem\n> + * \\param[in] name Name of the SharedMem\n> + * \\param[in] size Size of the shared memory to allocate and map\n> + */\n> +\n> +/**\n> + * \\fn SharedMem::SharedMem(SharedMem &&rhs)\n> + * \\brief Move constructor for SharedMem\n> + * \\param[in] rhs The object to move\n> + */\n> +\n> +/**\n> + * \\fn SharedMem::~SharedMem()\n> + * \\brief SharedMem destructor\n> + *\n> + * Unmaps the allocated shared memory. Decrements the shared memory descriptor use\n> + * count.\n> + */\n> +\n> +/**\n> + * \\fn SharedMem &SharedMem::operator=(SharedMem &&rhs)\n> + * \\brief Move constructor for SharedMem\n> + * \\param[in] rhs The object to move\n> + */\n> +\n> +/**\n> + * \\fn const SharedFD &SharedMem::fd() const\n> + * \\brief Gets the file descriptor for the underlying shared memory\n> + * \\return The file descriptor\n> + */\n> +\n> +/**\n> + * \\fn void *SharedMem::mem() const\n> + * \\brief Gets the pointer to the underlying shared memory\n> + * \\return The pointer to the shared memory\n> + */\n> +\n> +SharedMem::SharedMem(const std::string &name, std::size_t size)\n> +\t: name_(name), size_(size), mem_(nullptr)\n> +{\n> +\tint fd = memfd_create(name_.c_str(), MFD_CLOEXEC);\n> +\tif (fd < 0)\n> +\t\treturn;\n> +\n> +\tfd_ = SharedFD(std::move(fd));\n> +\tif (!fd_.isValid())\n> +\t\treturn;\n> +\n> +\tif (ftruncate(fd_.get(), size_) < 0)\n> +\t\treturn;\n> +\n> +\tmem_ = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED,\n> +\t\t fd_.get(), 0);\n> +\tif (mem_ == MAP_FAILED)\n> +\t\tmem_ = nullptr;\n> +}\n> +\n> +/**\n> + * \\var SharedMem::mem_\n> + * \\brief Pointer to the shared memory allocated\n> + */\n> +\n> +/**\n> + * \\class SharedMemObject\n> + * \\brief Helper class for allocating objects in shared memory\n> + *\n> + * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.\n> + *\n> + * Given the type of the object to be created in shared memory and the arguments\n> + * to pass to this object's constructor, SharedMemObject allocates the shared memory\n> + * of the size of the object and constructs the object in this memory. To ensure\n> + * that the SharedMemObject was created successfully, one needs to verify that the\n> + * overloaded bool() operator returns true. The object created in the shared memory\n> + * can be accessed using the SharedMemObject::operator*() indirection operator. Its\n> + * members can be accessed with the SharedMemObject::operator->() member of pointer\n> + * operator.\n> + *\n> + * To access the object from another process the SharedFD should be passed to that\n> + * process, and the shared memory should be mapped by calling mmap().\n> + *\n> + * A single memfd is created for every SharedMemObject. If there is a need to allocate\n> + * a large number of objects in shared memory, these objects should be grouped into a\n> + * single large object to keep the number of created memfd's reasonably small.\n> + */\n> +\n> +/**\n> + * \\var SharedMemObject::SIZE\n> + * \\brief The size of the object that is going to be stored here\n> + */\n> +\n> +/**\n> + * \\fn SharedMemObject< T >::SharedMemObject(const std::string &name, Args &&...args)\n> + * \\brief Constructor for the SharedMemObject\n> + * \\param[in] name Name of the SharedMemObject\n> + * \\param[in] args Args to pass to the constructor of the object in shared memory\n> + */\n> +\n> +/**\n> + * \\fn SharedMemObject::SharedMemObject(SharedMemObject &&rhs)\n> + * \\brief Move constructor for SharedMemObject\n> + * \\param[in] rhs The object to move\n> + */\n> +\n> +/**\n> + * \\fn SharedMemObject::~SharedMemObject()\n> + * \\brief SharedMemObject destructor\n> + *\n> + * Destroys the object created in the shared memory and then unmaps the shared memory.\n> + * Decrements the shared memory descriptor use count.\n> + */\n> +\n> +/**\n> + * \\fn SharedMemObject::operator=(SharedMemObject &&rhs)\n> + * \\brief Operator= for SharedMemObject\n> + * \\param[in] rhs The SharedMemObject object to take the data from\n> + */\n> +\n> +/**\n> + * \\fn SharedMemObject::operator->()\n> + * \\brief Operator-> for SharedMemObject\n> + * \\return The pointer to the object\n> + */\n> +\n> +/**\n> + * \\fn const T *SharedMemObject::operator->() const\n> + * \\brief Operator-> for SharedMemObject\n> + * \\return The pointer to the const object\n> + */\n> +\n> +/**\n> + * \\fn SharedMemObject::operator*()\n> + * \\brief Operator* for SharedMemObject\n> + * \\return The reference to the object\n> + */\n> +\n> +/**\n> + * \\fn const T &SharedMemObject::operator*() const\n> + * \\brief Operator* for SharedMemObject\n> + * \\return Const reference to the object\n> + */\n> +\n> +/**\n> + * \\fn SharedMemObject::operator bool()\n> + * \\brief Operator bool() for SharedMemObject\n> + * \\return True if the object was created OK in the shared memory, false otherwise\n> + */\n> +\n> +} // namespace libcamera","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 69AE1BD160\n\tfor ;\n\tTue, 12 Mar 2024 13:00:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5588262C80;\n\tTue, 12 Mar 2024 14:00:38 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C03856286C\n\tfor ;\n\tTue, 12 Mar 2024 14:00:36 +0100 (CET)","from mail-wm1-f72.google.com (mail-wm1-f72.google.com\n\t[209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-329--HPU8Q8oMIGGp6H1AEJroQ-1; Tue, 12 Mar 2024 09:00:33 -0400","by mail-wm1-f72.google.com with SMTP id\n\t5b1f17b1804b1-40e435a606aso32956325e9.3\n\tfor ;\n\tTue, 12 Mar 2024 06:00:32 -0700 (PDT)","from nuthatch (nat-pool-brq-t.redhat.com. [213.175.37.10])\n\tby smtp.gmail.com with ESMTPSA id\n\tl18-20020a05600c1d1200b004132f3ace5csm4255006wms.37.2024.03.12.06.00.30\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 12 Mar 2024 06:00:30 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"Hispqt32\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1710248435;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=VphCTNhjTCP8CJ28uPQeasHIU/B1Lq6GFtP0b2sj9yQ=;\n\tb=Hispqt32K+sOVS1vuvzPiVnkKAd/iEe03uLV+B560E4/zuDJC7trkqSimJBOfOvmrrVul8\n\tsG9Oj8zLqJboCuKN5/3gJN6whlRA9Fl8I/KAZHBGvsi3KGrlfK6yjPVM1yyfsPenTgtGj3\n\tSvaWrKxIntvcCcM3gKpsVC1jtbPmAiM=","X-MC-Unique":"-HPU8Q8oMIGGp6H1AEJroQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1710248432; x=1710853232;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=VphCTNhjTCP8CJ28uPQeasHIU/B1Lq6GFtP0b2sj9yQ=;\n\tb=nHl3Nz7UvX7qJc2p7JP4EInqfbblVclEk0/865kXqOoFjvV69rxy0f472nuWMHLHRn\n\tiNGygVV++VQ9mdMmMDdQdQijaiOPtZ0TxJvxATss3jKhnk+yHOEsi39b2qO6k2dYRRUk\n\trV6ofVqZGfqTzL9pGQddvkV+aDyzX89rh629HD9ezpt9HQQTRP/rT/l2UW4LhBj2ei3B\n\tc8iwvzNDMvQ0CYHeL8aSnbaG1w37UuG3dRwC4X+N53jociO9lO7dN+SzqmGGH4a1aXI/\n\t4ZUlXAQ1OHgiFJbCUImq22zXtDDvZSBzUqXCdJ5OOVsyf7uvrRdrpDvsV1bTUK24LpXo\n\t659w==","X-Gm-Message-State":"AOJu0YypWjkkT9eyKXx+hRv+nMOItpGkd6PCkfSMBipm7jreZ/D4jrrO\n\t0FBAsCl8W+80Hkd/TBW7ttw04i97nnGUhTGcUunbFYV8fWIYcMbDgVkLJ3bvoG0Z+zZRV3TwCnJ\n\tdqvUezzZQ+fkalb/OhDyiKjj0ONKKb1Uc8rmk26LFrOnqS55kvrXrutmWr4PybsEGXBC9g4g=","X-Received":["by 2002:a05:600c:3ba1:b0:412:e848:85a1 with SMTP id\n\tn33-20020a05600c3ba100b00412e84885a1mr1291034wms.35.1710248431800; \n\tTue, 12 Mar 2024 06:00:31 -0700 (PDT)","by 2002:a05:600c:3ba1:b0:412:e848:85a1 with SMTP id\n\tn33-20020a05600c3ba100b00412e84885a1mr1291002wms.35.1710248431315; \n\tTue, 12 Mar 2024 06:00:31 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IF1fgljQQykk8cIOZZKrGHk2ziJq7iXQNziYFeYEbjTBQiIG7/wlXLp1xbiJMf7t5wKDB1c7w==","From":"Milan Zamazal ","To":"Hans de Goede ","Subject":"Re: [PATCH v5 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","In-Reply-To":"<20240311141524.27192-6-hdegoede@redhat.com> (Hans de Goede's\n\tmessage of \"Mon, 11 Mar 2024 15:15:09 +0100\")","References":"<20240311141524.27192-1-hdegoede@redhat.com>\n\t<20240311141524.27192-6-hdegoede@redhat.com>","Date":"Tue, 12 Mar 2024 14:00:30 +0100","Message-ID":"<87cyrz4nbl.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Maxime Ripard , libcamera-devel@lists.libcamera.org, \n\tPavel Machek ,\n\tBryan O'Donoghue , \n\tDennis Bonke ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28931,"web_url":"https://patchwork.libcamera.org/comment/28931/","msgid":"","date":"2024-03-12T13:27:19","subject":"Re: [PATCH v5 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. március 11., hétfő 15:15 keltezéssel, Hans de Goede írta:\n\n> From: Andrei Konovalov \n> \n> Split the parts which doesn't otherwise depend on the type T or\n> arguments Args out of the SharedMemObject class into a new\n> SharedMem class.\n> \n> Doxygen documentation by Dennis Bonke and Andrei Konovalov.\n> \n> Reviewed-by: Pavel Machek \n> Co-developed-by: Dennis Bonke \n> Signed-off-by: Dennis Bonke \n> Signed-off-by: Andrei Konovalov \n> Signed-off-by: Hans de Goede \n> ---\n> .../libcamera/internal/shared_mem_object.h | 101 ++++++----\n> src/libcamera/meson.build | 1 +\n> src/libcamera/shared_mem_object.cpp | 190 ++++++++++++++++++\n> 3 files changed, 253 insertions(+), 39 deletions(-)\n> create mode 100644 src/libcamera/shared_mem_object.cpp\n> \n> [...]\n> template\n> -class SharedMemObject\n> +class SharedMemObject : public SharedMem\n> {\n> public:\n> \tstatic constexpr std::size_t SIZE = sizeof(T);\n> \n> \tSharedMemObject()\n> -\t\t: obj_(nullptr)\n> +\t\t: SharedMem(), obj_(nullptr)\n> \t{\n> \t}\n> \n> \ttemplate\n> \tSharedMemObject(const std::string &name, Args &&...args)\n> -\t\t: name_(name), obj_(nullptr)\n> +\t\t: SharedMem(name, SIZE), obj_(nullptr)\n> \t{\n> -\t\tvoid *mem;\n> -\t\tint ret;\n> -\n> -\t\tret = memfd_create(name_.c_str(), MFD_CLOEXEC);\n> -\t\tif (ret < 0)\n> +\t\tif (mem_ == nullptr)\n> \t\t\treturn;\n> \n> -\t\tfd_ = SharedFD(std::move(ret));\n> -\t\tif (!fd_.isValid())\n> -\t\t\treturn;\n> -\n> -\t\tret = ftruncate(fd_.get(), SIZE);\n> -\t\tif (ret < 0)\n> -\t\t\treturn;\n> -\n> -\t\tmem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,\n> -\t\t\t fd_.get(), 0);\n> -\t\tif (mem == MAP_FAILED)\n> -\t\t\treturn;\n> -\n> -\t\tobj_ = new (mem) T(std::forward(args)...);\n> +\t\tobj_ = new (mem_) T(std::forward(args)...);\n\nIf I see it correctly, every `SharedMemObject` stores the same pointer twice\n(`obj_` and `mem_` in the base class). I think this can be avoided.\n\n\n> \t}\n> \n> \tSharedMemObject(SharedMemObject &&rhs)\n> +\t\t: SharedMem(std::move(rhs))\n> \t{\n> -\t\tthis->name_ = std::move(rhs.name_);\n> -\t\tthis->fd_ = std::move(rhs.fd_);\n> \t\tthis->obj_ = rhs.obj_;\n> \t\trhs.obj_ = nullptr;\n> \t}\n> \n> \t~SharedMemObject()\n> \t{\n> -\t\tif (obj_) {\n> +\t\tif (obj_)\n> \t\t\tobj_->~T();\n> -\t\t\tmunmap(obj_, SIZE);\n> -\t\t}\n> \t}\n> \n> \t/* Make SharedMemObject non-copyable for now. */\n> @@ -78,8 +109,7 @@ public:\n> \n> \tSharedMemObject &operator=(SharedMemObject &&rhs)\n> \t{\n> -\t\tthis->name_ = std::move(rhs.name_);\n> -\t\tthis->fd_ = std::move(rhs.fd_);\n> +\t\tSharedMem::operator=(std::move(rhs));\n> \t\tthis->obj_ = rhs.obj_;\n> \t\trhs.obj_ = nullptr;\n> \t\treturn *this;\n> @@ -105,19 +135,12 @@ public:\n> \t\treturn *obj_;\n> \t}\n> \n> -\tconst SharedFD &fd() const\n> -\t{\n> -\t\treturn fd_;\n> -\t}\n> -\n> \texplicit operator bool() const\n> \t{\n> \t\treturn !!obj_;\n> \t}\n> \n> private:\n> -\tstd::string name_;\n> -\tSharedFD fd_;\n> \tT *obj_;\n> };\n> \n> [...]\n\n\nRegards,\nBarnabás Pőcze","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D97DBBD1F1\n\tfor ;\n\tTue, 12 Mar 2024 13:27:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2DF326295D;\n\tTue, 12 Mar 2024 14:27:28 +0100 (CET)","from mail-4316.protonmail.ch (mail-4316.protonmail.ch\n\t[185.70.43.16])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D094C6286C\n\tfor ;\n\tTue, 12 Mar 2024 14:27:26 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"gSqyK8n/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1710250046; x=1710509246;\n\tbh=yl2LXSkSMCBvOQ6jTylUZbnijdIdGE9S1+kdWF3GUdo=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=gSqyK8n/hxq3R3nH/M9KxeR5hmE1A/AdbvFZNRW7LabI8cv7uwpCYQKpbe1MsLY4s\n\ttIwWtNrbOMfBSWjtVxHlN07f/6K17PO7HjXmQVmVlGotkk89Lny8yEB8IbnIpBhCQK\n\tUj7eDoHWFFeJy1JvKlKa2GGMUXj20pqs5kiA4vXj+ZiZaANMfCvCba0ggnuA39F/Wc\n\tlVrrjyIhNTQ9tK7x5ZPBbjT7FEewtzLLZ1JINLRTeL0d+VRndOu0CCCSl3UAEqmwvm\n\tPj/zoSvt1OfC3wfnslSAg9cAutjMDVe8vez8Mn3w8UwvB498C27jAK4lQJPgEtozrw\n\twMph7dC0kc2Rg==","Date":"Tue, 12 Mar 2024 13:27:19 +0000","To":"Hans de Goede ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Subject":"Re: [PATCH v5 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","Message-ID":"","In-Reply-To":"<20240311141524.27192-6-hdegoede@redhat.com>","References":"<20240311141524.27192-1-hdegoede@redhat.com>\n\t<20240311141524.27192-6-hdegoede@redhat.com>","Feedback-ID":"20568564:user:proton","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Bryan O'Donoghue ,\n\tlibcamera-devel@lists.libcamera.org, Maxime Ripard , \n\tPavel Machek , Dennis Bonke ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29001,"web_url":"https://patchwork.libcamera.org/comment/29001/","msgid":"<87jzly4ah3.fsf@redhat.com>","date":"2024-03-19T13:28:24","subject":"Re: [PATCH v5 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi,\n\nBarnabás Pőcze writes:\n\n> Hi\n>\n>\n> 2024. március 11., hétfő 15:15 keltezéssel, Hans de Goede írta:\n>\n>> From: Andrei Konovalov \n>> \n>> Split the parts which doesn't otherwise depend on the type T or\n>> arguments Args out of the SharedMemObject class into a new\n>> SharedMem class.\n>> \n>> Doxygen documentation by Dennis Bonke and Andrei Konovalov.\n>> \n>> Reviewed-by: Pavel Machek \n>> Co-developed-by: Dennis Bonke \n>> Signed-off-by: Dennis Bonke \n>> Signed-off-by: Andrei Konovalov \n>> Signed-off-by: Hans de Goede \n>> ---\n>> .../libcamera/internal/shared_mem_object.h | 101 ++++++----\n>> src/libcamera/meson.build | 1 +\n>> src/libcamera/shared_mem_object.cpp | 190 ++++++++++++++++++\n>> 3 files changed, 253 insertions(+), 39 deletions(-)\n>> create mode 100644 src/libcamera/shared_mem_object.cpp\n>> \n>> [...]\n>> template\n>> -class SharedMemObject\n>> +class SharedMemObject : public SharedMem\n>> {\n>> public:\n>> \tstatic constexpr std::size_t SIZE = sizeof(T);\n>> \n>> \tSharedMemObject()\n>> -\t\t: obj_(nullptr)\n>> +\t\t: SharedMem(), obj_(nullptr)\n>> \t{\n>> \t}\n>> \n>> \ttemplate\n>> \tSharedMemObject(const std::string &name, Args &&...args)\n>> -\t\t: name_(name), obj_(nullptr)\n>> +\t\t: SharedMem(name, SIZE), obj_(nullptr)\n>> \t{\n>> -\t\tvoid *mem;\n>> -\t\tint ret;\n>> -\n>> -\t\tret = memfd_create(name_.c_str(), MFD_CLOEXEC);\n>> -\t\tif (ret < 0)\n>> +\t\tif (mem_ == nullptr)\n>> \t\t\treturn;\n>> \n>> -\t\tfd_ = SharedFD(std::move(ret));\n>> -\t\tif (!fd_.isValid())\n>> -\t\t\treturn;\n>> -\n>> -\t\tret = ftruncate(fd_.get(), SIZE);\n>> -\t\tif (ret < 0)\n>> -\t\t\treturn;\n>> -\n>> -\t\tmem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,\n>> -\t\t\t fd_.get(), 0);\n>> -\t\tif (mem == MAP_FAILED)\n>> -\t\t\treturn;\n>> -\n>> -\t\tobj_ = new (mem) T(std::forward(args)...);\n>> +\t\tobj_ = new (mem_) T(std::forward(args)...);\n>\n> If I see it correctly, every `SharedMemObject` stores the same pointer twice\n> (`obj_` and `mem_` in the base class). I think this can be avoided.\n\nPerhaps but I think it's better not to touch this code as we approach merging\nthis initial software ISP support, thus no change here in v6. If we feel an\nurge for improvement here, a patch can be created afterwards.\n\nRegards,\nMilan\n\n>> \t}\n>> \n>> \tSharedMemObject(SharedMemObject &&rhs)\n>> +\t\t: SharedMem(std::move(rhs))\n>> \t{\n>> -\t\tthis->name_ = std::move(rhs.name_);\n>> -\t\tthis->fd_ = std::move(rhs.fd_);\n>> \t\tthis->obj_ = rhs.obj_;\n>> \t\trhs.obj_ = nullptr;\n>> \t}\n>> \n>> \t~SharedMemObject()\n>> \t{\n>> -\t\tif (obj_) {\n>> +\t\tif (obj_)\n>> \t\t\tobj_->~T();\n>> -\t\t\tmunmap(obj_, SIZE);\n>> -\t\t}\n>> \t}\n>> \n>> \t/* Make SharedMemObject non-copyable for now. */\n>> @@ -78,8 +109,7 @@ public:\n>> \n>> \tSharedMemObject &operator=(SharedMemObject &&rhs)\n>> \t{\n>> -\t\tthis->name_ = std::move(rhs.name_);\n>> -\t\tthis->fd_ = std::move(rhs.fd_);\n>> +\t\tSharedMem::operator=(std::move(rhs));\n>> \t\tthis->obj_ = rhs.obj_;\n>> \t\trhs.obj_ = nullptr;\n>> \t\treturn *this;\n>> @@ -105,19 +135,12 @@ public:\n>> \t\treturn *obj_;\n>> \t}\n>> \n>> -\tconst SharedFD &fd() const\n>> -\t{\n>> -\t\treturn fd_;\n>> -\t}\n>> -\n>> \texplicit operator bool() const\n>> \t{\n>> \t\treturn !!obj_;\n>> \t}\n>> \n>> private:\n>> -\tstd::string name_;\n>> -\tSharedFD fd_;\n>> \tT *obj_;\n>> };\n>> \n>> [...]\n>\n>\n> Regards,\n> Barnabás Pőcze","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 41F2EC3272\n\tfor ;\n\tTue, 19 Mar 2024 13:28:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B74362CAE;\n\tTue, 19 Mar 2024 14:28:32 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6388D62CAA\n\tfor ;\n\tTue, 19 Mar 2024 14:28:31 +0100 (CET)","from mail-lf1-f71.google.com (mail-lf1-f71.google.com\n\t[209.85.167.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-115-7nWIkPvbOSOgwmvc9uZBwQ-1; Tue, 19 Mar 2024 09:28:27 -0400","by mail-lf1-f71.google.com with SMTP id\n\t2adb3069b0e04-513e50a5aaeso2382734e87.1\n\tfor ;\n\tTue, 19 Mar 2024 06:28:27 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tw9-20020a05600c474900b0041408af4b34sm10773415wmo.10.2024.03.19.06.28.24\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 19 Mar 2024 06:28:25 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"RodCPTKi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1710854910;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=qCmNdHYuz91WwDTS44KPE/dq5OiwQ8UT1TS/MnVGT0A=;\n\tb=RodCPTKiHPUNQEmx2AwjysRLNcW7MxtTfREWxfeXm0lVKAd9VH1xCDaaFER/CfC+b4jhGy\n\tJXkgQQBPHuVVYQ1iDdF7WJxLnFHt87UGv74k8yBU4wx5slXQR8vHUts91ij5dSUKxqf7r2\n\tj+4FTEWAel41FLTbbfv7KMwRBMBGuzg=","X-MC-Unique":"7nWIkPvbOSOgwmvc9uZBwQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1710854906; x=1711459706;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=MrPq+S5XRz7mHI2XCuvC/ywmSrkYJYrKGWV98fSqZOU=;\n\tb=B6/B3XYygHRp0g1c27TiKlJsxz60nrEk+uPKGQqbnoJcI+RLRKoEYZW630gPkG++bc\n\t4mfH77dkOXBb1PIJ86epLB34Yb8pinDZ2egf1Eka1EaoYup/th4jevMCK+4Nleg1fLra\n\tmefmIwPqMsLQoOSPwzd3O5x8vUPe9MelXzsxSccRaWwQvRPLMsu/wpTY4/muAwBIiKpE\n\t3UO/BOZOnAc7SCyVSQL6sUP9zHFul61SMHkDnCQZn8PTAmosm+VKl1jlkSU5erJLuacA\n\tp1yO9LcOl9IgZKCjxAm0PPAMSZD6srbwcLjL3lMx7V8O5V2abYusQiwXW9yWHn9YkD63\n\tltiA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXlgPljUnjfr8TSNzy9+C+lEQuM+VxtC1zkoJ/aubF37rmDebYnQ52QaJEH2YlkejJlFuoSKiYRJMCMCfPodOXp8Ln+nE613h45uLbKvFsjiZWfUQ==","X-Gm-Message-State":"AOJu0YyLHyeOd9COgvYOniNMMhV/KbXo+MVqxvoj+Xhq9apBKpjJ09EY\n\t1z5ptpkeYfcGx6guODDqJLnf47e7gOSzytAzG7tluRejzE0KzlBUFFny3oLgEZ5SLJz+yAokzXD\n\tpoKLTfxR3eablFSrJ21/Xnr4ee726EUwLco8hKuN3OdlDLDDacyMRGVZGUrOse0vQRN2zNKM=","X-Received":["by 2002:a19:4318:0:b0:513:e642:e7f0 with SMTP id\n\tq24-20020a194318000000b00513e642e7f0mr4141199lfa.54.1710854906376; \n\tTue, 19 Mar 2024 06:28:26 -0700 (PDT)","by 2002:a19:4318:0:b0:513:e642:e7f0 with SMTP id\n\tq24-20020a194318000000b00513e642e7f0mr4141183lfa.54.1710854905961; \n\tTue, 19 Mar 2024 06:28:25 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHwx69kjgibosb2PT6PQTCiQqrXV9im7y1H5DG25IcNOTqq8PvEyRAKkwvWwr1R52Zs7fkwWw==","From":"Milan Zamazal ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Hans de Goede , Bryan O'Donoghue\n\t, libcamera-devel@lists.libcamera.org,\n\tMaxime Ripard , Pavel Machek , Dennis\n\tBonke ","Subject":"Re: [PATCH v5 05/18] libcamera: shared_mem_object: reorganize the\n\tcode and document the SharedMemObject class","In-Reply-To":"\n\t( =?utf-8?b?IkJhcm5hYsOhcyBQxZFjemUiJ3M=?= message of \"Tue,\n\t12 Mar 2024 13:27:19 +0000\")","References":"<20240311141524.27192-1-hdegoede@redhat.com>\n\t<20240311141524.27192-6-hdegoede@redhat.com>\n\t","Date":"Tue, 19 Mar 2024 14:28:24 +0100","Message-ID":"<87jzly4ah3.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]