[{"id":16634,"web_url":"https://patchwork.libcamera.org/comment/16634/","msgid":"","date":"2021-04-27T07:16:57","subject":"Re: [libcamera-devel] [PATCH v5 3/5] ipa: raspberrypi: Add\n\tmove/copy ctors and operators to Metadata class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Mon, Apr 19, 2021 at 02:34:49PM +0100, Naushir Patuck wrote:\n> Add a default, move and copy constructor as well as a move operator\n> implementation RPiController::Metadata class.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> src/ipa/raspberrypi/controller/metadata.hpp | 24 +++++++++++++++++++++\n> 1 file changed, 24 insertions(+)\n> \n> diff --git a/src/ipa/raspberrypi/controller/metadata.hpp b/src/ipa/raspberrypi/controller/metadata.hpp\n> index 07dd28ed9e0a..319f2320fc70 100644\n> --- a/src/ipa/raspberrypi/controller/metadata.hpp\n> +++ b/src/ipa/raspberrypi/controller/metadata.hpp\n> @@ -19,6 +19,21 @@ namespace RPiController {\n> class Metadata\n> {\n> public:\n> +\tMetadata() = default;\n> +\n> +\tMetadata(Metadata const &other)\n> +\t{\n> +\t\tstd::lock_guard other_lock(other.mutex_);\n> +\t\tdata_ = other.data_;\n> +\t}\n> +\n> +\tMetadata(Metadata &&other)\n> +\t{\n> +\t\tstd::lock_guard other_lock(other.mutex_);\n> +\t\tdata_ = std::move(other.data_);\n> +\t\tother.data_.clear();\n> +\t}\n> +\n> \ttemplate\n> \tvoid Set(std::string const &tag, T const &value)\n> \t{\n> @@ -51,6 +66,15 @@ public:\n> \t\treturn *this;\n> \t}\n> \n> +\tMetadata &operator=(Metadata &&other)\n> +\t{\n> +\t\tstd::lock_guard lock(mutex_);\n> +\t\tstd::lock_guard other_lock(other.mutex_);\n\nThis could cause a deadlock if two threads we to move A to B and B to A.\nI suppose such a usage pattern never happens ? We may be able to improve\nlocking a bit, but that's out of scope for this series.\n\nReviewed-by: Laurent Pinchart \n\n> +\t\tdata_ = std::move(other.data_);\n> +\t\tother.data_.clear();\n> +\t\treturn *this;\n> +\t}\n> +\n> \ttemplate\n> \tT *GetLocked(std::string const &tag)\n> \t{","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 71812BDE19\n\tfor ;\n\tTue, 27 Apr 2021 07:17:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B8B8D688B3;\n\tTue, 27 Apr 2021 09:17:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 489D46885A\n\tfor ;\n\tTue, 27 Apr 2021 09:17:04 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AC649E9;\n\tTue, 27 Apr 2021 09:17:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Y/bvaFE5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619507823;\n\tbh=na74MdUcF5P6fZNhcBflcTE8JSIk/DnMLD5rLyYKuEI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Y/bvaFE5MzEHPVsHgiQrCBg2TYpYRLyP9gl5KO/y7rt2U+886tCu1iMx8LkTTqfSj\n\tpJNZeBecbCutj0hT50j/UGkKZ28zKX8n7Zs+oxWyU6Fnfa8YecgkXg1oul3ERL8vso\n\tGB2xf3IrOt6TuTvIee9PLKc2895NkuYoYPPuWJhA=","Date":"Tue, 27 Apr 2021 10:16:57 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210419133451.263733-1-naush@raspberrypi.com>\n\t<20210419133451.263733-4-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210419133451.263733-4-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v5 3/5] ipa: raspberrypi: Add\n\tmove/copy ctors and operators to Metadata class","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":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16650,"web_url":"https://patchwork.libcamera.org/comment/16650/","msgid":"","date":"2021-04-27T09:10:06","subject":"Re: [libcamera-devel] [PATCH v5 3/5] ipa: raspberrypi: Add\n\tmove/copy ctors and operators to Metadata class","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThanks for the comments. Just to add my explanation to your question...\n\nOn Tue, 27 Apr 2021 at 08:17, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Mon, Apr 19, 2021 at 02:34:49PM +0100, Naushir Patuck wrote:\n> > Add a default, move and copy constructor as well as a move operator\n> > implementation RPiController::Metadata class.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/controller/metadata.hpp | 24 +++++++++++++++++++++\n> > 1 file changed, 24 insertions(+)\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/metadata.hpp b/src/ipa/raspberrypi/controller/metadata.hpp\n> > index 07dd28ed9e0a..319f2320fc70 100644\n> > --- a/src/ipa/raspberrypi/controller/metadata.hpp\n> > +++ b/src/ipa/raspberrypi/controller/metadata.hpp\n> > @@ -19,6 +19,21 @@ namespace RPiController {\n> > class Metadata\n> > {\n> > public:\n> > + Metadata() = default;\n> > +\n> > + Metadata(Metadata const &other)\n> > + {\n> > + std::lock_guard other_lock(other.mutex_);\n> > + data_ = other.data_;\n> > + }\n> > +\n> > + Metadata(Metadata &&other)\n> > + {\n> > + std::lock_guard other_lock(other.mutex_);\n> > + data_ = std::move(other.data_);\n> > + other.data_.clear();\n> > + }\n> > +\n> > template\n> > void Set(std::string const &tag, T const &value)\n> > {\n> > @@ -51,6 +66,15 @@ public:\n> > return *this;\n> > }\n> >\n> > + Metadata &operator=(Metadata &&other)\n> > + {\n> > + std::lock_guard lock(mutex_);\n> > + std::lock_guard other_lock(other.mutex_);\n>\n> This could cause a deadlock if two threads we to move A to B and B to A.\n> I suppose such a usage pattern never happens ? We may be able to improve\n> locking a bit, but that's out of scope for this series.\n\nIndeed, that's true. But I think any situation where one thread does\n\"A = B\" and another, in parallel, does \"B = A\" is fundamentally\nproblematic. I can't imagine what one would expect to happen...\n\nActually, you may recall how there's been a general process of\nremoving locks from our IPAs because in the libcamera world everything\nis more synchronous than the place where that code originated. I do\nwonder whether these locks fall into the same category, but that's a\nseparate investigation for a separate patch series.\n\nThanks!\nDavid\n\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > + data_ = std::move(other.data_);\n> > + other.data_.clear();\n> > + return *this;\n> > + }\n> > +\n> > template\n> > T *GetLocked(std::string const &tag)\n> > {\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 E820DBDE19\n\tfor ;\n\tTue, 27 Apr 2021 09:10:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2460C688BA;\n\tTue, 27 Apr 2021 11:10:20 +0200 (CEST)","from mail-wm1-x335.google.com (mail-wm1-x335.google.com\n\t[IPv6:2a00:1450:4864:20::335])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 036D468880\n\tfor ;\n\tTue, 27 Apr 2021 11:10:18 +0200 (CEST)","by mail-wm1-x335.google.com with SMTP id\n\tp6-20020a05600c3586b029014131bbe5c7so3367310wmq.3\n\tfor ;\n\tTue, 27 Apr 2021 02:10:17 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"Aw55f3mh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=yn9sYW221jc9EQdclgRPH12FMXNDz3i3rl/sRnL4G2s=;\n\tb=Aw55f3mhnpd0TBDrzTz6tbKd3g7DNpy7cmNq3yopgkTQiekQip2unkwZIkiA4I/87h\n\tKN/8gcxJ9VIuo/ZQg2EoFDSKUI7sCkBhAG7eV2+Z64cU5JACZC9vwqJ49ri/YevVe8/G\n\tMlpQqsL4kuF49Rkim+k+PiWS+QB1S3Ag2oRXMOm97+Z+qcd23gxO3Bekv7OfIeBIgXVO\n\tl2SszUuwVLN3i467qSV3F5x7cqBVdzsaIhfOurHfVF4t24UHNdBaWyz5BfhkVMqllMq5\n\tc4SLzxCLzQFD0HfzGM0VM0bKthfSEhR1PMuaczKDUT1RrR8Eq1ZSDgcTW8JLhCmsORIv\n\t/Pfg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=yn9sYW221jc9EQdclgRPH12FMXNDz3i3rl/sRnL4G2s=;\n\tb=co7WqqDaKkmPEvYyyu6ruvf5+MLuW3Hi84mCrilAEQzAFv7GnKghuPESlnaxFlWu8Q\n\t52ZStkPG3qnxJuIeeCRuaY/OwRYFKH93trrKSKlVvrSie5TUukLUd9yTXBuOxHEoeUfJ\n\tMc7io/2og8rnl+OFOAwRpkXSvP/wuatHOKTrq9ARosgZFmJZILEYXD7zE1gAcKQMVZ/P\n\tzbxPQV0MePVF5Le2VT9hPU+yufgFVOSaS1JMVmjwz+IRCHrbi6m+hwZj3Bm10GveEZ2F\n\t87YfVD4fDJuCBxosOCNr/FFZ13hO7cs/yG2x28tY7btA5o4kSacbxGtT9vdSpYIXPksj\n\tMGtQ==","X-Gm-Message-State":"AOAM532sLJnUjoLEHyKeUI8hmtam5GBYb/rpX7LVaPnLYpebq5v2r7YY\n\tkAsHipc5F778+AtTwJsVnlaJHtbq4ljUq/ULa4+WqK2+tbc=","X-Google-Smtp-Source":"ABdhPJy0pJY5+kRx5Nt7uN4IS5YCdyAfOF/9ZGVbpqcfhWXeERQSmvof9qkbfTXCv3QGiDod2FvFM1kfae2OD1tEcHk=","X-Received":"by 2002:a7b:cde1:: with SMTP id p1mr3171523wmj.6.1619514617649; \n\tTue, 27 Apr 2021 02:10:17 -0700 (PDT)","MIME-Version":"1.0","References":"<20210419133451.263733-1-naush@raspberrypi.com>\n\t<20210419133451.263733-4-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Tue, 27 Apr 2021 10:10:06 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v5 3/5] ipa: raspberrypi: Add\n\tmove/copy ctors and operators to Metadata class","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":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]