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

[Laurent Pinchart]

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

> +
>  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);

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.

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

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,

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