Thread support broken

Hello,

the new mutex code has in combination with the new condition code a bug.
I attach two examples a.c and b.c :slight_smile: one with sdl threads the other
with
pthreads (Linux)

/* Recursive mutexes aren’t widespread among pthread implementations yet
*/
#define PTHREAD_NO_RECURSIVE_MUTEX

struct SDL_mutex {
pthread_mutex_t id;
#ifdef PTHREAD_NO_RECURSIVE_MUTEX
int recursive;
pthread_t owner;
#endif
};

Description===========

The new Mutex code tries to add a counter to make recusive mutexes for
the same thread possible.

This does not work, when you combine this with a conditional wait.

See the examples its hard to explain the bug.

b.c works and never deadlocks but a.c deadlocks after a seconds.

Explain

Thread 1 has a mutex and goes to sleep (reader) before he sleep he
sends a signal.
when he sleeps he frees the mutex, but does not decrease. the counter
in the new mutex struct.
thread 2 catches the signal and gets the mut with it as well.
he frees the mut, but after the next lock the lock thread is different
and the lockcounter was not decreased.

FIX

Remove the recusive mutexes I think its not easy to do this right.
At least I have no simple fix for this. (You must wrap the xx_cond
calls
as well to work with the new mutex structure)
Maybe it works, if you always free the counter, regardsless of the
owner:

#ifdef PTHREAD_NO_RECURSIVE_MUTEX
/* We can only unlock the mutex if we own it */
if ( pthread_self() == mutex->owner ) {
if ( mutex->recursive ) {
–mutex->recursive;

If you are already editing in this region
could you then please add a simple “pthread_mutex_try_lock” call to SDL,
I really
need it :slight_smile:

Martin
-------------- next part --------------

#include <SDL/SDL_thread.h>
#include <SDL/SDL_mutex.h>
#include <stdio.h>

SDL_mutex* ringMut;
SDL_cond* spaceCond;
SDL_cond* dataCond;
SDL_Thread* readerThread;
int fillgrade;
int ringSize=64;

int reader(void* arg) {
while(1) {
SDL_LockMutex(ringMut);
while (fillgrade == 0) {
SDL_CondSignal(spaceCond);
printf(“reader waits for data\n”);
SDL_CondWait(dataCond,ringMut);
}
printf(“reader reads one\n”);
fillgrade–;
SDL_UnlockMutex(ringMut);
}
}

int writer() {
while(1) {
SDL_LockMutex(ringMut);
while (fillgrade == ringSize) {
SDL_CondSignal(dataCond);
printf(“writer waits for space\n”);
SDL_CondWait(spaceCond,ringMut);
}
fillgrade++;
printf(“writer fills ring\n”);
SDL_UnlockMutex(ringMut);
}
}

int main() {
fillgrade=0;
ringMut=SDL_CreateMutex();
spaceCond=SDL_CreateCond();
dataCond=SDL_CreateCond();

SDL_CreateThread(reader,NULL);
sleep(1);
writer();
}

-------------- next part --------------

#include <pthread.h>
#include <stdio.h>

pthread_mutex_t ringMut;
pthread_cond_t spaceCond;
pthread_cond_t dataCond;
pthread_t readerThread;

int fillgrade;
int ringSize=64;

void* reader(void* arg) {
while(1) {
pthread_mutex_lock(&ringMut);
while (fillgrade == 0) {
pthread_cond_signal(&spaceCond);
printf(“reader waits for data\n”);
pthread_cond_wait(&dataCond,&ringMut);
}
printf(“reader reads one\n”);
fillgrade–;
pthread_mutex_unlock(&ringMut);
}
}

int writer() {
while(1) {
pthread_mutex_lock(&ringMut);
while (fillgrade == ringSize) {
pthread_cond_signal(&dataCond);
printf(“writer waits for space\n”);
pthread_cond_wait(&spaceCond,&ringMut);
}
fillgrade++;
printf(“writer fills ring\n”);
pthread_mutex_unlock(&ringMut);
}
}

int main() {
fillgrade=0;
pthread_mutex_init(&ringMut,NULL);
pthread_cond_init(&spaceCond,NULL);
pthread_cond_init(&dataCond,NULL);

pthread_create(&readerThread,NULL,reader,NULL);
sleep(1);
writer();
}

In article <39171D02.2A28026D at rhrk.uni-kl.de>,
Martin Vogt writes:

Hello,

the new mutex code has in combination with the new condition code a bug.
I attach two examples a.c and b.c :slight_smile: one with sdl threads the other
with
pthreads (Linux)

/* Recursive mutexes aren’t widespread among pthread implementations yet
*/
#define PTHREAD_NO_RECURSIVE_MUTEX

struct SDL_mutex {
pthread_mutex_t id;
#ifdef PTHREAD_NO_RECURSIVE_MUTEX
int recursive;
pthread_t owner;
#endif
};

Description

The new Mutex code tries to add a counter to make recusive mutexes for
the same thread possible.

This does not work, when you combine this with a conditional wait.

See the examples its hard to explain the bug.

b.c works and never deadlocks but a.c deadlocks after a seconds.

I’ve looked at your programs, and it looks to me that in order to do a
fair comparison you should use POSIX recursive mutexes as well, and see
if the behavior still differs.

Just use the PTHREAD_RECURSIVE_MUTEX_INITIALIZE_NP initializer value to
initialize your mutex.–
Stephane Peter
Programmer
Loki Entertainment Software

“Microsoft has done to computers what McDonald’s has done to gastronomy”

Stephane Peter wrote:

I’ve looked at your programs, and it looks to me that in order to do a
fair comparison you should use POSIX recursive mutexes as well, and see
if the behavior still differs.

Just use the PTHREAD_RECURSIVE_MUTEX_INITIALIZE_NP initializer value to
initialize your mutex.

I attach you the code. It works with the recursive initializer
in pthreads.
(But that was not my point)
The SDL version (with its own implementation of recusive mutex) does not
decrease the counter when the mutex is
unlocked during a conditional wait.
I think its not that easy to implement rerusive mutexes,
but I could be wrong.

Ah, I still would like a try_lock(mutex) in SDL.

Martin

#include <pthread.h>
#include <stdio.h>

pthread_mutex_t ringMut;
pthread_cond_t spaceCond;
pthread_cond_t dataCond;
pthread_t readerThread;

int fillgrade;
int ringSize=64;

void* reader(void* arg) {
while(1) {
pthread_mutex_lock(&ringMut);
while (fillgrade == 0) {
pthread_cond_signal(&spaceCond);
printf(“reader waits for data\n”);
pthread_cond_wait(&dataCond,&ringMut);
} printf(“reader reads one\n”);
fillgrade–;
pthread_mutex_unlock(&ringMut);
}
}

int writer() {
while(1) {
pthread_mutex_lock(&ringMut);
while (fillgrade == ringSize) {
pthread_cond_signal(&dataCond);
printf(“writer waits for space\n”);
pthread_cond_wait(&spaceCond,&ringMut);
}
fillgrade++;
printf(“writer fills ring\n”);
pthread_mutex_unlock(&ringMut);
}
}
int main() {
pthread_mutexattr_t attr;
pthread_mutexattr_init(&attr);
pthread_mutexattr_setkind_np(&attr,PTHREAD_MUTEX_RECURSIVE_NP);
fillgrade=0;
pthread_mutex_init(&ringMut,&attr);
pthread_cond_init(&spaceCond,NULL);
pthread_cond_init(&dataCond,NULL);

pthread_create(&readerThread,NULL,reader,NULL);
sleep(1);
writer();
}

Martin Vogt wrote:

Stephane Peter wrote:

I’ve looked at your programs, and it looks to me that in order to do a
fair comparison you should use POSIX recursive mutexes as well, and see
if the behavior still differs.

Just use the PTHREAD_RECURSIVE_MUTEX_INITIALIZE_NP initializer value to
initialize your mutex.

I attach you the code. It works with the recursive initializer
in pthreads.
Ah, sorry.

With work I meant: the pthread code works in the same way
as in the other pthread example,
which means, they work different to the SDL thread or short: it is still
a bug.

And I would like to have a try_lock in SDL.

regards,

Martin> (But that was not my point)

The SDL version (with its own implementation of recusive mutex) does not
decrease the counter when the mutex is
unlocked during a conditional wait.
I think its not that easy to implement rerusive mutexes,
but I could be wrong.

Ah, I still would like a try_lock(mutex) in SDL.

Martin

#include <pthread.h>
#include <stdio.h>

pthread_mutex_t ringMut;
pthread_cond_t spaceCond;
pthread_cond_t dataCond;
pthread_t readerThread;

int fillgrade;
int ringSize=64;

void* reader(void* arg) {
while(1) {
pthread_mutex_lock(&ringMut);
while (fillgrade == 0) {
pthread_cond_signal(&spaceCond);
printf(“reader waits for data\n”);
pthread_cond_wait(&dataCond,&ringMut);
} printf(“reader reads one\n”);
fillgrade–;
pthread_mutex_unlock(&ringMut);
}
}

int writer() {
while(1) {
pthread_mutex_lock(&ringMut);
while (fillgrade == ringSize) {
pthread_cond_signal(&dataCond);
printf(“writer waits for space\n”);
pthread_cond_wait(&spaceCond,&ringMut);
}
fillgrade++;
printf(“writer fills ring\n”);
pthread_mutex_unlock(&ringMut);
}
}
int main() {
pthread_mutexattr_t attr;
pthread_mutexattr_init(&attr);
pthread_mutexattr_setkind_np(&attr,PTHREAD_MUTEX_RECURSIVE_NP);
fillgrade=0;
pthread_mutex_init(&ringMut,&attr);
pthread_cond_init(&spaceCond,NULL);
pthread_cond_init(&dataCond,NULL);

pthread_create(&readerThread,NULL,reader,NULL);
sleep(1);
writer();
}

In article <3917D020.AF2993A5 at rhrk.uni-kl.de>,
Martin Vogt writes:

Martin Vogt wrote:

Stephane Peter wrote:

I’ve looked at your programs, and it looks to me that in order to do a
fair comparison you should use POSIX recursive mutexes as well, and see
if the behavior still differs.

Just use the PTHREAD_RECURSIVE_MUTEX_INITIALIZE_NP initializer value to
initialize your mutex.

I attach you the code. It works with the recursive initializer
in pthreads.
Ah, sorry.

With work I meant: the pthread code works in the same way
as in the other pthread example,
which means, they work different to the SDL thread or short: it is still
a bug.

Hum, you’re probably right. We’ve been lacking serious test programs so
far.

And I would like to have a try_lock in SDL.

That would make sense to have it in the API, if only for completeness.
Unless Sam objects to it ;-)–
Stephane Peter
Programmer
Loki Entertainment Software

“Microsoft has done to computers what McDonald’s has done to gastronomy”

And I would like to have a try_lock in SDL.

That would make sense to have it in the API, if only for completeness.
Unless Sam objects to it :wink:

Not at all. Can it be implemented on the various systems?

See ya,
-Sam Lantinga, Lead Programmer, Loki Entertainment Software