[musl] Suggestion for thread safety

[musl] Suggestion for thread safety

[Lee Shallis]

First I'll start with the code snippets I've copied from my own code:

/* The error locks only work for conforming code, anything that doesn't will
 * corrupt the result if it tries to do something that would need them */
typedef const char* LOCK;
typedef struct _GRIP { LOCK _; LOCK *_lock; struct _GRIP *_grip; } GRIP;
BASIC void    LockStdErr( LOCK *ud );
BASIC void    LockSysErr( LOCK *ud );
BASIC void    LockSiData( LOCK **shared, LOCK *ud );
BASIC void    LockMiData( GRIP *shared, GRIP *ud );
BASIC dint    FreeStdErr( LOCK *ud );
BASIC dint    FreeSysErr( LOCK *ud );
BASIC dint    FreeSiData( LOCK **shared, LOCK *ud );
BASIC void    FreeMiData( GRIP *shared, GRIP *ud );

#define LockErrors( ud ) LockStdErr( ud ); LockSysErr( ud )
#define FreeErrors( ud ) FreeSysErr( ud ); FreeStdErr( ud )

#define DO_LOCK( X ) { LOCK _lock = ""; X }
#define DO_GRIP( X ) { GRIP _grip = {NULL}; X }

#define LOCK_SIDATA( SID, X ) \
    DO_LOCK( LockSiData( SID, &_lock ); X *SID = NULL; )

#define LOCK_MIDATA( MID, X ) \
    DO_GRIP( LockMiData( MID, &_grip ); X FreeMiData( MID, &_grip ); )

#define LOCK_STDERR( X ) \
    DO_LOCK( LockStdErr( &_lock ); X FreeStdErr( &_lock ); )

#define LOCK_SYSERR( X ) \
    DO_LOCK( LockSysErr( &_lock ); X FreeSysErr( &_lock ); )

#define LOCK_ERRORS( X ) \
    DO_LOCK( LockErrors( &_lock ); X FreeErrors( &_lock ); )

#define LOCK_SIDATA_AND_ERRORS( SID, X ) \
    DO_LOCK \
    ( \
        LockSiData( SID, &_lock ); \
        LockErrors( &_lock ); \
        X \
        FreeErrors( &_lock ); \
        *SID = NULL; \
    )
...
SHARED_EXP void NoPause() {}

LOCK *stderr_lock = NULL;
LOCK *syserr_lock = NULL;
allot_cb AllotCB = Allot;
pause_cb pauseCB = NoPause;

void LockStdErr( LOCK *ud ) { LockSiData( &stderr_lock, ud ); }
dint FreeStdErr( LOCK *ud ) { return FreeSiData( &stderr_lock, ud ); }

void LockSysErr( LOCK *ud ) { LockSiData( &syserr_lock, ud ); }
dint FreeSysErr( LOCK *ud ) { return FreeSiData( &syserr_lock, ud ); }
...
SHARED_EXP dint    Allot( void *ud, void **data, size_t size )
{
    (void)ud;
    if ( size )
    {
        dint err;
        LOCK_ERRORS
        (
            errno = 0;
            *data = *data ? realloc( *data, size ) : malloc( size );
            err = *data ? 0 : errno;
        );
        return err;
    }
    else if ( *data )
    {
        free( *data );
        *data = NULL;
    }
    return 0;
}
SHARED_EXP void LockSiData( LOCK **shared, LOCK *ptr )
{
    while ( *shared != ptr )
    {
        if ( !(*shared) )
            *shared = ptr;
        pauseCB( ptr );
    }
}

SHARED_EXP dint FreeSiData( LOCK **shared, LOCK *ud )
{
    if ( *shared == ud )
    {
        *shared = NULL;
        return 0;
    }
    return EPERM;
}
/* These 2 are untested */
SHARED_EXP void LockMiData( GRIP *shared, GRIP *ud )
{
    while ( shared->_grip != ud )
    {
        while ( shared->_grip )
            shared = shared->_grip;
        shared->_grip = ud;
        pauseCB();
    }
}
SHARED_EXP void    FreeMiData( GRIP *shared, GRIP *ud )
{
    LOCK _ = NULL;
    LockSiData( &(shared->_lock), &_ );
    LockSiData( &(ud->_lock), &_ );
    while ( shared->_grip && shared->_grip != ud )
        shared = shared->_grip;
    if ( shared->_grip == ud )
    {
        shared->_grip = ud->_grip;
        ud->_grip = NULL;
    }
    (void)FreeSiData( &(ud->_lock), &_ );
    (void)FreeSiData( &(shared->_lock), &_ );
}

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.

GRIP on the other hand is meant for multi-threaded data, for example
assigning parts of a buffer, the owning thread should fill in the '_'
member after acquiring the grip, if the previous grip doesn't have
info about the section of the buffer it took then it should wait until
it does, when it does it is then free to take it's own chunk after
analysing what has already been taken by other grips linked to the
shared grip, this method would lend itself well to graphics generation
for example as a thread for each column could be launched in ascending
order (so that the row contents are process from left to right), at
least that's the theory on the grip one, requires testing which I'm
sure you can do better than I can at the moment.

Re: [musl] Suggestion for thread safety

[Markus Wichmann]

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

Re: [musl] Suggestion for thread safety

[Lee Shallis]

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

Re: [musl] Suggestion for thread safety

[Markus Wichmann]

On Wed, Feb 23, 2022 at 12:30:43AM +0000, Lee Shallis wrote:
> think of the lock as the same as a mutex, just simpler,

It isn't really simpler than a fast mutex, but a lot buggier.

> 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),

Any malloc implementation has to be thread safe. It is not sensible to
use one that isn't. It is also not sensible to pessimise the whole
program because one day a developer might choose to do something stupid.

> fprintf etc (which from
> what I've heard are NOT thread safe)

fprintf() takes a FILE and must therefore act as if flockfile() was
called on entry and funlockfile() on exit. IOW: Accesses to the same file
are ordered. If not, the implementation is broken.

> also errno might not be
> thread local under some implementations,

Any such implementation is fundamentally broken and cannot be repaired.
There is no way to call a blocking system call in such a system without
taking the lock on errno first, thereby suspending all other threads
that might try to access errno, which is pretty much all of them, except
maybe for some pure calculations somewhere, thereby negating any benefit
multi-threading might have brought the program.

> it's better to assume it's
> not then to assume it is and have all hell break loose.

I disagree. It is better to assume the standards are followed and fix
problems as they occur than to assume you are programming for some kind
of space alien computer that works by rules inconsistent with any normal
system. Report the bug, work around it if necessary, and move on.

I recently found myself on a system on which, unbeknownst to me,
sendmsg() always returns 0 when called on TCP sockets. I wrote a program
assuming it would work. It did not. I reported the bug and worked around
the problem with malloc() and send(). That is why we test.

> just use it to simplify any code that had to go through the
> effort of calling pthread_mutex_create/pthread_mutex_destroy or
> whatever,

PTHREAD_MUTEX_INITIALIZER exists.

> the code I gave was literally a simple
> example of how to hide system thread safety details in pure ansi C,

Nonsense. You didn't hide anything, you didn't make anything safer, and
by staying in ANSI C, you make it impossible to achieve your goal.

> As for your point about splitting paragraphs up, I'm not very good at
> that as you might have noticed by now,

If you don't care to be understood, I won't care to understand you. And
it is pretty difficult to convince people that don't understand you.

> 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,

You might have bitten off more than you can chew with that goal. Writing
a libc is no mean feat, and developing a library to the point it could
replace libc takes about as much effort. Somehow people seem to think
they'll start with memcpy() and it will stay on that level of
complexity. It won't.

> in other words just
> with LOCK & pauseCB I've achieved thread safety without the file
> knowing anything about the system api,

You have indeed not done that. You have instead written the word "lock"
enough times to give someone skim-reading the file false confidence that
this stuff will actually work in a multi-threaded context, only to then
fail under high load for inexplicable reasons.

I keep seeing this behavior from programmers that ought to know better.
You see, an exclusive lock consists of two parts: The mutual exclusion
and the sleep. And yes, spinlocks skip the second part, but my point is:
The mutual exclusion is actually the easy part, and any hack with a
Messiah complex and a CPU manual can do it. The sleep is the hard part,
if you want to do it right. It needs to be Goldilocks. Too short, and
you are wasting resources (every time your thread spins in the loop is
time the CPU could have better spent on other threads), too long and you
are wasting time.

Your sleep is definitely too short, and you didn't even get the mutual
exclusion part right.

Ciao,
Markus

Re: [musl] Suggestion for thread safety

[Rich Felker]

On Wed, Feb 23, 2022 at 07:57:46PM +0100, Markus Wichmann wrote:
> On Wed, Feb 23, 2022 at 12:30:43AM +0000, Lee Shallis wrote:
> > in other words just
> > with LOCK & pauseCB I've achieved thread safety without the file
> > knowing anything about the system api,
> 
> You have indeed not done that. You have instead written the word "lock"
> enough times to give someone skim-reading the file false confidence that
> this stuff will actually work in a multi-threaded context, only to then
> fail under high load for inexplicable reasons.
> 
> I keep seeing this behavior from programmers that ought to know better.
> You see, an exclusive lock consists of two parts: The mutual exclusion
> and the sleep. And yes, spinlocks skip the second part, but my point is:
> The mutual exclusion is actually the easy part, and any hack with a
> Messiah complex and a CPU manual can do it. The sleep is the hard part,
> if you want to do it right. It needs to be Goldilocks. Too short, and
> you are wasting resources (every time your thread spins in the loop is
> time the CPU could have better spent on other threads), too long and you
> are wasting time.
> 
> Your sleep is definitely too short, and you didn't even get the mutual
> exclusion part right.

It's worse: it has *three* parts, the third being the _synchronizing
memory_ part, which I'm guessing this made no attempt to do at all.
That's where all the time in a lock is actually spent, and if you
somehow avoid doing that (note: x86 will mostly do it for you and send
you the bill), things will blow up spectacularly.

Re: [musl] Suggestion for thread safety

[Lee Shallis]

On Wed, 23 Feb 2022 at 18:58, Markus Wichmann <nullplan@gmx.net> wrote:
>
> On Wed, Feb 23, 2022 at 12:30:43AM +0000, Lee Shallis wrote:
> > think of the lock as the same as a mutex, just simpler,
>
> It isn't really simpler than a fast mutex, but a lot buggier.
>
There are no bugs, I made sure to test this after all, I deliberately
created a situation where any bugs would show them selves, the only
thing that was a potential problem I've now fixed (after doing some
research to find out if 0 is ever a valid thread id), the concept
remains the same, just with support for developer calling LockSiData
twice:

#ifdef _WIN32
typedef DWORD WHICH;
#else
#include <dlfcn.h>
#include <pthread.h>
typedef pid_t WHICH;
#endif

/* Which Thread Id */
BASIC WHICH    Which();
/* Pause thread execution to yield resources */
BASIC void    Pause();

/* The error locks only work for conforming code, anything that doesn't will
 * corrupt the result if it tries to do something that would need them */
typedef struct _LOCK LOCK;
typedef struct _GRIP GRIP;
struct _LOCK { uint num; WHICH tid; };
struct _GRIP { uint num; WHICH tid; GRIP *next, *prev; void *ud; };

Instead of initialising with NULL, it's now just {0}, that should be a
correct way to initialise any correctly designed object, LockSiData &
FreeSiData now only need a pointer to the shared lock, the code
remains pretty much the same but instead of filling with a pointer it
fills the tid member, the num member is just for how many calls to
FreeSiData need to be made to match the calls to LockSiData, don't
call something buggy if you've never tried it, that's like being a
flat earther, talking out your ass.

> > 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),
>
> Any malloc implementation has to be thread safe. It is not sensible to
> use one that isn't. It is also not sensible to pessimise the whole
> program because one day a developer might choose to do something stupid.
>
> > fprintf etc (which from
> > what I've heard are NOT thread safe)
>
> fprintf() takes a FILE and must therefore act as if flockfile() was
> called on entry and funlockfile() on exit. IOW: Accesses to the same file
> are ordered. If not, the implementation is broken.

And under my current implementation it can actually do that, it
doesn't have to care if they were called at all and just lock anyways,
the developer is already expecting a slow down because of system
calls, and since musl is a separate implementation it should address
the deficiencies of the standard implementation which in turn might
make it the default implementation on linux in the future, wouldn't
that be a grand trophy, to say your code is so good it's now the
default on a popular operating system

>
> > also errno might not be
> > thread local under some implementations,
>
> Any such implementation is fundamentally broken and cannot be repaired.
> There is no way to call a blocking system call in such a system without
> taking the lock on errno first, thereby suspending all other threads
> that might try to access errno, which is pretty much all of them, except
> maybe for some pure calculations somewhere, thereby negating any benefit
> multi-threading might have brought the program.
>
> > it's better to assume it's
> > not then to assume it is and have all hell break loose.
>
> I disagree. It is better to assume the standards are followed and fix
> problems as they occur than to assume you are programming for some kind
> of space alien computer that works by rules inconsistent with any normal
> system. Report the bug, work around it if necessary, and move on.

That last line of yours highlights the point of this code, just
because you haven't encountered the problem yet doesn't mean you
shouldn't take advantage of code that's proven to be able to work
around the problem directly should, it's like health insurance, you
don't want to need it but you'll kick yourself if you didn't have it
when you do

> I recently found myself on a system on which, unbeknownst to me,
> sendmsg() always returns 0 when called on TCP sockets. I wrote a program
> assuming it would work. It did not. I reported the bug and worked around
> the problem with malloc() and send(). That is why we test.

Another thing that highlights my point of "better to assume then to
not assume and have all hell break loose"

> > just use it to simplify any code that had to go through the
> > effort of calling pthread_mutex_create/pthread_mutex_destroy or
> > whatever,
>
> PTHREAD_MUTEX_INITIALIZER exists.

That I actually wasn't aware of, doesn't change that my initialisation
method is still simpler, I also have no need for a DestroySiData, as
long as it's free'd then it'll be in the same state it started

> > the code I gave was literally a simple
> > example of how to hide system thread safety details in pure ansi C,
>
> Nonsense. You didn't hide anything, you didn't make anything safer, and
> by staying in ANSI C, you make it impossible to achieve your goal.

Again, try the code before you talk out of your ass

> > As for your point about splitting paragraphs up, I'm not very good at
> > that as you might have noticed by now,
>
> If you don't care to be understood, I won't care to understand you. And
> it is pretty difficult to convince people that don't understand you.

So in your eyes everyone is capable of everything you can do? That's
called being ignorant, you better learn to change that because sooner
or later it's gonna bite you in the ass.

> > 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,
>
> You might have bitten off more than you can chew with that goal. Writing
> a libc is no mean feat, and developing a library to the point it could
> replace libc takes about as much effort. Somehow people seem to think
> they'll start with memcpy() and it will stay on that level of
> complexity. It won't.

I didn't say it would be simple, also I did say "if I ever put enough
work into my library", the whole point of that line is because I knew
that in it's current state it still needs libc and I'm not heavily
motivated to change that, simply motivated enough to put some effort
towards it as I go along.

> > in other words just
> > with LOCK & pauseCB I've achieved thread safety without the file
> > knowing anything about the system api,
>
> You have indeed not done that. You have instead written the word "lock"
> enough times to give someone skim-reading the file false confidence that
> this stuff will actually work in a multi-threaded context, only to then
> fail under high load for inexplicable reasons.

Well then they should be looking at the comments too, anyways I did
after all decide to change that to just having the code directly in
file, I'm already using dlopen etc in the file, what's one more,
additionally my thread.h file includes pthreads causing the same
library to link to pthreads anyways so I might as well just include it
and directly fill the functions I need

> I keep seeing this behavior from programmers that ought to know better.
> You see, an exclusive lock consists of two parts: The mutual exclusion
> and the sleep. And yes, spinlocks skip the second part, but my point is:
> The mutual exclusion is actually the easy part, and any hack with a
> Messiah complex and a CPU manual can do it. The sleep is the hard part,
> if you want to do it right. It needs to be Goldilocks. Too short, and
> you are wasting resources (every time your thread spins in the loop is
> time the CPU could have better spent on other threads), too long and you
> are wasting time.

I see you have no idea what pthread_yield does otherwise you wouldn't
talk outta your ass like that, the whole point of the pause call is to
yield those resources, there's no getting round that part

> Your sleep is definitely too short, and you didn't even get the mutual
> exclusion part right.

You do realise that other threads will get in before that sleep ends
right? there's no way for them to take it if they entered their sleep
before the current one, and as I've stated before I created a
deliberate race scenario to test this on and it worked perfectly, you
REALLY need to start trying code before you talk about it. For
reference this is my test code:

LOCK tsidata = {0};

#ifndef _DEBUG
pthread_mutex_t debug_mutex;
#endif

dint TestPseudoMutex( THREAD *thread )
{
    ucap tid = SeekThreadTid(thread);
    ach name[bitsof(ucap)*2] = {0};

    sprintf( name, "%s %zu", __func__, tid );
    SetThreadName( thread, name );
    SetThreadDieOnExit( thread );

    while ( tsidata.tid )
        Pause();

    /* Test our pseduo mutices */
    LOCK_ERRORS( printf("%s(%zu) died\n", __func__, tid ); );

    return 0;
}

dint HelloWorld( THREAD *thread )
{
    dint i, err = 0;
    ARM *arm = SeekThreadArm(thread);
    ucap tid = SeekThreadTid( thread );
    ach name[bitsof(ucap)*2] = {0};

    sprintf( name, "%s %zu", __func__, tid );
    SetThreadName( thread, name );
    SetThreadDieOnExit( thread );

    LOCK_ERRORS( printf( "Thread %zu: Hello World!\n", tid ); );

    LockSiData( &tsidata );
    for ( i = 0; i < 3; ++i )
    {
        THREAD *child = NULL;
        LOCK_ERRORS( printf( "Spawning thread %d\n", i ); );
        err = MakeThread( thread, &child );
        if ( err )
        {
            LOCK_ERRORS
            (
                SHARED_ECHO_ERRNO( stdout, err );
                printf( "Failed to spawn thread %d\n", i );
            );
            break;
        }

        err = InitThread( thread, child, TestPseudoMutex );
        if ( err )
        {
            LOCK_ERRORS
            (
                SHARED_ECHO_ERRNO( stdout, err );
                printf( "Failed to run thread %d\n", i );
            );
            KillThread( thread, child );
            break;
        }
    }
    FreeSiData( &tsidata );

    while ( SeekBranchNumKid( arm ) )
        Pause();

    LOCK_ERRORS( printf("%s(%zu) died\n", __func__, tid ); );

    return err;
}

As you can see there's no way for the worst scenario to not be hit if
my code was buggy, it works as intended,

Re: [musl] Suggestion for thread safety

[Joakim Sindholt]

[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]

On Sat, 26 Feb 2022 09:56:04 +0000, Lee Shallis <gb2985@gmail.com> wrote:
> On Wed, 23 Feb 2022 at 18:58, Markus Wichmann <nullplan@gmx.net> wrote:
> >
> > On Wed, Feb 23, 2022 at 12:30:43AM +0000, Lee Shallis wrote:
> > > think of the lock as the same as a mutex, just simpler,
> >
> > It isn't really simpler than a fast mutex, but a lot buggier.
> >
> There are no bugs, I made sure to test this after all, I deliberately
> created a situation where any bugs would show them selves, the only
> thing that was a potential problem I've now fixed (after doing some
> research to find out if 0 is ever a valid thread id), the concept
> remains the same, just with support for developer calling LockSiData
> twice:

Did I use it wrong then?

zhasha@wirbelwind /home/zhasha ; gcc -lpthread buglock.c
zhasha@wirbelwind /home/zhasha ; ./a.out
var = 1, expected 0
zhasha@wirbelwind /home/zhasha ; ./a.out
var = 1, expected 0
zhasha@wirbelwind /home/zhasha ; ./a.out
var = 2, expected 1
zhasha@wirbelwind /home/zhasha ; ./a.out
var = 2, expected 1
var = 1, expected 0
zhasha@wirbelwind /home/zhasha ; ./a.out
var = 1, expected 0

[-- Attachment #2: buglock.c --]
[-- Type: text/plain, Size: 1270 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <stdint.h>
#include <errno.h>

static void NoPause(void *ud)
{
    (void)ud;
}

typedef int dint;
typedef const char* LOCK;
void (*pauseCB)(void *) = NoPause;

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

static dint FreeSiData( LOCK **shared, LOCK *ud )
{
    if ( *shared == ud )
    {
        *shared = NULL;
        return 0;
    }
    return EPERM;
}

static LOCK *l = 0;
static volatile int var = 0;

static void *thread(void *arg)
{
    int i;
    while (1) {
        LockSiData(&l, arg);
        i = var++;
        if (i != 0) {
            printf("var = %d, expected 0\n", i);
            exit(1);
        }
        i = var--;
        if (i != 1) {
            printf("var = %d, expected 1\n", i);
            exit(1);
        }
        FreeSiData(&l, arg);
    }
    return 0;
}

int main(void)
{
    pthread_t pt;
    uintptr_t i;

    for (i = 1; i <= 2; i++) {
        if ((errno = pthread_create(&pt, 0, thread, (void *)i)) != 0) {
            printf("pthread_create failed: %m\n");
            return 1;
        }
    }
    pthread_exit(0);
    return 0;
}

Re: [musl] Suggestion for thread safety

[Lee Shallis]

Yes, as I mentioned before, pauseCB is supposed to have it's pointer
be changed by the developer, in other words you forgot to plugin a
pthreads compatible call prior to your threads starting, considering
you made that mistake I suppose it is a good thing I since switched to
a non-redirectable pointer:

#ifdef _WIN32
typedef DWORD WHICH;
#else
#include <dlfcn.h>
#include <pthread.h>
typedef pid_t WHICH;
#endif

/* Which Thread Id */
BASIC WHICH    Which();
/* Pause thread execution to yield resources */
BASIC void    Pause();

/* The error locks only work for conforming code, anything that doesn't will
 * corrupt the result if it tries to do something that would need them */
typedef struct _LOCK LOCK;
typedef struct _GRIP GRIP;
struct _LOCK { uint num; WHICH tid; };
struct _GRIP { uint num; WHICH tid; GRIP *next, *prev; void *ud; };
...
#ifdef _WIN32
SHARED_EXP WHICH    Which() { return GetCurrentThreadId(); }
SHARED_EXP void        Pause() { SwitchToThread(); }
#else
SHARED_EXP WHICH    Which() { return gettid(); }
SHARED_EXP void        Pause() { pthread_yield(); }
#endif
...
SHARED_EXP void LockSiData( LOCK *shared )
{
    WHICH tid = Which();
    while ( shared->tid != tid )
    {
        if ( !(shared->tid) )
            shared->tid = tid;
        Pause();
    }
    shared->num++;
}

SHARED_EXP dint FreeSiData( LOCK *shared )
{
    if ( shared->tid == Which() )
    {
        shared->num--;
        if ( !(shared->num) )
            shared->tid = (WHICH)0;
        return 0;
    }
    return EACCES;
}

I'm not a fan of plugging in APIs directly but in this case I
eventually decided I ought to make an exception like I did with dlopen
etc, the name NoPause() was supposed to clue you in that to switch to
threading you needed to change what pauseCB pointed to, I guess that
wasn't clear enough, anyways whether you stick to the original code or
dump my (copy-pasted) code from my library into the test and edit
whatever you deem necessary for a valid test in your eyes, as for why
NoPause was even necessary, it's because I'd planned on documenting
the library to say that by default it's only single threaded mode
compatible but with a simple change of callback from NoPause to a
developer wrapper for the equivalent of pthread_yield() it would
become multi-threaded safe. I switched to the object with a reference
count because I kept making the mistake of not thinking through how I
used it well enough causing me to eventually decide the method wasn't
complex enough under the hood, for the malloc example I gave before it
can be extended to support thread specific errno, all it takes is page
locks when connecting pages together, memory locks when taking a
section of said pages & then some #ifdef code that switches between:

a LOCK_ERRORS( errno = err; ); statement &
a plain errno = err; statement

Either way the function can be programmed the same right up until that
point (unless there's some way to detect in code which is suitable)

On Sat, 26 Feb 2022 at 11:39, Joakim Sindholt <opensource@zhasha.com> wrote:
>
> On Sat, 26 Feb 2022 09:56:04 +0000, Lee Shallis <gb2985@gmail.com> wrote:
> > On Wed, 23 Feb 2022 at 18:58, Markus Wichmann <nullplan@gmx.net> wrote:
> > >
> > > On Wed, Feb 23, 2022 at 12:30:43AM +0000, Lee Shallis wrote:
> > > > think of the lock as the same as a mutex, just simpler,
> > >
> > > It isn't really simpler than a fast mutex, but a lot buggier.
> > >
> > There are no bugs, I made sure to test this after all, I deliberately
> > created a situation where any bugs would show them selves, the only
> > thing that was a potential problem I've now fixed (after doing some
> > research to find out if 0 is ever a valid thread id), the concept
> > remains the same, just with support for developer calling LockSiData
> > twice:
>
> Did I use it wrong then?
>
> zhasha@wirbelwind /home/zhasha ; gcc -lpthread buglock.c
> zhasha@wirbelwind /home/zhasha ; ./a.out
> var = 1, expected 0
> zhasha@wirbelwind /home/zhasha ; ./a.out
> var = 1, expected 0
> zhasha@wirbelwind /home/zhasha ; ./a.out
> var = 2, expected 1
> zhasha@wirbelwind /home/zhasha ; ./a.out
> var = 2, expected 1
> var = 1, expected 0
> zhasha@wirbelwind /home/zhasha ; ./a.out
> var = 1, expected 0

Re: [musl] Suggestion for thread safety

[Rich Felker]

On Sun, Feb 27, 2022 at 11:32:47PM +0000, Lee Shallis wrote:
> Yes, as I mentioned before, pauseCB is supposed to have it's pointer
> be changed by the developer, in other words you forgot to plugin a
> pthreads compatible call prior to your threads starting, considering
> you made that mistake I suppose it is a good thing I since switched to
> a non-redirectable pointer:
> 
> #ifdef _WIN32
> typedef DWORD WHICH;
> #else
> #include <dlfcn.h>
> #include <pthread.h>
> typedef pid_t WHICH;
> #endif
> 
> /* Which Thread Id */
> BASIC WHICH    Which();
> /* Pause thread execution to yield resources */
> BASIC void    Pause();
> 
> /* The error locks only work for conforming code, anything that doesn't will
>  * corrupt the result if it tries to do something that would need them */
> typedef struct _LOCK LOCK;
> typedef struct _GRIP GRIP;
> struct _LOCK { uint num; WHICH tid; };
> struct _GRIP { uint num; WHICH tid; GRIP *next, *prev; void *ud; };
> ....
> #ifdef _WIN32
> SHARED_EXP WHICH    Which() { return GetCurrentThreadId(); }
> SHARED_EXP void        Pause() { SwitchToThread(); }
> #else
> SHARED_EXP WHICH    Which() { return gettid(); }
> SHARED_EXP void        Pause() { pthread_yield(); }
> #endif
> ....
> SHARED_EXP void LockSiData( LOCK *shared )
> {
>     WHICH tid = Which();
>     while ( shared->tid != tid )
>     {
>         if ( !(shared->tid) )
>             shared->tid = tid;
>         Pause();
>     }
>     shared->num++;
> }
> 
> SHARED_EXP dint FreeSiData( LOCK *shared )
> {
>     if ( shared->tid == Which() )
>     {
>         shared->num--;
>         if ( !(shared->num) )
>             shared->tid = (WHICH)0;
>         return 0;
>     }
>     return EACCES;
> }
> 
> I'm not a fan of plugging in APIs directly but in this case I
> eventually decided I ought to make an exception like I did with dlopen
> etc, the name NoPause() was supposed to clue you in that to switch to
> threading you needed to change what pauseCB pointed to, I guess that
> wasn't clear enough, anyways whether you stick to the original code or
> dump my (copy-pasted) code from my library into the test and edit
> whatever you deem necessary for a valid test in your eyes, as for why
> NoPause was even necessary, it's because I'd planned on documenting
> the library to say that by default it's only single threaded mode
> compatible but with a simple change of callback from NoPause to a
> developer wrapper for the equivalent of pthread_yield() it would
> become multi-threaded safe. I switched to the object with a reference
> count because I kept making the mistake of not thinking through how I
> used it well enough causing me to eventually decide the method wasn't
> complex enough under the hood, for the malloc example I gave before it
> can be extended to support thread specific errno, all it takes is page
> locks when connecting pages together, memory locks when taking a
> section of said pages & then some #ifdef code that switches between:
> 
> a LOCK_ERRORS( errno = err; ); statement &
> a plain errno = err; statement
> 
> Either way the function can be programmed the same right up until that
> point (unless there's some way to detect in code which is suitable)

If this is supposed to have any relevance to musl, can you clarify
what that is? If not, another venue would probably be more
appropriate.

Rich

Re: [musl] Suggestion for thread safety

[Joakim Sindholt]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]

On Sun, 27 Feb 2022 23:32:47 +0000, Lee Shallis <gb2985@gmail.com> wrote:
> Yes, as I mentioned before, pauseCB is supposed to have it's pointer
> be changed by the developer, in other words you forgot to plugin a
> pthreads compatible call prior to your threads starting, considering
> you made that mistake I suppose it is a good thing I since switched to
> a non-redirectable pointer:

How many times do you want to do this?

zhasha@wirbelwind /home/zhasha ; gcc -D_GNU_SOURCE -lpthread buglock.c
zhasha@wirbelwind /home/zhasha ; ./a.out
var = 1, expected 0
zhasha@wirbelwind /home/zhasha ; ./a.out
var = 1, expected 0
zhasha@wirbelwind /home/zhasha ; ./a.out
var = 1, expected 0
zhasha@wirbelwind /home/zhasha ; ./a.out
var = 1, expected 0
zhasha@wirbelwind /home/zhasha ; ./a.out
var = 1, expected 0
var = 2, expected 1
zhasha@wirbelwind /home/zhasha ; ./a.out
var = 1, expected 0
var = 2, expected 1

[-- Attachment #2: buglock.c --]
[-- Type: text/plain, Size: 1475 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <stdint.h>
#include <errno.h>
#include <unistd.h>

typedef int dint;
typedef pid_t WHICH;
typedef struct _LOCK LOCK;
struct _LOCK { uint num; WHICH tid; };

WHICH Which() { return gettid(); }
void Pause() { pthread_yield(); }

void LockSiData( LOCK *shared )
{
    WHICH tid = Which();
    while ( shared->tid != tid )
    {
        if ( !(shared->tid) )
            shared->tid = tid;
        Pause();
    }
    shared->num++;
}

dint FreeSiData( LOCK *shared )
{
    if ( shared->tid == Which() )
    {
        shared->num--;
        if ( !(shared->num) )
            shared->tid = (WHICH)0;
        return 0;
    }
    return EACCES;
}

static LOCK l;
static volatile int var = 0;

static void *thread(void *arg)
{
    int i;
    while (1) {
        LockSiData(&l);
        i = var++;
        if (i != 0) {
            printf("var = %d, expected 0\n", i);
            exit(1);
        }
        clock_nanosleep(CLOCK_MONOTONIC, 0, &(struct timespec){.tv_sec = 0, .tv_nsec = 1}, 0);
        i = var--;
        if (i != 1) {
            printf("var = %d, expected 1\n", i);
            exit(1);
        }
        FreeSiData(&l);
    }
    return 0;
}

int main(void)
{
    pthread_t pt;
    int i;

    for (i = 0; i < 2; i++) {
        if ((errno = pthread_create(&pt, 0, thread, 0)) != 0) {
            printf("pthread_create failed: %m\n");
            return 1;
        }
    }
    pthread_exit(0);
}

Re: [musl] Suggestion for thread safety

[Lee Shallis]

[-- Attachment #1: Type: text/plain, Size: 1317 bytes --]

Seems the wait just wasn't long enough, at about 4 yields onwards the
results become consistent success, I've attached the file I did the
experiments in, I even tried it under -O3 and no exits were
encountered, so yes my method works, just needs a bit more wait time
for extreme cases

On Mon, 28 Feb 2022 at 08:48, Joakim Sindholt <opensource@zhasha.com> wrote:
>
> On Sun, 27 Feb 2022 23:32:47 +0000, Lee Shallis <gb2985@gmail.com> wrote:
> > Yes, as I mentioned before, pauseCB is supposed to have it's pointer
> > be changed by the developer, in other words you forgot to plugin a
> > pthreads compatible call prior to your threads starting, considering
> > you made that mistake I suppose it is a good thing I since switched to
> > a non-redirectable pointer:
>
> How many times do you want to do this?
>
> zhasha@wirbelwind /home/zhasha ; gcc -D_GNU_SOURCE -lpthread buglock.c
> zhasha@wirbelwind /home/zhasha ; ./a.out
> var = 1, expected 0
> zhasha@wirbelwind /home/zhasha ; ./a.out
> var = 1, expected 0
> zhasha@wirbelwind /home/zhasha ; ./a.out
> var = 1, expected 0
> zhasha@wirbelwind /home/zhasha ; ./a.out
> var = 1, expected 0
> zhasha@wirbelwind /home/zhasha ; ./a.out
> var = 1, expected 0
> var = 2, expected 1
> zhasha@wirbelwind /home/zhasha ; ./a.out
> var = 1, expected 0
> var = 2, expected 1

[-- Attachment #2: lock.c --]
[-- Type: text/x-csrc, Size: 1725 bytes --]

#define _GNU_SOURCE
#include <stdbool.h>
#include <unistd.h>
#include <errno.h>
#include <time.h>
#include <sched.h>
#include <pthread.h>
#include <stdlib.h>
#include <stdio.h>

typedef unsigned int uint;
typedef struct _LOCK { uint num; pid_t tid; } LOCK;

void LockSiData( LOCK *shared )
{
	pid_t tid = gettid();
	uint const yield = 4;
	uint i;
	/*
	struct timespec ts = {0};
	ts.tv_nsec = 1;
	*/

	while ( shared->tid != tid )
	{
		if ( !(shared->tid) )
			shared->tid = tid;
		//clock_nanosleep(CLOCK_MONOTONIC, 0, &ts, 0);
		for ( i = 0; i < yield; ++i )
			sched_yield();
	}

	shared->num++;
}

void FreeSiData( LOCK *shared )
{
	pid_t tid = gettid();
	if ( shared->tid != tid )
		return;
	shared->num--;
	if ( shared->num )
		return;
	shared->tid = (pid_t)0;
	//sched_yield();
}

volatile uint quit = 0;
volatile uint data = 0;
LOCK lock = {0};

void* thread( void *ud )
{
	uint i;
	struct timespec ts = {0};
	ts.tv_nsec = 1;
	(void)ud;

	while ( quit < CLOCKS_PER_SEC )
	{
		LockSiData( &lock );

		i = data++;
		if (i != 0)
		{
			flockfile( stdout );
			printf("var = %u, expected 0\n", i);
			funlockfile( stdout );
			FreeSiData( &lock );
			exit(1);
		}

		/* Why is needed for the test? */
		clock_nanosleep(CLOCK_MONOTONIC, 0, &ts, 0);

		i = data--;
		if (i != 1)
		{
			flockfile( stdout );
			printf("var = %u, expected 1\n", i);
			funlockfile( stdout );
			FreeSiData( &lock );
			exit(1);
		}

		++quit;

		FreeSiData( &lock );
	}
	return ud;
}

int main()
{
	pthread_t pt;
	int i;

	for (i = 0; i < 2; i++)
	{
		if ((errno = pthread_create(&pt, 0, thread, 0)) != 0 )
		{
			flockfile( stdout );
			printf("pthread_create failed: %m\n");
			funlockfile( stdout );
			return 1;
		}
	}
	pthread_exit(0);
}

Re: [musl] Suggestion for thread safety

[Rich Felker]

On Mon, Feb 28, 2022 at 02:43:36PM +0000, Lee Shallis wrote:
> Seems the wait just wasn't long enough, at about 4 yields onwards the
> results become consistent success, I've attached the file I did the
> experiments in, I even tried it under -O3 and no exits were
> encountered, so yes my method works, just needs a bit more wait time
> for extreme cases

We have a word for "synchronization" that is timing-dependent: broken.
This code does not work. If you want to keep promoting crank locking
theories, please find another forum for it, as this does not seem to
have anything to do with musl.

Rich

Re: [musl] Suggestion for thread safety

[Joakim Sindholt]

[-- Attachment #1: Type: text/plain, Size: 1881 bytes --]

On Mon, 28 Feb 2022 14:43:36 +0000, Lee Shallis <gb2985@gmail.com> wrote:
> Seems the wait just wasn't long enough, at about 4 yields onwards the
> results become consistent success, I've attached the file I did the
> experiments in, I even tried it under -O3 and no exits were
> encountered, so yes my method works, just needs a bit more wait time
> for extreme cases

Between the lines
> if ( !(shared->tid) )
and
> shared->tid = tid;
the kernel might suspend the running thread and allow the other to run,
or you might simply get unlucky and have the two threads do the checks
close enough to simultaneously that the memory hasn't been synchronized
yet. Either way you end up with both threads seeing that shared->tid is
zero and both of them writing their tids to it, and thus both enter the
critical section at the same time. And so the lock fails at the very
first hurdle: mutual exclusion. No amount of sleeping will make the bug
go away, only slightly more difficult to trigger.
The point of the clock_nanosleep call was to force a reschedule while
holding the lock. This also increases the runtime inside the lock which
in this case increases the likelihood that the thread trying to take the
lock will be waiting for it and end up racing with the thread that
currently has it when it unlocks and tries to relock it.

Now that you've inserted lots of sched_yield()s your lock is not only
still broken (in more ways than the one we've been trying to get you to
understand) but also extremely slow.

As a hint for your future education: the first (and far from only) thing
you'll need is compare-and-swap, aka. CAS.
You can read up on this class of bugs if you'd like. It's called "Time
Of Check to Time Of Use" or "TOCTOU" for short.

I didn't even need to poke at the code this time as the code you sent
breaks just the same on my machine.

I hope you'll learn from this.

[-- Attachment #2: not-on-my-machine.txt --]
[-- Type: text/plain, Size: 2849 bytes --]

zhasha@wirbelwind /home/zhasha ; cat lock.c
#define _GNU_SOURCE
#include <stdbool.h>
#include <unistd.h>
#include <errno.h>
#include <time.h>
#include <sched.h>
#include <pthread.h>
#include <stdlib.h>
#include <stdio.h>

typedef unsigned int uint;
typedef struct _LOCK { uint num; pid_t tid; } LOCK;

void LockSiData( LOCK *shared )
{
        pid_t tid = gettid();
        uint const yield = 4;
        uint i;
        /*
        struct timespec ts = {0};
        ts.tv_nsec = 1;
        */

        while ( shared->tid != tid )
        {
                if ( !(shared->tid) )
                        shared->tid = tid;
                //clock_nanosleep(CLOCK_MONOTONIC, 0, &ts, 0);
                for ( i = 0; i < yield; ++i )
                        sched_yield();
        }

        shared->num++;
}

void FreeSiData( LOCK *shared )
{
        pid_t tid = gettid();
        if ( shared->tid != tid )
                return;
        shared->num--;
        if ( shared->num )
                return;
        shared->tid = (pid_t)0;
        //sched_yield();
}

volatile uint quit = 0;
volatile uint data = 0;
LOCK lock = {0};

void* thread( void *ud )
{
        uint i;
        struct timespec ts = {0};
        ts.tv_nsec = 1;
        (void)ud;

        while ( quit < CLOCKS_PER_SEC )
        {
                LockSiData( &lock );

                i = data++;
                if (i != 0)
                {
                        flockfile( stdout );
                        printf("var = %u, expected 0\n", i);
                        funlockfile( stdout );
                        FreeSiData( &lock );
                        exit(1);
                }

                /* Why is needed for the test? */
                clock_nanosleep(CLOCK_MONOTONIC, 0, &ts, 0);

                i = data--;
                if (i != 1)
                {
                        flockfile( stdout );
                        printf("var = %u, expected 1\n", i);
                        funlockfile( stdout );
                        FreeSiData( &lock );
                        exit(1);
                }

                ++quit;

                FreeSiData( &lock );
        }
        return ud;
}

int main()
{
        pthread_t pt;
        int i;

        for (i = 0; i < 2; i++)
        {
                if ((errno = pthread_create(&pt, 0, thread, 0)) != 0 )
                {
                        flockfile( stdout );
                        printf("pthread_create failed: %m\n");
                        funlockfile( stdout );
                        return 1;
                }
        }
        pthread_exit(0);
}
zhasha@wirbelwind /home/zhasha ; gcc -lpthread -O3 lock.c
zhasha@wirbelwind /home/zhasha ; ./a.out
var = 1, expected 0
zhasha@wirbelwind /home/zhasha ; ./a.out
var = 1, expected 0
var = 2, expected 1
zhasha@wirbelwind /home/zhasha ;

Re: [musl] Suggestion for thread safety

[Lee Shallis]

On Mon, 28 Feb 2022 at 15:51, Joakim Sindholt <opensource@zhasha.com> wrote:
>
> On Mon, 28 Feb 2022 14:43:36 +0000, Lee Shallis <gb2985@gmail.com> wrote:
> > Seems the wait just wasn't long enough, at about 4 yields onwards the
> > results become consistent success, I've attached the file I did the
> > experiments in, I even tried it under -O3 and no exits were
> > encountered, so yes my method works, just needs a bit more wait time
> > for extreme cases
>
> Between the lines
> > if ( !(shared->tid) )
> and
> > shared->tid = tid;
> the kernel might suspend the running thread and allow the other to run,
> or you might simply get unlucky and have the two threads do the checks
> close enough to simultaneously that the memory hasn't been synchronized
> yet. Either way you end up with both threads seeing that shared->tid is
> zero and both of them writing their tids to it, and thus both enter the
That's the point of the loop, to check it's the same as what they
wrote, if it's not then it's either locked to another thread or empty,
the point in doing the yield after the write is to allow that failure
to occur, basically I'm using the race condition itself as the point
of success, rather than expect the CPU to perform an atomic lock that
could be just as broken as timing based locks, I already have my ideas
on how to fix the need for many yields to need only 2, I'm about to
try it now
> critical section at the same time. And so the lock fails at the very
> first hurdle: mutual exclusion. No amount of sleeping will make the bug
> go away, only slightly more difficult to trigger.

No it doesn't, think through the loop properly and you'll see that the
concept is the best one to go with, implementation just needs a little
work

> The point of the clock_nanosleep call was to force a reschedule while
> holding the lock. This also increases the runtime inside the lock which
> in this case increases the likelihood that the thread trying to take the
> lock will be waiting for it and end up racing with the thread that
> currently has it when it unlocks and tries to relock it.

How so? It still takes time for the jump condition to be evaluated and
the call to LockSiData to start, the other thread will already be in
the call loop ready to lock it, I designed this function specifically
around the idea that multiple threads could see an empty tid at the
same time, that's the reason for the yield call, so that all those
writes get in before the execution resumes.

> Now that you've inserted lots of sched_yield()s your lock is not only
> still broken (in more ways than the one we've been trying to get you to
> understand) but also extremely slow.
>
> As a hint for your future education: the first (and far from only) thing
> you'll need is compare-and-swap, aka. CAS.
> You can read up on this class of bugs if you'd like. It's called "Time
> Of Check to Time Of Use" or "TOCTOU" for short.
>
> I didn't even need to poke at the code this time as the code you sent
> breaks just the same on my machine.
>
> I hope you'll learn from this.

I hope you'll learn to think through the code before you speak out of
your ass, the concept is perfect, it's only that my implementation of
that concept isn't

Re: [musl] Suggestion for thread safety

[Lee Shallis]

[-- Attachment #1: Type: text/plain, Size: 3888 bytes --]

Welp, I think I finally managed to fix my implementation, wasn't quite
what I had in mind but it was the only method that seemed to work
without the bulky code pthread_mutex_lock falls to, it is however
slightly slower so I would treat it as a fallback for systems that
don't provide a mutex for now, the solution I ended up with utilises
kill( getpid(), SIGCONT ) & an additional member to identify which
thread managed to get their pid_t in at the time of the claim.

On Mon, 28 Feb 2022 at 16:07, Lee Shallis <gb2985@gmail.com> wrote:
>
> On Mon, 28 Feb 2022 at 15:51, Joakim Sindholt <opensource@zhasha.com> wrote:
> >
> > On Mon, 28 Feb 2022 14:43:36 +0000, Lee Shallis <gb2985@gmail.com> wrote:
> > > Seems the wait just wasn't long enough, at about 4 yields onwards the
> > > results become consistent success, I've attached the file I did the
> > > experiments in, I even tried it under -O3 and no exits were
> > > encountered, so yes my method works, just needs a bit more wait time
> > > for extreme cases
> >
> > Between the lines
> > > if ( !(shared->tid) )
> > and
> > > shared->tid = tid;
> > the kernel might suspend the running thread and allow the other to run,
> > or you might simply get unlucky and have the two threads do the checks
> > close enough to simultaneously that the memory hasn't been synchronized
> > yet. Either way you end up with both threads seeing that shared->tid is
> > zero and both of them writing their tids to it, and thus both enter the
> That's the point of the loop, to check it's the same as what they
> wrote, if it's not then it's either locked to another thread or empty,
> the point in doing the yield after the write is to allow that failure
> to occur, basically I'm using the race condition itself as the point
> of success, rather than expect the CPU to perform an atomic lock that
> could be just as broken as timing based locks, I already have my ideas
> on how to fix the need for many yields to need only 2, I'm about to
> try it now
> > critical section at the same time. And so the lock fails at the very
> > first hurdle: mutual exclusion. No amount of sleeping will make the bug
> > go away, only slightly more difficult to trigger.
>
> No it doesn't, think through the loop properly and you'll see that the
> concept is the best one to go with, implementation just needs a little
> work
>
> > The point of the clock_nanosleep call was to force a reschedule while
> > holding the lock. This also increases the runtime inside the lock which
> > in this case increases the likelihood that the thread trying to take the
> > lock will be waiting for it and end up racing with the thread that
> > currently has it when it unlocks and tries to relock it.
>
> How so? It still takes time for the jump condition to be evaluated and
> the call to LockSiData to start, the other thread will already be in
> the call loop ready to lock it, I designed this function specifically
> around the idea that multiple threads could see an empty tid at the
> same time, that's the reason for the yield call, so that all those
> writes get in before the execution resumes.
>
> > Now that you've inserted lots of sched_yield()s your lock is not only
> > still broken (in more ways than the one we've been trying to get you to
> > understand) but also extremely slow.
> >
> > As a hint for your future education: the first (and far from only) thing
> > you'll need is compare-and-swap, aka. CAS.
> > You can read up on this class of bugs if you'd like. It's called "Time
> > Of Check to Time Of Use" or "TOCTOU" for short.
> >
> > I didn't even need to poke at the code this time as the code you sent
> > breaks just the same on my machine.
> >
> > I hope you'll learn from this.
>
> I hope you'll learn to think through the code before you speak out of
> your ass, the concept is perfect, it's only that my implementation of
> that concept isn't

[-- Attachment #2: lock.c --]
[-- Type: text/x-csrc, Size: 4497 bytes --]

#define _GNU_SOURCE
#include <limits.h>
#include <stdbool.h>
#include <unistd.h>
#include <errno.h>
#include <linux/types.h>
#include <time.h>
#include <sys/resource.h>
#include <sched.h>
#include <setjmp.h>
#include <signal.h>
#include <pthread.h>
#include <string.h>
#include <stdlib.h>
#include <stdio.h>

//#define PRINT_LOCKS
//#define PRINT_ATTEMPTS
/* Seconds */
#define TIMED_TEST 0
/* Loops, not used if TIMED_TEST != 0 */
#define TRIES_TODO CLOCKS_PER_SEC

typedef unsigned int uint;
typedef unsigned long int ulong;
typedef struct _LOCK
{
	uint num;
	void *ud;
	struct timespec ts;
	volatile pid_t tid;
	volatile pid_t trying;
} LOCK;

volatile LOCK *_shared = NULL;

void lock_handler( int signal )
{
	/* We don't want the pointer we're working with to change midway
	 * through so we take a copy then work with that */
	volatile LOCK *shared = _shared;
	(void)signal;

	if ( !(shared->tid) )
		shared->tid = shared->trying;
#ifdef PRINT_ATTEMPTS
	flockfile( stdout );
	printf( "Thread %lu attempted lock\n", (ulong)(shared->trying) );
	funlockfile( stdout );
#endif
}

int LockSiData( LOCK *shared )
{
	int const sig = SIGCONT;
	pid_t tid = gettid(), was;
	struct sigaction this = {NULL}, prev = {NULL};

	/* Possible our signal handler will be called before _shared is not
	 * NULL so we set it prior to trying then continue on */
	_shared = shared;
	this.sa_handler = lock_handler;

	sigaction( sig, &this, &prev );

	for ( was = shared->tid; was != tid; was = shared->tid )
	{
		if ( !was )
		{
			shared->trying = tid;
			_shared = shared;
			kill( getpid(), sig );
		}
	}

	sigaction( sig, &prev, &this );

	clock_gettime( CLOCK_PROCESS_CPUTIME_ID, &(shared->ts) );
	shared->num++;

#ifdef PRINT_LOCKS
	flockfile( stdout );
	printf( "Thread %lu took lock\n", (ulong)tid );
	funlockfile( stdout );
#endif
	return 0;
}

int FreeSiData( LOCK *shared )
{
	pid_t tid = gettid();
	if ( shared->tid != tid )
		return 0;
	shared->num--;
	if ( shared->num )
		return 0;
#ifdef PRINT_LOCKS
	flockfile( stdout );
	printf( "Thread %lu released lock\n", (ulong)tid );
	funlockfile( stdout );
#endif
	shared->tid = (pid_t)0;
	return 0;
}

LOCK tlock = {0};
pthread_mutex_t mutex;

typedef int (*lock_cb)( void *ud );
typedef struct _TEST
{
	volatile uint quit;
	volatile uint data;
	void *ud;
	char *name;
	lock_cb lock;
	lock_cb free;
} TEST;

void* Abort( TEST *test, uint got, uint expected, clock_t start )
{
	ulong ticks = (ulong)(clock() - start);
	test->free( test->ud );
	flockfile( stdout );
	printf
	(
		"Thread %lu (lock%s) ended at %lu ticks, "
		"got = %u, expected %u\n",
		(ulong)gettid(), test->name, ticks, got, expected
	);
	funlockfile( stdout );
	exit(1);
	/* Prevents going further than expected */
	return test;
}

void* thread( void *ud )
{
	TEST *test = ud;
	uint got, expected;
	pid_t tid = gettid();
	clock_t start = clock(), end = start + (CLOCKS_PER_SEC * TIMED_TEST);
	struct timespec ts = {0};
	ts.tv_nsec = 1;
	(void)ud;

	flockfile( stdout );
	printf( "Thread %lu (lock%s)\n", (ulong)tid, test->name );
	funlockfile( stdout );

#if TIMED_TEST
	while ( end > clock() )
#else
	while ( test->quit < TRIES_TODO )
#endif
	{
		test->lock( test->ud );

		expected = 0;
		got = (test->data)++;
		if (got != expected)
			return Abort( test, got, expected, start );

		clock_nanosleep(CLOCK_MONOTONIC, 0, &ts, 0);

		expected = 1;
		got = (test->data)--;
		if (got != expected)
			return Abort( test, got, expected, start );

		test->quit++;
		test->free( test->ud );
	}

	end = clock();
	flockfile( stdout );
	printf
	(
		"lock%s (%lu) took %5lu clock ticks\n",
		test->name, (ulong)tid, (ulong)(end - start)
	);
	funlockfile( stdout );
	return ud;
}

int main()
{
	pthread_t pt;
	int i;

	TEST *test;
	TEST tests[2] = {{0}};

	setbuf(stdout,NULL);

	test = tests;
	test->ud = &tlock;
	test->name = "sidata";
	test->lock = (lock_cb)LockSiData;
	test->free = (lock_cb)FreeSiData;

	test = tests + 1;
	test->ud = &mutex;
	test->name = "mutex";
	test->lock = (lock_cb)pthread_mutex_lock;
	test->free = (lock_cb)pthread_mutex_unlock;

	for (i = 0; i < 2; i++)
	{
		if ((errno = pthread_create(&pt, 0, thread, tests)) != 0 )
		{
			flockfile( stdout );
			printf("pthread_create failed: %m\n");
			funlockfile( stdout );
			return 1;
		}

		if ((errno = pthread_create(&pt, 0, thread, tests + 1)) != 0 )
		{
			flockfile( stdout );
			printf("pthread_create failed: %m\n");
			funlockfile( stdout );
			return 1;
		}
	}

	pthread_exit(0);
	pthread_mutex_destroy( &mutex );
}

Re: [musl] Suggestion for thread safety

[Rich Felker]

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:
> [...]
> 
> 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
> [...]

Is there any context to this? Some problem you're setting out to solve
in relationship to musl? We already have synchronization, using
well-understood standard primitives wherever it's possible and
practical. Are you advocating for doing something different, or just
explaining something you made?

Rich