Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 09 May 2011 15:40:49 +0200
From:      Andreas Tobler <andreast@FreeBSD.org>
To:        Attilio Rao <attilio@FreeBSD.org>
Cc:        svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Andreas Tobler <andreast@FreeBSD.org>, Nathan Whitehorn <nwhitehorn@FreeBSD.org>
Subject:   Re: svn commit: r221550 - head/sys/powerpc/conf
Message-ID:  <4DC7EEE1.8000901@FreeBSD.org>
In-Reply-To: <BANLkTik6P8CkOCjzJ4pt0vyMOAZCPRkF1Q@mail.gmail.com>
References:  <201105062043.p46Kh2Vs065320@svn.freebsd.org>	<BANLkTimiKGWeJfdCQrcPvujdfuyvMX8wbQ@mail.gmail.com>	<BANLkTimNC4rZvvaH5Dk%2BQpNgE90BjoQ-Sg@mail.gmail.com>	<4DC691AE.701@FreeBSD.org> <BANLkTik6P8CkOCjzJ4pt0vyMOAZCPRkF1Q@mail.gmail.com>

next in thread | previous in thread | raw e-mail | index | archive | help
On 09.05.11 00:15, Attilio Rao wrote:
> 2011/5/8 Andreas Tobler<andreast@freebsd.org>:
>> On 08.05.11 00:17, Attilio Rao wrote:
>>>
>>> 2011/5/6 Attilio Rao<attilio@freebsd.org>:
>>>>
>>>> 2011/5/6 Nathan Whitehorn<nwhitehorn@freebsd.org>:
>>>>>
>>>>> Author: nwhitehorn
>>>>> Date: Fri May  6 20:43:02 2011
>>>>> New Revision: 221550
>>>>> URL: http://svn.freebsd.org/changeset/base/221550
>>>>>
>>>>> Log:
>>>>>   SMP has worked perfectly for a very long time on 32-bit PowerPC on both
>>>>>   UP and SMP hardware. Enable it in GENERIC.
>>>>>
>>>>
>>>> While working on largeSMP, I think there is a breakage in atomic.h.
>>>> More specifically, atomic_store_rel_long() (and related functions) are
>>>> not going to properly work because powerpc defines them as:
>>>>
>>>> atomic_store_rel_long ->    atomic_store_rel_32(volatile u_int *p, u_int v)
>>>>
>>>> while this should really follow the long arguments.
>>>>
>>>> This happens because powerpc doesn't follow the other architectures
>>>> semantic on defining the "similar" atomic operations.
>>>> Other arches define an hardcode version of _type version of the
>>>> function and than make a macro the _32 (or whatever) version.
>>>> In other words this is what they do:
>>>>
>>>> void
>>>> atomic_store_rel_32()
>>>> {
>>>> ...
>>>> }
>>>>
>>>> #define atomic_store_rel_int atomic_store_rel_32
>>>>
>>>> which si clearly dangerous for cases as reported above. Maybe that
>>>> could be fixed by passing sized types, rather than simply int or long
>>>> in numbered version, but I'd really prefer to follow the semantic by
>>>> other architectures and then have:
>>>>
>>>> void
>>>> atomic_store_rel_int()
>>>> {
>>>> ...
>>>> }
>>>>
>>>> #define atomic_store_rel_32 atomic_store_rel_int
>>>>
>>>> I fixed the ATOMIC_STORE_LOAD case in my code, because I needed it,
>>>> but the final cleanup is much bigger.
>>>> I can make a patch tomorrow if you can test it.
>>>>
>>>
>>> Can you please test and review this patch?:
>>> http://www.freebsd.org/~attilio/largeSMP/atomic-powerpc.diff
>>>
>>> Unfortunately I'm having issues with the toolchains in atm, so I can't
>>> really neither test compile it.
>>
>> I built kernel and world on both, on a 32-bit system and on a 64-bit system
>> with 32-bit compat support.
>>
>> The 32-bit world failed due to type punning issues from umtx.h:121ff
>>
>> Attached my try to workaround these issues. With my try I can build both,
>> kernel and world. Right now I have a world running with it (32-bit).
>> I do not know if my try is correct. Even less after reading the comments
>> from Bruce.
>> But I think if it is on a somehow correct way, then we need something
>> similar for the other _long functions on 32-bit where we go from the u_long
>> to u_int. (e.g, atomic_add_long etc.)
>>
>> I'm ready for more testing.
>
> So based on your and Bruce's feedbacks I've reworked the patch a bit:
> http://www.freebsd.org/~attilio/largeSMP/atomic-powerpc2.diff
>
> This should make type-pun correctly and avoid auto-casting on _ptr functions.
>
> I'm sorry I couldn't even test-compile it, but I'm confident you can
> fix edge cases alone.
> Let me know.

Both builds ok and booted.

Thanks,
Andreas





Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4DC7EEE1.8000901>