[RFC PATCH v2 4/9] libcamera: base: log: Make `LogCategory::severity_` atomic

[Jacopo Mondi]

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 at ideasonboard.com>

Thanks
  j


>
> Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at 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
>
>