Reorganization of spinlock defines

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Prompted by confusion over Itanium/Opterion, I have written a patch to
improve the way we define spinlocks for platforms and cpu's.

The main.c part of the patch strikes me as irrelevant to the claimed
purpose and unlikely to accomplish anything except breaking things.
Do you have a system the main.c "__alpha" code is relevant to, on which
to test that you did not break it?

Other than that, it looks like basically a good idea. But...

I plan to apply this to 7.4.

This seems like living dangerously. You do realize that this patch will
invalidate our trust that the code works on every single supported
platform? I think beta3 may be a bit late in the game for this sort of
thing, because we've already gotten a good bit of the testing we can
expect to get for lesser-used platforms during this beta cycle.

At the very least I'd like to see the decision discussed on -hackers
and not buried in -patches.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Prompted by confusion over Itanium/Opterion, I have written a patch to
improve the way we define spinlocks for platforms and cpu's.

The main.c part of the patch strikes me as irrelevant to the claimed
purpose and unlikely to accomplish anything except breaking things.
Do you have a system the main.c "__alpha" code is relevant to, on which
to test that you did not break it?

Other than that, it looks like basically a good idea. But...

I was going to have an alpha guy test it --- that was the one change I
wasn't sure about. We did test for __alpha__ all over the port/*.h
files, so it wasn't clear which alpha's were being hit. I can throw in
a comment and skip that part --- not sure.

I plan to apply this to 7.4.

This seems like living dangerously. You do realize that this patch will
invalidate our trust that the code works on every single supported
platform? I think beta3 may be a bit late in the game for this sort of
thing, because we've already gotten a good bit of the testing we can
expect to get for lesser-used platforms during this beta cycle.

At the very least I'd like to see the decision discussed on -hackers
and not buried in -patches.

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. We had the FreeBSD report
with not finding Opteron/Itanium, and that's what got me going. Also,
if it doesn't find the spinlock code, it will report an error, so we
should flesh this all out as we collect supported platforms, which we
haven't started yet.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

This is the email describing the changes in the patch for 7.4.

---------------------------------------------------------------------------

Bruce Momjian wrote:

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
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> 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.

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?

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> 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 haven't collected any known reports yet. That happens later, I
thought, nearer to RC1.

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?

Yes, but to throw an error if spinlocks aren't found, we need this
patch. We would have to test for Opteron in all the platforms that test
for specific CPU's but don't test for opteron, and might support
opterion/itanium, but even then, we don't have any way of getting a
report of a failure.

Yea, I should have thought of this before beta1, but now that I see the
bug reports, seems we have to go this way.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

On Thu, 11 Sep 2003, Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> 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?

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
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

--On Thursday, September 11, 2003 23:46:56 -0300 "Marc G. Fournier"
<scrappy@postgresql.org> wrote:

On Thu, 11 Sep 2003, Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> 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: ler@lerctr.org
US Mail: 1905 Steamboat Springs Drive, Garland, TX 75044-6749

On Thu, 11 Sep 2003, Bruce Momjian wrote:

Yes, but to throw an error if spinlocks aren't found, we need this
patch. We would have to test for Opteron in all the platforms that test
for specific CPU's but don't test for opteron, and might support
opterion/itanium, but even then, we don't have any way of getting a
report of a failure.

'K, but apparently right now we are broken on Opteron/Itanium without this
patch ... so, to fix, we either:

a. add appropriate tests to the individual port files based on individual
failure reports (albeit not clean, definitely safer), or:

b. we do massive, sweeping changes to the whole HAVE_TEST_AND_SET
detection code (definitely cleaner, but has potential of breaking more
then it fixes) :(

personally, as late in the cycle as we are, I think that a. is the wiser
move for v7.4, with b. being something that should happen as soon as
possible once we've branched and start working on v7.5 ...

"Marc G. Fournier" <scrappy@postgresql.org> writes:

On Thu, 11 Sep 2003, Tom Lane wrote:

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 ... ?

We hadn't, no. My point is that in the past we've continued to list
platforms as supported if we've had a successful report in the past
release or two. Fooling with the spinlock code is delicate enough
that I'd want to insist on moving everything to the "unsupported"
category until we get a success report with the modified code.

Maybe we should just do that. It's likely that the only platforms
that end up marked unsupported are ones that no one cares about any
more anyway. But I think we have to realize that this is not a
trivial set of changes, even if it looks like it "should work".
(Which it does, just for the record. I'm just feeling paranoid
because of where we are in the release cycle.)

regards, tom lane

Bruce Momjian <pgman@candle.pha.pa.us> writes:

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.

Agreed, the silent fallback to semaphores isn't such a hot idea in
hindsight. But the part of the patch that requires a configure option
to use that code path could be applied without touching anything else.

regards, tom lane

Larry Rosenman <ler@lerctr.org> 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.

regards, tom lane

Tom Lane wrote:

Larry Rosenman <ler@lerctr.org> 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
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

--On Thursday, September 11, 2003 23:13:54 -0400 Tom Lane
<tgl@sss.pgh.pa.us> wrote:

Larry Rosenman <ler@lerctr.org> 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'm seeing failures with the 2nd patch as well. Seems like it's not liking
UnixWare's
cc defines.

the documentation is at:

http://www.lerctr.org:8458/

the cc man page is at:

http://www.lerctr.org:8458/en/man/html.1/cc.1.html

Tom, You still have an account here.

Bruce, if you'd like an account, that is easily arranged.

LER

regards, tom lane

--
Larry Rosenman http://www.lerctr.org/~ler
Phone: +1 972-414-9812 E-Mail: ler@lerctr.org
US Mail: 1905 Steamboat Springs Drive, Garland, TX 75044-6749

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

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.

Agreed, the silent fallback to semaphores isn't such a hot idea in
hindsight. But the part of the patch that requires a configure option
to use that code path could be applied without touching anything else.

Yes, we could do just the configure warning, then plaster tests into the
port files to try to hit all the opteron/itanium cases. I am a little
concerned that this might throw up a bunch of problem cases that we will
patching for a while.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Yes, we could do just the configure warning, then plaster tests into the
port files to try to hit all the opteron/itanium cases. I am a little
concerned that this might throw up a bunch of problem cases that we will
patching for a while.

Probably so --- but we'd only be breaking new platforms that people are
starting to use, not old ones that might not be getting tested
regularly.

Understand that I'm not dead set against applying this patch for 7.4.
(On a code-cleanliness point of view I favor it.) What I want is some
open discussion about the risks and benefits before we decide.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Yes, we could do just the configure warning, then plaster tests into the
port files to try to hit all the opteron/itanium cases. I am a little
concerned that this might throw up a bunch of problem cases that we will
patching for a while.

Probably so --- but we'd only be breaking new platforms that people are
starting to use, not old ones that might not be getting tested
regularly.

Looking at the code, I wonder if we already have folks not using
spinlocks, and not even knowing it. I don't think problem reports will
be limited to new platforms.

Understand that I'm not dead set against applying this patch for 7.4.
(On a code-cleanliness point of view I favor it.) What I want is some
open discussion about the risks and benefits before we decide.

Sure, and I am not pushing the patch. I am just saying it would have
been ideal a few weeks ago --- I am not sure if we are worse off with or
without it.

I just learned from Larry that Unixware defines intel as i386, not
__i386 or __i386__, at least of the native SCO compiler that he uses.

What the code used to do is define NEED_I386_TAS_ASM unconditionally on
some platforms (negating the need to test for a compiler symbol) or test
for each platform compiler symbol (and not test all possible ways it
could be specified), like FreeBSD did. That's why things are so messy.
I am going to test for __cpu, __cpu__, and cpu on non-gcc compiler for
consistency. It is only done in one place in the patch, so that should
be good.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Looking at the code, I wonder if we already have folks not using
spinlocks, and not even knowing it. I don't think problem reports will
be limited to new platforms.

Very likely --- I heard from someone recently who was trying to run
HPUX/Itanium. After we got past the hard-wired assumption that HPUX
== HPPA, it was still giving lousy performance for lack of spinlocks.
I like the part of the patch that is in-your-face about that.

I just learned from Larry that Unixware defines intel as i386, not
__i386 or __i386__, at least of the native SCO compiler that he uses.

[blink] Namespace infringement 'r us, huh?

I am going to test for __cpu, __cpu__, and cpu on non-gcc compiler for
consistency. It is only done in one place in the patch, so that should
be good.

Please, only the first two. Make the Unixware template add __i386__.
Don't add assumptions about valid user-namespace symbols.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Looking at the code, I wonder if we already have folks not using
spinlocks, and not even knowing it. I don't think problem reports will
be limited to new platforms.

Very likely --- I heard from someone recently who was trying to run
HPUX/Itanium. After we got past the hard-wired assumption that HPUX
== HPPA, it was still giving lousy performance for lack of spinlocks.
I like the part of the patch that is in-your-face about that.

I just learned from Larry that Unixware defines intel as i386, not
__i386 or __i386__, at least of the native SCO compiler that he uses.

[blink] Namespace infringement 'r us, huh?

I am going to test for __cpu, __cpu__, and cpu on non-gcc compiler for
consistency. It is only done in one place in the patch, so that should
be good.

Please, only the first two. Make the Unixware template add __i386__.
Don't add assumptions about valid user-namespace symbols.

Roger!

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073