[libcamera-devel] [PATCH v3 1/2] cam: event_loop: Stop queuing calls when the event loop are exiting

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2021-02-01 10:35:18 +0100, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Sat, Jan 30, 2021 at 01:19:14AM +0100, Niklas Söderlund wrote:
> > Terminating the event loop with EventLoop::exit() does not grantee that
> 
> s/grantee/guarantee
> 
> > it will terminate. If the event loops 'call later' queue can be kept
> 
> event loop
> 
> > non-empty by continuously queuing new calls using EventLoop::callLater()
> > either from a different thread or from callbacks in the loop itself
> > EventLoop::dispatchCalls() will never complete and the loop will run
> > forever.
> >
> > Solve this by only executing the already queued calls each invocation of
> > dispatchCalls() and only enter the idle loop if dispatchCalls() had no
> > calls to dispatch.
> >
> > Reported-by: Sebastian Fricke <sebastian.fricke at posteo.net>
> > Fixes: f49e93338b6309a6 ("cam: event_loop: Add deferred calls support")
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/cam/event_loop.cpp | 21 +++++++++------------
> >  src/cam/event_loop.h   |  2 +-
> >  2 files changed, 10 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> > index 94c5d1d362455f33..f0b1ecbb6244c40a 100644
> > --- a/src/cam/event_loop.cpp
> > +++ b/src/cam/event_loop.cpp
> > @@ -41,8 +41,8 @@ int EventLoop::exec()
> >  	exit_.store(false, std::memory_order_release);
> >
> >  	while (!exit_.load(std::memory_order_acquire)) {
> > -		dispatchCalls();
> > -		event_base_loop(event_, EVLOOP_NO_EXIT_ON_EMPTY);
> > +		if (!dispatchCalls())
> > +			event_base_loop(event_, EVLOOP_NO_EXIT_ON_EMPTY);
> >  	}
> >
> >  	return exitCode_;
> > @@ -70,17 +70,14 @@ void EventLoop::callLater(const std::function<void()> &func)
> >  	interrupt();
> >  }
> >
> > -void EventLoop::dispatchCalls()
> > +bool EventLoop::dispatchCalls()
> >  {
> > -	std::unique_lock<std::mutex> locker(lock_);
> > +	lock_.lock();
> > +	std::list<std::function<void()>> calls = std::move(calls_);
> > +	lock_.unlock();
> 
> After this last unlock:
> calls contains all the callbacks
> calls_ now empty
> calls_ can be populated with more callbacks as there's no lock held

Correct.

> 
> >
> > -	for (auto iter = calls_.begin(); iter != calls_.end(); ) {
> > -		std::function<void()> call = std::move(*iter);
> > -
> > -		iter = calls_.erase(iter);
> > -
> > -		locker.unlock();
> > +	for (std::function<void()> call : calls)
> >  		call();
> 
> > -		locker.lock();
> > -	}
> > +
> > +	return !calls.empty();
> 
> How can calls be empty as you never remove items ?

If calls_ was empty at the time of the std::move(). And this only 
happens when there is no calls to be processed and we should enter the 
event_base_loop() idle loop.

Reading the diff now I could have done something like this to make this 
more clear,

	bool EventLoop::dispatchCalls()
	{
	    lock_.lock();
	    std::list<std::function<void()>> calls = std::move(calls_);
	    lock_.unlock();

	    if (calls.empty())
		    return false;

	    for (std::function<void()> call : calls)
		    call();

	    return true;
	}

> As callbacks can be add to calls_ while you are iteraring here, won't
> them be lost forever ?

How?

While iterating here more calls may be added calls_. If we iterate one 
ore more calls here dispatchCalls() will return true. The only call site 
is exec(),

    int EventLoop::exec()
    {
	    exitCode_ = -1;
	    exit_.store(false, std::memory_order_release);

	    while (!exit_.load(std::memory_order_acquire)) {
		    if (!dispatchCalls())
			    event_base_loop(event_, EVLOOP_NO_EXIT_ON_EMPTY);
	    }

	    return exitCode_;
    }

On dispatchCalls() returning true we do not enter the event_base_loop() 
idle loop and instead call dispatchCalls() once more consuming any calls 
appended to calls_.

> 
> I feel like I'm missing something

Might as well be me who are missing things :-)

> 
> >  }
> > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
> > index 408073c50594d09d..b2535f7bdd96742a 100644
> > --- a/src/cam/event_loop.h
> > +++ b/src/cam/event_loop.h
> > @@ -38,7 +38,7 @@ private:
> >  	std::mutex lock_;
> >
> >  	void interrupt();
> > -	void dispatchCalls();
> > +	bool dispatchCalls();
> >  };
> >
> >  #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

-- 
Regards,
Niklas Söderlund