svn commit: r221614 - projects/largeSMP/sys/powerpc/include

[Attilio Rao]

2011/5/13 Warner Losh <imp at bsdimp.com>:
>
> On May 12, 2011, at 6:06 PM, Attilio Rao wrote:
>
>> 2011/5/12  <mdf at freebsd.org>:
>>> On Thu, May 12, 2011 at 2:12 PM, Attilio Rao <attilio at freebsd.org> wrote:
>>>> 2011/5/12 Artem Belevich <art at freebsd.org>:
>>>>> On Thu, May 12, 2011 at 10:05 PM, Attilio Rao <attilio at freebsd.org> wrote:
>>>>>> I spoke in person with Artem and in the end I just decided to make the
>>>>>> smallest possible subset of changes to fix the _long on 32 bits and
>>>>>> then "completed" (as some of them already exist today) the macro
>>>>>> converting the arguments to u_int stuff:
>>>>>> http://www.freebsd.org/~attilio/largeSMP/mips-atomic2.diff
>>>>>
>>>>> Attilio,
>>>>>
>>>>> Let's get back for a second to the original issue you had that propted
>>>>> you to do atomic ops changes.
>>>>> If I understand you correctly, your code was passing cpuset_t* as an
>>>>> argument to atomic_something_long and that caused compiler to complain
>>>>> that cpuset_t* is not uint32_t*.
>>>>>
>>>>> 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  cpuset_t, it does atomic of members of
>>>> cpuset_t which are actually long.
>>>
>>> Isn't this an argument for making it an array of u_int, even though
>>> it's marginally pessimal on amd64 and other 64-bit arches?  There is
>>> guaranteed support for a int-sized (or perhaps 32-bit sized) atomic
>>> op.
>>>
>>
>> There is also guaratees for long-sized atomic operations. I'm not sure
>> what atomic(9) says, but the truth is that long on FreeBSD always
>> matches word boundry, so there is no problem (Bruce thinks that long
>> actually had to be double the size of words, hence the theoretically
>> possible difficulties for atomic operation, but actually that never
>> was the case on FreeBSD).
>
> If we can't pass a long-sized operand to the atomic_long operations, then I'd say that's a bug.
>
> I think that the current ABI zoo on MIPS may mean that it makes sense to define atomic_foo_32, atomic_foo_64, etc and then define atomic_long in terms of them, such that type-safety is ensured.  Since the _32/_64 functions are defined in .S files, adding aliases based on ABI is easy and we can just have the right prototypes in the mips atomic.h.  While a little gross in some sense, we know that we can audit things such that it will be correct, and also optimal.  This is, after all, low level code and that code often calls for tricks to get optimal performance.

_32 and _64 aren't defined in support.S. They were but now they are
moved to atomic.h directly (they are "#if 0" in support.S now).

There are several ways to fix this:
- Rewrite entirely atomic.h for supporting C standard types from which
derivate uintXX_t versions (that was my first attempt, which has bugs,
as Artem pointed out, due to my misunderstanding of what every mips
flavors need to access in terms of operations)
- You can offer inline functions for offering type-pun avoidance in
the _long() version
- You can just crudely cast the _long() version arguments

The latest is the dirtiest in my idea, but still has the smallest
patchset. It is implemented in the patch I already sent here (I think
we already do that, for some of the _long() versions, which are
probabilly the only ones already used in mips kernel).

So I'd go with the first or the third case, I'm not strongly in favor
of one or another, but please just pick one.

Thanks,
Attilio


-- 
Peace can only be achieved by understanding - A. Einstein