Re: [musl] Suggestion for thread safety

[Lee Shallis <gb2985@gmail.com>]

I already explained that pauseCB is supposed to be a thin wrapper to
the system equiv of pthread_yield, that should tell you all you need
to know about that function, I just named it pauseCB because yieldCB
as name does not imply the thread will pause execution, pauseCB as a
name does, anyways you're free to use pthread_yield directly as for
you it IS a wrapper itself, I used a callback pointer because this is
part of a custom library that decouples the thread API from thread
safety, pauseCB was the ONLY callback linked to the thread API that
both the header & file know of, it doesn't know anything about
pthread_yield or sched_yield etc, think of the lock as the same as a
mutex, just simpler, a lock only allows one thread to operate on
whatever the pointer is for, in the case of LockErrors it is supposed
to prevent execution races for potentially non-thread safe system
calls such a poorly implemented malloc (which can have it's symbol
overwritten by a developer implementation), fprintf etc (which from
what I've heard are NOT thread safe) etc, also errno might not be
thread local under some implementations, it's better to assume it's
not then to assume it is and have all hell break loose. Anyways I only
showed you how I used the lock system so you have a clue on how to
implement your own version with this as inspiration (hence the "take
what you will" statement), I'm not expecting you to do a flat copy
paste, just use it to simplify any code that had to go through the
effort of calling pthread_mutex_create/pthread_mutex_destroy or
whatever, with this method you can just declare a local pointer in the
relevant file/s and set them all to NULL before ever using them (as
the stderr_lock/syserr_lock examples indicate, syserr being for things
like GetLastError in windows), the code I gave was literally a simple
example of how to hide system thread safety details in pure ansi C, if
you want to call those APIs directly, go right ahead, I'm not stopping
you.

As for GRIP I wanted to simulate a semaphore just as I did with LOCK
to mutex, and I wanted to do it with pointers as that was the one
thing that can be guaranteed to be thread safe (assuming the developer
uses the api as intended), the alternative is using thread IDs but I
wasn't sure 0 would not be used for the main thread & I wasn't sure
that a thread ID is always signed so I just chose to go with pointers
where NULL is always understood to mean empty, anyways these where
just suggestion for a simple to implement & understand mutex &
semaphore substitute that can work on any system since it doesn't need
to know if there's threading to operate correctly, just needs the
developer to fill in pauseCB with a function that just calls the equiv
of pthread_yield, nothing else, just that, the point of the call to
pauseCB is to wait long enough for other writes to the same pointer to
get through, even if only one gets through while waiting it's enough
to fail the check after execution resumes, basically the idea is to
"try to fail" instead of "try to succeed", hence why the call to
pauseCB is AFTER the write, if it occured before then a data race
would occur, instead we go with the assumption the data race will
always occur on the pointer and just wait and see if the current
thread write was the last to get through, it will be rare that many
threads try to take the same pointer when it involves objects so the
only thing that will potentially slow the code down a "lot" will be
the system calls which one should expect to slow the code a "lot"
anyways, I've actually already used the LOCK api successfully when
opengl calls my debugging function I pass it AND in some custom code
that is specifically designed to cause a data race where the api would
show if it succeeds in emulating a mutex, I basically launched a
thread to lock a pointer, then launch 3 more threads who each wait on
the lock to be released before trying to print their details & exit
(which is where LockErrors gets called).
your point about the lack of braces around the LockErrno & LockError
is duly noted though.

As for your point about splitting paragraphs up, I'm not very good at
that as you might have noticed by now, anyways the point of these is
that I wanted a simpler system than the one that is provided so that
if I ever put enough work into my library that it no longer needs libc
then I would be able to do so rather seamlessly, in other words just
with LOCK & pauseCB I've achieved thread safety without the file
knowing anything about the system api, since I stilled ended up using
dlopen/dlclose etc in the same file though I might just do away with
pauseCB and just adding a Pause() function in the header for which to
map the system's yield function anyways

On Mon, 21 Feb 2022 at 17:42, Markus Wichmann <nullplan@gmx.net> wrote:
>
> 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