Re: [PATCHES] Reorganization of spinlock defines
Peter Eisentraut wrote: Bruce Momjian writes: o adds a configure option --without-spinlocks to allow non-spinlock compiles --disable-spinlocks please. I can make the change, but --without seemed to more closely match because it was like readline where you didn't have it, and had to say so specifically. I don't see how we can say --disable because this is case were we clearly don't have spinlocks to enable, no? -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [HACKERS] [PATCHES] Reorganization of spinlock defines
Tom Lane wrote: Larry Rosenman [EMAIL PROTECTED] writes: Bruce sent me a copy of the patch, and it BREAKS UnixWare (If y'all= =20 care). Unfixably? Or just a small oversight? I'm actually not worried about platforms that are actively being tested. It's the stuff that hasn't been confirmed recently that is going to be at risk. I sent him a new patch just now. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
[PATCHES] Reorganization of spinlock defines
Prompted by confusion over Itanium/Opterion, I have written a patch to
improve the way we define spinlocks for platforms and cpu's. It
basically decouples the OS from the CPU spinlock code. In almost all
cases, the spinlock code cares only about the compiler and CPU, not the
OS.
The patch:
o defines HAS_TEST_AND_SET inside each spinlock routine, not in
platform-specific files
o moves slock_t defines into the spinlock routines
o remove NEED_{CPU}_TAS_ASM define because it is no longer needed
o reports a compile error if spinlocks are not defined
o adds a configure option --without-spinlocks to allow
non-spinlock compiles
Looking at the patch, I realize this is how we should have done it all
along.
It would be nice to report the lack of spinlocks in configure, rather
than during the compile, but I can't compile s_lock.h and test for
HAS_TEST_AND_SET until configure completes.
I plan to apply this to 7.4.
--
Bruce Momjian| http://candle.pha.pa.us
[EMAIL PROTECTED] | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup.| Newtown Square, Pennsylvania 19073
Index: configure
===
RCS file: /cvsroot/pgsql-server/configure,v
retrieving revision 1.295
diff -c -c -r1.295 configure
*** configure 7 Sep 2003 16:49:41 - 1.295
--- configure 12 Sep 2003 01:36:28 -
***
*** 869,874
--- 869,875
--with-rendezvous build with Rendezvous support
--with-openssl[=DIR]build with OpenSSL support [/usr/local/ssl]
--without-readline do not use Readline
+ --without-spinlocks do not use Spinlocks
--without-zlib do not use Zlib
--with-gnu-ld assume the C compiler uses GNU ld default=no
***
*** 3494,3499
--- 3495,3530
#
+ # Spinlocks
+ #
+
+
+
+ # Check whether --with-spinlocks or --without-spinlocks was given.
+ if test ${with_spinlocks+set} = set; then
+ withval=$with_spinlocks
+
+ case $withval in
+ yes)
+ :
+ ;;
+ no)
+ :
+ ;;
+ *)
+ { { echo $as_me:$LINENO: error: no argument expected for --with-spinlocks
option 5
+ echo $as_me: error: no argument expected for --with-spinlocks option 2;}
+{ (exit 1); exit 1; }; }
+ ;;
+ esac
+
+ else
+ with_spinlocks=yes
+
+ fi;
+
+
+ #
# Zlib
#
***
*** 3523,3529
fi;
-
#
# Elf
#
--- 3554,3559
***
*** 6060,6065
--- 6090,6108
{ (exit 1); exit 1; }; }
fi
+ fi
+
+ if test $with_spinlocks = yes; then
+
+ cat confdefs.h \_ACEOF
+ #define HAVE_SPINLOCKS 1
+ _ACEOF
+
+ else
+ { echo $as_me:$LINENO: WARNING:
+ *** Not using spinlocks will cause poor performance. 5
+ echo $as_me: WARNING:
+ *** Not using spinlocks will cause poor performance. 2;}
fi
if test $with_krb4 = yes ; then
Index: configure.in
===
RCS file: /cvsroot/pgsql-server/configure.in,v
retrieving revision 1.286
diff -c -c -r1.286 configure.in
*** configure.in7 Sep 2003 16:38:05 - 1.286
--- configure.in12 Sep 2003 01:36:31 -
***
*** 522,533
[ --without-readline do not use Readline])
#
# Zlib
#
PGAC_ARG_BOOL(with, zlib, yes,
[ --without-zlib do not use Zlib])
-
#
# Elf
#
--- 522,538
[ --without-readline do not use Readline])
#
+ # Spinlocks
+ #
+ PGAC_ARG_BOOL(with, spinlocks, yes,
+ [ --without-spinlocks do not use Spinlocks])
+
+ #
# Zlib
#
PGAC_ARG_BOOL(with, zlib, yes,
[ --without-zlib do not use Zlib])
#
# Elf
#
***
*** 676,681
--- 681,693
If you have zlib already installed, see config.log for details on the
failure. It is possible the compiler isn't looking in the proper directory.
Use --without-zlib to disable zlib support.])])
+ fi
+
+ if test $with_spinlocks = yes; then
+ AC_DEFINE(HAVE_SPINLOCKS, 1, [Define to 1 if you have spinlocks.])
+ else
+ AC_MSG_WARN([
+ *** Not using spinlocks will cause poor performance.])
fi
if test $with_krb4 = yes ; then
Index: doc/src/sgml/installation.sgml
===
RCS file: /cvsroot/pgsql-server/doc/src/sgml/installation.sgml,v
retrieving revision 1.141
diff -c -c -r1.141 installation.sgml
*** doc/src/sgml/installation.sgml 11 Sep 2003 21:42:20 - 1.141
--- doc/src/sgml/installation.sgml 12 Sep 2003 01:36:34 -
***
*** 900,905
--- 900,915
/varlistentry
varlistentry
+termoption--without-spinlocks/option/term
+listitem
+ para
+
Re: [PATCHES] Reorganization of spinlock defines
Marc G. Fournier wrote: But it seems to me that this is mostly a cosmetic cleanup and therefore not the kind of thing to be doing late in beta. Couldn't we do something that affects only Opteron/Itanium and doesn't take a chance on breaking everything else? I just went through the whole patch myself, and as much as I like the overall simplification, I tend to agree with Tom here on questioning the requirement to do suck a massive change so late in the end cycle ... is there no smaller bandaid that can be applied to handle the Opteron/Itanium issue for v7.4, with the cleanup patch being applied right away after v7.4? Well, the problem was that we defined HAS_TEST_AND_SET inside the ports. I guess we could splatter a test for Itanium and Opterion in every port that could possibly use it, but then again, if we fall back to not finding it for some reason, we don't get a report because we silently fall back to semaphores. That's what has me worried, that if we don't do it, we will not know what platforms really aren't working properly. Take FreeBSD for example, that couldn't find Opteron. It lists every cpu like this: #if defined(__i386__) #define NEED_I386_TAS_ASM #define HAS_TEST_AND_SET typedef unsigned char slock_t; #endif #if defined(__sparc__) #define NEED_SPARC_TAS_ASM #define HAS_TEST_AND_SET typedef unsigned char slock_t; #endif We would have to add an opteron/itanium to port that does this, but if we miss some opteron/itanium define, we might never know because of the silent fallback. I don't care if we save it for 7.5 --- I just don't know how we will be sure we have things working properly without it. We could apply it tomorrow and see how things look on Monday. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] Reorganization of spinlock defines
--On Thursday, September 11, 2003 23:46:56 -0300 Marc G. Fournier [EMAIL PROTECTED] wrote: On Thu, 11 Sep 2003, Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: The problem with waiting for 7.5 is that we will have no error reporting when our non-spinlock code is being executed, and with Opteron/Itanium, it seems like a good time to get it working. Well, as long as you're prepared to reduce the list of known supported platforms to zero as of 7.4beta3, and issue a fresh call for port reports. I didn't think we had done that yet ... had we? called for port reports, that is ... ? But it seems to me that this is mostly a cosmetic cleanup and therefore not the kind of thing to be doing late in beta. Couldn't we do something that affects only Opteron/Itanium and doesn't take a chance on breaking everything else? I just went through the whole patch myself, and as much as I like the overall simplification, I tend to agree with Tom here on questioning the requirement to do suck a massive change so late in the end cycle ... is there no smaller bandaid that can be applied to handle the Opteron/Itanium issue for v7.4, with the cleanup patch being applied right away after v7.4? Bruce sent me a copy of the patch, and it BREAKS UnixWare (If y'all care). LER -- Larry Rosenman http://www.lerctr.org/~ler Phone: +1 972-414-9812 E-Mail: [EMAIL PROTECTED] US Mail: 1905 Steamboat Springs Drive, Garland, TX 75044-6749 pgp0.pgp Description: PGP signature
