Skip site navigation (1) Skip section navigation (2)

Re: [PATCH v2] Use CC atomic builtins as a fallback

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Martin Pitt <mpitt(at)debian(dot)org>
Cc: pgsql-bugs(at)postgresql(dot)org
Subject: Re: [PATCH v2] Use CC atomic builtins as a fallback
Date: 2011-12-21 02:14:49
Message-ID: (view raw, whole thread or download thread mbox)
Lists: pgsql-bugs
Martin Pitt <mpitt(at)debian(dot)org> writes:
> The updated patch only uses the gcc builtins if there is no explicit
> implementation, but drops the arm one as this doesn't work on ARMv7
> and newer, as stated in the original mail.

Getting this thread back to the original patch ... I'm afraid that if we
apply this as-is, what will happen is that we fix ARMv7 and break older
versions.  Some googling found this, for instance:

which suggests that (1) ARM gcc hasn't had __sync_lock_test_and_set for
very long, and (2) what it generates doesn't work pre-ARMv6.

So I'm thinking that removing the swpb ASM option is not such a good
idea.  We could possibly test for __sync_lock_test_and_set first, and
only use swpb if we're on ARM and don't have the builtin.

Another thing that is bothering me is that according to the gcc manual,
eg here,
__sync_lock_test_and_set is nominally provided for datatypes 1, 2, 4,
or 8 bytes in length, but the underlying hardware doesn't necessarily
support all those widths natively.  If you pick the wrong width then
you don't get an inline operation at all, but a call to some possibly
inefficient library subroutine.  I see that your patch just assumes that
"int" will be a good width for the lock type, but it's unclear to me
what that choice is based on and whether or not it might be a really bad
choice on some platforms.  A look through s_lock.h suggests that only a
minority of platforms prefer int-width locks ... but I have no idea
how many of those assembly snippets could have been coded to use a
different lock datatype without penalty.  Some other evidence that
4-byte __sync_lock_test_and_set isn't universal is here:

Google is also finding some rather worrisome suggestions that
__sync_lock_test_and_set might involve a kernel call on some flavors of
ARM.  That would be pretty disastrous from a performance standpoint.

			regards, tom lane

In response to


pgsql-bugs by date

Next:From: Martin PittDate: 2011-12-21 08:09:41
Subject: Re: [PATCH v2] Use CC atomic builtins as a fallback
Previous:From: Craig RingerDate: 2011-12-20 23:59:51
Subject: Re: R: R: BUG #6342: libpq blocks forever in "poll" function

Privacy Policy | About PostgreSQL
Copyright © 1996-2017 The PostgreSQL Global Development Group