Re: [systemd-devel] EXT: sdbus_event loop state mark as volatile?
On Fr, 23.04.21 11:24, Stephen Hemminger (step...@networkplumber.org) wrote: > On Fri, 6 Sep 2019 16:04:33 +0100 > Simon McVittie wrote: > > > On Fri, 06 Sep 2019 at 06:57:22 +, Ray, Ian (GE Healthcare) wrote: > > > If thread-safety is a design goal (and I don’t believe that it is [1]) > > > then atomic or thread-safe primitives should be used. > > > > > > [1] > > > https://lists.freedesktop.org/archives/systemd-devel/2017-March/038519.html > > > > [1] is about sd-bus, not sd-event, and doesn't say anything about whether > > sd-event is designed to be thread-safe or not. > > > > However, I think you're correct to say that struct sd_event is also only > > designed to be used from the single thread that "owns" it. > > > > If you need a thread-safe event loop, then you need something like > > GLib's GMainContext, with mutexes to protect its data structures against > > concurrent access, and a well-defined mechanism for one main-context to > > "post" events to other main-contexts (which might be running concurrently > > in a different thread). Many other event loops are available; GMainContext > > happens to be the one I'm most familiar with, and I know that it is > > designed to be thread-safe. > > > > The price that things like GMainContext pay for being thread-safe is > > that they are more complex and less efficient than sd-event: in general, > > all operations on a thread-aware event loop have to pay the complexity > > and performance cost of being thread-aware, even if the current program > > only has one thread. > > > > smcv > > Excuse me for reviving an old thread. But I see similar problem today > (especially on Arm). The sd-event model uses signals so it is inherently > subject to thread issues. Hmm? sd-event uses signals only via signalfd(), so that it can dispatch them as part of regular event loop handling. It doesn't install a signal handler or anything like that. > It looks like a stronger memory model is needed here (not volatile). > Other projects use __atomic builtins for this. All of sd-event's data structures should be accessed from a single thread only, in a single non-signal execution context. Lennart -- Lennart Poettering, Berlin ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] EXT: sdbus_event loop state mark as volatile?
On Fri, 6 Sep 2019 16:04:33 +0100 Simon McVittie wrote: > On Fri, 06 Sep 2019 at 06:57:22 +, Ray, Ian (GE Healthcare) wrote: > > If thread-safety is a design goal (and I don’t believe that it is [1]) > > then atomic or thread-safe primitives should be used. > > > > [1] > > https://lists.freedesktop.org/archives/systemd-devel/2017-March/038519.html > > > > [1] is about sd-bus, not sd-event, and doesn't say anything about whether > sd-event is designed to be thread-safe or not. > > However, I think you're correct to say that struct sd_event is also only > designed to be used from the single thread that "owns" it. > > If you need a thread-safe event loop, then you need something like > GLib's GMainContext, with mutexes to protect its data structures against > concurrent access, and a well-defined mechanism for one main-context to > "post" events to other main-contexts (which might be running concurrently > in a different thread). Many other event loops are available; GMainContext > happens to be the one I'm most familiar with, and I know that it is > designed to be thread-safe. > > The price that things like GMainContext pay for being thread-safe is > that they are more complex and less efficient than sd-event: in general, > all operations on a thread-aware event loop have to pay the complexity > and performance cost of being thread-aware, even if the current program > only has one thread. > > smcv Excuse me for reviving an old thread. But I see similar problem today (especially on Arm). The sd-event model uses signals so it is inherently subject to thread issues. It looks like a stronger memory model is needed here (not volatile). Other projects use __atomic builtins for this. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] EXT: sdbus_event loop state mark as volatile?
> >while (e->state != SD_EVENT_FINISHED) {
> >r = sd_event_run(e, (uint64_t) -1);
> >
> > But since e->state is changed by another thread it
Well..then the game is up because sd-bus does not claim to be thread
safe or even aspires to be.. accessing e from different threads will
cause unspecified behaviour.
In any case this patch is not quite what is needed to make it safe.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] EXT: sdbus_event loop state mark as volatile?
On Fri, 06 Sep 2019 at 06:57:22 +, Ray, Ian (GE Healthcare) wrote: > If thread-safety is a design goal (and I don’t believe that it is [1]) > then atomic or thread-safe primitives should be used. > > [1] > https://lists.freedesktop.org/archives/systemd-devel/2017-March/038519.html [1] is about sd-bus, not sd-event, and doesn't say anything about whether sd-event is designed to be thread-safe or not. However, I think you're correct to say that struct sd_event is also only designed to be used from the single thread that "owns" it. If you need a thread-safe event loop, then you need something like GLib's GMainContext, with mutexes to protect its data structures against concurrent access, and a well-defined mechanism for one main-context to "post" events to other main-contexts (which might be running concurrently in a different thread). Many other event loops are available; GMainContext happens to be the one I'm most familiar with, and I know that it is designed to be thread-safe. The price that things like GMainContext pay for being thread-safe is that they are more complex and less efficient than sd-event: in general, all operations on a thread-aware event loop have to pay the complexity and performance cost of being thread-aware, even if the current program only has one thread. smcv ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] EXT: sdbus_event loop state mark as volatile?
> On 5 Sep 2019, at 20.46, Stephen Hemminger wrote:
>
> The libsystemd bus event loop is:
>
>
>while (e->state != SD_EVENT_FINISHED) {
>r = sd_event_run(e, (uint64_t) -1);
>
> But since e->state is changed by another thread it
What other thread?
> should be marked volatile to avoid compiler thinking
> the state doesn't get changed.
>
The “volatile” keyword does not equate to thread safety.
If thread-safety is a design goal (and I don’t believe that it is [1])
then atomic or thread-safe primitives should be used.
[1] https://lists.freedesktop.org/archives/systemd-devel/2017-March/038519.html
While “volatile” _is_ a useful hint to the compiler, it provides *no*
atomic or thread-safe guarantees: for example about the ordering and
visibility of operations in a multi-core system.
It is true that for _certain_ { chipset, compiler, compiler-option }
combinations we can effectively reason about atomicity [for example
of word-size integers], however this does not generalise, and is
certainly not portable.
Ian
>
> diff --git a/src/libsystemd/sd-event/sd-event.c
> b/src/libsystemd/sd-event/sd-event.c
> index 5adbceeb0247..b7be2472a398 100644
> --- a/src/libsystemd/sd-event/sd-event.c
> +++ b/src/libsystemd/sd-event/sd-event.c
> @@ -90,7 +90,7 @@ struct sd_event {
>
> uint64_t iteration;
> triple_timestamp timestamp;
> -int state;
> +volatile int state;
>
> bool exit_requested:1;
> bool need_process_child:1;
> ___
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel
