On Tuesday 10 March 2009 03:53:01 pm Will Newton wrote:
> On Tue, Mar 10, 2009 at 12:23 PM, Denys Vlasenko
> <[email protected]> wrote:
> > On Thursday 26 February 2009 05:27:50 pm Will Newton wrote:
> >> On Thu, Feb 26, 2009 at 4:06 PM, Denys Vlasenko
> >> <[email protected]> wrote:
> >>
> >> >> Also,
> >> >> what mechanism does linuxthreads_new use to make sure it's using the
> >> >> functions in libc, rather than something overridden by the application?
> >> >
> >> > Have no idea. There is no evidence linuxthreads_new works for
> >> > any architecture. And no, I'm not going to check 12 architectures
> >> > just because you want to know it for sure. That had to be done by
> >> > the submitter of that code, at the time of submission.
> >> > Obviously, [s]he did not even test it on the most easily available
> >> > arch, x86.
> >>
> >> The new linuxthreads does work and is at this moment shipping in
> >> products. I posted a patch some time ago to update it to the latest
> >> version of linuxthreads upstream (and fix a serious bug in the
> >> process)
> >
> > Which part of the patch fixes the bug? What the bug is?
>
> The bug is this one:
>
> http://sourceware.org/ml/libc-ports/2005-10/msg00025.html
>
> >> but it does not seem to have been applied. I've attached it
> >> again if anyone's interested.
> >
> > Applying it as is without understanding what I am fixing
> > would be a bit dangerous. Need your comments.
> >
> > --- metag-uClibc2/libpthread/linuxthreads/descr.h 5 Feb 2008 14:49:33
> > -0000 1.2
> > +++ metag-uClibc2/libpthread/linuxthreads/descr.h 23 Apr 2008
> > 13:27:00 -0000
> > @@ -123,9 +123,9 @@
> > union dtv *dtvp;
> > pthread_descr self; /* Pointer to this structure */
> > int multiple_threads;
> > -# ifdef NEED_DL_SYSINFO
> > uintptr_t sysinfo;
> > -# endif
> > + uintptr_t stack_guard;
> > + uintptr_t pointer_guard;
> > ...
> > + /* Copy the stack guard canary. */
> > +#ifdef THREAD_COPY_STACK_GUARD
> > + THREAD_COPY_STACK_GUARD (new_thread);
> > +#endif
> > +
> > + /* Copy the pointer guard value. */
> > +#ifdef THREAD_COPY_POINTER_GUARD
> > + THREAD_COPY_POINTER_GUARD (new_thread);
> > +#endif
> >
> > these members appear to be only written to.
> > Why do you need write-only members?
> > THREAD_COPY_STACK_GUARD and THREAD_COPY_POINTER_GUARD
> > are not defined anywhere.
>
> They are defined in the TLS code from glibc. They do not have any
> particular effect if they are not defined but they reduce the diff
> from upstream LinuxThreads.
I take it means that we can omit them and have smaller code.
> > pid = __clone2(__pthread_manager_event,
> > (void **) __pthread_manager_thread_bos,
> > THREAD_MANAGER_STACK_SIZE,
> > - CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND,
> > + CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND
> > | CLONE_SYSVSEM,
> > mgr);
> >
> > What does it fix? (Imagine that know almost nothing about threads).
>
> Ok. This allows pthreads to share Sys V semaphores in order to get
> appropriate SEM_UNDO semantics.
Ok.
> > + /* Make sure we come back here after suspend(), in case we entered
> > + from a signal handler. */
> > + THREAD_SETMEM(self, p_signal_jmp, NULL);
> > +
> >
> > what does this fix?
>
> http://sources.redhat.com/ml/libc-ports/2006-05/msg00000.html
> > - If no threads have been created yet, clear it just in the
> > - current thread. */
> > + If no threads have been created yet, or if we are exiting, clear
> > + it just in the current thread. */
> >
> > struct pthread_key_delete_helper_args args;
> > args.idx1st = key / PTHREAD_KEY_2NDLEVEL_SIZE;
> > args.idx2nd = key % PTHREAD_KEY_2NDLEVEL_SIZE;
> > - if (__pthread_manager_request != -1)
> > + if (__pthread_manager_request != -1
> > + && !(__builtin_expect (__pthread_exit_requested, 0)))
> >
> > what does this fix? (I'd also remove __builtin_expect, it uglifies code
> > much more than it helps wrt performance).
>
> It's trade off with reducing the diff from upstream LinuxThreads. It's
> easy to break code cleaning it up and e.g. losing a !.
So what does it fix?
> > __pthread_lock(THREAD_GETMEM(self, p_lock), self);
> > for (i = 0; i < PTHREAD_KEY_1STLEVEL_SIZE; i++) {
> > if (THREAD_GETMEM_NC(self, p_specific[i]) != NULL) {
> > - free(THREAD_GETMEM_NC(self, p_specific[i]));
> > + void *p = THREAD_GETMEM_NC(self, p_specific[i]);
> > THREAD_SETMEM_NC(self, p_specific[i], NULL);
> > + free(p);
> > }
> > }
> >
> > what does this fix?
>
> It looks like a possible race of some kind. I am not aware of the details.
Ok.
> > #if defined HAS_COMPARE_AND_SWAP
> > wait_node_dequeue(pp_head, pp_max_prio, p_max_prio);
> > #endif
> > +
> > + /* Release the spinlock before restarting. */
> > +#if defined TEST_FOR_COMPARE_AND_SWAP
> > + if (!__pthread_has_cas)
> > +#endif
> > +#if !defined HAS_COMPARE_AND_SWAP || defined TEST_FOR_COMPARE_AND_SWAP
> > + {
> > + __pthread_release(&lock->__spinlock);
> > + }
> > +#endif
> > +
> > restart(p_max_prio->thr);
> > - break;
> > +
> > + return;
> > }
> > }
> >
> > what does this fix?
>
> The spinlock unlock issue. That was the main issue that I needed to
> fix with this patch set, most of the rest is just synching up with
> LinuxThreads HEAD so we can more easily compare and merge in the
> future.
I spend 20 minutes to understand that the reason here is:
Release the spinlock *before* restarting
with emphasis on "before", because the code already does that
via "break" statement - it falls out of the while() loop
and does __pthread_release(&lock->__spinlock). But it happens
after restart(p_max_prio->thr).
What bad things currently happen if we do it other way around?
This fine code:
#if defined TEST_FOR_COMPARE_AND_SWAP
if (!__pthread_has_cas)
#endif
#if !defined HAS_COMPARE_AND_SWAP || defined TEST_FOR_COMPARE_AND_SWAP
*pp_max_prio = p_max_prio->next;
#endif
#if defined TEST_FOR_COMPARE_AND_SWAP
else
#endif
#if defined HAS_COMPARE_AND_SWAP
wait_node_dequeue(pp_head, pp_max_prio, p_max_prio);
#endif
which is abundant in spinlock.c, IIUC can be rewritten as:
#if !defined TEST_FOR_COMPARE_AND_SWAP
# ifdef HAS_COMPARE_AND_SWAP
# define __pthread_has_cas 1
# else
# define __pthread_has_cas 0
# endif
#endif
if (!__pthread_has_cas)
*pp_max_prio = p_max_prio->next;
else
wait_node_dequeue(pp_head, pp_max_prio, p_max_prio);
This is "a bit" more readable. Oh well. The whole threading lib looks grim.
See 4.patch which I attached to svn, and 5.patch which contains bits I
did not apply (as they seem to be useless).
Let me know if there is some problem.
--
vda
diff -d -urpN uClibc.3/libpthread/linuxthreads/descr.h uClibc.4/libpthread/linuxthreads/descr.h
--- uClibc.3/libpthread/linuxthreads/descr.h 2009-03-05 20:44:10.000000000 +0100
+++ uClibc.4/libpthread/linuxthreads/descr.h 2009-03-12 21:39:06.000000000 +0100
@@ -123,9 +123,7 @@ struct _pthread_descr_struct
union dtv *dtvp;
pthread_descr self; /* Pointer to this structure */
int multiple_threads;
-# ifdef NEED_DL_SYSINFO
uintptr_t sysinfo;
-# endif
} data;
void *__padding[16];
} p_header;
diff -d -urpN uClibc.3/libpthread/linuxthreads/manager.c uClibc.4/libpthread/linuxthreads/manager.c
--- uClibc.3/libpthread/linuxthreads/manager.c 2009-03-05 20:44:10.000000000 +0100
+++ uClibc.4/libpthread/linuxthreads/manager.c 2009-03-12 21:36:20.000000000 +0100
@@ -742,15 +742,15 @@ static int pthread_handle_create(pthread
pid = __clone2(pthread_start_thread_event,
(void **)new_thread_bottom,
(char *)stack_addr - new_thread_bottom,
- CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND |
+ CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_SYSVSEM |
__pthread_sig_cancel, new_thread);
#elif _STACK_GROWS_UP
pid = __clone(pthread_start_thread_event, (void *) new_thread_bottom,
- CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND |
+ CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_SYSVSEM |
__pthread_sig_cancel, new_thread);
#else
pid = __clone(pthread_start_thread_event, stack_addr,
- CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND |
+ CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_SYSVSEM |
__pthread_sig_cancel, new_thread);
#endif
saved_errno = errno;
@@ -783,15 +783,15 @@ static int pthread_handle_create(pthread
pid = __clone2(pthread_start_thread,
(void **)new_thread_bottom,
(char *)stack_addr - new_thread_bottom,
- CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND |
+ CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_SYSVSEM |
__pthread_sig_cancel, new_thread);
#elif _STACK_GROWS_UP
pid = __clone(pthread_start_thread, (void *) new_thread_bottom,
- CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND |
+ CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_SYSVSEM |
__pthread_sig_cancel, new_thread);
#else
pid = __clone(pthread_start_thread, stack_addr,
- CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND |
+ CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_SYSVSEM |
__pthread_sig_cancel, new_thread);
#endif /* !NEED_SEPARATE_REGISTER_STACK */
saved_errno = errno;
@@ -892,10 +892,11 @@ static void pthread_free(pthread_descr t
#ifdef _STACK_GROWS_UP
# ifdef USE_TLS
size_t stacksize = guardaddr - th->p_stackaddr;
+ guardaddr = th->p_stackaddr;
# else
size_t stacksize = guardaddr - (char *)th;
-# endif
guardaddr = (char *)th;
+# endif
#else
/* Guardaddr is always set, even if guardsize is 0. This allows
us to compute everything else. */
diff -d -urpN uClibc.3/libpthread/linuxthreads/pthread.c uClibc.4/libpthread/linuxthreads/pthread.c
--- uClibc.3/libpthread/linuxthreads/pthread.c 2009-03-05 20:44:10.000000000 +0100
+++ uClibc.4/libpthread/linuxthreads/pthread.c 2009-03-12 21:36:20.000000000 +0100
@@ -740,17 +740,17 @@ int __pthread_initialize_manager(void)
pid = __clone2(__pthread_manager_event,
(void **) __pthread_manager_thread_bos,
THREAD_MANAGER_STACK_SIZE,
- CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND,
+ CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_SYSVSEM,
mgr);
#elif _STACK_GROWS_UP
pid = __clone(__pthread_manager_event,
(void **) __pthread_manager_thread_bos,
- CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND,
+ CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_SYSVSEM,
mgr);
#else
pid = __clone(__pthread_manager_event,
(void **) __pthread_manager_thread_tos,
- CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND,
+ CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_SYSVSEM,
mgr);
#endif
@@ -780,13 +780,13 @@ int __pthread_initialize_manager(void)
#ifdef NEED_SEPARATE_REGISTER_STACK
pid = __clone2(__pthread_manager, (void **) __pthread_manager_thread_bos,
THREAD_MANAGER_STACK_SIZE,
- CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND, mgr);
+ CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_SYSVSEM, mgr);
#elif _STACK_GROWS_UP
pid = __clone(__pthread_manager, (void **) __pthread_manager_thread_bos,
- CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND, mgr);
+ CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_SYSVSEM, mgr);
#else
pid = __clone(__pthread_manager, (void **) __pthread_manager_thread_tos,
- CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND, mgr);
+ CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_SYSVSEM, mgr);
#endif
}
if (__builtin_expect (pid, 0) == -1) {
@@ -972,6 +972,10 @@ static void pthread_onexit_process(int r
struct pthread_request request;
pthread_descr self = thread_self();
+ /* Make sure we come back here after suspend(), in case we entered
+ from a signal handler. */
+ THREAD_SETMEM(self, p_signal_jmp, NULL);
+
request.req_thread = self;
request.req_kind = REQ_PROCESS_EXIT;
request.req_args.exit.code = retcode;
@@ -1201,13 +1205,13 @@ void __pthread_wait_for_restart_signal(p
void __pthread_restart_old(pthread_descr th)
{
- if (atomic_increment(&th->p_resume_count) == -1)
+ if (pthread_atomic_increment(&th->p_resume_count) == -1)
kill(th->p_pid, __pthread_sig_restart);
}
void __pthread_suspend_old(pthread_descr self)
{
- if (atomic_decrement(&self->p_resume_count) <= 0)
+ if (pthread_atomic_decrement(&self->p_resume_count) <= 0)
__pthread_wait_for_restart_signal(self);
}
@@ -1218,7 +1222,7 @@ __pthread_timedsuspend_old(pthread_descr
int was_signalled = 0;
sigjmp_buf jmpbuf;
- if (atomic_decrement(&self->p_resume_count) == 0) {
+ if (pthread_atomic_decrement(&self->p_resume_count) == 0) {
/* Set up a longjmp handler for the restart signal, unblock
the signal and sleep. */
@@ -1275,9 +1279,9 @@ __pthread_timedsuspend_old(pthread_descr
being delivered. */
if (!was_signalled) {
- if (atomic_increment(&self->p_resume_count) != -1) {
+ if (pthread_atomic_increment(&self->p_resume_count) != -1) {
__pthread_wait_for_restart_signal(self);
- atomic_decrement(&self->p_resume_count); /* should be zero now! */
+ pthread_atomic_decrement(&self->p_resume_count); /* should be zero now! */
/* woke spontaneously and consumed restart signal */
return 1;
}
diff -d -urpN uClibc.3/libpthread/linuxthreads/specific.c uClibc.4/libpthread/linuxthreads/specific.c
--- uClibc.3/libpthread/linuxthreads/specific.c 2009-03-05 20:44:10.000000000 +0100
+++ uClibc.4/libpthread/linuxthreads/specific.c 2009-03-12 21:36:20.000000000 +0100
@@ -104,13 +104,14 @@ int pthread_key_delete(pthread_key_t key
that if the key is reallocated later by pthread_key_create, its
associated values will be NULL in all threads.
- If no threads have been created yet, clear it just in the
- current thread. */
+ If no threads have been created yet, or if we are exiting, clear
+ it just in the current thread. */
struct pthread_key_delete_helper_args args;
args.idx1st = key / PTHREAD_KEY_2NDLEVEL_SIZE;
args.idx2nd = key % PTHREAD_KEY_2NDLEVEL_SIZE;
- if (__pthread_manager_request != -1)
+ if (__pthread_manager_request != -1
+ && !(__builtin_expect (__pthread_exit_requested, 0)))
{
struct pthread_request request;
@@ -203,8 +204,9 @@ void __pthread_destroy_specifics()
__pthread_lock(THREAD_GETMEM(self, p_lock), self);
for (i = 0; i < PTHREAD_KEY_1STLEVEL_SIZE; i++) {
if (THREAD_GETMEM_NC(self, p_specific[i]) != NULL) {
- free(THREAD_GETMEM_NC(self, p_specific[i]));
+ void *p = THREAD_GETMEM_NC(self, p_specific[i]);
THREAD_SETMEM_NC(self, p_specific[i], NULL);
+ free(p);
}
}
__pthread_unlock(THREAD_GETMEM(self, p_lock));
diff -d -urpN uClibc.3/libpthread/linuxthreads/spinlock.c uClibc.4/libpthread/linuxthreads/spinlock.c
--- uClibc.3/libpthread/linuxthreads/spinlock.c 2009-03-05 20:44:10.000000000 +0100
+++ uClibc.4/libpthread/linuxthreads/spinlock.c 2009-03-12 21:36:20.000000000 +0100
@@ -637,8 +637,20 @@ void __pthread_alt_unlock(struct _pthrea
#if defined HAS_COMPARE_AND_SWAP
wait_node_dequeue(pp_head, pp_max_prio, p_max_prio);
#endif
+
+ /* Release the spinlock *before* restarting. */
+#if defined TEST_FOR_COMPARE_AND_SWAP
+ if (!__pthread_has_cas)
+#endif
+#if !defined HAS_COMPARE_AND_SWAP || defined TEST_FOR_COMPARE_AND_SWAP
+ {
+ __pthread_release(&lock->__spinlock);
+ }
+#endif
+
restart(p_max_prio->thr);
- break;
+
+ return;
}
}
diff -d -urpN uClibc.3/libpthread/linuxthreads/spinlock.h uClibc.4/libpthread/linuxthreads/spinlock.h
--- uClibc.3/libpthread/linuxthreads/spinlock.h 2009-03-05 20:44:10.000000000 +0100
+++ uClibc.4/libpthread/linuxthreads/spinlock.h 2009-03-12 21:36:20.000000000 +0100
@@ -172,7 +172,8 @@ static __inline__ int __pthread_alt_tryl
/* Operations on pthread_atomic, which is defined in internals.h */
-static __inline__ long atomic_increment(struct pthread_atomic *pa)
+static __inline__ long
+pthread_atomic_increment (struct pthread_atomic *pa)
{
long oldval;
@@ -184,7 +185,8 @@ static __inline__ long atomic_increment(
}
-static __inline__ long atomic_decrement(struct pthread_atomic *pa)
+static __inline__ long
+pthread_atomic_decrement (struct pthread_atomic *pa)
{
long oldval;
diff -d -urpN uClibc.3/libpthread/linuxthreads/sysdeps/i386/tls.h uClibc.4/libpthread/linuxthreads/sysdeps/i386/tls.h
--- uClibc.3/libpthread/linuxthreads/sysdeps/i386/tls.h 2009-03-05 20:44:08.000000000 +0100
+++ uClibc.4/libpthread/linuxthreads/sysdeps/i386/tls.h 2009-03-12 21:39:11.000000000 +0100
@@ -46,9 +46,7 @@ typedef struct
dtv_t *dtv;
void *self; /* Pointer to the thread descriptor. */
int multiple_threads;
-#ifdef NEED_DL_SYSINFO
uintptr_t sysinfo;
-#endif
} tcbhead_t;
#else /* __ASSEMBLER__ */
diff -d -urpN uClibc.4/libpthread/linuxthreads/descr.h uClibc.5/libpthread/linuxthreads/descr.h
--- uClibc.4/libpthread/linuxthreads/descr.h 2009-03-12 21:39:06.000000000 +0100
+++ uClibc.5/libpthread/linuxthreads/descr.h 2009-03-12 21:06:19.000000000 +0100
@@ -124,6 +124,8 @@ struct _pthread_descr_struct
pthread_descr self; /* Pointer to this structure */
int multiple_threads;
uintptr_t sysinfo;
+ uintptr_t stack_guard;
+ uintptr_t pointer_guard;
} data;
void *__padding[16];
} p_header;
@@ -191,6 +193,13 @@ struct _pthread_descr_struct
size_t p_alloca_cutoff; /* Maximum size which should be allocated
using alloca() instead of malloc(). */
/* New elements must be added at the end. */
+
+ /* This member must be last. */
+ char end_padding[];
+
+#define PTHREAD_STRUCT_END_PADDING \
+ (sizeof (struct _pthread_descr_struct) \
+ - offsetof (struct _pthread_descr_struct, end_padding))
} __attribute__ ((aligned(32))); /* We need to align the structure so that
doubles are aligned properly. This is 8
bytes on MIPS and 16 bytes on MIPS64.
diff -d -urpN uClibc.4/libpthread/linuxthreads/manager.c uClibc.5/libpthread/linuxthreads/manager.c
--- uClibc.4/libpthread/linuxthreads/manager.c 2009-03-12 21:36:20.000000000 +0100
+++ uClibc.5/libpthread/linuxthreads/manager.c 2009-03-12 21:06:19.000000000 +0100
@@ -679,6 +679,17 @@ static int pthread_handle_create(pthread
new_thread->p_inheritsched = attr ? attr->__inheritsched : 0;
new_thread->p_alloca_cutoff = stksize / 4 > __MAX_ALLOCA_CUTOFF
? __MAX_ALLOCA_CUTOFF : stksize / 4;
+
+ /* Copy the stack guard canary. */
+#ifdef THREAD_COPY_STACK_GUARD
+ THREAD_COPY_STACK_GUARD (new_thread);
+#endif
+
+ /* Copy the pointer guard value. */
+#ifdef THREAD_COPY_POINTER_GUARD
+ THREAD_COPY_POINTER_GUARD (new_thread);
+#endif
+
/* Initialize the thread handle */
__pthread_init_lock(&__pthread_handles[sseg].h_lock);
__pthread_handles[sseg].h_descr = new_thread;
diff -d -urpN uClibc.4/libpthread/linuxthreads/pthread.c uClibc.5/libpthread/linuxthreads/pthread.c
--- uClibc.4/libpthread/linuxthreads/pthread.c 2009-03-12 21:36:20.000000000 +0100
+++ uClibc.5/libpthread/linuxthreads/pthread.c 2009-03-12 21:06:19.000000000 +0100
@@ -700,6 +700,16 @@ int __pthread_initialize_manager(void)
mgr = &__pthread_manager_thread;
#endif
+ /* Copy the stack guard canary. */
+#ifdef THREAD_COPY_STACK_GUARD
+ THREAD_COPY_STACK_GUARD (mgr);
+#endif
+
+ /* Copy the pointer guard value. */
+#ifdef THREAD_COPY_POINTER_GUARD
+ THREAD_COPY_POINTER_GUARD (mgr);
+#endif
+
__pthread_manager_request = manager_pipe[1]; /* writing end */
__pthread_manager_reader = manager_pipe[0]; /* reading end */
_______________________________________________
uClibc mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/uclibc
