[{"id":14920,"web_url":"https://patchwork.libcamera.org/comment/14920/","msgid":"<20210203113524.7l6q7go4uw2jkqyc@uno.localdomain>","date":"2021-02-03T11:35:24","subject":"Re: [libcamera-devel] [PATCH v4 2/3] cam: event_loop: Execute\n\tevents in the libevent loop","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Tue, Feb 02, 2021 at 11:10:50PM +0100, Niklas Söderlund wrote:\n> Cam uses libevent to deal with threads and idle loop while still\n> implementing its own event queue. This creates issues when the event\n> loop is terminated as it might get stuck in the idle loop if exit() is\n> called while the thread is busy with dispatchCalls().\n>\n> Solve this my removing the custom event execution and instead injecting\n\ns/my/by\n\n> the calls as events to the base event loop.\n>\n> Reported-by: Sebastian Fricke \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/cam/event_loop.cpp | 43 ++++++++++++++++++++----------------------\n> src/cam/event_loop.h | 6 +-----\n> 2 files changed, 21 insertions(+), 28 deletions(-)\n>\n> diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp\n> index 3a2b665abdc063de..e84e437f1f8ff6d7 100644\n> --- a/src/cam/event_loop.cpp\n> +++ b/src/cam/event_loop.cpp\n> @@ -13,6 +13,13 @@\n>\n> EventLoop *EventLoop::instance_ = nullptr;\n>\n> +static void dispatchCallback([[maybe_unused]] evutil_socket_t fd,\n> +\t\t\t [[maybe_unused]] short flags, void *param)\n> +{\n> +\tEventLoop *loop = static_cast(param);\n> +\tloop->dispatchCall();\n> +}\n> +\n> EventLoop::EventLoop()\n> {\n> \tassert(!instance_);\n> @@ -38,25 +45,13 @@ EventLoop *EventLoop::instance()\n> int EventLoop::exec()\n> {\n> \texitCode_ = -1;\n> -\texit_.store(false, std::memory_order_release);\n> -\n> -\twhile (!exit_.load(std::memory_order_acquire)) {\n> -\t\tdispatchCalls();\n> -\t\tevent_base_loop(base_, EVLOOP_NO_EXIT_ON_EMPTY);\n> -\t}\n> -\n> +\tevent_base_loop(base_, EVLOOP_NO_EXIT_ON_EMPTY);\n> \treturn exitCode_;\n\nIs this returning -1 unconditionally ? Is this intended ?\n\n> }\n>\n> void EventLoop::exit(int code)\n> {\n> \texitCode_ = code;\n> -\texit_.store(true, std::memory_order_release);\n> -\tinterrupt();\n> -}\n> -\n> -void EventLoop::interrupt()\n> -{\n> \tevent_base_loopbreak(base_);\n> }\n>\n> @@ -67,20 +62,22 @@ void EventLoop::callLater(const std::function &func)\n> \t\tcalls_.push_back(func);\n> \t}\n>\n> -\tinterrupt();\n> +\tstruct event *event = event_new(base_, -1, 0, dispatchCallback, this);\n> +\tevent_active(event, 0, 0);\n> }\n>\n> -void EventLoop::dispatchCalls()\n> +void EventLoop::dispatchCall()\n\nThis now doesn't dispatch but rather run the first callback.\nexecCallback() ? execOne() ? runCallback() ?\n\n> {\n> -\tstd::unique_lock locker(lock_);\n> +\tstd::function call;\n>\n> -\tfor (auto iter = calls_.begin(); iter != calls_.end(); ) {\n> -\t\tstd::function call = std::move(*iter);\n> +\t{\n> +\t\tstd::unique_lock locker(lock_);\n> +\t\tif (calls_.empty())\n> +\t\t\treturn;\n>\n> -\t\titer = calls_.erase(iter);\n> -\n> -\t\tlocker.unlock();\n> -\t\tcall();\n> -\t\tlocker.lock();\n> +\t\tcall = calls_.front();\n> +\t\tcalls_.pop_front();\n> \t}\n> +\n> +\tcall();\n> }\n> diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h\n> index d0d5b5a53c830670..5f1cd88c2f5c830e 100644\n> --- a/src/cam/event_loop.h\n> +++ b/src/cam/event_loop.h\n> @@ -7,7 +7,6 @@\n> #ifndef __CAM_EVENT_LOOP_H__\n> #define __CAM_EVENT_LOOP_H__\n>\n> -#include \n> #include \n> #include \n> #include \n> @@ -26,19 +25,16 @@ public:\n> \tvoid exit(int code = 0);\n>\n> \tvoid callLater(const std::function &func);\n> +\tvoid dispatchCall();\n>\n> private:\n> \tstatic EventLoop *instance_;\n>\n> \tstruct event_base *base_;\n> -\tstd::atomic exit_;\n> \tint exitCode_;\n>\n> \tstd::list> calls_;\n> \tstd::mutex lock_;\n> -\n> -\tvoid interrupt();\n> -\tvoid dispatchCalls();\n\nChange looks ok!\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> };\n>\n> #endif /* __CAM_EVENT_LOOP_H__ */\n> --\n> 2.30.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 88882BD161\n\tfor ;\n\tWed, 3 Feb 2021 11:35:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1A77A68441;\n\tWed, 3 Feb 2021 12:35:05 +0100 (CET)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6F8E9683FE\n\tfor ;\n\tWed, 3 Feb 2021 12:35:03 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id E6DF9C0003;\n\tWed, 3 Feb 2021 11:35:02 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 3 Feb 2021 12:35:24 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20210203113524.7l6q7go4uw2jkqyc@uno.localdomain>","References":"<20210202221051.1740237-1-niklas.soderlund@ragnatech.se>\n\t<20210202221051.1740237-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210202221051.1740237-3-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v4 2/3] cam: event_loop: Execute\n\tevents in the libevent loop","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14939,"web_url":"https://patchwork.libcamera.org/comment/14939/","msgid":"","date":"2021-02-04T00:44:04","subject":"Re: [libcamera-devel] [PATCH v4 2/3] cam: event_loop: Execute\n\tevents in the libevent loop","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Tue, Feb 02, 2021 at 11:10:50PM +0100, Niklas Söderlund wrote:\n> Cam uses libevent to deal with threads and idle loop while still\n> implementing its own event queue. This creates issues when the event\n> loop is terminated as it might get stuck in the idle loop if exit() is\n> called while the thread is busy with dispatchCalls().\n> \n> Solve this my removing the custom event execution and instead injecting\n\ns/my/by/\n\n> the calls as events to the base event loop.\n> \n> Reported-by: Sebastian Fricke \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/cam/event_loop.cpp | 43 ++++++++++++++++++++----------------------\n> src/cam/event_loop.h | 6 +-----\n> 2 files changed, 21 insertions(+), 28 deletions(-)\n> \n> diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp\n> index 3a2b665abdc063de..e84e437f1f8ff6d7 100644\n> --- a/src/cam/event_loop.cpp\n> +++ b/src/cam/event_loop.cpp\n> @@ -13,6 +13,13 @@\n> \n> EventLoop *EventLoop::instance_ = nullptr;\n> \n> +static void dispatchCallback([[maybe_unused]] evutil_socket_t fd,\n> +\t\t\t [[maybe_unused]] short flags, void *param)\n> +{\n> +\tEventLoop *loop = static_cast(param);\n> +\tloop->dispatchCall();\n> +}\n\nThis could be a private static function of the EventLoop class, which\nwould allow EventLoop::dispatchCall() to become a private function. The\nevent_loop.h header will have an issue with the undefined\nevutil_socket_t type, which could be solved by either including\nevent2/util.h, or using the int type instead (evutil_socket_t is used to\nabstract platforms, I really doubt libevent will switch to another type\nthan int on Linux).\n\n> +\n> EventLoop::EventLoop()\n> {\n> \tassert(!instance_);\n> @@ -38,25 +45,13 @@ EventLoop *EventLoop::instance()\n> int EventLoop::exec()\n> {\n> \texitCode_ = -1;\n> -\texit_.store(false, std::memory_order_release);\n> -\n> -\twhile (!exit_.load(std::memory_order_acquire)) {\n> -\t\tdispatchCalls();\n> -\t\tevent_base_loop(base_, EVLOOP_NO_EXIT_ON_EMPTY);\n> -\t}\n> -\n> +\tevent_base_loop(base_, EVLOOP_NO_EXIT_ON_EMPTY);\n> \treturn exitCode_;\n> }\n> \n> void EventLoop::exit(int code)\n> {\n> \texitCode_ = code;\n> -\texit_.store(true, std::memory_order_release);\n> -\tinterrupt();\n> -}\n> -\n> -void EventLoop::interrupt()\n> -{\n> \tevent_base_loopbreak(base_);\n> }\n> \n> @@ -67,20 +62,22 @@ void EventLoop::callLater(const std::function &func)\n> \t\tcalls_.push_back(func);\n> \t}\n> \n> -\tinterrupt();\n> +\tstruct event *event = event_new(base_, -1, 0, dispatchCallback, this);\n> +\tevent_active(event, 0, 0);\n\nI believe the memory allocated by event_new() is leaked. valgrind seems\nto agree with me :-) The following may fix it:\n\n\tevent_base_once(base_, -1, EV_TIMEOUT, dispatchCallback, this, NULL);\n\nWe could go one step further and drop the calls_ list, with the\nfollowing:\n\nvoid EventLoop::callLater(const std::function &func)\n{\n\tevent_base_once(base_, -1, EV_TIMEOUT, EventLoop::dispatchCallback,\n\t\t\tnew std::function(func), NULL);\n}\n\nvoid EventLoop::dispatchCallback([[maybe_unused]] int fd,\n\t\t\t\t [[maybe_unused]] short flags, void *param)\n{\n\tstd::function *call = static_cast *>(param);\n\t(*call)();\n\tdelete call;\n}\n\nThere's one issue though, as libevent (at least starting with version\n2.1) guarantees that the event allocated internally by event_base_once()\nwill always be freed even if the event is never triggered (I believe\nthis can happen if event_base_loopbreak() races with event_base_once(),\nbut that may actually not be the case), but we would in that case leak\nthe memory allocated for the argument to the callback. The\nevent_base_once() explains this, and states that applications are on\ntheir own to free this memory. The simplest way to do so would be to\nstore the call in a calls_ list, so we would be back to where we are\ntoday.\n\nWithout support in libevent to handle this issue, I don't think there's\nmuch we could do to reduce the amonut of code we have here, but I may\nhave missed something.\n\n> }\n> \n> -void EventLoop::dispatchCalls()\n> +void EventLoop::dispatchCall()\n> {\n> -\tstd::unique_lock locker(lock_);\n> +\tstd::function call;\n> \n> -\tfor (auto iter = calls_.begin(); iter != calls_.end(); ) {\n> -\t\tstd::function call = std::move(*iter);\n> +\t{\n> +\t\tstd::unique_lock locker(lock_);\n> +\t\tif (calls_.empty())\n> +\t\t\treturn;\n> \n> -\t\titer = calls_.erase(iter);\n> -\n> -\t\tlocker.unlock();\n> -\t\tcall();\n> -\t\tlocker.lock();\n> +\t\tcall = calls_.front();\n> +\t\tcalls_.pop_front();\n> \t}\n> +\n> +\tcall();\n\nWe may be reducing the efficiency a little bit by queuing one event to\nlibevent for each call to dispatch, but I think that's fine. cam's goal\nisn't to optimize event handling, relying on features provided by\nlibevent as much as possible to keep the code simple is best.\n\nThe part that bothers me a bit with this patch is that I half recall\ndoing something similar when switching to libevent, and running into\nissues. As that memory could very well be incorrect, and as this patch\nseems fine,\n\nReviewed-by: Laurent Pinchart \n\nwith the above issues addressed.\n\n> }\n> diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h\n> index d0d5b5a53c830670..5f1cd88c2f5c830e 100644\n> --- a/src/cam/event_loop.h\n> +++ b/src/cam/event_loop.h\n> @@ -7,7 +7,6 @@\n> #ifndef __CAM_EVENT_LOOP_H__\n> #define __CAM_EVENT_LOOP_H__\n> \n> -#include \n> #include \n> #include \n> #include \n> @@ -26,19 +25,16 @@ public:\n> \tvoid exit(int code = 0);\n> \n> \tvoid callLater(const std::function &func);\n> +\tvoid dispatchCall();\n> \n> private:\n> \tstatic EventLoop *instance_;\n> \n> \tstruct event_base *base_;\n> -\tstd::atomic exit_;\n> \tint exitCode_;\n> \n> \tstd::list> calls_;\n> \tstd::mutex lock_;\n> -\n> -\tvoid interrupt();\n> -\tvoid dispatchCalls();\n> };\n> \n> #endif /* __CAM_EVENT_LOOP_H__ */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 009DEBD161\n\tfor ;\n\tThu, 4 Feb 2021 00:44:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 80C33613E0;\n\tThu, 4 Feb 2021 01:44:28 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 07EA960302\n\tfor ;\n\tThu, 4 Feb 2021 01:44:27 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6CDFB51B;\n\tThu, 4 Feb 2021 01:44:26 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"c0bAZFP9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612399466;\n\tbh=r/CkYboy/dHdsWEhlvvspiNqlJ8CL26JFotF9RGflik=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=c0bAZFP9DK1l5UbfBcNf/A2Li5IXYySTfRsZqzsjLlTjqjVTTIhuH0PSPQVDXEmXg\n\tJTj5ChBiy05ZZj9mhswXo5B0r7C9+OTE7MAWu/j0cIJQ7kargFmDvuLhFHLtwwGn2p\n\t/6qKdfDQV8201xDaOyBec5fVhwc1UyTso2n4/g60=","Date":"Thu, 4 Feb 2021 02:44:04 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20210202221051.1740237-1-niklas.soderlund@ragnatech.se>\n\t<20210202221051.1740237-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210202221051.1740237-3-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v4 2/3] cam: event_loop: Execute\n\tevents in the libevent loop","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15023,"web_url":"https://patchwork.libcamera.org/comment/15023/","msgid":"","date":"2021-02-05T09:14:40","subject":"Re: [libcamera-devel] [PATCH v4 2/3] cam: event_loop: Execute\n\tevents in the libevent loop","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2021-02-04 02:44:04 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Tue, Feb 02, 2021 at 11:10:50PM +0100, Niklas Söderlund wrote:\n> > Cam uses libevent to deal with threads and idle loop while still\n> > implementing its own event queue. This creates issues when the event\n> > loop is terminated as it might get stuck in the idle loop if exit() is\n> > called while the thread is busy with dispatchCalls().\n> > \n> > Solve this my removing the custom event execution and instead injecting\n> \n> s/my/by/\n> \n> > the calls as events to the base event loop.\n> > \n> > Reported-by: Sebastian Fricke \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/cam/event_loop.cpp | 43 ++++++++++++++++++++----------------------\n> > src/cam/event_loop.h | 6 +-----\n> > 2 files changed, 21 insertions(+), 28 deletions(-)\n> > \n> > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp\n> > index 3a2b665abdc063de..e84e437f1f8ff6d7 100644\n> > --- a/src/cam/event_loop.cpp\n> > +++ b/src/cam/event_loop.cpp\n> > @@ -13,6 +13,13 @@\n> > \n> > EventLoop *EventLoop::instance_ = nullptr;\n> > \n> > +static void dispatchCallback([[maybe_unused]] evutil_socket_t fd,\n> > +\t\t\t [[maybe_unused]] short flags, void *param)\n> > +{\n> > +\tEventLoop *loop = static_cast(param);\n> > +\tloop->dispatchCall();\n> > +}\n> \n> This could be a private static function of the EventLoop class, which\n> would allow EventLoop::dispatchCall() to become a private function. The\n> event_loop.h header will have an issue with the undefined\n> evutil_socket_t type, which could be solved by either including\n> event2/util.h, or using the int type instead (evutil_socket_t is used to\n> abstract platforms, I really doubt libevent will switch to another type\n> than int on Linux).\n\nThat is a very good idea! I will include event2/util.h as I think the \ncorrect type really should be used.\n\n> \n> > +\n> > EventLoop::EventLoop()\n> > {\n> > \tassert(!instance_);\n> > @@ -38,25 +45,13 @@ EventLoop *EventLoop::instance()\n> > int EventLoop::exec()\n> > {\n> > \texitCode_ = -1;\n> > -\texit_.store(false, std::memory_order_release);\n> > -\n> > -\twhile (!exit_.load(std::memory_order_acquire)) {\n> > -\t\tdispatchCalls();\n> > -\t\tevent_base_loop(base_, EVLOOP_NO_EXIT_ON_EMPTY);\n> > -\t}\n> > -\n> > +\tevent_base_loop(base_, EVLOOP_NO_EXIT_ON_EMPTY);\n> > \treturn exitCode_;\n> > }\n> > \n> > void EventLoop::exit(int code)\n> > {\n> > \texitCode_ = code;\n> > -\texit_.store(true, std::memory_order_release);\n> > -\tinterrupt();\n> > -}\n> > -\n> > -void EventLoop::interrupt()\n> > -{\n> > \tevent_base_loopbreak(base_);\n> > }\n> > \n> > @@ -67,20 +62,22 @@ void EventLoop::callLater(const std::function &func)\n> > \t\tcalls_.push_back(func);\n> > \t}\n> > \n> > -\tinterrupt();\n> > +\tstruct event *event = event_new(base_, -1, 0, dispatchCallback, this);\n> > +\tevent_active(event, 0, 0);\n> \n> I believe the memory allocated by event_new() is leaked. valgrind seems\n> to agree with me :-) The following may fix it:\n> \n> \tevent_base_once(base_, -1, EV_TIMEOUT, dispatchCallback, this, NULL);\n\nThanks!\n\n> \n> We could go one step further and drop the calls_ list, with the\n> following:\n> \n> void EventLoop::callLater(const std::function &func)\n> {\n> \tevent_base_once(base_, -1, EV_TIMEOUT, EventLoop::dispatchCallback,\n> \t\t\tnew std::function(func), NULL);\n> }\n> \n> void EventLoop::dispatchCallback([[maybe_unused]] int fd,\n> \t\t\t\t [[maybe_unused]] short flags, void *param)\n> {\n> \tstd::function *call = static_cast *>(param);\n> \t(*call)();\n> \tdelete call;\n> }\n> \n> There's one issue though, as libevent (at least starting with version\n> 2.1) guarantees that the event allocated internally by event_base_once()\n> will always be freed even if the event is never triggered (I believe\n> this can happen if event_base_loopbreak() races with event_base_once(),\n> but that may actually not be the case), but we would in that case leak\n> the memory allocated for the argument to the callback. The\n> event_base_once() explains this, and states that applications are on\n> their own to free this memory. The simplest way to do so would be to\n> store the call in a calls_ list, so we would be back to where we are\n> today.\n> \n> Without support in libevent to handle this issue, I don't think there's\n> much we could do to reduce the amonut of code we have here, but I may\n> have missed something.\n\nI tried something similar before posting this, and as you point out I \ncould not find a good way to handle events still queued after the loop \nis stopped. I think I will keep this as is for now and if we learn more \nof libevent we could switch to it later.\n\n> \n> > }\n> > \n> > -void EventLoop::dispatchCalls()\n> > +void EventLoop::dispatchCall()\n> > {\n> > -\tstd::unique_lock locker(lock_);\n> > +\tstd::function call;\n> > \n> > -\tfor (auto iter = calls_.begin(); iter != calls_.end(); ) {\n> > -\t\tstd::function call = std::move(*iter);\n> > +\t{\n> > +\t\tstd::unique_lock locker(lock_);\n> > +\t\tif (calls_.empty())\n> > +\t\t\treturn;\n> > \n> > -\t\titer = calls_.erase(iter);\n> > -\n> > -\t\tlocker.unlock();\n> > -\t\tcall();\n> > -\t\tlocker.lock();\n> > +\t\tcall = calls_.front();\n> > +\t\tcalls_.pop_front();\n> > \t}\n> > +\n> > +\tcall();\n> \n> We may be reducing the efficiency a little bit by queuing one event to\n> libevent for each call to dispatch, but I think that's fine. cam's goal\n> isn't to optimize event handling, relying on features provided by\n> libevent as much as possible to keep the code simple is best.\n\nAgreed.\n\n> \n> The part that bothers me a bit with this patch is that I half recall\n> doing something similar when switching to libevent, and running into\n> issues. As that memory could very well be incorrect, and as this patch\n> seems fine,\n\nYea this seems too simple, but I can't think how it can blow up at a \nmoment. Oh well for future me to discover and hopefully solve ;-)\n\n> \n> Reviewed-by: Laurent Pinchart \n> \n> with the above issues addressed.\n> \n> > }\n> > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h\n> > index d0d5b5a53c830670..5f1cd88c2f5c830e 100644\n> > --- a/src/cam/event_loop.h\n> > +++ b/src/cam/event_loop.h\n> > @@ -7,7 +7,6 @@\n> > #ifndef __CAM_EVENT_LOOP_H__\n> > #define __CAM_EVENT_LOOP_H__\n> > \n> > -#include \n> > #include \n> > #include \n> > #include \n> > @@ -26,19 +25,16 @@ public:\n> > \tvoid exit(int code = 0);\n> > \n> > \tvoid callLater(const std::function &func);\n> > +\tvoid dispatchCall();\n> > \n> > private:\n> > \tstatic EventLoop *instance_;\n> > \n> > \tstruct event_base *base_;\n> > -\tstd::atomic exit_;\n> > \tint exitCode_;\n> > \n> > \tstd::list> calls_;\n> > \tstd::mutex lock_;\n> > -\n> > -\tvoid interrupt();\n> > -\tvoid dispatchCalls();\n> > };\n> > \n> > #endif /* __CAM_EVENT_LOOP_H__ */\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9243BBD162\n\tfor ;\n\tFri, 5 Feb 2021 09:14:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A2CB614AA;\n\tFri, 5 Feb 2021 10:14:44 +0100 (CET)","from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com\n\t[IPv6:2a00:1450:4864:20::22e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE1A860106\n\tfor ;\n\tFri, 5 Feb 2021 10:14:42 +0100 (CET)","by mail-lj1-x22e.google.com with SMTP id v15so6830571ljk.13\n\tfor ;\n\tFri, 05 Feb 2021 01:14:42 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\td15sm917700lfm.183.2021.02.05.01.14.40\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 05 Feb 2021 01:14:40 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"ZMQ9bAvj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=Ab8G1P6SfaleAwqzSbBrHqILxOb/wa9X4gu9342MNg4=;\n\tb=ZMQ9bAvjOK3ptyhWXSsSFIlo1lMdWeakPlEVsNWo829HiVuJ8QYi4miL3dqdUKWXs0\n\th5NPFgVa/wqQGdEYV7QG4p0fB2117Cao240y2bsQc4pxsSa5qg+vBOwezcMfTgtudGEP\n\tunIi6WaGBbKxeVRnbFafZH2IvaMnHkWUmbcdGOS9KVfkFRH5gdXbMkkJNufJLvZtfy8W\n\t4Yc1GbnvhN6PBSylHJ9LSrLezRqC1WW6AbuGv3Dpv/pvOhH9dRT+f27ybQ9kpENQjheW\n\t29rv2du2FCDQr4BdWOtII2pgBpNjaFWMVMWXU18tucJXHQP4o/Q0ZaZyjsOn9QeMGAhp\n\toaHQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=Ab8G1P6SfaleAwqzSbBrHqILxOb/wa9X4gu9342MNg4=;\n\tb=DdktszkiSVQpK8FbbWKcysY+g3glJMNVMGlt2a8pGGfrDqtgiXv5KybNJOHqgl0Fb8\n\tpsStEXIlAL1EkbY13iUjYN1STW4Zy6DL7nFucjkJIL0xhe9sEgxvi/5Mibh2OMg+w8H0\n\tGOjqrrEVPbZ9hjdZAy6G4qfVtzYiqN9e4Ck8f0UIh2ay2SlPrwYgR3g4AhQCxuCOBJKQ\n\tV9syCEsU/CLkOJvc3MwmSXfzW/+KVP6PtYTUQmz5bsUjto6Fd1ZrR0k3s00TZ9nKx/dH\n\tEl23DgDWysBTt9T3FebRTDYsR9TRIpGEcWTnK2aGZfOCq0iZxZtRO6P9//jUtFy7/ZcP\n\taqbg==","X-Gm-Message-State":"AOAM5316JkloMo96+yB3aHHpnf2WB4H4Bh/teMH5n98WPnVz8CI/UKDx\n\tSeCd95VaH++GjGBJL6fQy2408fmfyNyym32n","X-Google-Smtp-Source":"ABdhPJx/jxYSiC7eDCNMs/SLIuBi3KrkVOAihG2Z2zh5Fkq5bgKUjfy3keHm04eHYPRXuysaSjbBhw==","X-Received":"by 2002:a2e:8e85:: with SMTP id z5mr2187633ljk.322.1612516482015;\n\tFri, 05 Feb 2021 01:14:42 -0800 (PST)","Date":"Fri, 5 Feb 2021 10:14:40 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"","References":"<20210202221051.1740237-1-niklas.soderlund@ragnatech.se>\n\t<20210202221051.1740237-3-niklas.soderlund@ragnatech.se>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 2/3] cam: event_loop: Execute\n\tevents in the libevent loop","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]