[libcamera-devel] [PATCH 12/19] libcamera: signal: Make connection and disconnection thread-safe

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jan 20, 2020 at 04:02:31PM +0100, Jacopo Mondi wrote:
> On Mon, Jan 20, 2020 at 02:24:30AM +0200, Laurent Pinchart wrote:
> > Make the signal connection and disconnection thread-safe, and document
> > them as such. This is required to make objects connectable from
> > different threads.
> >
> > The connect(), disconnect() and emit() methods are now all protected by
> > a global mutex, which may generate a high lock contention. This could be
> > improved with finer-grained locks or with a pool of mutexes.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/signal.cpp | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >
> > diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp
> > index 9bb7eca8ed6e..6eab1fa74d42 100644
> > --- a/src/libcamera/signal.cpp
> > +++ b/src/libcamera/signal.cpp
> > @@ -7,6 +7,8 @@
> >
> >  #include <libcamera/signal.h>
> >
> > +#include "thread.h"
> > +
> 
> Isn't it weird we need to include thread.h to access MutexLocker ?

We could move that to a separate file, but it's so small that I didn't
think it was worth when I added it. Mutexes are useless without threads
anyway.

> Anyway,
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> >  /**
> >   * \file signal.h
> >   * \brief Signal & slot implementation
> > @@ -14,8 +16,21 @@
> >
> >  namespace libcamera {
> >
> > +namespace {
> > +
> > +/*
> > + * Mutex to protect the SignalBase::slots_ and Object::signals_ lists. If lock
> > + * contention needs to be decreased, this could be replaced with locks in
> > + * Object and SignalBase, or with a mutex pool.
> > + */
> > +Mutex signalsLock;
> > +
> > +} /* namespace */
> > +
> >  void SignalBase::connect(BoundMethodBase *slot)
> >  {
> > +	MutexLocker locker(signalsLock);
> > +
> >  	Object *object = slot->object();
> >  	if (object)
> >  		object->connect(this);
> > @@ -31,6 +46,8 @@ void SignalBase::disconnect(Object *object)
> >
> >  void SignalBase::disconnect(std::function<bool(SlotList::iterator &)> match)
> >  {
> > +	MutexLocker locker(signalsLock);
> > +
> >  	for (auto iter = slots_.begin(); iter != slots_.end(); ) {
> >  		if (match(iter)) {
> >  			Object *object = (*iter)->object();
> > @@ -47,6 +64,7 @@ void SignalBase::disconnect(std::function<bool(SlotList::iterator &)> match)
> >
> >  SignalBase::SlotList SignalBase::slots()
> >  {
> > +	MutexLocker locker(signalsLock);
> >  	return slots_;
> >  }
> >
> > @@ -99,23 +117,31 @@ SignalBase::SlotList SignalBase::slots()
> >   * disconnected from the \a func slot of \a object when \a object is destroyed.
> >   * Otherwise the caller shall disconnect signals manually before destroying \a
> >   * object.
> > + *
> > + * \context This function is \threadsafe.
> >   */
> >
> >  /**
> >   * \fn Signal::connect(R (*func)(Args...))
> >   * \brief Connect the signal to a static function slot
> >   * \param[in] func The slot static function
> > + *
> > + * \context This function is \threadsafe.
> >   */
> >
> >  /**
> >   * \fn Signal::disconnect()
> >   * \brief Disconnect the signal from all slots
> > + *
> > + * \context This function is \threadsafe.
> >   */
> >
> >  /**
> >   * \fn Signal::disconnect(T *object)
> >   * \brief Disconnect the signal from all slots of the \a object
> >   * \param[in] object The object pointer whose slots to disconnect
> > + *
> > + * \context This function is \threadsafe.
> >   */
> >
> >  /**
> > @@ -123,12 +149,16 @@ SignalBase::SlotList SignalBase::slots()
> >   * \brief Disconnect the signal from the \a object slot member function \a func
> >   * \param[in] object The object pointer whose slots to disconnect
> >   * \param[in] func The slot member function to disconnect
> > + *
> > + * \context This function is \threadsafe.
> >   */
> >
> >  /**
> >   * \fn Signal::disconnect(R (*func)(Args...))
> >   * \brief Disconnect the signal from the slot static function \a func
> >   * \param[in] func The slot static function to disconnect
> > + *
> > + * \context This function is \threadsafe.
> >   */
> >
> >  /**
> > @@ -141,6 +171,9 @@ SignalBase::SlotList SignalBase::slots()
> >   * function are passed to the slot functions unchanged. If a slot modifies one
> >   * of the arguments (when passed by pointer or reference), the modification is
> >   * thus visible to all subsequently called slots.
> > + *
> > + * This function is not \threadsafe, but thread-safety is guaranteed against
> > + * concurrent connect() and disconnect() calls.
> >   */
> >
> >  } /* namespace libcamera */
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel



-- 
Regards,

Laurent Pinchart