[libcamera-devel] [PATCH v4 2/3] cam: event_loop: Execute events in the libevent loop

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2021-02-04 02:44:04 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Tue, Feb 02, 2021 at 11:10:50PM +0100, Niklas Söderlund wrote:
> > Cam uses libevent to deal with threads and idle loop while still
> > implementing its own event queue. This creates issues when the event
> > loop is terminated as it might get stuck in the idle loop if exit() is
> > called while the thread is busy with dispatchCalls().
> > 
> > Solve this my removing the custom event execution and instead injecting
> 
> s/my/by/
> 
> > the calls as events to the base event loop.
> > 
> > Reported-by: Sebastian Fricke <sebastian.fricke at posteo.net>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/cam/event_loop.cpp | 43 ++++++++++++++++++++----------------------
> >  src/cam/event_loop.h   |  6 +-----
> >  2 files changed, 21 insertions(+), 28 deletions(-)
> > 
> > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> > index 3a2b665abdc063de..e84e437f1f8ff6d7 100644
> > --- a/src/cam/event_loop.cpp
> > +++ b/src/cam/event_loop.cpp
> > @@ -13,6 +13,13 @@
> >  
> >  EventLoop *EventLoop::instance_ = nullptr;
> >  
> > +static void dispatchCallback([[maybe_unused]] evutil_socket_t fd,
> > +			     [[maybe_unused]] short flags, void *param)
> > +{
> > +	EventLoop *loop = static_cast<EventLoop *>(param);
> > +	loop->dispatchCall();
> > +}
> 
> This could be a private static function of the EventLoop class, which
> would allow EventLoop::dispatchCall() to become a private function. The
> event_loop.h header will have an issue with the undefined
> evutil_socket_t type, which could be solved by either including
> event2/util.h, or using the int type instead (evutil_socket_t is used to
> abstract platforms, I really doubt libevent will switch to another type
> than int on Linux).

That is a very good idea! I will include event2/util.h as I think the 
correct type really should be used.

> 
> > +
> >  EventLoop::EventLoop()
> >  {
> >  	assert(!instance_);
> > @@ -38,25 +45,13 @@ EventLoop *EventLoop::instance()
> >  int EventLoop::exec()
> >  {
> >  	exitCode_ = -1;
> > -	exit_.store(false, std::memory_order_release);
> > -
> > -	while (!exit_.load(std::memory_order_acquire)) {
> > -		dispatchCalls();
> > -		event_base_loop(base_, EVLOOP_NO_EXIT_ON_EMPTY);
> > -	}
> > -
> > +	event_base_loop(base_, EVLOOP_NO_EXIT_ON_EMPTY);
> >  	return exitCode_;
> >  }
> >  
> >  void EventLoop::exit(int code)
> >  {
> >  	exitCode_ = code;
> > -	exit_.store(true, std::memory_order_release);
> > -	interrupt();
> > -}
> > -
> > -void EventLoop::interrupt()
> > -{
> >  	event_base_loopbreak(base_);
> >  }
> >  
> > @@ -67,20 +62,22 @@ void EventLoop::callLater(const std::function<void()> &func)
> >  		calls_.push_back(func);
> >  	}
> >  
> > -	interrupt();
> > +	struct event *event = event_new(base_, -1, 0, dispatchCallback, this);
> > +	event_active(event, 0, 0);
> 
> I believe the memory allocated by event_new() is leaked. valgrind seems
> to agree with me :-) The following may fix it:
> 
> 	event_base_once(base_, -1, EV_TIMEOUT, dispatchCallback, this, NULL);

Thanks!

> 
> We could go one step further and drop the calls_ list, with the
> following:
> 
> void EventLoop::callLater(const std::function<void()> &func)
> {
> 	event_base_once(base_, -1, EV_TIMEOUT, EventLoop::dispatchCallback,
> 			new std::function<void()>(func), NULL);
> }
> 
> void EventLoop::dispatchCallback([[maybe_unused]] int fd,
> 				 [[maybe_unused]] short flags, void *param)
> {
> 	std::function<void()> *call = static_cast<std::function<void()> *>(param);
> 	(*call)();
> 	delete call;
> }
> 
> There's one issue though, as libevent (at least starting with version
> 2.1) guarantees that the event allocated internally by event_base_once()
> will always be freed even if the event is never triggered (I believe
> this can happen if event_base_loopbreak() races with event_base_once(),
> but that may actually not be the case), but we would in that case leak
> the memory allocated for the argument to the callback. The
> event_base_once() explains this, and states that applications are on
> their own to free this memory. The simplest way to do so would be to
> store the call in a calls_ list, so we would be back to where we are
> today.
> 
> Without support in libevent to handle this issue, I don't think there's
> much we could do to reduce the amonut of code we have here, but I may
> have missed something.

I tried something similar before posting this, and as you point out I 
could not find a good way to handle events still queued after the loop 
is stopped. I think I will keep this as is for now and if we learn more 
of libevent we could switch to it later.

> 
> >  }
> >  
> > -void EventLoop::dispatchCalls()
> > +void EventLoop::dispatchCall()
> >  {
> > -	std::unique_lock<std::mutex> locker(lock_);
> > +	std::function<void()> call;
> >  
> > -	for (auto iter = calls_.begin(); iter != calls_.end(); ) {
> > -		std::function<void()> call = std::move(*iter);
> > +	{
> > +		std::unique_lock<std::mutex> locker(lock_);
> > +		if (calls_.empty())
> > +			return;
> >  
> > -		iter = calls_.erase(iter);
> > -
> > -		locker.unlock();
> > -		call();
> > -		locker.lock();
> > +		call = calls_.front();
> > +		calls_.pop_front();
> >  	}
> > +
> > +	call();
> 
> We may be reducing the efficiency a little bit by queuing one event to
> libevent for each call to dispatch, but I think that's fine. cam's goal
> isn't to optimize event handling, relying on features provided by
> libevent as much as possible to keep the code simple is best.

Agreed.

> 
> The part that bothers me a bit with this patch is that I half recall
> doing something similar when switching to libevent, and running into
> issues. As that memory could very well be incorrect, and as this patch
> seems fine,

Yea this seems too simple, but I can't think how it can blow up at a 
moment. Oh well for future me to discover and hopefully solve ;-)

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> with the above issues addressed.
> 
> >  }
> > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
> > index d0d5b5a53c830670..5f1cd88c2f5c830e 100644
> > --- a/src/cam/event_loop.h
> > +++ b/src/cam/event_loop.h
> > @@ -7,7 +7,6 @@
> >  #ifndef __CAM_EVENT_LOOP_H__
> >  #define __CAM_EVENT_LOOP_H__
> >  
> > -#include <atomic>
> >  #include <functional>
> >  #include <list>
> >  #include <mutex>
> > @@ -26,19 +25,16 @@ public:
> >  	void exit(int code = 0);
> >  
> >  	void callLater(const std::function<void()> &func);
> > +	void dispatchCall();
> >  
> >  private:
> >  	static EventLoop *instance_;
> >  
> >  	struct event_base *base_;
> > -	std::atomic<bool> exit_;
> >  	int exitCode_;
> >  
> >  	std::list<std::function<void()>> calls_;
> >  	std::mutex lock_;
> > -
> > -	void interrupt();
> > -	void dispatchCalls();
> >  };
> >  
> >  #endif /* __CAM_EVENT_LOOP_H__ */
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund