[libcamera-devel] [PATCH v3 1/2] cam: event_loop: Stop queuing calls when the event loop are exiting
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue Feb 2 23:08:40 CET 2021
Hi Jacopo,
Thanks for your feedback.
On 2021-02-01 18:07:37 +0100, Jacopo Mondi wrote:
> Hi Niklas,
>
> On Mon, Feb 01, 2021 at 05:18:00PM +0100, Niklas Söderlund wrote:
> > 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;
> > }
>
> Ah ok, this is cleaner and..
>
> >
> > > 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(),
>
> .. make the windown smaller, as if I'm not mistaken you still have a
> window.
>
> lock()
> calls = move(calls_)
> unlock()
> <------------- calls_.push_back(new callback)
> if (calls.empty())
> return false;
>
> Can't this happen ? I don't know very much the cam event model, so I
> might be missing something.
>
> (nit: I would make dispatchCalls() return true if all calls are
> dispatched)
>
> if (dispatchCalls())
> exit();
This is a good point. I have dug a bit in libevnet itself and I believe
I have solution where we can avoid this and the problem I try to address
in this patch all together. Will post patches with this new idea as v4.
>
> >
> > 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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list