[RFC PATCH v2 8/9] libcamera: base: log: Protect log categories with lock

[Barnabás Pőcze]

Hi


2025. február 6., csütörtök 18:11 keltezéssel, Laurent Pinchart <laurent.pinchart at ideasonboard.com> írta:

> Hi Barnabás,
> 
> Thank you for the patch.
> 
> On Thu, Jan 30, 2025 at 07:58:57PM +0000, Barnabás Pőcze wrote:
> > Log categories may be added from any thread, so it is important
> > to synchronize access to the `Logger::categories_` list between
> > its two users: `Logger::findOrCreateCategory()` and `Logger::logSetLevel()`.
> 
> findOrCreateCategory() is a new function, it's not a current user. You
> could write something along those lines:
> 
> Log categories may be added from any thread, so it is important to
> synchronize access to the `Logger::categories_` list between its two
> users: category creation (by LogCategory::create(), which calls
> Logger::findCategory() and Logger::registerCategory()) and log level
> setting (by Logger::logSetLevel()).
> 
> > To achieve that, `Logger::{find,register}Category()` are merged into
> > a single function: `Logger::findOrCreateCategory()`; and the mutex
> > from `LogCategory::create()` is moved into the `Logger` class.
> >
> > Furthermore, appropriate `MutexLocker`s are placed in
> > `Logger::findOrCreateCategory()` and `Logger::logSetLevel()`.
> 
> It took me a bit of time to understand the patch, here's a proposal for
> an improved commit message.
> 
> The LogCategory::create() function uses a mutex to serialize category
> creation, but Logger::logSetLevel() can access Logger::categories_
> concurrently without any protection. To fix the issue, move the mutex to
> the Logger class, and use it to protect all accesses to the categories
> list. This requires moving all the logic of LogCategory::create() to a
> new Logger::findOrCreateCategory() function that combines both
> Logger::findCategory() and Logger::registerCategory() in order to make
> the two operations exacute atomically.

ACK


> 
> > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > ---
> >  include/libcamera/base/log.h |  2 +-
> >  src/libcamera/base/log.cpp   | 53 ++++++++++++------------------------
> >  2 files changed, 19 insertions(+), 36 deletions(-)
> >
> > diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h
> > index 1fb92603f..b32ec4516 100644
> > --- a/include/libcamera/base/log.h
> > +++ b/include/libcamera/base/log.h
> > @@ -30,6 +30,7 @@ enum LogSeverity {
> >  class LogCategory
> >  {
> >  public:
> > +	explicit LogCategory(std::string_view name);
> 
> It's not very nice to make this publicly available, as it shouldn't be
> used anywhere else than from the create() function (now) or the
> findOrCreateCategory() function (with this patch applied). Wouldn't it
> be better to keep the constructor private and make the Logger class a
> friend ?

I think this change might be here by mistake. I think it might only be needed
for the last patch. (It is unfortunate that having only private constructors
prevents the use of `make_{unique,shared}()`, `emplace_back()`, etc.)


> 
> >  	static LogCategory *create(std::string_view name);
> >
> >  	const std::string &name() const { return name_; }
> > @@ -39,7 +40,6 @@ public:
> >  	static const LogCategory &defaultCategory();
> >
> >  private:
> > -	explicit LogCategory(std::string_view name);
> >
> >  	const std::string name_;
> >
> > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> > index 19fc5cc67..69aa16c4c 100644
> > --- a/src/libcamera/base/log.cpp
> > +++ b/src/libcamera/base/log.cpp
> > @@ -402,11 +402,11 @@ private:
> >  	void parseLogFile();
> >
> >  	friend LogCategory;
> > -	void registerCategory(LogCategory *category);
> > -	LogCategory *findCategory(std::string_view name) const;
> > +	LogCategory *findOrCreateCategory(std::string_view name);
> >
> >  	static bool destroyed_;
> >
> > +	Mutex mutex_;
> >  	std::vector<LogCategory *> categories_;
> 
> Can we add thread safety annotations to indicate that categories_ is
> protected by mutex_ ?

I think so.


> 
> >  	std::list<std::pair<std::string, LogSeverity>> levels_;
> >
> > @@ -657,6 +657,8 @@ void Logger::logSetLevel(const char *category, const char *level)
> >  	if (severity == LogInvalid)
> >  		return;
> >
> > +	MutexLocker locker(mutex_);
> > +
> >  	for (LogCategory *c : categories_) {
> >  		if (c->name() == category) {
> >  			c->setSeverity(severity);
> > @@ -707,17 +709,21 @@ void Logger::parseLogFile()
> >  }
> >
> >  /**
> > - * \brief Register a log category with the logger
> > - * \param[in] category The log category
> > - *
> > - * Log categories must have unique names. It is invalid to call this function
> > - * if a log category with the same name already exists.
> > + * \brief Find an existing log category with the given name or create one
> > + * \param[in] name Name of the log category
> > + * \return The pointer to the log category found or created
> >   */
> > -void Logger::registerCategory(LogCategory *category)
> > +LogCategory *Logger::findOrCreateCategory(std::string_view name)
> >  {
> > -	categories_.push_back(category);
> > +	MutexLocker locker(mutex_);
> > +
> > +	for (LogCategory *category : categories_) {
> > +		if (category->name() == name)
> > +			return category;
> > +	}
> 
> So nobody likes std::find_if() ? :-( I can survive its removal.
> 
> > +
> > +	LogCategory *category = categories_.emplace_back(new LogCategory(name));
> >
> > -	const std::string &name = category->name();
> >  	for (const std::pair<std::string, LogSeverity> &level : levels_) {
> >  		bool match = true;
> >
> > @@ -737,22 +743,8 @@ void Logger::registerCategory(LogCategory *category)
> >  			break;
> >  		}
> >  	}
> > -}
> >
> > -/**
> > - * \brief Find an existing log category with the given name
> > - * \param[in] name Name of the log category
> > - * \return The pointer to the found log category or nullptr if not found
> > - */
> > -LogCategory *Logger::findCategory(std::string_view name) const
> > -{
> > -	if (auto it = std::find_if(categories_.begin(), categories_.end(),
> > -				   [name](auto c) { return c->name() == name; });
> > -	    it != categories_.end()) {
> > -		return *it;
> > -	}
> > -
> > -	return nullptr;
> > +	return category;
> >  }
> >
> >  /**
> > @@ -790,16 +782,7 @@ LogCategory *Logger::findCategory(std::string_view name) const
> >   */
> >  LogCategory *LogCategory::create(std::string_view name)
> >  {
> > -	static Mutex mutex_;
> > -	MutexLocker locker(mutex_);
> > -	LogCategory *category = Logger::instance()->findCategory(name);
> > -
> > -	if (!category) {
> > -		category = new LogCategory(name);
> > -		Logger::instance()->registerCategory(category);
> > -	}
> > -
> > -	return category;
> > +	return Logger::instance()->findOrCreateCategory(name);
> >  }
> >
> >  /**
> 
> --
> Regards,
> 
> Laurent Pinchart
>