Re: [musl] Suggestion for thread safety

[Markus Wichmann <nullplan@gmx.net>]

On Mon, Feb 21, 2022 at 11:36:06AM +0000, Lee Shallis wrote:
> First I'll start with the code snippets I've copied from my own code:
>
> [...]
>
> #define LockErrors( ud ) LockStdErr( ud ); LockSysErr( ud )
> #define FreeErrors( ud ) FreeSysErr( ud ); FreeStdErr( ud )
>

Style problem: Put a "do {} while (0)" around these, or else you can
have unintended effects. The code "if (cond) LockErrors(foo);" will
always execute "LockSysErr()". And if you put in complementary code at
the bottom of the function, it will always execute "FreeStdErr()",
leading to undefined behavior, and good luck finding it.

> [...]
>         dint err;
>         LOCK_ERRORS
>         (
>             errno = 0;
>             *data = *data ? realloc( *data, size ) : malloc( size );
>             err = *data ? 0 : errno;
>         );

Why lock anything here? errno is thread local, and the allocation
functions must be thread-safe. There is no need to lock anything here.

> SHARED_EXP void LockSiData( LOCK **shared, LOCK *ptr )
> {
>     while ( *shared != ptr )
>     {
>         if ( !(*shared) )
>             *shared = ptr;
>         pauseCB( ptr );
>     }
> }
>

And now I have to ask what the possible point of this is. Your call to
an external function manages to make you avoid the need for a volatile
keyword (pauseCB might access global variables, and shared might be
pointing at one, so the compiler doesn't know the state of *shared after
the return of pauseCB), however, there is probably also some kind of
barrier missing. And in practice this is going to burn up a CPU core
with completely unnecessary work.

After you have established to your satisfaction that the lock is taken,
it would be best to suspend the current thread for exactly as long as
the lock holder takes to reach the complementary unlock function. You
can use futexes for that. Better yet, you can just use a pthread_mutex_t
for that, which does all that you need internally.

Besides, if "shared" points to a shared variable (as the name suggests),
then this constitutes a data race (multiple threads read, some threads
write) and is therefore undefined behavior. It also has a race condition
beyond that (if one thread is suspended after seeing *shared is NULL,
but before setting *shared, another can pass the same check, set
*shared, and exit the loop. A function like pthread_yield() doesn't have
to do anything useful against that).

It looks like you actually want some kind of compare-and-swap function
here, but again, you can also just skip it in favor of a
pthread_mutex_t.

> [...]
>
> Take what you will of the above, the Allot function was included just
> to give you an idea of how the locks are made, as for pauseCB that
> should be re-mapped to a variant that calls something like
> pthread_yield(), it's what I tested with, anyways the general idea is
> that the shared lock defaults to NULL when not locked, when you want
> to lock it you 1st wait for it to be NULL then as soon as it's empty
> you fill it with your own pointer then wait for other threads who
> detected the same situation to fill there pointers into the shared
> pointer (assuming any other threads were trying to lock the pointer),
> when the wait is over you check again if the pointer is not matching
> your own, if not then you failed to lock and should wait longer, using
> pointers lends itself well to threading as the pointer being used to
> take the shared pointer will normally be unique to the thread (because
> rarely will anyone try to use the same pointer in multiple threads for
> this process that has a clear example in it's declaration), using
> const char * for the LOCK typedef means a thread can give more
> information about itself or the pointer it locked with if it so
> chooses.
>

Structure your thoughts into sentences, and your sentences into
paragraphs, please. The above is one long run-on sentence, that starts
with a point about the Allot function and ends in a justification for
the type, and in between has visited many other topics. You are not
James Joyce, and you are not writing the Ulysses.

Anyway, what I'm still trying to find is the point to all of this.
Mutual exclusion is a solved problem, and is in general hard to solve
while maintaining some semblance of performance and remaining safe.
People with more knowledge than me have poured a lot of thought into
libraries that do this, so why not use them? Especially since they are
included in the threading implementation already.

> [GRIP is a fine-grained lock]

Another solved problem. You can use counting semaphores to solve the
same problem in a simpler way, at least if I understood correctly.
Alternatively, you can use an interesting system of mutexes and
conditions, but that typically doesn't end up looking very pretty.

Ciao,
Markus