Date: Thu, 12 May 2011 23:12:55 +0200 From: Attilio Rao <attilio@freebsd.org> To: Artem Belevich <art@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: <BANLkTikj%2Bszgd%2BptzD6y%2BofPs%2B8bR7Z8ew@mail.gmail.com> In-Reply-To: <BANLkTi=DOD9p-YUMm33D5ZShTjS_Q1hEvg@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>
next in thread | previous in thread | raw e-mail | index | archive | help
2011/5/12 Artem Belevich <art@freebsd.org>: > On Thu, May 12, 2011 at 10:05 PM, Attilio Rao <attilio@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. For example: #define CPU_OR_ATOMIC(d, s) do { \ __size_t __i; \ for (__i = 0; __i < _NCPUWORDS; __i++) \ atomic_set_long(&(d)->__bits[__i], \ (s)->__bits[__i]); \ } while (0) It is a plain C problem, uint32_t mismatches with long also on amd64, in order to demonstrate the problem check this: #include <sys/types.h> void quqa(volatile uint32_t *p) { *p = 10; } int main(void) { long d; quqa(&d); return (0); } >> I compiled several kernels for MIPS (with sparse configurations and >> they all compile well). >> >> 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 downside of this patch is that it typecasts everything. Along with > potentially false positive warnings it also would force compiler to > ignore real errors in the code. I'd prefer to see typecast at the > caller level on as-needed basis. *If* it's needed, that is. Even that > is not pretty either, especially if there are many places like that. I really think that this is what it is intended since the beginning. If you look at the current code, it already does, likely for the functions actually used in the kernel. The others may be just unused. > Before we decide what's the best way to deal with this I would like to > get to the bottom of the original compilation issue first. I suspect > we're still missing something. The atomic implementation gets messy and prototypes mismatches as exposed earlier. 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" :) I'm actually fine with whatever solution actual developers of our mips port came with, I don't have too strong feelings on that. > If you could send me a pointer to your repository and to the steps to > reproduce compilation issue, I'll take a closer look at what's > happening on MIPS. You can checkout as: svn checkout svn+ssh://svn.freebsd.org/base/projects/largeSMP/ But it doesn't already have the mips bit. If you are interested in the conversion, I can just send you patches. Thanks, Attilio -- 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?BANLkTikj%2Bszgd%2BptzD6y%2BofPs%2B8bR7Z8ew>