Date: Fri, 13 May 2011 17:22:00 +0200 From: Artem Belevich <art@freebsd.org> To: Attilio Rao <attilio@freebsd.org> Cc: svn-src-projects@freebsd.org, Oleksandr Tymoshenko <gonzo@freebsd.org>, src-committers@freebsd.org, Warner Losh <imp@freebsd.org>, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r221614 - projects/largeSMP/sys/powerpc/include Message-ID: <BANLkTini3Q2MYn8e8UF2G0KbdWAnu9qWtw@mail.gmail.com> In-Reply-To: <BANLkTikj%2Bszgd%2BptzD6y%2BofPs%2B8bR7Z8ew@mail.gmail.com> References: <201105080039.p480doiZ021493@svn.freebsd.org> <BANLkTi=e7GtBM-PTq9yJHSLRoaOWh62AeA@mail.gmail.com> <BANLkTiktwEvRktZrGOqKKB2iSB99a3Jw=g@mail.gmail.com> <BANLkTik17r-XampEdO%2BsQ7aMOL_SDyhG=g@mail.gmail.com> <BANLkTinaWDcaiZiB3G5Szoaho1jVSeniMA@mail.gmail.com> <BANLkTimj3ohmvACmvcPa3yrdsUj=4D2V3Q@mail.gmail.com> <BANLkTikSgEXZz8vjj7kuyeWQE_oKqzB8ug@mail.gmail.com> <BANLkTinHGpL5tC3-5jOPUq6bJ2Ks7j_Dww@mail.gmail.com> <BANLkTi=DOD9p-YUMm33D5ZShTjS_Q1hEvg@mail.gmail.com> <BANLkTikj%2Bszgd%2BptzD6y%2BofPs%2B8bR7Z8ew@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, May 12, 2011 at 11:12 PM, Attilio Rao <attilio@freebsd.org> wrote:
>> Could you post definition of cpuset_t ?
>>
>> It's possible that compiler was actually correct. For instance,
>> compiler would be right to complain if cpuset_t is a packed structure,
>> even if that structure is made of a single uint32_t field.
>
> It doesn't do the atomic of =A0cpuset_t, it does atomic of members of
> cpuset_t which are actually long.
> For example:
> #define CPU_OR_ATOMIC(d, s) do { =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0\
> =A0 =A0 =A0 =A0__size_t __i; =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 \
> =A0 =A0 =A0 =A0for (__i =3D 0; __i < _NCPUWORDS; __i++) =A0 =A0 =A0 =A0 =
=A0\
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0atomic_set_long(&(d)->__bits[__i], =A0 =A0=
=A0\
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(s)->__bits[__i]); =A0 =A0 =A0 =A0=
=A0 =A0 =A0 =A0 =A0\
> } while (0)
>
> It is a plain C problem, uint32_t mismatches with long also on amd64,
Ah! Indeed, that's exactly what your problem is. uint32_t is not
necessarily long on all platforms which means that using atomic_*_long
on uint32_t variables is not going to work, even if you shut compiler
warnings up by typecasting pointers to long*.
The way I see it, your options are:
* add explicit atomic ops for uint32_t and uint64_t
* or use long/int within cpuset_t
> in order to demonstrate the problem check this:
> #include <sys/types.h>
>
> void
> quqa(volatile uint32_t *p)
> {
> =A0 =A0 =A0 =A0*p =3D 10;
> }
>
> int
> main(void)
> {
> =A0 =A0 =A0 =A0long d;
>
> =A0 =A0 =A0 =A0quqa(&d);
> =A0 =A0 =A0 =A0return (0);
> }
Well, long is not uint32_t so compiler would be correct to issue a warning.
>
>>> I compiled several kernels for MIPS (with sparse configurations and
>>> they all compile well).
But now they will not work for N64 builds because the data you point
to would be uint32_t, but atomic_*_long() would do 64-bit loads and
stores.
>>>
>>> If you agree with this patch, tonight I'll commit to my tree and add
>>> the mips support for cpuset_t soon, so that it all can be tested.
The patch is OK as long as developer does point to correctly sized
variables. Typecasts remove type checking that compiler would do, so
it would be very easy to make an error that would go unnoticed until
it causes issues at runtime. Your code one such case. It will compile,
but will not work on MIPS/n64.
> The atomic implementation gets messy and prototypes mismatches as
> exposed earlier.
It's not just a proto mismatch. It's a type mismatch which is a real
problem. uint32_t is not necessarily the same type as long. You may
get by by using atomic_*_int calls as that would happen to perform
atomic op on 32-bit scalars, but you should either use types that
current atomic ops support, or extend atomic API to support
explicitly-sized scalar types.
> IMHO the real fix is completing the conversion I was doing as I don't
> really see a valid point against it, if not "mips does it weird, pay
> attention" :)
Mips just happens to have few ILP32/LP64 variants supported within the
same include file, so it's more noticeable. The bottom line is that
calling atomic_*_long on uint32_t* will not work correctly on any LP64
platform, even if the code compiles because typecasts silenced
compiler warnings.
Are there any fundamental objections to extending atomic API to
include _32 and _64 ops? That's what atomic implementations do under
the hood anyways.
--Artem
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BANLkTini3Q2MYn8e8UF2G0KbdWAnu9qWtw>