[{"id":14871,"web_url":"https://patchwork.libcamera.org/comment/14871/","msgid":"<20210201093518.quoth2l6hupzkino@uno.localdomain>","date":"2021-02-01T09:35:18","subject":"Re: [libcamera-devel] [PATCH v3 1/2] cam: event_loop: Stop queuing\n\tcalls when the event loop are exiting","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Sat, Jan 30, 2021 at 01:19:14AM +0100, Niklas Söderlund wrote:\n> Terminating the event loop with EventLoop::exit() does not grantee that\n\ns/grantee/guarantee\n\n> it will terminate. If the event loops 'call later' queue can be kept\n\nevent loop\n\n> non-empty by continuously queuing new calls using EventLoop::callLater()\n> either from a different thread or from callbacks in the loop itself\n> EventLoop::dispatchCalls() will never complete and the loop will run\n> forever.\n>\n> Solve this by only executing the already queued calls each invocation of\n> dispatchCalls() and only enter the idle loop if dispatchCalls() had no\n> calls to dispatch.\n>\n> Reported-by: Sebastian Fricke \n> Fixes: f49e93338b6309a6 (\"cam: event_loop: Add deferred calls support\")\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/cam/event_loop.cpp | 21 +++++++++------------\n> src/cam/event_loop.h | 2 +-\n> 2 files changed, 10 insertions(+), 13 deletions(-)\n>\n> diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp\n> index 94c5d1d362455f33..f0b1ecbb6244c40a 100644\n> --- a/src/cam/event_loop.cpp\n> +++ b/src/cam/event_loop.cpp\n> @@ -41,8 +41,8 @@ int EventLoop::exec()\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(event_, EVLOOP_NO_EXIT_ON_EMPTY);\n> +\t\tif (!dispatchCalls())\n> +\t\t\tevent_base_loop(event_, EVLOOP_NO_EXIT_ON_EMPTY);\n> \t}\n>\n> \treturn exitCode_;\n> @@ -70,17 +70,14 @@ void EventLoop::callLater(const std::function &func)\n> \tinterrupt();\n> }\n>\n> -void EventLoop::dispatchCalls()\n> +bool EventLoop::dispatchCalls()\n> {\n> -\tstd::unique_lock locker(lock_);\n> +\tlock_.lock();\n> +\tstd::list> calls = std::move(calls_);\n> +\tlock_.unlock();\n\nAfter this last unlock:\ncalls contains all the callbacks\ncalls_ now empty\ncalls_ can be populated with more callbacks as there's no lock held\n\n>\n> -\tfor (auto iter = calls_.begin(); iter != calls_.end(); ) {\n> -\t\tstd::function call = std::move(*iter);\n> -\n> -\t\titer = calls_.erase(iter);\n> -\n> -\t\tlocker.unlock();\n> +\tfor (std::function call : calls)\n> \t\tcall();\n\n> -\t\tlocker.lock();\n> -\t}\n> +\n> +\treturn !calls.empty();\n\nHow can calls be empty as you never remove items ?\nAs callbacks can be add to calls_ while you are iteraring here, won't\nthem be lost forever ?\n\nI feel like I'm missing something\n\n> }\n> diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h\n> index 408073c50594d09d..b2535f7bdd96742a 100644\n> --- a/src/cam/event_loop.h\n> +++ b/src/cam/event_loop.h\n> @@ -38,7 +38,7 @@ private:\n> \tstd::mutex lock_;\n>\n> \tvoid interrupt();\n> -\tvoid dispatchCalls();\n> +\tbool dispatchCalls();\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 33E67BD808\n\tfor ;\n\tMon, 1 Feb 2021 09:34:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C569D683F8;\n\tMon, 1 Feb 2021 10:34:58 +0100 (CET)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 82A0C60307\n\tfor ;\n\tMon, 1 Feb 2021 10:34:57 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id ED42DFF8B0;\n\tMon, 1 Feb 2021 09:34:56 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Mon, 1 Feb 2021 10:35:18 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20210201093518.quoth2l6hupzkino@uno.localdomain>","References":"<20210130001915.489703-1-niklas.soderlund@ragnatech.se>\n\t<20210130001915.489703-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210130001915.489703-2-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] cam: event_loop: Stop queuing\n\tcalls when the event loop are exiting","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":14884,"web_url":"https://patchwork.libcamera.org/comment/14884/","msgid":"","date":"2021-02-01T16:18:00","subject":"Re: [libcamera-devel] [PATCH v3 1/2] cam: event_loop: Stop queuing\n\tcalls when the event loop are exiting","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your feedback.\n\nOn 2021-02-01 10:35:18 +0100, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Sat, Jan 30, 2021 at 01:19:14AM +0100, Niklas Söderlund wrote:\n> > Terminating the event loop with EventLoop::exit() does not grantee that\n> \n> s/grantee/guarantee\n> \n> > it will terminate. If the event loops 'call later' queue can be kept\n> \n> event loop\n> \n> > non-empty by continuously queuing new calls using EventLoop::callLater()\n> > either from a different thread or from callbacks in the loop itself\n> > EventLoop::dispatchCalls() will never complete and the loop will run\n> > forever.\n> >\n> > Solve this by only executing the already queued calls each invocation of\n> > dispatchCalls() and only enter the idle loop if dispatchCalls() had no\n> > calls to dispatch.\n> >\n> > Reported-by: Sebastian Fricke \n> > Fixes: f49e93338b6309a6 (\"cam: event_loop: Add deferred calls support\")\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/cam/event_loop.cpp | 21 +++++++++------------\n> > src/cam/event_loop.h | 2 +-\n> > 2 files changed, 10 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp\n> > index 94c5d1d362455f33..f0b1ecbb6244c40a 100644\n> > --- a/src/cam/event_loop.cpp\n> > +++ b/src/cam/event_loop.cpp\n> > @@ -41,8 +41,8 @@ int EventLoop::exec()\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(event_, EVLOOP_NO_EXIT_ON_EMPTY);\n> > +\t\tif (!dispatchCalls())\n> > +\t\t\tevent_base_loop(event_, EVLOOP_NO_EXIT_ON_EMPTY);\n> > \t}\n> >\n> > \treturn exitCode_;\n> > @@ -70,17 +70,14 @@ void EventLoop::callLater(const std::function &func)\n> > \tinterrupt();\n> > }\n> >\n> > -void EventLoop::dispatchCalls()\n> > +bool EventLoop::dispatchCalls()\n> > {\n> > -\tstd::unique_lock locker(lock_);\n> > +\tlock_.lock();\n> > +\tstd::list> calls = std::move(calls_);\n> > +\tlock_.unlock();\n> \n> After this last unlock:\n> calls contains all the callbacks\n> calls_ now empty\n> calls_ can be populated with more callbacks as there's no lock held\n\nCorrect.\n\n> \n> >\n> > -\tfor (auto iter = calls_.begin(); iter != calls_.end(); ) {\n> > -\t\tstd::function call = std::move(*iter);\n> > -\n> > -\t\titer = calls_.erase(iter);\n> > -\n> > -\t\tlocker.unlock();\n> > +\tfor (std::function call : calls)\n> > \t\tcall();\n> \n> > -\t\tlocker.lock();\n> > -\t}\n> > +\n> > +\treturn !calls.empty();\n> \n> How can calls be empty as you never remove items ?\n\nIf calls_ was empty at the time of the std::move(). And this only \nhappens when there is no calls to be processed and we should enter the \nevent_base_loop() idle loop.\n\nReading the diff now I could have done something like this to make this \nmore clear,\n\n\tbool EventLoop::dispatchCalls()\n\t{\n\t lock_.lock();\n\t std::list> calls = std::move(calls_);\n\t lock_.unlock();\n\n\t if (calls.empty())\n\t\t return false;\n\n\t for (std::function call : calls)\n\t\t call();\n\n\t return true;\n\t}\n\n> As callbacks can be add to calls_ while you are iteraring here, won't\n> them be lost forever ?\n\nHow?\n\nWhile iterating here more calls may be added calls_. If we iterate one \nore more calls here dispatchCalls() will return true. The only call site \nis exec(),\n\n int EventLoop::exec()\n {\n\t exitCode_ = -1;\n\t exit_.store(false, std::memory_order_release);\n\n\t while (!exit_.load(std::memory_order_acquire)) {\n\t\t if (!dispatchCalls())\n\t\t\t event_base_loop(event_, EVLOOP_NO_EXIT_ON_EMPTY);\n\t }\n\n\t return exitCode_;\n }\n\nOn dispatchCalls() returning true we do not enter the event_base_loop() \nidle loop and instead call dispatchCalls() once more consuming any calls \nappended to calls_.\n\n> \n> I feel like I'm missing something\n\nMight as well be me who are missing things :-)\n\n> \n> > }\n> > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h\n> > index 408073c50594d09d..b2535f7bdd96742a 100644\n> > --- a/src/cam/event_loop.h\n> > +++ b/src/cam/event_loop.h\n> > @@ -38,7 +38,7 @@ private:\n> > \tstd::mutex lock_;\n> >\n> > \tvoid interrupt();\n> > -\tvoid dispatchCalls();\n> > +\tbool dispatchCalls();\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 0F08EC33BB\n\tfor ;\n\tMon, 1 Feb 2021 16:18:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9FCD86840C;\n\tMon, 1 Feb 2021 17:18:03 +0100 (CET)","from mail-lf1-x132.google.com (mail-lf1-x132.google.com\n\t[IPv6:2a00:1450:4864:20::132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 66FED683FF\n\tfor ;\n\tMon, 1 Feb 2021 17:18:02 +0100 (CET)","by mail-lf1-x132.google.com with SMTP id m22so23539559lfg.5\n\tfor ;\n\tMon, 01 Feb 2021 08:18:02 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tp15sm3010466lfc.286.2021.02.01.08.18.00\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 01 Feb 2021 08:18:00 -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=\"D6eADXoe\"; 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=PKc0nrJ7lz7Lz8ZLk71xhA3x++sSGhyDG5V9k8YYKIo=;\n\tb=D6eADXoeg2+kYBQ5cpsHDGFNdN8AosKPKewA+y40A0+jOvabjhCNG6+HucXOngurO6\n\tqJ4AHnNdj3N41VqU/tQYAmVVqUm37681UWi6tg5hyVqJW6DFr8NTPez4e2xPjT03C8C8\n\tV9Gsc5gUS/MGdjDifRRAcd7ej6Xo9mSYC3s5VCJI7B7GcnfCrfsFql4npMcguA6OT9Xe\n\tI2wXNaUWInNPbLcZzfFuqpAdOJgunXvXZddBP5k49IJhEP7dQO1Q3IuuRsr3O0NWPkT1\n\tsihRY3+BE/IF1EnipTIbQ5KA6/1t57SJ+vK9p8voJkLlTzMc0r/0gbsq8DT3ontcdOkA\n\t0nVA==","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=PKc0nrJ7lz7Lz8ZLk71xhA3x++sSGhyDG5V9k8YYKIo=;\n\tb=q9sQo6ylV4tbX64rYAlP2zMiEnYDxp5741dwcLQOEJjO47Rjl6fx6BwaAk+M/cUqa9\n\t0Bllm4XLX7S89CqizhPHK6SwVMpALNjgHCqYNlhH74cd77bepUfkAd/rDsjHtjDKJNmD\n\tskDIBD1oKFqDHd2ecAZup3/G0ZG8PVIqh6zd8TCvBbsCcNYQl71BiNQdlD1lYADEx6WO\n\t4WA2OyayW3NW3G09ZkOR52n/0cw4UvCUfl0dJ6UqWu8ok/TAehQSFkJ8USx8rDbAghxJ\n\ttmnA9UJCFw1uF5fewLssK0qiimuuy6YUcTzb1TOaQl6/3o9XHMpIcoMzRXNvom1vd+/j\n\tF2DA==","X-Gm-Message-State":"AOAM530oiQsxD5csn07EQi2MXG/HgbFy0mJqRBhdGP2pz+YHCkZHE3hW\n\tHAQBllh9v0Z+vHYh2Ki+cjOV709s96NVew==","X-Google-Smtp-Source":"ABdhPJwjgEg4ASgQt726/YOsN6SJTMW/Xhg8m2dqJNcwTKBS0k8FQ576Bb7NR6+omrLKQpME2w3dlw==","X-Received":"by 2002:a05:6512:110a:: with SMTP id\n\tl10mr9060463lfg.140.1612196281737; \n\tMon, 01 Feb 2021 08:18:01 -0800 (PST)","Date":"Mon, 1 Feb 2021 17:18:00 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210130001915.489703-1-niklas.soderlund@ragnatech.se>\n\t<20210130001915.489703-2-niklas.soderlund@ragnatech.se>\n\t<20210201093518.quoth2l6hupzkino@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210201093518.quoth2l6hupzkino@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] cam: event_loop: Stop queuing\n\tcalls when the event loop are exiting","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\" "}},{"id":14887,"web_url":"https://patchwork.libcamera.org/comment/14887/","msgid":"<20210201170737.2wotupq7kol6jngy@uno.localdomain>","date":"2021-02-01T17:07:37","subject":"Re: [libcamera-devel] [PATCH v3 1/2] cam: event_loop: Stop queuing\n\tcalls when the event loop are exiting","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Mon, Feb 01, 2021 at 05:18:00PM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your feedback.\n>\n> On 2021-02-01 10:35:18 +0100, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >\n> > On Sat, Jan 30, 2021 at 01:19:14AM +0100, Niklas Söderlund wrote:\n> > > Terminating the event loop with EventLoop::exit() does not grantee that\n> >\n> > s/grantee/guarantee\n> >\n> > > it will terminate. If the event loops 'call later' queue can be kept\n> >\n> > event loop\n> >\n> > > non-empty by continuously queuing new calls using EventLoop::callLater()\n> > > either from a different thread or from callbacks in the loop itself\n> > > EventLoop::dispatchCalls() will never complete and the loop will run\n> > > forever.\n> > >\n> > > Solve this by only executing the already queued calls each invocation of\n> > > dispatchCalls() and only enter the idle loop if dispatchCalls() had no\n> > > calls to dispatch.\n> > >\n> > > Reported-by: Sebastian Fricke \n> > > Fixes: f49e93338b6309a6 (\"cam: event_loop: Add deferred calls support\")\n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/cam/event_loop.cpp | 21 +++++++++------------\n> > > src/cam/event_loop.h | 2 +-\n> > > 2 files changed, 10 insertions(+), 13 deletions(-)\n> > >\n> > > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp\n> > > index 94c5d1d362455f33..f0b1ecbb6244c40a 100644\n> > > --- a/src/cam/event_loop.cpp\n> > > +++ b/src/cam/event_loop.cpp\n> > > @@ -41,8 +41,8 @@ int EventLoop::exec()\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(event_, EVLOOP_NO_EXIT_ON_EMPTY);\n> > > +\t\tif (!dispatchCalls())\n> > > +\t\t\tevent_base_loop(event_, EVLOOP_NO_EXIT_ON_EMPTY);\n> > > \t}\n> > >\n> > > \treturn exitCode_;\n> > > @@ -70,17 +70,14 @@ void EventLoop::callLater(const std::function &func)\n> > > \tinterrupt();\n> > > }\n> > >\n> > > -void EventLoop::dispatchCalls()\n> > > +bool EventLoop::dispatchCalls()\n> > > {\n> > > -\tstd::unique_lock locker(lock_);\n> > > +\tlock_.lock();\n> > > +\tstd::list> calls = std::move(calls_);\n> > > +\tlock_.unlock();\n> >\n> > After this last unlock:\n> > calls contains all the callbacks\n> > calls_ now empty\n> > calls_ can be populated with more callbacks as there's no lock held\n>\n> Correct.\n>\n> >\n> > >\n> > > -\tfor (auto iter = calls_.begin(); iter != calls_.end(); ) {\n> > > -\t\tstd::function call = std::move(*iter);\n> > > -\n> > > -\t\titer = calls_.erase(iter);\n> > > -\n> > > -\t\tlocker.unlock();\n> > > +\tfor (std::function call : calls)\n> > > \t\tcall();\n> >\n> > > -\t\tlocker.lock();\n> > > -\t}\n> > > +\n> > > +\treturn !calls.empty();\n> >\n> > How can calls be empty as you never remove items ?\n>\n> If calls_ was empty at the time of the std::move(). And this only\n> happens when there is no calls to be processed and we should enter the\n> event_base_loop() idle loop.\n>\n> Reading the diff now I could have done something like this to make this\n> more clear,\n>\n> \tbool EventLoop::dispatchCalls()\n> \t{\n> \t lock_.lock();\n> \t std::list> calls = std::move(calls_);\n> \t lock_.unlock();\n>\n> \t if (calls.empty())\n> \t\t return false;\n>\n> \t for (std::function call : calls)\n> \t\t call();\n>\n> \t return true;\n> \t}\n\nAh ok, this is cleaner and..\n\n>\n> > As callbacks can be add to calls_ while you are iteraring here, won't\n> > them be lost forever ?\n>\n> How?\n>\n> While iterating here more calls may be added calls_. If we iterate one\n> ore more calls here dispatchCalls() will return true. The only call site\n> is exec(),\n\n.. make the windown smaller, as if I'm not mistaken you still have a\nwindow.\n\n lock()\n calls = move(calls_)\n unlock()\n <------------- calls_.push_back(new callback)\n if (calls.empty())\n return false;\n\nCan't this happen ? I don't know very much the cam event model, so I\nmight be missing something.\n\n(nit: I would make dispatchCalls() return true if all calls are\ndispatched)\n\n if (dispatchCalls())\n exit();\n\n>\n> int EventLoop::exec()\n> {\n> \t exitCode_ = -1;\n> \t exit_.store(false, std::memory_order_release);\n>\n> \t while (!exit_.load(std::memory_order_acquire)) {\n> \t\t if (!dispatchCalls())\n> \t\t\t event_base_loop(event_, EVLOOP_NO_EXIT_ON_EMPTY);\n> \t }\n>\n> \t return exitCode_;\n> }\n>\n> On dispatchCalls() returning true we do not enter the event_base_loop()\n> idle loop and instead call dispatchCalls() once more consuming any calls\n> appended to calls_.\n>\n> >\n> > I feel like I'm missing something\n>\n> Might as well be me who are missing things :-)\n>\n> >\n> > > }\n> > > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h\n> > > index 408073c50594d09d..b2535f7bdd96742a 100644\n> > > --- a/src/cam/event_loop.h\n> > > +++ b/src/cam/event_loop.h\n> > > @@ -38,7 +38,7 @@ private:\n> > > \tstd::mutex lock_;\n> > >\n> > > \tvoid interrupt();\n> > > -\tvoid dispatchCalls();\n> > > +\tbool dispatchCalls();\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\n>\n> --\n> Regards,\n> Niklas Söderlund","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 C31FBBD808\n\tfor ;\n\tMon, 1 Feb 2021 17:07:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 514BD68412;\n\tMon, 1 Feb 2021 18:07:17 +0100 (CET)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4F150683FF\n\tfor ;\n\tMon, 1 Feb 2021 18:07:16 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id BB930FF819;\n\tMon, 1 Feb 2021 17:07:15 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Mon, 1 Feb 2021 18:07:37 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20210201170737.2wotupq7kol6jngy@uno.localdomain>","References":"<20210130001915.489703-1-niklas.soderlund@ragnatech.se>\n\t<20210130001915.489703-2-niklas.soderlund@ragnatech.se>\n\t<20210201093518.quoth2l6hupzkino@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] cam: event_loop: Stop queuing\n\tcalls when the event loop are exiting","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":14913,"web_url":"https://patchwork.libcamera.org/comment/14913/","msgid":"","date":"2021-02-02T22:08:40","subject":"Re: [libcamera-devel] [PATCH v3 1/2] cam: event_loop: Stop queuing\n\tcalls when the event loop are exiting","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your feedback.\n\nOn 2021-02-01 18:07:37 +0100, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Mon, Feb 01, 2021 at 05:18:00PM +0100, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for your feedback.\n> >\n> > On 2021-02-01 10:35:18 +0100, Jacopo Mondi wrote:\n> > > Hi Niklas,\n> > >\n> > > On Sat, Jan 30, 2021 at 01:19:14AM +0100, Niklas Söderlund wrote:\n> > > > Terminating the event loop with EventLoop::exit() does not grantee that\n> > >\n> > > s/grantee/guarantee\n> > >\n> > > > it will terminate. If the event loops 'call later' queue can be kept\n> > >\n> > > event loop\n> > >\n> > > > non-empty by continuously queuing new calls using EventLoop::callLater()\n> > > > either from a different thread or from callbacks in the loop itself\n> > > > EventLoop::dispatchCalls() will never complete and the loop will run\n> > > > forever.\n> > > >\n> > > > Solve this by only executing the already queued calls each invocation of\n> > > > dispatchCalls() and only enter the idle loop if dispatchCalls() had no\n> > > > calls to dispatch.\n> > > >\n> > > > Reported-by: Sebastian Fricke \n> > > > Fixes: f49e93338b6309a6 (\"cam: event_loop: Add deferred calls support\")\n> > > > Signed-off-by: Niklas Söderlund \n> > > > ---\n> > > > src/cam/event_loop.cpp | 21 +++++++++------------\n> > > > src/cam/event_loop.h | 2 +-\n> > > > 2 files changed, 10 insertions(+), 13 deletions(-)\n> > > >\n> > > > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp\n> > > > index 94c5d1d362455f33..f0b1ecbb6244c40a 100644\n> > > > --- a/src/cam/event_loop.cpp\n> > > > +++ b/src/cam/event_loop.cpp\n> > > > @@ -41,8 +41,8 @@ int EventLoop::exec()\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(event_, EVLOOP_NO_EXIT_ON_EMPTY);\n> > > > +\t\tif (!dispatchCalls())\n> > > > +\t\t\tevent_base_loop(event_, EVLOOP_NO_EXIT_ON_EMPTY);\n> > > > \t}\n> > > >\n> > > > \treturn exitCode_;\n> > > > @@ -70,17 +70,14 @@ void EventLoop::callLater(const std::function &func)\n> > > > \tinterrupt();\n> > > > }\n> > > >\n> > > > -void EventLoop::dispatchCalls()\n> > > > +bool EventLoop::dispatchCalls()\n> > > > {\n> > > > -\tstd::unique_lock locker(lock_);\n> > > > +\tlock_.lock();\n> > > > +\tstd::list> calls = std::move(calls_);\n> > > > +\tlock_.unlock();\n> > >\n> > > After this last unlock:\n> > > calls contains all the callbacks\n> > > calls_ now empty\n> > > calls_ can be populated with more callbacks as there's no lock held\n> >\n> > Correct.\n> >\n> > >\n> > > >\n> > > > -\tfor (auto iter = calls_.begin(); iter != calls_.end(); ) {\n> > > > -\t\tstd::function call = std::move(*iter);\n> > > > -\n> > > > -\t\titer = calls_.erase(iter);\n> > > > -\n> > > > -\t\tlocker.unlock();\n> > > > +\tfor (std::function call : calls)\n> > > > \t\tcall();\n> > >\n> > > > -\t\tlocker.lock();\n> > > > -\t}\n> > > > +\n> > > > +\treturn !calls.empty();\n> > >\n> > > How can calls be empty as you never remove items ?\n> >\n> > If calls_ was empty at the time of the std::move(). And this only\n> > happens when there is no calls to be processed and we should enter the\n> > event_base_loop() idle loop.\n> >\n> > Reading the diff now I could have done something like this to make this\n> > more clear,\n> >\n> > \tbool EventLoop::dispatchCalls()\n> > \t{\n> > \t lock_.lock();\n> > \t std::list> calls = std::move(calls_);\n> > \t lock_.unlock();\n> >\n> > \t if (calls.empty())\n> > \t\t return false;\n> >\n> > \t for (std::function call : calls)\n> > \t\t call();\n> >\n> > \t return true;\n> > \t}\n> \n> Ah ok, this is cleaner and..\n> \n> >\n> > > As callbacks can be add to calls_ while you are iteraring here, won't\n> > > them be lost forever ?\n> >\n> > How?\n> >\n> > While iterating here more calls may be added calls_. If we iterate one\n> > ore more calls here dispatchCalls() will return true. The only call site\n> > is exec(),\n> \n> .. make the windown smaller, as if I'm not mistaken you still have a\n> window.\n> \n> lock()\n> calls = move(calls_)\n> unlock()\n> <------------- calls_.push_back(new callback)\n> if (calls.empty())\n> return false;\n> \n> Can't this happen ? I don't know very much the cam event model, so I\n> might be missing something.\n> \n> (nit: I would make dispatchCalls() return true if all calls are\n> dispatched)\n> \n> if (dispatchCalls())\n> exit();\n\nThis is a good point. I have dug a bit in libevnet itself and I believe \nI have solution where we can avoid this and the problem I try to address \nin this patch all together. Will post patches with this new idea as v4.\n\n> \n> >\n> > int EventLoop::exec()\n> > {\n> > \t exitCode_ = -1;\n> > \t exit_.store(false, std::memory_order_release);\n> >\n> > \t while (!exit_.load(std::memory_order_acquire)) {\n> > \t\t if (!dispatchCalls())\n> > \t\t\t event_base_loop(event_, EVLOOP_NO_EXIT_ON_EMPTY);\n> > \t }\n> >\n> > \t return exitCode_;\n> > }\n> >\n> > On dispatchCalls() returning true we do not enter the event_base_loop()\n> > idle loop and instead call dispatchCalls() once more consuming any calls\n> > appended to calls_.\n> >\n> > >\n> > > I feel like I'm missing something\n> >\n> > Might as well be me who are missing things :-)\n> >\n> > >\n> > > > }\n> > > > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h\n> > > > index 408073c50594d09d..b2535f7bdd96742a 100644\n> > > > --- a/src/cam/event_loop.h\n> > > > +++ b/src/cam/event_loop.h\n> > > > @@ -38,7 +38,7 @@ private:\n> > > > \tstd::mutex lock_;\n> > > >\n> > > > \tvoid interrupt();\n> > > > -\tvoid dispatchCalls();\n> > > > +\tbool dispatchCalls();\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\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","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 1B08CBD160\n\tfor ;\n\tTue, 2 Feb 2021 22:08:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A41A968431;\n\tTue, 2 Feb 2021 23:08:44 +0100 (CET)","from mail-lj1-x230.google.com (mail-lj1-x230.google.com\n\t[IPv6:2a00:1450:4864:20::230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C0D6860307\n\tfor ;\n\tTue, 2 Feb 2021 23:08:42 +0100 (CET)","by mail-lj1-x230.google.com with SMTP id a17so737240ljq.2\n\tfor ;\n\tTue, 02 Feb 2021 14:08: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\tp5sm26910lfc.300.2021.02.02.14.08.41\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 02 Feb 2021 14:08:41 -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=\"tnOIGcxG\"; 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=SBbIm2ONcnfU3YgUA5FQgRasfrnCY5AhWaNz6FCSZ/s=;\n\tb=tnOIGcxG0X4R86T40aLsjUVVq6QoKTKBaXueublKpLb1Xmj7xGph5EHmNpnP1e2drd\n\t6Wdd5YkotAoC4Rug0dQyGXSwLNmLqlrVJZAI4Xx2KtkT9/pD1+KatiyQBa8rmdZ/zo/a\n\tRYVAqsSpy00CyAlmj4SbozOtinJYwWYh0+n+33gtbr19O4yZLng9AnV79TWn4xmCjQTi\n\tqBCD+rKgNJJCjCj4Ei1u1Rv35fbLZxgS3JtdSr7JmJqI/MnzdiNXO9Xe4eaucL8ZdY6p\n\tnQnGdNuKl0KLqjQ6gQg3ZQ+AGd1bYTSQL9PUAKaoY3mvk3a/NIaJqokhY5JwLIhI2iq9\n\t5Ttg==","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=SBbIm2ONcnfU3YgUA5FQgRasfrnCY5AhWaNz6FCSZ/s=;\n\tb=AxMHgbY4gjUwYJ66YeEJstlCA2BtTxX1ySiRLSGRVgLLkGFruNWXqZjV3YU88ii+s+\n\tBvGM8/V4xiTt+N98gda455TKeP0oBltvJcUgw6oFOZj/1wk1kORlzgtg6RVc00R+4lEb\n\tpc24kBOg/YejDGH7xbDe/53sLbUj8GiijXQxGNyaHFBE4Kw7QpiYEr0dMKeveLNe+CeA\n\tsNEE24wgsaSPy4FpmqIBs4WhJn2jEOwC/pnTn46DLZ+YYR1PIpfEwemvkUNvOHpW2BgE\n\tgmgUIGtBAZS7nwGg/JcSejnB22bLSxELL10y6Hi7QUWkSdGh2bjwalXNcFl98gbHJSfL\n\tPoog==","X-Gm-Message-State":"AOAM533iyCmi7RAkFoE+yAI7jNQR4soWJ1w7m41aspHXCGkqLAidI/dX\n\tCNG9QH/GQbbCMjCOmHdtBx5VB5PB+tgDsQ==","X-Google-Smtp-Source":"ABdhPJzNRzdjPBQp2hyxHoO1Eqhjvhxk3TrQMh2nsrei7lSN+PdG6tajfOTLjZbAf5qpNuMbz7aIpQ==","X-Received":"by 2002:a2e:9188:: with SMTP id\n\tf8mr13871544ljg.462.1612303721934; \n\tTue, 02 Feb 2021 14:08:41 -0800 (PST)","Date":"Tue, 2 Feb 2021 23:08:40 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210130001915.489703-1-niklas.soderlund@ragnatech.se>\n\t<20210130001915.489703-2-niklas.soderlund@ragnatech.se>\n\t<20210201093518.quoth2l6hupzkino@uno.localdomain>\n\t\n\t<20210201170737.2wotupq7kol6jngy@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210201170737.2wotupq7kol6jngy@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] cam: event_loop: Stop queuing\n\tcalls when the event loop are exiting","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\" "}}]