| Message ID | 20210130001915.489703-2-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@posteo.net> > Fixes: f49e93338b6309a6 ("cam: event_loop: Add deferred calls support") > Signed-off-by: Niklas Söderlund <niklas.soderlund@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 > > - 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 ? As callbacks can be add to calls_ while you are iteraring here, won't them be lost forever ? I feel like I'm missing something > } > 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@posteo.net> > > Fixes: f49e93338b6309a6 ("cam: event_loop: Add deferred calls support") > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
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@posteo.net> > > > Fixes: f49e93338b6309a6 ("cam: event_loop: Add deferred calls support") > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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(); > > 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@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
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@posteo.net> > > > > Fixes: f49e93338b6309a6 ("cam: event_loop: Add deferred calls support") > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund
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(); - 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(); } 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__ */
Terminating the event loop with EventLoop::exit() does not grantee that it will terminate. If the event loops 'call later' queue can be kept 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@posteo.net> Fixes: f49e93338b6309a6 ("cam: event_loop: Add deferred calls support") Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/cam/event_loop.cpp | 21 +++++++++------------ src/cam/event_loop.h | 2 +- 2 files changed, 10 insertions(+), 13 deletions(-)