[RFC,v2,4/9] libcamera: base: log: Make `LogCategory::severity_` atomic
diff mbox series

Message ID 20250130195811.1230581-5-pobrn@protonmail.com
State New
Headers show
Series
  • libcamera: base: log: Misc. changes
Related show

Commit Message

Barnabás Pőcze Jan. 30, 2025, 7:58 p.m. UTC
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(-)

Comments

Jacopo Mondi Feb. 3, 2025, 4:43 p.m. UTC | #1
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
>
>
Barnabás Pőcze Feb. 3, 2025, 6:18 p.m. UTC | #2
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
> >
> >
>
Jacopo Mondi Feb. 3, 2025, 6:46 p.m. UTC | #3
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
> > >
> > >
> >

Patch
diff mbox series

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