| Message ID | 20250121185554.301901-2-pobrn@protonmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Thank you for the patch. On Tue, Jan 21, 2025 at 06:56:02PM +0000, Barnabás Pőcze wrote: > The severity of a log category may be changed from a different thread, > so it is important to ensure that the reads and writes happen atomically. > Using `std::memory_order_relaxed` should not introduce any synchronization > overhead, it should only guarantee that the operation itself is atomic. > > Secondly, inline `LogCategory::setSeverity()`, as it is merely an > assignment, so going through a DSO call is a big pessimization. > `LogCategory` is not part of the public API, so this change has > no external effects. > > Thirdly, assert that the atomic variable is lock free so as to ensure > it won't silently fall back to libatomic (or similar) on any platform. > If this assertion fails, this needs to be revisited. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/base/log.h | 9 ++++++--- > src/libcamera/base/log.cpp | 5 +---- > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h > index d73aa7913..89f707e54 100644 > --- a/include/libcamera/base/log.h > +++ b/include/libcamera/base/log.h > @@ -7,6 +7,7 @@ > > #pragma once > > +#include <atomic> > #include <sstream> > > #include <libcamera/base/private.h> > @@ -31,8 +32,8 @@ public: > static LogCategory *create(const char *name); > > const std::string &name() const { return name_; } > - LogSeverity severity() const { return severity_; } > - void setSeverity(LogSeverity severity); > + LogSeverity severity() const { return severity_.load(std::memory_order_relaxed); } > + void setSeverity(LogSeverity severity) { severity_.store(severity, std::memory_order_relaxed); } > > static const LogCategory &defaultCategory(); > > @@ -40,7 +41,9 @@ private: > explicit LogCategory(const char *name); > > const std::string name_; > - LogSeverity severity_; > + > + std::atomic<LogSeverity> severity_; > + static_assert(decltype(severity_)::is_always_lock_free); > }; > > #define LOG_DECLARE_CATEGORY(name) \ > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp > index 1513e41fb..795260b0a 100644 > --- a/src/libcamera/base/log.cpp > +++ b/src/libcamera/base/log.cpp > @@ -825,15 +825,12 @@ LogCategory::LogCategory(const char *name) > */ > > /** > + * \fn LogCategory::setSeverity(LogSeverity severity) > * \brief Set the severity of the log category > * > * Messages of severity higher than or equal to the severity of the log category > * are printed, other messages are discarded. > */ > -void LogCategory::setSeverity(LogSeverity severity) > -{ > - severity_ = severity; > -} > > /** > * \brief Retrieve the default log category
diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h index d73aa7913..89f707e54 100644 --- a/include/libcamera/base/log.h +++ b/include/libcamera/base/log.h @@ -7,6 +7,7 @@ #pragma once +#include <atomic> #include <sstream> #include <libcamera/base/private.h> @@ -31,8 +32,8 @@ public: static LogCategory *create(const char *name); const std::string &name() const { return name_; } - LogSeverity severity() const { return severity_; } - void setSeverity(LogSeverity severity); + LogSeverity severity() const { return severity_.load(std::memory_order_relaxed); } + void setSeverity(LogSeverity severity) { severity_.store(severity, std::memory_order_relaxed); } static const LogCategory &defaultCategory(); @@ -40,7 +41,9 @@ private: explicit LogCategory(const char *name); const std::string name_; - LogSeverity severity_; + + std::atomic<LogSeverity> severity_; + static_assert(decltype(severity_)::is_always_lock_free); }; #define LOG_DECLARE_CATEGORY(name) \ diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp index 1513e41fb..795260b0a 100644 --- a/src/libcamera/base/log.cpp +++ b/src/libcamera/base/log.cpp @@ -825,15 +825,12 @@ LogCategory::LogCategory(const char *name) */ /** + * \fn LogCategory::setSeverity(LogSeverity severity) * \brief Set the severity of the log category * * Messages of severity higher than or equal to the severity of the log category * are printed, other messages are discarded. */ -void LogCategory::setSeverity(LogSeverity severity) -{ - severity_ = severity; -} /** * \brief Retrieve the default log category
The severity of a log category may be changed from a different thread, so it is important to ensure that the reads and writes happen atomically. Using `std::memory_order_relaxed` should not introduce any synchronization overhead, it should only guarantee that the operation itself is atomic. Secondly, inline `LogCategory::setSeverity()`, as it is merely an assignment, so going through a DSO call is a big pessimization. `LogCategory` is not part of the public API, so this change has no external effects. Thirdly, assert that the atomic variable is lock free so as to ensure it won't silently fall back to libatomic (or similar) on any platform. If this assertion fails, this needs to be revisited. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- include/libcamera/base/log.h | 9 ++++++--- src/libcamera/base/log.cpp | 5 +---- 2 files changed, 7 insertions(+), 7 deletions(-)