| Message ID | 20250128130308.509297-2-pobrn@protonmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Thank you for the patch. On Tue, Jan 28, 2025 at 01:03:14PM +0000, Barnabás Pőcze wrote: > Use an appropriate compare-and-exchange atomic instruction > to set the event dispatcher pointer. This ensures that there > is only ever a single event dispatcher associated with a > given thread. > > Consider e.g. the following series of events: > > 1. thread 0 :: if (!data_->dispatcher_.load(std::memory_order_relaxed)) > // reads nullptr and proceeds into the `if`'s block > > 2. thread 1 :: if (!data_->dispatcher_.load(std::memory_order_relaxed)) > // reads nullptr and proceeds into the `if`'s block > > 3. thread 0 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release); > // creates dispatcher and sets pointer > > 4. thread 1 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release); > // creates dispatcher and sets pointer > > At the end, one of the event dispatchers is leaked, and > furthermore the `eventDispatcher()` calls might return > different values in the calling threads. Yes, the current code is clearly broken if called from different threads concurrently. The question is, do we want to call this function from different threads ? As far as I can see, it's only called for the current thread in all locations. By definition that means the thread is started, with its run() function called. By default run() calls exec(), which creates the event dispatcher. Even if a thread reimplements run() and either doesn't call exec(), or call eventDispatcher() before calling exec(), we should be safe. The only unsafe situation would be calling eventDispatcher() from a separate thread. The eventDispatcher() function is documented as thread-safe, so its API allows that, even if the implementation doesn't. Do we want to allow that ? Is there a use case for accessing the event dispatcher from other threads (other than through the Thread class API, such as Thread::exit()) ? We currently need to access the event dispatcher to manage event notifiers and timers. In particular, the following functions access the event dispatcher *of the current thread*: - EventNotifier::setEnabled() - Timer::registerTimer() - Timer::unregisterTimer() - IPCPipeUnixSocket::call() - IPAProxy*Worker::run() The first three functions can only be called from the thread the event notifier or timer belong to. Usage of the event dispatcher in IPCPipeUnixSocket::call() is a bit of a hack that I'd like to fix, but in any case the function only accesses the event dispatcher of the current thread. IPAProxy*Worker::run() also accesses the event dispatcher of the current thread, but in this case the Thread is not running (as in having his run() function called) as it's a MainThread instance that represents a main application thread, outside of the control of libcamera. It should therefore be safe to consider Thread::eventDispatcher() as callable only from its Thread. We could even take it one step further, and create the event dispatcher in Thread::exec() instead of in Thread::eventDispatcher(). We would have to consider some corner cases, such as moving a timer or event notifier to a thread that has not been started yet. Callers of Thread::eventDispatcher() may need to perform null checks. We would also need to figure out how to handle IPAProxy*Worker::run(). What do you think ? > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > src/libcamera/base/thread.cpp | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp > index de60567f6..93ec7b5a0 100644 > --- a/src/libcamera/base/thread.cpp > +++ b/src/libcamera/base/thread.cpp > @@ -518,11 +518,16 @@ pid_t Thread::currentId() > */ > EventDispatcher *Thread::eventDispatcher() > { > - if (!data_->dispatcher_.load(std::memory_order_relaxed)) > - data_->dispatcher_.store(new EventDispatcherPoll(), > - std::memory_order_release); > + if (EventDispatcher *dispatcher = data_->dispatcher_.load(std::memory_order_acquire)) > + return dispatcher; > > - return data_->dispatcher_.load(std::memory_order_relaxed); > + auto dispatcher = std::make_unique<EventDispatcherPoll>(); > + EventDispatcher *expected = nullptr; > + > + if (data_->dispatcher_.compare_exchange_strong(expected, dispatcher.get(), std::memory_order_acq_rel)) > + return dispatcher.release(); > + > + return expected; > } > > /**
2025. 08. 14. 15:56 keltezéssel, Laurent Pinchart írta: > Hi Barnabás, > > Thank you for the patch. > > On Tue, Jan 28, 2025 at 01:03:14PM +0000, Barnabás Pőcze wrote: >> Use an appropriate compare-and-exchange atomic instruction >> to set the event dispatcher pointer. This ensures that there >> is only ever a single event dispatcher associated with a >> given thread. >> >> Consider e.g. the following series of events: >> >> 1. thread 0 :: if (!data_->dispatcher_.load(std::memory_order_relaxed)) >> // reads nullptr and proceeds into the `if`'s block >> >> 2. thread 1 :: if (!data_->dispatcher_.load(std::memory_order_relaxed)) >> // reads nullptr and proceeds into the `if`'s block >> >> 3. thread 0 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release); >> // creates dispatcher and sets pointer >> >> 4. thread 1 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release); >> // creates dispatcher and sets pointer >> >> At the end, one of the event dispatchers is leaked, and >> furthermore the `eventDispatcher()` calls might return >> different values in the calling threads. > > Yes, the current code is clearly broken if called from different threads > concurrently. > > The question is, do we want to call this function from different threads > ? As far as I can see, it's only called for the current thread in all > locations. By definition that means the thread is started, with its > run() function called. By default run() calls exec(), which creates the > event dispatcher. Even if a thread reimplements run() and either doesn't > call exec(), or call eventDispatcher() before calling exec(), we should > be safe. > > The only unsafe situation would be calling eventDispatcher() from a > separate thread. The eventDispatcher() function is documented as > thread-safe, so its API allows that, even if the implementation doesn't. > Do we want to allow that ? Is there a use case for accessing the event > dispatcher from other threads (other than through the Thread class API, > such as Thread::exit()) ? > > We currently need to access the event dispatcher to manage event > notifiers and timers. In particular, the following functions access the > event dispatcher *of the current thread*: > > - EventNotifier::setEnabled() > - Timer::registerTimer() > - Timer::unregisterTimer() > - IPCPipeUnixSocket::call() > - IPAProxy*Worker::run() > > The first three functions can only be called from the thread the event > notifier or timer belong to. > > Usage of the event dispatcher in IPCPipeUnixSocket::call() is a bit of a > hack that I'd like to fix, but in any case the function only accesses > the event dispatcher of the current thread. > > IPAProxy*Worker::run() also accesses the event dispatcher of the current > thread, but in this case the Thread is not running (as in having his > run() function called) as it's a MainThread instance that represents a > main application thread, outside of the control of libcamera. > > It should therefore be safe to consider Thread::eventDispatcher() as > callable only from its Thread. > > We could even take it one step further, and create the event dispatcher > in Thread::exec() instead of in Thread::eventDispatcher(). We would have > to consider some corner cases, such as moving a timer or event notifier > to a thread that has not been started yet. Callers of > Thread::eventDispatcher() may need to perform null checks. We would also > need to figure out how to handle IPAProxy*Worker::run(). > > What do you think ? The motivation for this change, as far as I remember, was that the documentation states that this function is thread safe. The variable also has an atomic type, so I did not do more investigations because the the intent seemed clear. If the conclusion is that thread safety is not needed, then I believe the mention of thread safety should be dropped from the documentation and the type modified to be non-atomic. I don't have a concrete opinion on creating the event loop just in exec(). I suppose it could make sense, but I'd probably decouple the two types first by removing `Thread::current()->dispatchMessages()` from `EventDispatcherPoll::processEvents` and instead registering an eventfd for the messages. Regards, Barnabás Pőcze > >> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> >> --- >> src/libcamera/base/thread.cpp | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp >> index de60567f6..93ec7b5a0 100644 >> --- a/src/libcamera/base/thread.cpp >> +++ b/src/libcamera/base/thread.cpp >> @@ -518,11 +518,16 @@ pid_t Thread::currentId() >> */ >> EventDispatcher *Thread::eventDispatcher() >> { >> - if (!data_->dispatcher_.load(std::memory_order_relaxed)) >> - data_->dispatcher_.store(new EventDispatcherPoll(), >> - std::memory_order_release); >> + if (EventDispatcher *dispatcher = data_->dispatcher_.load(std::memory_order_acquire)) >> + return dispatcher; >> >> - return data_->dispatcher_.load(std::memory_order_relaxed); >> + auto dispatcher = std::make_unique<EventDispatcherPoll>(); >> + EventDispatcher *expected = nullptr; >> + >> + if (data_->dispatcher_.compare_exchange_strong(expected, dispatcher.get(), std::memory_order_acq_rel)) >> + return dispatcher.release(); >> + >> + return expected; >> } >> >> /** >
On Thu, Aug 14, 2025 at 04:30:14PM +0200, Barnabás Pőcze wrote: > 2025. 08. 14. 15:56 keltezéssel, Laurent Pinchart írta: > > On Tue, Jan 28, 2025 at 01:03:14PM +0000, Barnabás Pőcze wrote: > >> Use an appropriate compare-and-exchange atomic instruction > >> to set the event dispatcher pointer. This ensures that there > >> is only ever a single event dispatcher associated with a > >> given thread. > >> > >> Consider e.g. the following series of events: > >> > >> 1. thread 0 :: if (!data_->dispatcher_.load(std::memory_order_relaxed)) > >> // reads nullptr and proceeds into the `if`'s block > >> > >> 2. thread 1 :: if (!data_->dispatcher_.load(std::memory_order_relaxed)) > >> // reads nullptr and proceeds into the `if`'s block > >> > >> 3. thread 0 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release); > >> // creates dispatcher and sets pointer > >> > >> 4. thread 1 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release); > >> // creates dispatcher and sets pointer > >> > >> At the end, one of the event dispatchers is leaked, and > >> furthermore the `eventDispatcher()` calls might return > >> different values in the calling threads. > > > > Yes, the current code is clearly broken if called from different threads > > concurrently. > > > > The question is, do we want to call this function from different threads > > ? As far as I can see, it's only called for the current thread in all > > locations. By definition that means the thread is started, with its > > run() function called. By default run() calls exec(), which creates the > > event dispatcher. Even if a thread reimplements run() and either doesn't > > call exec(), or call eventDispatcher() before calling exec(), we should > > be safe. > > > > The only unsafe situation would be calling eventDispatcher() from a > > separate thread. The eventDispatcher() function is documented as > > thread-safe, so its API allows that, even if the implementation doesn't. > > Do we want to allow that ? Is there a use case for accessing the event > > dispatcher from other threads (other than through the Thread class API, > > such as Thread::exit()) ? > > > > We currently need to access the event dispatcher to manage event > > notifiers and timers. In particular, the following functions access the > > event dispatcher *of the current thread*: > > > > - EventNotifier::setEnabled() > > - Timer::registerTimer() > > - Timer::unregisterTimer() > > - IPCPipeUnixSocket::call() > > - IPAProxy*Worker::run() > > > > The first three functions can only be called from the thread the event > > notifier or timer belong to. > > > > Usage of the event dispatcher in IPCPipeUnixSocket::call() is a bit of a > > hack that I'd like to fix, but in any case the function only accesses > > the event dispatcher of the current thread. > > > > IPAProxy*Worker::run() also accesses the event dispatcher of the current > > thread, but in this case the Thread is not running (as in having his > > run() function called) as it's a MainThread instance that represents a > > main application thread, outside of the control of libcamera. > > > > It should therefore be safe to consider Thread::eventDispatcher() as > > callable only from its Thread. > > > > We could even take it one step further, and create the event dispatcher > > in Thread::exec() instead of in Thread::eventDispatcher(). We would have > > to consider some corner cases, such as moving a timer or event notifier > > to a thread that has not been started yet. Callers of > > Thread::eventDispatcher() may need to perform null checks. We would also > > need to figure out how to handle IPAProxy*Worker::run(). > > > > What do you think ? > > The motivation for this change, as far as I remember, was that the documentation > states that this function is thread safe. The variable also has an atomic type, > so I did not do more investigations because the the intent seemed clear. > > If the conclusion is that thread safety is not needed, then I believe the mention of > thread safety should be dropped from the documentation and the type modified to be non-atomic. Note that eventDispatcher_ still needs to be accessed from other threads through Thread::exit() and Thread::postMessage(). I haven't checked if this requires (or could benefit from) an atomic type. > I don't have a concrete opinion on creating the event loop just in exec(). I suppose > it could make sense, but I'd probably decouple the two types first by removing > `Thread::current()->dispatchMessages()` from `EventDispatcherPoll::processEvents` > and instead registering an eventfd for the messages. Wouldn't that impact performance ? Messages are completely internal to libcamera, I'd keep them that way if possible. In any case, we can decouple the thread safety issue from the event dispatcher creation issue. Let's address the former for now. > >> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > >> --- > >> src/libcamera/base/thread.cpp | 13 +++++++++---- > >> 1 file changed, 9 insertions(+), 4 deletions(-) > >> > >> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp > >> index de60567f6..93ec7b5a0 100644 > >> --- a/src/libcamera/base/thread.cpp > >> +++ b/src/libcamera/base/thread.cpp > >> @@ -518,11 +518,16 @@ pid_t Thread::currentId() > >> */ > >> EventDispatcher *Thread::eventDispatcher() > >> { > >> - if (!data_->dispatcher_.load(std::memory_order_relaxed)) > >> - data_->dispatcher_.store(new EventDispatcherPoll(), > >> - std::memory_order_release); > >> + if (EventDispatcher *dispatcher = data_->dispatcher_.load(std::memory_order_acquire)) > >> + return dispatcher; > >> > >> - return data_->dispatcher_.load(std::memory_order_relaxed); > >> + auto dispatcher = std::make_unique<EventDispatcherPoll>(); > >> + EventDispatcher *expected = nullptr; > >> + > >> + if (data_->dispatcher_.compare_exchange_strong(expected, dispatcher.get(), std::memory_order_acq_rel)) > >> + return dispatcher.release(); > >> + > >> + return expected; > >> } > >> > >> /** > > >
2025. 08. 14. 16:41 keltezéssel, Laurent Pinchart írta: > On Thu, Aug 14, 2025 at 04:30:14PM +0200, Barnabás Pőcze wrote: >> 2025. 08. 14. 15:56 keltezéssel, Laurent Pinchart írta: >>> On Tue, Jan 28, 2025 at 01:03:14PM +0000, Barnabás Pőcze wrote: >>>> Use an appropriate compare-and-exchange atomic instruction >>>> to set the event dispatcher pointer. This ensures that there >>>> is only ever a single event dispatcher associated with a >>>> given thread. >>>> >>>> Consider e.g. the following series of events: >>>> >>>> 1. thread 0 :: if (!data_->dispatcher_.load(std::memory_order_relaxed)) >>>> // reads nullptr and proceeds into the `if`'s block >>>> >>>> 2. thread 1 :: if (!data_->dispatcher_.load(std::memory_order_relaxed)) >>>> // reads nullptr and proceeds into the `if`'s block >>>> >>>> 3. thread 0 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release); >>>> // creates dispatcher and sets pointer >>>> >>>> 4. thread 1 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release); >>>> // creates dispatcher and sets pointer >>>> >>>> At the end, one of the event dispatchers is leaked, and >>>> furthermore the `eventDispatcher()` calls might return >>>> different values in the calling threads. >>> >>> Yes, the current code is clearly broken if called from different threads >>> concurrently. >>> >>> The question is, do we want to call this function from different threads >>> ? As far as I can see, it's only called for the current thread in all >>> locations. By definition that means the thread is started, with its >>> run() function called. By default run() calls exec(), which creates the >>> event dispatcher. Even if a thread reimplements run() and either doesn't >>> call exec(), or call eventDispatcher() before calling exec(), we should >>> be safe. >>> >>> The only unsafe situation would be calling eventDispatcher() from a >>> separate thread. The eventDispatcher() function is documented as >>> thread-safe, so its API allows that, even if the implementation doesn't. >>> Do we want to allow that ? Is there a use case for accessing the event >>> dispatcher from other threads (other than through the Thread class API, >>> such as Thread::exit()) ? >>> >>> We currently need to access the event dispatcher to manage event >>> notifiers and timers. In particular, the following functions access the >>> event dispatcher *of the current thread*: >>> >>> - EventNotifier::setEnabled() >>> - Timer::registerTimer() >>> - Timer::unregisterTimer() >>> - IPCPipeUnixSocket::call() >>> - IPAProxy*Worker::run() >>> >>> The first three functions can only be called from the thread the event >>> notifier or timer belong to. >>> >>> Usage of the event dispatcher in IPCPipeUnixSocket::call() is a bit of a >>> hack that I'd like to fix, but in any case the function only accesses >>> the event dispatcher of the current thread. >>> >>> IPAProxy*Worker::run() also accesses the event dispatcher of the current >>> thread, but in this case the Thread is not running (as in having his >>> run() function called) as it's a MainThread instance that represents a >>> main application thread, outside of the control of libcamera. >>> >>> It should therefore be safe to consider Thread::eventDispatcher() as >>> callable only from its Thread. >>> >>> We could even take it one step further, and create the event dispatcher >>> in Thread::exec() instead of in Thread::eventDispatcher(). We would have >>> to consider some corner cases, such as moving a timer or event notifier >>> to a thread that has not been started yet. Callers of >>> Thread::eventDispatcher() may need to perform null checks. We would also >>> need to figure out how to handle IPAProxy*Worker::run(). >>> >>> What do you think ? >> >> The motivation for this change, as far as I remember, was that the documentation >> states that this function is thread safe. The variable also has an atomic type, >> so I did not do more investigations because the the intent seemed clear. >> >> If the conclusion is that thread safety is not needed, then I believe the mention of >> thread safety should be dropped from the documentation and the type modified to be non-atomic. > > Note that eventDispatcher_ still needs to be accessed from other threads > through Thread::exit() and Thread::postMessage(). I haven't checked if > this requires (or could benefit from) an atomic type. Ahh, that is true. `Thread::postMessage()` most certainly needs atomic access to the ptr. Same with `Thread::exit()`. > >> I don't have a concrete opinion on creating the event loop just in exec(). I suppose >> it could make sense, but I'd probably decouple the two types first by removing >> `Thread::current()->dispatchMessages()` from `EventDispatcherPoll::processEvents` >> and instead registering an eventfd for the messages. > > Wouldn't that impact performance ? Messages are completely internal to > libcamera, I'd keep them that way if possible. I don't see a substantial difference. Now the message passing mechanism uses `EventDispatcher::interrupt()`, which will still just signal an eventfd. The main difference is where that eventfd is. I would argue that by keeping it in `Thread`, and letting `EventDispatcher` not to have to deal with message passing, things become more separated. > > In any case, we can decouple the thread safety issue from the event > dispatcher creation issue. Let's address the former for now. So I suppose the question is whether `Thread::eventDispatcher()` should be thread safe or not. I don't have a concrete opinion on that. I believe the proposed changes here shouldn't incur significant performance penalty, so we might want to make it thread safe. Although arguably `EventDispatcher::interrupt()` is the only thread safe function, so it might not have a lot of utility. Regards, Barnabás Pőcze > >>>> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> >>>> --- >>>> src/libcamera/base/thread.cpp | 13 +++++++++---- >>>> 1 file changed, 9 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp >>>> index de60567f6..93ec7b5a0 100644 >>>> --- a/src/libcamera/base/thread.cpp >>>> +++ b/src/libcamera/base/thread.cpp >>>> @@ -518,11 +518,16 @@ pid_t Thread::currentId() >>>> */ >>>> EventDispatcher *Thread::eventDispatcher() >>>> { >>>> - if (!data_->dispatcher_.load(std::memory_order_relaxed)) >>>> - data_->dispatcher_.store(new EventDispatcherPoll(), >>>> - std::memory_order_release); >>>> + if (EventDispatcher *dispatcher = data_->dispatcher_.load(std::memory_order_acquire)) >>>> + return dispatcher; >>>> >>>> - return data_->dispatcher_.load(std::memory_order_relaxed); >>>> + auto dispatcher = std::make_unique<EventDispatcherPoll>(); >>>> + EventDispatcher *expected = nullptr; >>>> + >>>> + if (data_->dispatcher_.compare_exchange_strong(expected, dispatcher.get(), std::memory_order_acq_rel)) >>>> + return dispatcher.release(); >>>> + >>>> + return expected; >>>> } >>>> >>>> /** >>> >> > > -- > Regards, > > Laurent Pinchart
On Thu, Aug 14, 2025 at 05:03:39PM +0200, Barnabás Pőcze wrote: > 2025. 08. 14. 16:41 keltezéssel, Laurent Pinchart írta: > > On Thu, Aug 14, 2025 at 04:30:14PM +0200, Barnabás Pőcze wrote: > >> 2025. 08. 14. 15:56 keltezéssel, Laurent Pinchart írta: > >>> On Tue, Jan 28, 2025 at 01:03:14PM +0000, Barnabás Pőcze wrote: > >>>> Use an appropriate compare-and-exchange atomic instruction > >>>> to set the event dispatcher pointer. This ensures that there > >>>> is only ever a single event dispatcher associated with a > >>>> given thread. > >>>> > >>>> Consider e.g. the following series of events: > >>>> > >>>> 1. thread 0 :: if (!data_->dispatcher_.load(std::memory_order_relaxed)) > >>>> // reads nullptr and proceeds into the `if`'s block > >>>> > >>>> 2. thread 1 :: if (!data_->dispatcher_.load(std::memory_order_relaxed)) > >>>> // reads nullptr and proceeds into the `if`'s block > >>>> > >>>> 3. thread 0 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release); > >>>> // creates dispatcher and sets pointer > >>>> > >>>> 4. thread 1 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release); > >>>> // creates dispatcher and sets pointer > >>>> > >>>> At the end, one of the event dispatchers is leaked, and > >>>> furthermore the `eventDispatcher()` calls might return > >>>> different values in the calling threads. > >>> > >>> Yes, the current code is clearly broken if called from different threads > >>> concurrently. > >>> > >>> The question is, do we want to call this function from different threads > >>> ? As far as I can see, it's only called for the current thread in all > >>> locations. By definition that means the thread is started, with its > >>> run() function called. By default run() calls exec(), which creates the > >>> event dispatcher. Even if a thread reimplements run() and either doesn't > >>> call exec(), or call eventDispatcher() before calling exec(), we should > >>> be safe. > >>> > >>> The only unsafe situation would be calling eventDispatcher() from a > >>> separate thread. The eventDispatcher() function is documented as > >>> thread-safe, so its API allows that, even if the implementation doesn't. > >>> Do we want to allow that ? Is there a use case for accessing the event > >>> dispatcher from other threads (other than through the Thread class API, > >>> such as Thread::exit()) ? > >>> > >>> We currently need to access the event dispatcher to manage event > >>> notifiers and timers. In particular, the following functions access the > >>> event dispatcher *of the current thread*: > >>> > >>> - EventNotifier::setEnabled() > >>> - Timer::registerTimer() > >>> - Timer::unregisterTimer() > >>> - IPCPipeUnixSocket::call() > >>> - IPAProxy*Worker::run() > >>> > >>> The first three functions can only be called from the thread the event > >>> notifier or timer belong to. > >>> > >>> Usage of the event dispatcher in IPCPipeUnixSocket::call() is a bit of a > >>> hack that I'd like to fix, but in any case the function only accesses > >>> the event dispatcher of the current thread. > >>> > >>> IPAProxy*Worker::run() also accesses the event dispatcher of the current > >>> thread, but in this case the Thread is not running (as in having his > >>> run() function called) as it's a MainThread instance that represents a > >>> main application thread, outside of the control of libcamera. > >>> > >>> It should therefore be safe to consider Thread::eventDispatcher() as > >>> callable only from its Thread. > >>> > >>> We could even take it one step further, and create the event dispatcher > >>> in Thread::exec() instead of in Thread::eventDispatcher(). We would have > >>> to consider some corner cases, such as moving a timer or event notifier > >>> to a thread that has not been started yet. Callers of > >>> Thread::eventDispatcher() may need to perform null checks. We would also > >>> need to figure out how to handle IPAProxy*Worker::run(). > >>> > >>> What do you think ? > >> > >> The motivation for this change, as far as I remember, was that the documentation > >> states that this function is thread safe. The variable also has an atomic type, > >> so I did not do more investigations because the the intent seemed clear. > >> > >> If the conclusion is that thread safety is not needed, then I believe the mention of > >> thread safety should be dropped from the documentation and the type modified to be non-atomic. > > > > Note that eventDispatcher_ still needs to be accessed from other threads > > through Thread::exit() and Thread::postMessage(). I haven't checked if > > this requires (or could benefit from) an atomic type. > > Ahh, that is true. `Thread::postMessage()` most certainly needs atomic access to > the ptr. Same with `Thread::exit()`. > > >> I don't have a concrete opinion on creating the event loop just in exec(). I suppose > >> it could make sense, but I'd probably decouple the two types first by removing > >> `Thread::current()->dispatchMessages()` from `EventDispatcherPoll::processEvents` > >> and instead registering an eventfd for the messages. > > > > Wouldn't that impact performance ? Messages are completely internal to > > libcamera, I'd keep them that way if possible. > > I don't see a substantial difference. Now the message passing mechanism > uses `EventDispatcher::interrupt()`, which will still just signal an > eventfd. The main difference is where that eventfd is. I would argue > that by keeping it in `Thread`, and letting `EventDispatcher` not to > have to deal with message passing, things become more separated. I see messages as a kind of event, but that's debatable :-) We would still need to be able to interrupt the event dispatcher, so an eventfd would still be needed there. Maybe I don't understand your proposal correctly, but it also feels like there isn't really anything to fix there, so refactoring may be low priority. > > In any case, we can decouple the thread safety issue from the event > > dispatcher creation issue. Let's address the former for now. > > So I suppose the question is whether `Thread::eventDispatcher()` should be > thread safe or not. I don't have a concrete opinion on that. I believe the > proposed changes here shouldn't incur significant performance penalty, so > we might want to make it thread safe. Although arguably `EventDispatcher::interrupt()` > is the only thread safe function, so it might not have a lot of utility. At the moment I'm leaning slightly towards the "keep is simple" approach, given we have no use case for calling Thread::eventDispatcher() from a different thread. Even if your patch does not cause significant performance issues, it makes the code more complicated, and therefore has a maintenance cost. > >>>> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > >>>> --- > >>>> src/libcamera/base/thread.cpp | 13 +++++++++---- > >>>> 1 file changed, 9 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp > >>>> index de60567f6..93ec7b5a0 100644 > >>>> --- a/src/libcamera/base/thread.cpp > >>>> +++ b/src/libcamera/base/thread.cpp > >>>> @@ -518,11 +518,16 @@ pid_t Thread::currentId() > >>>> */ > >>>> EventDispatcher *Thread::eventDispatcher() > >>>> { > >>>> - if (!data_->dispatcher_.load(std::memory_order_relaxed)) > >>>> - data_->dispatcher_.store(new EventDispatcherPoll(), > >>>> - std::memory_order_release); > >>>> + if (EventDispatcher *dispatcher = data_->dispatcher_.load(std::memory_order_acquire)) > >>>> + return dispatcher; > >>>> > >>>> - return data_->dispatcher_.load(std::memory_order_relaxed); > >>>> + auto dispatcher = std::make_unique<EventDispatcherPoll>(); > >>>> + EventDispatcher *expected = nullptr; > >>>> + > >>>> + if (data_->dispatcher_.compare_exchange_strong(expected, dispatcher.get(), std::memory_order_acq_rel)) > >>>> + return dispatcher.release(); > >>>> + > >>>> + return expected; > >>>> } > >>>> > >>>> /**
2025. 08. 15. 0:02 keltezéssel, Laurent Pinchart írta: > On Thu, Aug 14, 2025 at 05:03:39PM +0200, Barnabás Pőcze wrote: >> 2025. 08. 14. 16:41 keltezéssel, Laurent Pinchart írta: >>> On Thu, Aug 14, 2025 at 04:30:14PM +0200, Barnabás Pőcze wrote: >>>> 2025. 08. 14. 15:56 keltezéssel, Laurent Pinchart írta: >>>>> On Tue, Jan 28, 2025 at 01:03:14PM +0000, Barnabás Pőcze wrote: >>>>>> Use an appropriate compare-and-exchange atomic instruction >>>>>> to set the event dispatcher pointer. This ensures that there >>>>>> is only ever a single event dispatcher associated with a >>>>>> given thread. >>>>>> >>>>>> Consider e.g. the following series of events: >>>>>> >>>>>> 1. thread 0 :: if (!data_->dispatcher_.load(std::memory_order_relaxed)) >>>>>> // reads nullptr and proceeds into the `if`'s block >>>>>> >>>>>> 2. thread 1 :: if (!data_->dispatcher_.load(std::memory_order_relaxed)) >>>>>> // reads nullptr and proceeds into the `if`'s block >>>>>> >>>>>> 3. thread 0 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release); >>>>>> // creates dispatcher and sets pointer >>>>>> >>>>>> 4. thread 1 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release); >>>>>> // creates dispatcher and sets pointer >>>>>> >>>>>> At the end, one of the event dispatchers is leaked, and >>>>>> furthermore the `eventDispatcher()` calls might return >>>>>> different values in the calling threads. >>>>> >>>>> Yes, the current code is clearly broken if called from different threads >>>>> concurrently. >>>>> >>>>> The question is, do we want to call this function from different threads >>>>> ? As far as I can see, it's only called for the current thread in all >>>>> locations. By definition that means the thread is started, with its >>>>> run() function called. By default run() calls exec(), which creates the >>>>> event dispatcher. Even if a thread reimplements run() and either doesn't >>>>> call exec(), or call eventDispatcher() before calling exec(), we should >>>>> be safe. >>>>> >>>>> The only unsafe situation would be calling eventDispatcher() from a >>>>> separate thread. The eventDispatcher() function is documented as >>>>> thread-safe, so its API allows that, even if the implementation doesn't. >>>>> Do we want to allow that ? Is there a use case for accessing the event >>>>> dispatcher from other threads (other than through the Thread class API, >>>>> such as Thread::exit()) ? >>>>> >>>>> We currently need to access the event dispatcher to manage event >>>>> notifiers and timers. In particular, the following functions access the >>>>> event dispatcher *of the current thread*: >>>>> >>>>> - EventNotifier::setEnabled() >>>>> - Timer::registerTimer() >>>>> - Timer::unregisterTimer() >>>>> - IPCPipeUnixSocket::call() >>>>> - IPAProxy*Worker::run() >>>>> >>>>> The first three functions can only be called from the thread the event >>>>> notifier or timer belong to. >>>>> >>>>> Usage of the event dispatcher in IPCPipeUnixSocket::call() is a bit of a >>>>> hack that I'd like to fix, but in any case the function only accesses >>>>> the event dispatcher of the current thread. >>>>> >>>>> IPAProxy*Worker::run() also accesses the event dispatcher of the current >>>>> thread, but in this case the Thread is not running (as in having his >>>>> run() function called) as it's a MainThread instance that represents a >>>>> main application thread, outside of the control of libcamera. >>>>> >>>>> It should therefore be safe to consider Thread::eventDispatcher() as >>>>> callable only from its Thread. >>>>> >>>>> We could even take it one step further, and create the event dispatcher >>>>> in Thread::exec() instead of in Thread::eventDispatcher(). We would have >>>>> to consider some corner cases, such as moving a timer or event notifier >>>>> to a thread that has not been started yet. Callers of >>>>> Thread::eventDispatcher() may need to perform null checks. We would also >>>>> need to figure out how to handle IPAProxy*Worker::run(). >>>>> >>>>> What do you think ? >>>> >>>> The motivation for this change, as far as I remember, was that the documentation >>>> states that this function is thread safe. The variable also has an atomic type, >>>> so I did not do more investigations because the the intent seemed clear. >>>> >>>> If the conclusion is that thread safety is not needed, then I believe the mention of >>>> thread safety should be dropped from the documentation and the type modified to be non-atomic. >>> >>> Note that eventDispatcher_ still needs to be accessed from other threads >>> through Thread::exit() and Thread::postMessage(). I haven't checked if >>> this requires (or could benefit from) an atomic type. >> >> Ahh, that is true. `Thread::postMessage()` most certainly needs atomic access to >> the ptr. Same with `Thread::exit()`. >> >>>> I don't have a concrete opinion on creating the event loop just in exec(). I suppose >>>> it could make sense, but I'd probably decouple the two types first by removing >>>> `Thread::current()->dispatchMessages()` from `EventDispatcherPoll::processEvents` >>>> and instead registering an eventfd for the messages. >>> >>> Wouldn't that impact performance ? Messages are completely internal to >>> libcamera, I'd keep them that way if possible. >> >> I don't see a substantial difference. Now the message passing mechanism >> uses `EventDispatcher::interrupt()`, which will still just signal an >> eventfd. The main difference is where that eventfd is. I would argue >> that by keeping it in `Thread`, and letting `EventDispatcher` not to >> have to deal with message passing, things become more separated. > > I see messages as a kind of event, but that's debatable :-) We would > still need to be able to interrupt the event dispatcher, so an eventfd > would still be needed there. Maybe I don't understand your proposal > correctly, but it also feels like there isn't really anything to fix > there, so refactoring may be low priority. > >>> In any case, we can decouple the thread safety issue from the event >>> dispatcher creation issue. Let's address the former for now. >> >> So I suppose the question is whether `Thread::eventDispatcher()` should be >> thread safe or not. I don't have a concrete opinion on that. I believe the >> proposed changes here shouldn't incur significant performance penalty, so >> we might want to make it thread safe. Although arguably `EventDispatcher::interrupt()` >> is the only thread safe function, so it might not have a lot of utility. > > At the moment I'm leaning slightly towards the "keep is simple" > approach, given we have no use case for calling > Thread::eventDispatcher() from a different thread. Even if your patch > does not cause significant performance issues, it makes the code more > complicated, and therefore has a maintenance cost. Okay, then I'll send a patch removing the mention of thread safety from the documentation. > >>>>>> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> >>>>>> --- >>>>>> src/libcamera/base/thread.cpp | 13 +++++++++---- >>>>>> 1 file changed, 9 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp >>>>>> index de60567f6..93ec7b5a0 100644 >>>>>> --- a/src/libcamera/base/thread.cpp >>>>>> +++ b/src/libcamera/base/thread.cpp >>>>>> @@ -518,11 +518,16 @@ pid_t Thread::currentId() >>>>>> */ >>>>>> EventDispatcher *Thread::eventDispatcher() >>>>>> { >>>>>> - if (!data_->dispatcher_.load(std::memory_order_relaxed)) >>>>>> - data_->dispatcher_.store(new EventDispatcherPoll(), >>>>>> - std::memory_order_release); >>>>>> + if (EventDispatcher *dispatcher = data_->dispatcher_.load(std::memory_order_acquire)) >>>>>> + return dispatcher; >>>>>> >>>>>> - return data_->dispatcher_.load(std::memory_order_relaxed); >>>>>> + auto dispatcher = std::make_unique<EventDispatcherPoll>(); >>>>>> + EventDispatcher *expected = nullptr; >>>>>> + >>>>>> + if (data_->dispatcher_.compare_exchange_strong(expected, dispatcher.get(), std::memory_order_acq_rel)) >>>>>> + return dispatcher.release(); >>>>>> + >>>>>> + return expected; >>>>>> } >>>>>> >>>>>> /** > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp index de60567f6..93ec7b5a0 100644 --- a/src/libcamera/base/thread.cpp +++ b/src/libcamera/base/thread.cpp @@ -518,11 +518,16 @@ pid_t Thread::currentId() */ EventDispatcher *Thread::eventDispatcher() { - if (!data_->dispatcher_.load(std::memory_order_relaxed)) - data_->dispatcher_.store(new EventDispatcherPoll(), - std::memory_order_release); + if (EventDispatcher *dispatcher = data_->dispatcher_.load(std::memory_order_acquire)) + return dispatcher; - return data_->dispatcher_.load(std::memory_order_relaxed); + auto dispatcher = std::make_unique<EventDispatcherPoll>(); + EventDispatcher *expected = nullptr; + + if (data_->dispatcher_.compare_exchange_strong(expected, dispatcher.get(), std::memory_order_acq_rel)) + return dispatcher.release(); + + return expected; } /**
Use an appropriate compare-and-exchange atomic instruction to set the event dispatcher pointer. This ensures that there is only ever a single event dispatcher associated with a given thread. Consider e.g. the following series of events: 1. thread 0 :: if (!data_->dispatcher_.load(std::memory_order_relaxed)) // reads nullptr and proceeds into the `if`'s block 2. thread 1 :: if (!data_->dispatcher_.load(std::memory_order_relaxed)) // reads nullptr and proceeds into the `if`'s block 3. thread 0 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release); // creates dispatcher and sets pointer 4. thread 1 :: data_->dispatcher_.store(new EventDispatcherPoll(), std::memory_order_release); // creates dispatcher and sets pointer At the end, one of the event dispatchers is leaked, and furthermore the `eventDispatcher()` calls might return different values in the calling threads. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- src/libcamera/base/thread.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) -- 2.48.1