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

[Barnabás Pőcze]

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.


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