[{"id":33263,"web_url":"https://patchwork.libcamera.org/comment/33263/","msgid":"","date":"2025-02-03T16:43:58","subject":"Re: [RFC PATCH v2 4/9] libcamera: base: log: Make\n\t`LogCategory::severity_` atomic","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Thu, Jan 30, 2025 at 07:58:34PM +0000, Barnabás Pőcze wrote:\n> The severity of a log category may be changed from a different thread,\n> so it is important to ensure that the reads and writes happen atomically.\n> Using `std::memory_order_relaxed` should not introduce any synchronization\n> overhead, it should only guarantee that the operation itself is atomic.\n>\n> Secondly, inline `LogCategory::setSeverity()`, as it is merely an\n> assignment, so going through a DSO call is a big pessimization.\n> `LogCategory` is not part of the public API, so this change has\n> no external effects.\n>\n> Thirdly, assert that the atomic variable is lock free so as to ensure\n> it won't silently fall back to libatomic (or similar) on any platform.\n> If this assertion fails, this needs to be revisited.\n\nWould it be that bad if some platforms require locking in this case ?\nChanging log level at run-time doesn't seem like an hot path or even\nsomething that common.\n\nAnyway\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n\n>\n> Signed-off-by: Barnabás Pőcze \n> Reviewed-by: Laurent Pinchart \n> ---\n> include/libcamera/base/log.h | 9 ++++++---\n> src/libcamera/base/log.cpp | 5 +----\n> 2 files changed, 7 insertions(+), 7 deletions(-)\n>\n> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n> index 6d2c93019..4137d87a4 100644\n> --- a/include/libcamera/base/log.h\n> +++ b/include/libcamera/base/log.h\n> @@ -7,6 +7,7 @@\n>\n> #pragma once\n>\n> +#include \n> #include \n>\n> #include \n> @@ -31,8 +32,8 @@ public:\n> \tstatic LogCategory *create(const char *name);\n>\n> \tconst std::string &name() const { return name_; }\n> -\tLogSeverity severity() const { return severity_; }\n> -\tvoid setSeverity(LogSeverity severity);\n> +\tLogSeverity severity() const { return severity_.load(std::memory_order_relaxed); }\n> +\tvoid setSeverity(LogSeverity severity) { severity_.store(severity, std::memory_order_relaxed); }\n>\n> \tstatic const LogCategory &defaultCategory();\n>\n> @@ -40,7 +41,9 @@ private:\n> \texplicit LogCategory(const char *name);\n>\n> \tconst std::string name_;\n> -\tLogSeverity severity_;\n> +\n> +\tstd::atomic severity_;\n> +\tstatic_assert(decltype(severity_)::is_always_lock_free);\n> };\n>\n> #define LOG_DECLARE_CATEGORY(name)\t\t\t\t\t\\\n> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp\n> index 51a5cd470..2abbad478 100644\n> --- a/src/libcamera/base/log.cpp\n> +++ b/src/libcamera/base/log.cpp\n> @@ -824,15 +824,12 @@ LogCategory::LogCategory(const char *name)\n> */\n>\n> /**\n> + * \\fn LogCategory::setSeverity(LogSeverity severity)\n> * \\brief Set the severity of the log category\n> *\n> * Messages of severity higher than or equal to the severity of the log category\n> * are printed, other messages are discarded.\n> */\n> -void LogCategory::setSeverity(LogSeverity severity)\n> -{\n> -\tseverity_ = severity;\n> -}\n>\n> /**\n> * \\brief Retrieve the default log category\n> --\n> 2.48.1\n>\n>","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 D19C3BD80A\n\tfor ;\n\tMon, 3 Feb 2025 16:44:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C441D68592;\n\tMon, 3 Feb 2025 17:44:03 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 26C1B61876\n\tfor ;\n\tMon, 3 Feb 2025 17:44:02 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7C343497;\n\tMon, 3 Feb 2025 17:42:50 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"c6DnGziz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738600970;\n\tbh=fwjSWNDJa2ajLT3lEZi3ie25M4las9kzivp4txyURWk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=c6DnGzizocBhR4TvHAnoNLr/RaKiCdHOhxPyrDWp6w2eAM2Yk9wCwg+NuLP1B9gHn\n\tw6LG8ibjTEPFc/PcxkdjAERm8je+FIdKGl3cgmhBOVKSMcYTqyLla2OAnlqQ5+mdLV\n\tg4/w0yE9AJiwojgbKTY0S49BnLGqj1+LYJZvR5LY=","Date":"Mon, 3 Feb 2025 17:43:58 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, \n\tLaurent Pinchart ","Subject":"Re: [RFC PATCH v2 4/9] libcamera: base: log: Make\n\t`LogCategory::severity_` atomic","Message-ID":"","References":"<20250130195811.1230581-1-pobrn@protonmail.com>\n\t<20250130195811.1230581-5-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250130195811.1230581-5-pobrn@protonmail.com>","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\" "}},{"id":33272,"web_url":"https://patchwork.libcamera.org/comment/33272/","msgid":"","date":"2025-02-03T18:18:44","subject":"Re: [RFC PATCH v2 4/9] libcamera: base: log: Make\n\t`LogCategory::severity_` atomic","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"2025. február 3., hétfő 17:43 keltezéssel, Jacopo Mondi írta:\n\n> Hi Barnabás\n> \n> On Thu, Jan 30, 2025 at 07:58:34PM +0000, Barnabás Pőcze wrote:\n> > The severity of a log category may be changed from a different thread,\n> > so it is important to ensure that the reads and writes happen atomically.\n> > Using `std::memory_order_relaxed` should not introduce any synchronization\n> > overhead, it should only guarantee that the operation itself is atomic.\n> >\n> > Secondly, inline `LogCategory::setSeverity()`, as it is merely an\n> > assignment, so going through a DSO call is a big pessimization.\n> > `LogCategory` is not part of the public API, so this change has\n> > no external effects.\n> >\n> > Thirdly, assert that the atomic variable is lock free so as to ensure\n> > it won't silently fall back to libatomic (or similar) on any platform.\n> > If this assertion fails, this needs to be revisited.\n> \n> Would it be that bad if some platforms require locking in this case ?\n> Changing log level at run-time doesn't seem like an hot path or even\n> something that common.\n\nThis applies to reads as well, which is done in the destructor of every `LogMessage`.\nI believe that is too big of a slowdown to accept without more consideration,\nhence the assertion.\n\n\n> \n> Anyway\n> Reviewed-by: Jacopo Mondi \n> \n> Thanks\n> j\n> \n> \n> >\n> > Signed-off-by: Barnabás Pőcze \n> > Reviewed-by: Laurent Pinchart \n> > ---\n> > include/libcamera/base/log.h | 9 ++++++---\n> > src/libcamera/base/log.cpp | 5 +----\n> > 2 files changed, 7 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n> > index 6d2c93019..4137d87a4 100644\n> > --- a/include/libcamera/base/log.h\n> > +++ b/include/libcamera/base/log.h\n> > @@ -7,6 +7,7 @@\n> >\n> > #pragma once\n> >\n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -31,8 +32,8 @@ public:\n> > \tstatic LogCategory *create(const char *name);\n> >\n> > \tconst std::string &name() const { return name_; }\n> > -\tLogSeverity severity() const { return severity_; }\n> > -\tvoid setSeverity(LogSeverity severity);\n> > +\tLogSeverity severity() const { return severity_.load(std::memory_order_relaxed); }\n> > +\tvoid setSeverity(LogSeverity severity) { severity_.store(severity, std::memory_order_relaxed); }\n> >\n> > \tstatic const LogCategory &defaultCategory();\n> >\n> > @@ -40,7 +41,9 @@ private:\n> > \texplicit LogCategory(const char *name);\n> >\n> > \tconst std::string name_;\n> > -\tLogSeverity severity_;\n> > +\n> > +\tstd::atomic severity_;\n> > +\tstatic_assert(decltype(severity_)::is_always_lock_free);\n> > };\n> >\n> > #define LOG_DECLARE_CATEGORY(name)\t\t\t\t\t\\\n> > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp\n> > index 51a5cd470..2abbad478 100644\n> > --- a/src/libcamera/base/log.cpp\n> > +++ b/src/libcamera/base/log.cpp\n> > @@ -824,15 +824,12 @@ LogCategory::LogCategory(const char *name)\n> > */\n> >\n> > /**\n> > + * \\fn LogCategory::setSeverity(LogSeverity severity)\n> > * \\brief Set the severity of the log category\n> > *\n> > * Messages of severity higher than or equal to the severity of the log category\n> > * are printed, other messages are discarded.\n> > */\n> > -void LogCategory::setSeverity(LogSeverity severity)\n> > -{\n> > -\tseverity_ = severity;\n> > -}\n> >\n> > /**\n> > * \\brief Retrieve the default log category\n> > --\n> > 2.48.1\n> >\n> >\n>","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 EAC17BD80A\n\tfor ;\n\tMon, 3 Feb 2025 18:18:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 163296859E;\n\tMon, 3 Feb 2025 19:18:52 +0100 (CET)","from mail-10630.protonmail.ch (mail-10630.protonmail.ch\n\t[79.135.106.30])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1D6FC68598\n\tfor ;\n\tMon, 3 Feb 2025 19:18:50 +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=\"yIEzDoRm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1738606729; x=1738865929;\n\tbh=fxj2rbfwQsBnZYk+wNntB9xPAKm5+7W2ZVIAM3xBZt8=;\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:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=yIEzDoRmqErlkG7oUuVqfSrilAuVzj/6CH50DgQqDqwRr7HNa6xvwLUdhlj4JEjE7\n\t1m/dZuLimMBi8Mse6T9YCeKerslHyE3JktdeePmEvIE4eQKE1HnbxZUQrP9cupkcly\n\txu3jXmcEr1ky9tvfUKNI+sWlQKeM11XOhvvmc3eL795ExhcCU2ig1qeapAAB+bGRvx\n\tHhzzcIrQEc1QIpMEkq4lRnzyTSXMgt381bi0oXuVmmbX7GWklbBs4Yl7FfZp60GZXS\n\t7ICBuw/Ku4Q57EN+ksoQO+1Bx37df8GhPloq1HPB9lr/f4FEWTEl5FV/bFYfrXjpu2\n\tPrEJCFGpWls2Q==","Date":"Mon, 03 Feb 2025 18:18:44 +0000","To":"Jacopo Mondi ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tLaurent Pinchart ","Subject":"Re: [RFC PATCH v2 4/9] libcamera: base: log: Make\n\t`LogCategory::severity_` atomic","Message-ID":"","In-Reply-To":"","References":"<20250130195811.1230581-1-pobrn@protonmail.com>\n\t<20250130195811.1230581-5-pobrn@protonmail.com>\n\t","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"b36abf3401e2be076e6dc98b23917a0c1cf9a998","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33275,"web_url":"https://patchwork.libcamera.org/comment/33275/","msgid":"<7lnm54wi4qlfuhhwwwyybh7ljjflk7aw243bqduuff2qo5kyol@buuipyhblhl5>","date":"2025-02-03T18:46:16","subject":"Re: [RFC PATCH v2 4/9] libcamera: base: log: Make\n\t`LogCategory::severity_` atomic","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Mon, Feb 03, 2025 at 06:18:44PM +0000, Barnabás Pőcze wrote:\n> 2025. február 3., hétfő 17:43 keltezéssel, Jacopo Mondi írta:\n>\n> > Hi Barnabás\n> >\n> > On Thu, Jan 30, 2025 at 07:58:34PM +0000, Barnabás Pőcze wrote:\n> > > The severity of a log category may be changed from a different thread,\n> > > so it is important to ensure that the reads and writes happen atomically.\n> > > Using `std::memory_order_relaxed` should not introduce any synchronization\n> > > overhead, it should only guarantee that the operation itself is atomic.\n> > >\n> > > Secondly, inline `LogCategory::setSeverity()`, as it is merely an\n> > > assignment, so going through a DSO call is a big pessimization.\n> > > `LogCategory` is not part of the public API, so this change has\n> > > no external effects.\n> > >\n> > > Thirdly, assert that the atomic variable is lock free so as to ensure\n> > > it won't silently fall back to libatomic (or similar) on any platform.\n> > > If this assertion fails, this needs to be revisited.\n> >\n> > Would it be that bad if some platforms require locking in this case ?\n> > Changing log level at run-time doesn't seem like an hot path or even\n> > something that common.\n>\n> This applies to reads as well, which is done in the destructor of every `LogMessage`.\n> I believe that is too big of a slowdown to accept without more consideration,\n> hence the assertion.\n\nVery good point, I had missed that read.\n\nFull steam ahead then!\n\nThanks\n j\n>\n>\n> >\n> > Anyway\n> > Reviewed-by: Jacopo Mondi \n> >\n> > Thanks\n> > j\n> >\n> >\n> > >\n> > > Signed-off-by: Barnabás Pőcze \n> > > Reviewed-by: Laurent Pinchart \n> > > ---\n> > > include/libcamera/base/log.h | 9 ++++++---\n> > > src/libcamera/base/log.cpp | 5 +----\n> > > 2 files changed, 7 insertions(+), 7 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n> > > index 6d2c93019..4137d87a4 100644\n> > > --- a/include/libcamera/base/log.h\n> > > +++ b/include/libcamera/base/log.h\n> > > @@ -7,6 +7,7 @@\n> > >\n> > > #pragma once\n> > >\n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > > @@ -31,8 +32,8 @@ public:\n> > > \tstatic LogCategory *create(const char *name);\n> > >\n> > > \tconst std::string &name() const { return name_; }\n> > > -\tLogSeverity severity() const { return severity_; }\n> > > -\tvoid setSeverity(LogSeverity severity);\n> > > +\tLogSeverity severity() const { return severity_.load(std::memory_order_relaxed); }\n> > > +\tvoid setSeverity(LogSeverity severity) { severity_.store(severity, std::memory_order_relaxed); }\n> > >\n> > > \tstatic const LogCategory &defaultCategory();\n> > >\n> > > @@ -40,7 +41,9 @@ private:\n> > > \texplicit LogCategory(const char *name);\n> > >\n> > > \tconst std::string name_;\n> > > -\tLogSeverity severity_;\n> > > +\n> > > +\tstd::atomic severity_;\n> > > +\tstatic_assert(decltype(severity_)::is_always_lock_free);\n> > > };\n> > >\n> > > #define LOG_DECLARE_CATEGORY(name)\t\t\t\t\t\\\n> > > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp\n> > > index 51a5cd470..2abbad478 100644\n> > > --- a/src/libcamera/base/log.cpp\n> > > +++ b/src/libcamera/base/log.cpp\n> > > @@ -824,15 +824,12 @@ LogCategory::LogCategory(const char *name)\n> > > */\n> > >\n> > > /**\n> > > + * \\fn LogCategory::setSeverity(LogSeverity severity)\n> > > * \\brief Set the severity of the log category\n> > > *\n> > > * Messages of severity higher than or equal to the severity of the log category\n> > > * are printed, other messages are discarded.\n> > > */\n> > > -void LogCategory::setSeverity(LogSeverity severity)\n> > > -{\n> > > -\tseverity_ = severity;\n> > > -}\n> > >\n> > > /**\n> > > * \\brief Retrieve the default log category\n> > > --\n> > > 2.48.1\n> > >\n> > >\n> >","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 C3E27BD80A\n\tfor ;\n\tMon, 3 Feb 2025 18:46:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F41AD685A6;\n\tMon, 3 Feb 2025 19:46:21 +0100 (CET)","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 9777461876\n\tfor ;\n\tMon, 3 Feb 2025 19:46:19 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CF255497;\n\tMon, 3 Feb 2025 19:45:07 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"RJicy6Ld\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738608307;\n\tbh=Mp5nnfMExUBYIkHmE7VoPyr6z82shLW3AkxidJ/m6IE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RJicy6LdUjFPLvUfghCNDdjHnUkBzReiCLlyXDcjAVL42mE889YIsNPx/0wJdEPYI\n\thMOtxfHWALgQLT/44tNNdo/X/V4yN1uwA5Y/MSSLiGK25n88LfSOZWcJlIUGqRLPl+\n\th1P7zQLYlLur0D7fBP6fEADWkw6u+hk1WvMqiTFk=","Date":"Mon, 3 Feb 2025 19:46:16 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tLaurent Pinchart ","Subject":"Re: [RFC PATCH v2 4/9] libcamera: base: log: Make\n\t`LogCategory::severity_` atomic","Message-ID":"<7lnm54wi4qlfuhhwwwyybh7ljjflk7aw243bqduuff2qo5kyol@buuipyhblhl5>","References":"<20250130195811.1230581-1-pobrn@protonmail.com>\n\t<20250130195811.1230581-5-pobrn@protonmail.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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\" "}}]