Re: [drow@false.org: Re: DB_THREAD support in Berkeley DB/glibc]

[Geoff Keating <geoffk at cygnus dot com>]

> Mailing-List: contact libc-alpha-help@sourceware.cygnus.com; run by ezmlm
> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-geoffk=cygnus.com@sourceware.cygnus.com>
> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.cygnus.com>
> List-Archive: <http://sourceware.cygnus.com/ml/libc-alpha/>
> List-Post: <mailto:libc-alpha@sourceware.cygnus.com>
> List-Help: <mailto:libc-alpha-help@sourceware.cygnus.com>, <http://sourceware.cygnus.com/ml/#faqs>
> Date: Tue, 28 Dec 1999 17:17:08 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Mail-Followup-To: libc-alpha@sourceware.cygnus.com
> User-Agent: Mutt/1.0i
> 
> ----- Forwarded message from Daniel Jacobowitz <drow@false.org> -----
> 
> Date: Tue, 28 Dec 1999 17:05:39 -0500
> From: Daniel Jacobowitz <drow@false.org>
> To: linuxppc-dev@lists.linuxppc.org, libc-alpha@cygnus.com
> Subject: Re: DB_THREAD support in Berkeley DB/glibc
> Mail-Followup-To: linuxppc-dev@lists.linuxppc.org, libc-alpha@cygnus.com
> 
> I'm fixing this up right now...  From what I could see, the tsl_t is a
> char but this accesses it as a long; dare I just assume that struct
> alignment will do the right thing?  My current code conditionally sets
> tsl_t to an unsigned long on powerpc, via sysdeps.

No, it should be a 'long' (actually, an 'int').  Changing tsl_t is right.
If you _can't_ change tsl_t, you need to do something like:

#define TSL_SET(tsl) ({
  unsigned int *aligned_tsl = (unsigned int *)((uintptr_t)(tsl) & ~3);
  int shiftamount = 24 - ((uintptr_t)(tsl) & 3)*8;
  unsigned int mask = 0xFF << shiftamount;
  unsigned int ones = 0x01 << shiftamount;
  unsigned int result, temp;
  __asm__ ("
0:	lwarx %0,0,%2
	and.  %1,%0,%3
	bne+  1f
	or    %0,%0,%4
	stwcx. %0,0,%2
	bne-  0b
1:" : "=&r"(temp), "=&r"(result)
    : "r"(aligned_tsl), "r"(mask), "r"(ones)
    : "cr0", "memory"); !result; })

[that's completely untested code, and you need to stick 'sync' in at
the right places...]

but this can cause livelock, so it's good to avoid it.

If you know that 'tsl' is always aligned properly (to a cache line
boundary), you alway avoid livelock and can simplify this code quite a
bit.  It becomes

#define TSL_SET(tsl) ({
  unsigned int result, temp;
  __asm__ ("
0:	lwarx %0,0,%2
	clrrwi.  %1,%0,24
	bne+  1f
	oris   %0,%0,0x0100
	stwcx. %0,0,%2
	bne-  0b
1:" : "=&r"(temp), "=&r"(result)
    : "r"(tsl)
    : "cr0", "memory"); !result; })

but if 'tsl' ever crosses a cache line, you will get a signal.

> And no, UNSET is not sufficient, from what I can tell - at least on
> SMP.
> 
> I used sync at both ends, by analogy to the mutexes for linuxthreads;
> is this wrong/unnecessary?

It's not wrong.  It may be unnecessary.

> Does this implementation look right?  I'm away from my powerpc at the
> moment, so I haven't even been able to verify that it compiles.
> 
> 
> On Tue, Dec 28, 1999 at 02:02:03PM -0500, David Edelsohn wrote:
> > 
> > +/*
> > + * PowerPC spinlock, adapted from the Alpha and m68k ones by dhd@debian.org
> > + *
> > + * For gcc/powerpc, 0 is clear, 1 is set (but *tsl will always be 0 since it's a char)
> > + */
> > +#define TSL_SET(tsl) ({						\
> > +	register tsl_t *__l = (tsl);					\
> > +	register tsl_t __r1;						\
> > +	__asm__ volatile("						\n\
> > +	   10: lwarx  %0,0,%1						\n\
> > +	       cmpwi  %0,0						\n\
> > +	       bne+   20f						\n\
> > +	       stwcx. %2,0,%1						\n\
> > +	       bne-   10b						\n\
> > +	   20: "							\
> > +	  : "=&r" (__r1)						\
> > +	  : "r" (__l), "r" (-1) : "cr0", "memory");			\
> > +	!__r1;								\
> > +})
> > +
> > +#define	TSL_UNSET(tsl)	(*(tsl) = 0)
> > +#define	TSL_INIT(tsl)	TSL_UNSET(tsl)
> > 
> > 	The TSL_SET macro is basically correct for PowerPC uniprocessor,
> > but it is not MP safe.  For cases where this needs to be safe across a
> > multiprocessor complex, it should be preceded by a "sync" instruction and
> > ended with an "isync" instruction, or something similar depending on the
> > semantics one uses for accessing the word.
> > 
> > 	It is not clear to me why the TSL_UNSET macro is sufficient.
> 
> 
> Dan
> 
> /--------------------------------\  /--------------------------------\
> |       Daniel Jacobowitz        |__|        SCS Class of 2002       |
> |   Debian GNU/Linux Developer    __    Carnegie Mellon University   |
> |         dan@debian.org         |  |       dmj+@andrew.cmu.edu      |
> \--------------------------------/  \--------------------------------/

[lots of stuff deleted.]

> --- db2.orig/mutex/powerpc.gcc	Wed Dec 31 19:00:00 1969
> +++ db2/mutex/powerpc.gcc	Tue Dec 28 16:57:31 1999
> @@ -0,0 +1,31 @@
> +/*
> + * PowerPC spinlock, adapted from the Alpha and m68k ones by dhd@debian.org
> + * and dan@debian.org.
> + *
> + * For gcc/powerpc, 0 is clear, 1 is set.
> + */
> +#define TSL_SET(tsl) ({							\
> +	register tsl_t *__l = (tsl);					\
> +	register tsl_t __r1;						\
> +	__asm__ volatile("						\n\
> +	       sync							\n\
> +	   10: lwarx  %0,0,%1						\n\
> +	       cmpwi  %0,0						\n\
> +	       bne+   20f						\n\
> +	       stwcx. %2,0,%1						\n\
> +	       bne-   10b						\n\
> +	       sync							\n\
> +	   20: "							\
> +	  : "=&r" (__r1)						\
> +	  : "r" (__l), "r" (1) : "cr0", "memory");			\
> +	!__r1;								\
> +})

You don't need to say 'volatile' here.  The 'memory' clobber handles
it.

> +
> +#define TSL_UNSET(tsl) ({						\
> +	register tsl_t *__l = (tsl);					\
> +	__asm__ __volatile__("						\n\
> +		sync							\n\
> +		stw	%1, %0" : "=&r" (__l) : "r" (0));		\
> +	})

This won't work.  You do need the 'memory' clobber.  You want

#define TSL_UNSET(tsl) do { \
  __asm__ ("sync" : : : "memory"); \
  *tsl = 0; \
} while (0)

This will ensure that any memory stores that were made before the
'sync' are visible before *tsl is reset to zero.  If it is also
necessary that the change to *tsl is visible before any subsequent
stores, you can duplicate the 'asm' after the store.

> +#define	TSL_INIT(tsl)	TSL_UNSET(tsl)
> diff -uNr sysdeps/powerpc.orig/Makefile sysdeps/powerpc/Makefile
> --- sysdeps/powerpc.orig/Makefile	Tue Dec 28 15:50:56 1999
> +++ sysdeps/powerpc/Makefile	Tue Dec 28 15:53:01 1999
> @@ -25,6 +25,10 @@
>  CFLAGS-gmon-start.o = -G0
>  endif
>  
> +ifeq ($(subdir),db2)
> +CPPFLAGS += -DHAVE_SPINLOCKS=1 -DHAVE_ASSEM_POWERPC_GCC=1 -DSPINLOCK_TYPE='unsigned long' -DMUTEX_ALIGNMENT=4
> +endif
> +

You need MUTEX_ALIGNMENT=32 for powerpc.

-- 
- Geoffrey Keating <geoffk@cygnus.com>