Date: Fri, 13 May 2011 15:56:09 +0200 From: Attilio Rao <attilio@freebsd.org> To: Warner Losh <imp@bsdimp.com> Cc: mdf@freebsd.org, src-committers@freebsd.org, Artem Belevich <art@freebsd.org>, Oleksandr Tymoshenko <gonzo@freebsd.org>, Bruce Evans <brde@optusnet.com.au>, svn-src-projects@freebsd.org, Warner Losh <imp@freebsd.org> Subject: Re: svn commit: r221614 - projects/largeSMP/sys/powerpc/include Message-ID: <BANLkTi=mntpmJNWM81jmmLgAdB4xrE1MLw@mail.gmail.com> In-Reply-To: <31ABDF1E-1D1E-4DDB-B89D-D36E9B7DDC63@bsdimp.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> <BANLkTi=hB=ytsGFD8NbG7q56qTQJjroPHg@mail.gmail.com> <BANLkTimkW-S%2BmGrwPMYdHTqZ%2BhoYA=xxeA@mail.gmail.com> <31ABDF1E-1D1E-4DDB-B89D-D36E9B7DDC63@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
2011/5/13 Warner Losh <imp@bsdimp.com>: > > On May 12, 2011, at 6:06 PM, Attilio Rao wrote: > >> 2011/5/12 =C2=A0<mdf@freebsd.org>: >>> On Thu, May 12, 2011 at 2:12 PM, Attilio Rao <attilio@freebsd.org> wrot= e: >>>> 2011/5/12 Artem Belevich <art@freebsd.org>: >>>>> On Thu, May 12, 2011 at 10:05 PM, Attilio Rao <attilio@freebsd.org> w= rote: >>>>>> I spoke in person with Artem and in the end I just decided to make t= he >>>>>> 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 propte= d >>>>> 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 complai= n >>>>> 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 =C2=A0cpuset_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? =C2=A0There = 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 ter= ms of them, such that type-safety is ensured. =C2=A0Since the _32/_64 funct= ions are defined in .S files, adding aliases based on ABI is easy and we ca= n just have the right prototypes in the mips atomic.h. =C2=A0While a little= gross in some sense, we know that we can audit things such that it will be= correct, and also optimal. =C2=A0This is, after all, low level code and th= at 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 --=20 Peace can only be achieved by understanding - A. Einstein
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?BANLkTi=mntpmJNWM81jmmLgAdB4xrE1MLw>