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

[Jacopo Mondi]

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 at 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 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
> > >
> > >
> >