svn commit: branches/uClibc-nptl: libc/sysdeps/linux/common libpthread/lin etc...
Denys Vlasenko
vda.linux at googlemail.com
Thu Mar 12 20:57:58 UTC 2009
On Tuesday 10 March 2009 03:53:01 pm Will Newton wrote:
> On Tue, Mar 10, 2009 at 12:23 PM, Denys Vlasenko
> <vda.linux at googlemail.com> wrote:
> > On Thursday 26 February 2009 05:27:50 pm Will Newton wrote:
> >> On Thu, Feb 26, 2009 at 4:06 PM, Denys Vlasenko
> >> <vda.linux at googlemail.com> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 4.patch
Type: text/x-diff
Size: 10374 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/uclibc/attachments/20090312/ac4cb574/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 5.patch
Type: text/x-diff
Size: 2777 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/uclibc/attachments/20090312/ac4cb574/attachment-0001.bin>
More information about the uClibc
mailing list