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

[Jacopo Mondi]

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();
> +}
> +
>  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_;

Is this returning -1 unconditionally ? Is this intended ?

>  }
>
>  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);
>  }
>
> -void EventLoop::dispatchCalls()
> +void EventLoop::dispatchCall()

This now doesn't dispatch but rather run the first callback.
execCallback() ? execOne() ? runCallback() ?

>  {
> -	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();
>  }
> 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();

Change looks ok!
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

>  };
>
>  #endif /* __CAM_EVENT_LOOP_H__ */
> --
> 2.30.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel