Re: atomic ops on i386

On 04/14/2014 11:54 AM, Marcus Kool wrote:
> gcc defines the symbols __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 and
> __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8
> and I use code that looks like this:
>
> #if defined(__GNUC__) && __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 &&
> __SIZEOF_LONG_LONG__ == 4
> (void) __sync_add_and_fetch( &longLongVar, 1 );
> #elif defined(__GNUC__) && __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8 &&
> __SIZEOF_LONG_LONG__ == 8
> (void) __sync_add_and_fetch( &longLongVar, 1 );
> #else
> pthread_mutex_lock( &counterMutex ); // or other mutex lock not
> based on pthread
> longLongVar++;
> pthread_mutex_unlock( &counterMutex );
> #endif

The above is about emulating missing [GCC] support for atomics, which is
a different topic compared to detecting systems where atomics (native or
emulated) do not fully work (for any reason). The first topic is
illustrated by your code above. The second topic can be illustrated by
what will happen to your code when neither atomics nor pthreads are
supported.

Both topics are important, but they are quite distinct, and each
deserves a separate email thread. For now, I am going to focus on the
second topic simply because that is what Stuart needs the most at the
moment (AFAICT).

> I think that the root cause of the problem is in src/ipc/AtomicWord.h
> where it is assumed
> that if HAVE_ATOMIC_OPS is defined, atomic ops are defined for all types
> (and all sizes) which is an incorrect assumption.

The intended meaning of HAVE_ATOMIC_OPS is "All atomic things that Squid
SMP code may need are supported". In other words, it is not "we have
some atomic ops" but "we have all atomic ops that we may need".

The fact that HAVE_ATOMIC_OPS gets defined despite some used atomics not
being supported is a bug (that Stuart is trying to fix).

> Changing the configure script to detect atomic ops for long long instead
> of int is a workaround, but this prevents the use of atomic ops for 4-byte
> types on systems that support it.

AFAIK, there is no trunk module that both uses atomics and can work
without 64-bit atomics. Thus, splitting HAVE_ATOMIC_OPS into
HAVE_ATOMIC_OPS_32 and HAVE_ATOMIC_OPS_64 in trunk would be pointless. I
agree that such a split will be needed if such a module is added.

Hope this clarifies,

Alex.

> On 04/14/2014 11:45 AM, Alex Rousskov wrote:
>> On 04/13/2014 03:19 PM, Stuart Henderson wrote:
>>> On 2014-04-13, Alex Rousskov <rousskov_at_measurement-factory.com> wrote:
>>>> On 04/13/2014 06:36 AM, Stuart Henderson wrote:
>>>>
>>>>> I'm just trying to build 3.5-HEAD on OpenBSD/i386 (i.e. 32-bit
>>>>> mode) for
>>>>> the first time. It fails due to use of 64-bit atomic ops:
>>>>>
>>>>> MemStore.o(.text+0xc90): In function
>>>>> `MemStore::anchorEntry(StoreEntry&, int, Ipc::StoreMapAnchor const&)':
>>>>> : undefined reference to `__sync_fetch_and_add_8'
>>>>> MemStore.o(.text+0x3aa3): In function
>>>>> `MemStore::copyFromShm(StoreEntry&, int, Ipc::StoreMapAnchor const&)':
>>>>> : undefined reference to `__sync_fetch_and_add_8'
>>>>> MemStore.o(.text+0x3cce): In function
>>>>> `MemStore::copyFromShm(StoreEntry&, int, Ipc::StoreMapAnchor const&)':
>>>>> : undefined reference to `__sync_fetch_and_add_8'
>>>>> MemStore.o(.text+0x4040): In function
>>>>> `MemStore::copyFromShm(StoreEntry&, int, Ipc::StoreMapAnchor const&)':
>>>>> : undefined reference to `__sync_fetch_and_add_8'
>>>>> MemStore.o(.text+0x435f): In function
>>>>> `MemStore::copyFromShm(StoreEntry&, int, Ipc::StoreMapAnchor const&)':
>>>>> : undefined reference to `__sync_fetch_and_add_8'
>>>>> MemStore.o(.text+0x473d): more undefined references to
>>>>> `__sync_fetch_and_add_8' follow
>>>>> collect2: error: ld returned 1 exit status
>>>>
>>>> I am not an expert on this, but googling suggests building with
>>>> -march=i586 or a similar GCC option may solve your problem. More
>>>> possibly relevant details at
>>>
>>> That does fix the problem building, but I need this for package builds
>>> which are supposed to still work on 486, so I can't rely on users having
>>> 586 (cmpxchg8b).
>>
>>
>>>> http://www.squid-cache.org/mail-archive/squid-dev/201308/0103.html
>>>
>>> specifically "because swap_file_sz that they need to keep in sync
>>> across Squid kids is 64 bits" - so I think fixing the autoconf check is
>>> probably what's needed then.
>>
>> Probably, assuming your users do not care about SMP-shared caching
>> (memory or disk).
>>
>>
>>>>> Should the autoconf test be changed to check for working 64-bit
>>>>> ops, or
>>>>> is something more involved wanted?
>>>>
>>>> Filing a bug report may be a good idea, especially if you cannot make
>>>> this work.
>>>
>>> I suppose the simplest fix would be something like this,
>>>
>>> --- configure.ac.orig Fri Apr 4 21:31:38 2014
>>> +++ configure.ac Sun Apr 13 15:12:37 2014
>>> @@ -416,7 +416,7 @@ dnl Check for atomic operations support in the
>>> compile
>>> dnl
>>> AC_MSG_CHECKING([for GNU atomic operations support])
>>> AC_RUN_IFELSE([AC_LANG_PROGRAM([[
>>> - int n = 0;
>>> + long long n = 0;
>>> ]],[[
>>> __sync_add_and_fetch(&n, 10); // n becomes 10
>>> __sync_fetch_and_add(&n, 20); // n becomes 30
>>
>> Nitpick: s/long long/long long int/
>>
>> but I think it would be safer to test both 32- and 64-bit sizes (using
>> two different variables instead of a single n). Also, ideally, we should
>> use int32_t and uint64_t types if possible, but that probably requires
>> #inclusion of other headers and may become difficult unless there are
>> already working ./configure test cases using those types that you can
>> copy from.
>>
>>
>>> Happy to open a bug report if that's preferred, I thought I'd ask here
>>> first to work out the best direction.
>>
>> Again, I am not an expert on this, but I think you are on the right
>> track. If you can test your patch and post here, somebody will probably
>> commit it, especially if you can polish it based on the above
>> suggestions.
>>
>> A bug report just makes it easier to remember to do it (and to point
>> others to the fix).
>>
>>
>> Thank you,
>>
>> Alex.
>>
>>
>>
Received on Mon Apr 14 2014 - 19:16:32 MDT