libspectrum Merge Request #2: Thread safe myglib using C11 atomics (merged)

[BogDan Vatra]

Someone which knows autotools please check configure.ac changes

[BogDan Vatra]

Similar with https://sourceforge.net/p/fuse-emulator/libspectrum/merge-requests/1/ but with less changes

[Fredrick Meunier]

Hi BogDan,
I like this a lot more than the earlier versions :) It is very clean and self-contained and while it uses C11 when available, it remains compatible (without locking) with C89 as used in Fuse.

Any thoughts about using pthreads instead for an option that works for C89 too?

I think we should take the following additional steps before merging:

• Update the libspectrum documentation to describe the standard thread safety features
  • I guess we are basically saying that with this change the libspectrum myglib replacement is like GLib?:
    • "GLib itself is internally completely thread-safe (all global data is automatically locked), but individual data structure instances are not automatically locked for performance reasons. For example, you must coordinate accesses to the same GHashTable from multiple threads."
• glib structures are used in libspectrum_tape and libspectrum_ide_channel and they therefore should follow these restrictions
• lock_hash(), unlock_hash(), lock_list() and unlock_list() should be static functions?
• Any reason that we don't share implementations of the lock and unlock functions between the GSList and GHashTable implementations?

[BogDan Vatra]

Well, we can use pthread too, but considering that C11 is pertty old (+5 years) all modern compilers have it. I'm afraid that pthread is not available on all OSes out there.

• yep, I think we can ditch glib with this change.
• I don't know anything about these structures that are used in libspectrum_tape and libspectrum_ide_channel :(
• yep, these function should be static :)
• for performance reason, I don't want to block a thread if it tries to add an item to a list while another one is adding something to a hash.

[BogDan Vatra]

Where is the libspectrum documentation?

[BogDan Vatra]

made [un]lock_XXXX functions static

[Fredrick Meunier]

The libspectrum documentation is in libspectrum/doc/libspectrum.txt

for performance reason, I don't want to block a thread if it tries to add an item to a list while another one is adding something to a hash.

I just mean that there would be lock() and unlock() functions in a shared file with a parameter for the list or hash lock atomic to save on duplication, what do you think?

[BogDan Vatra]

The libspectrum documentation is in libspectrum/doc/libspectrum.txt
there is no mentioning about [my]glib in that file ...

I just mean that there would be lock() and unlock() functions in a shared file with a parameter for the list or hash lock atomic to save on duplication, what do you think?

Done !

[BogDan Vatra]

Can we automatically switch to myglib if "stdatomic.h" is found? If yes, can somebody help me with configure.ac changes, as I said I have no experience with autoconf/tools.

[Fredrick Meunier]

I expect that most Linux desktops will have glib installed and be using the GTK+ UI and it is better/required to use the standard glib library in those cases.

I've refactored the patch a little to reduce duplication further in the attached patch, do you see any problems with these changes?

[BogDan Vatra]

Yep there are two problems :)
- I think you should use

-# define lock_hash() atomic_lock(&atomic_locker)
-# define unlock_hash() atomic_unlock(&atomic_locker)
-#else
-# define lock_hash()
-# define unlock_hash()

because
static atomic_char atomic_locker = ATOMIC_VAR_INIT(0);
is available only when #ifdef HAVE_STDATOMIC_H

• lock and unlock functions are wrong now, because
  atomic_char unlocked = ATOMIC_VAR_INIT(0);
  atomic_char locked = ATOMIC_VAR_INIT(1);
  must be initialized everyt time before atomic_compare_exchange_strong called because if it fails it will replace it's expected paramater with the desired value (check http://en.cppreference.com/w/c/atomic/atomic_compare_exchange for more info).

[Fredrick Meunier]

Thanks BogDan, I think I've fixed the second issue based on your feedback (and corrected the types of the variables in the functions) in the attached extra patch that applies on top of the last.

I don't think the first point is an issue as when there is no stdatomic.h the preprocessor consumes the argment to the lock() function so no error is raised from atomic_locker being missing. Stylistically, I'd probably prefer to have an initialisation function that returns an opaque type variable we use in either case but I'm not sure there will be a payoff in this scenario.

[BogDan Vatra]

The patch is still not ok, the second paramater is changed not the third one, so the second paramater must be reset every time.

Regarding the first point,IMHO even there is no error, it's still confusing to developers ...

I'll update my patch with (some) of your changes.

[BogDan Vatra]

I updated my patch with some of your changes.

[Fredrick Meunier]

I just pushed another middle ground version to the merge-request-2-threadsafe branch on sf, let me know if you see any remaining bugs or have any other comments. If not, I'll merge over the weekend.

[BogDan Vatra]

No bug, you can ship it!
Is it any reason why you choose not to use atomic_char for locked/unlocked vars ?

[Fredrick Meunier]

Thanks, will do.

I chose that just because my compiler was telling me that expected and value (desired) are not atomic :) :

template <typename _Tp>
static inline bool __c11_atomic_compare_exchange_strong(
    volatile _Atomic(_Tp)* __a, _Tp* __expected, _Tp __value,
    memory_order __success, memory_order __failure) {

[Sergio Baldoví]

Lot of context is lost when the original repository is deleted. See for example
https://sourceforge.net/p/fuse-emulator/libspectrum/merge-requests/1/

The pushed commits have been [9c2f9c] and [bbbcee].