Message ID | 20250130195811.1230581-5-pobrn@protonmail.com |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
Hi Barnabás On Thu, Jan 30, 2025 at 07:58:34PM +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. Would it be that bad if some platforms require locking in this case ? Changing log level at run-time doesn't seem like an hot path or even something that common. Anyway Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > > 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 6d2c93019..4137d87a4 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 51a5cd470..2abbad478 100644 > --- a/src/libcamera/base/log.cpp > +++ b/src/libcamera/base/log.cpp > @@ -824,15 +824,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 > -- > 2.48.1 > >
2025. február 3., hétfő 17:43 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta: > Hi Barnabás > > On Thu, Jan 30, 2025 at 07:58:34PM +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. > > Would it be that bad if some platforms require locking in this case ? > Changing log level at run-time doesn't seem like an hot path or even > something that common. This applies to reads as well, which is done in the destructor of every `LogMessage`. I believe that is too big of a slowdown to accept without more consideration, hence the assertion. > > Anyway > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks > j > > > > > > 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 6d2c93019..4137d87a4 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 51a5cd470..2abbad478 100644 > > --- a/src/libcamera/base/log.cpp > > +++ b/src/libcamera/base/log.cpp > > @@ -824,15 +824,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 > > -- > > 2.48.1 > > > > >
Hi Barnabás On Mon, Feb 03, 2025 at 06:18:44PM +0000, Barnabás Pőcze wrote: > 2025. február 3., hétfő 17:43 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta: > > > Hi Barnabás > > > > On Thu, Jan 30, 2025 at 07:58:34PM +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. > > > > Would it be that bad if some platforms require locking in this case ? > > Changing log level at run-time doesn't seem like an hot path or even > > something that common. > > This applies to reads as well, which is done in the destructor of every `LogMessage`. > I believe that is too big of a slowdown to accept without more consideration, > hence the assertion. Very good point, I had missed that read. Full steam ahead then! Thanks j > > > > > > Anyway > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > Thanks > > j > > > > > > > > > > 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 6d2c93019..4137d87a4 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 51a5cd470..2abbad478 100644 > > > --- a/src/libcamera/base/log.cpp > > > +++ b/src/libcamera/base/log.cpp > > > @@ -824,15 +824,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 > > > -- > > > 2.48.1 > > > > > > > >
diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h index 6d2c93019..4137d87a4 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 51a5cd470..2abbad478 100644 --- a/src/libcamera/base/log.cpp +++ b/src/libcamera/base/log.cpp @@ -824,15 +824,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