| Message ID | 20210202221051.1740237-3-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@posteo.net> > Signed-off-by: Niklas Söderlund <niklas.soderlund@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@jmondi.org> Thanks j > }; > > #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, 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@posteo.net> > Signed-off-by: Niklas Söderlund <niklas.soderlund@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@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__ */
Hi Laurent, Thanks for your feedback. On 2021-02-04 02:44:04 +0200, Laurent Pinchart wrote: > 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@posteo.net> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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). That is a very good idea! I will include event2/util.h as I think the correct type really should be used. > > > + > > 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); Thanks! > > 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. I tried something similar before posting this, and as you point out I could not find a good way to handle events still queued after the loop is stopped. I think I will keep this as is for now and if we learn more of libevent we could switch to it later. > > > } > > > > -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. Agreed. > > 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, Yea this seems too simple, but I can't think how it can blow up at a moment. Oh well for future me to discover and hopefully solve ;-) > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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
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_; } 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() { - 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(); }; #endif /* __CAM_EVENT_LOOP_H__ */
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 the calls as events to the base event loop. Reported-by: Sebastian Fricke <sebastian.fricke@posteo.net> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/cam/event_loop.cpp | 43 ++++++++++++++++++++---------------------- src/cam/event_loop.h | 6 +----- 2 files changed, 21 insertions(+), 28 deletions(-)