Re: [libc-alpha] linuxthreads bug in 2.2.4 under ppc linux

[Franz Sirl <Franz dot Sirl-kernel at lauterbach dot com>]

At 16:43 09.12.2001, Kevin B. Hendricks wrote:
>Hi,
>
>I think this is optimization related (but I am still not sure).
>
>Simply adding the one line:
>      if (p_node == (struct wait_node *) 0) MSG("huh %p\n",&lock);
>
>in spinlock.c in __pthread_alt_unlock() as shown below is enough to make
>things work (and this if is never taken).

Yeah, because you take the address of a parameter, which forces it on the 
stack, leading to quite different code layout. Best call a single argument 
function here or directly abort().



>     READ_MEMORY_BARRIER(); /* Prevent access to stale data through p_node
>*/
>
>     while (p_node != (struct wait_node *) 1) {
>       int prio;
>
>       if (p_node == (struct wait_node *) 0) MSG("huh %p\n",&lock);
>
>
>The question here is why does accessing p_node in this way make such a big
>difference to the compiler.

Hmm, actually p_node == lock->__status == *lock and lock == pp_head, with 
O3 inlining stuff the value is actually held in a 2 or 3 registers at once 
and doesn't change it's value (it isn't even reloaded) over the whole function.

Kevin, can you try to replace alt_unlock with the manually inlined version 
below and recompile glibc at -O2? If the problem remains, there must be a 
race somewhere in there I guess (the produced code is nearly 100% identical 
to the -O3 compiler inlined version). I wonder though why this code uses 
CAS at all, wouldn't it be better to have a thread safe list insertion code 
in pt-machine.h, like outlined in the PPC manuals?

Franz.

void __pthread_alt_unlock(struct _pthread_fastlock *lock)
{
   struct wait_node *p_node, **pp_node, *p_max_prio, **pp_max_prio;
   struct wait_node ** const pp_head = (struct wait_node **) &lock->__status;
   int maxprio;

   __asm__ __volatile__ ("sync" : : : "memory");
   while (1)
     {
         {
           long oldstatus = lock->__status;
           if (oldstatus == 0 || oldstatus == 1)
             {
               if (__compare_and_swap_with_release_semantics 
(&lock->__status, oldstatus, 0))
                 break;
               else
                 continue;
             }
         }

       pp_max_prio = pp_node = pp_head;
       p_max_prio = p_node = *pp_head;
       maxprio = (-2147483647 -1);

       __asm__ __volatile__ ("sync" : : : "memory");

       while (p_node != (struct wait_node *) 1)
         {
           int prio;

           if (p_node == (struct wait_node *) 0) abort (&lock->__status);

           if (p_node->abandoned)
             {
                 {
                   struct wait_node **pp_headi = pp_head;
                   struct wait_node **pp_nodei = pp_node;
                   struct wait_node *p_nodei = p_node;
                   if (pp_nodei == pp_headi)
                     {
                       long oldvalue = (long) p_nodei;
                       long newvalue = (long) p_nodei->next;

                       if (__compare_and_swap((long *) pp_nodei, oldvalue, 
newvalue))
                         goto exit1;

                       for (pp_nodei = pp_headi; p_nodei != *pp_nodei; )
                         {
                           pp_nodei = &(*pp_nodei)->next;
                           __asm__ __volatile__ ("sync" : : : "memory");
                         }
                     }
                   *pp_nodei = p_nodei->next;
                 }
exit1:
                 {
                   struct wait_node *wn = p_node;
                   long oldvalue, newvalue;
                   do
                     {
                       oldvalue = wait_node_free_list;
                       wn->next = (struct wait_node *) oldvalue;
                       newvalue = (long) wn;

                       __asm__ __volatile__ ("sync" : : : "memory");
                     } while (! __compare_and_swap(&wait_node_free_list, 
oldvalue, newvalue));
                 }

               p_node = *pp_node;
               if (pp_node == pp_head)
                 __asm__ __volatile__ ("sync" : : : "memory");
               continue;
             }
           else if ((prio = p_node->thr->p_priority) >= maxprio)
             {
               maxprio = prio;
               pp_max_prio = pp_node;
               p_max_prio = p_node;
             }

           pp_node = &p_node->next;
           p_node = *pp_node;
         }

       if (maxprio == (-2147483647 -1))
         continue;

       if (!!__compare_and_swap((long int *) &p_max_prio->abandoned, 0, 1))
         {
             {
               struct wait_node **pp_headi = pp_head;
               struct wait_node **pp_nodei = pp_max_prio;
               struct wait_node *p_nodei = p_max_prio;
               if (pp_nodei == pp_headi)
                 {
                   long oldvalue = (long) p_nodei;
                   long newvalue = (long) p_nodei->next;

                   if (__compare_and_swap((long *) pp_nodei, oldvalue, 
newvalue))
                     goto exit2;

                   for (pp_nodei = pp_headi; p_nodei != *pp_nodei; )
                     {
                       pp_nodei = &(*pp_nodei)->next;
                       __asm__ __volatile__ ("sync" : : : "memory");
                     }
                 }
               *pp_nodei = p_nodei->next;
             }
exit2:

           restart(p_max_prio->thr);
           break;
         }
     }
}