Date: Thu, 22 Nov 2018 10:42:14 -0700 From: Warner Losh <imp@bsdimp.com> To: Mateusz Guzik <mjguzik@gmail.com> Cc: Konstantin Belousov <kostikbel@gmail.com>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r340676 - in head/sys: kern sys Message-ID: <CANCZdfqQkgpr3QLiBYaAKjmU=ZtQgnXq8yd8RzPqtsfUTamBDA@mail.gmail.com> In-Reply-To: <CAGudoHFL-B6e5oufAKMY=_gvgto1o07DPHxfv3rZGnaiEwB9yg@mail.gmail.com> References: <201811201458.wAKEwftP033152@repo.freebsd.org> <20181120150756.GD2378@kib.kiev.ua> <CAGudoHG-8VpSpTMX=ZL4LhSsfUe9fEkjr_-KE83K1NQGsskihw@mail.gmail.com> <CANCZdfrDyj1qjkp1XGjYP_bCHJfmcdbHxUQ%2BncNE=quQfOabUw@mail.gmail.com> <CAGudoHFL-B6e5oufAKMY=_gvgto1o07DPHxfv3rZGnaiEwB9yg@mail.gmail.com>
index | next in thread | previous in thread | raw e-mail
On Thu, Nov 22, 2018 at 10:28 AM Mateusz Guzik <mjguzik@gmail.com> wrote: > On 11/20/18, Warner Losh <imp@bsdimp.com> wrote: > > On Tue, Nov 20, 2018 at 8:28 AM Mateusz Guzik <mjguzik@gmail.com> wrote: > > > >> On 11/20/18, Konstantin Belousov <kostikbel@gmail.com> wrote: > >> >> +#if defined(__mips__) || defined(__powerpc__) > >> > Please note what I asked about this #ifdefs in the review. mips > >> > and powerpc machine/atomic.h should define some symbol like > >> > __ATOMIC_NO_64_OPS and this case should be handled in less > >> > arch-explicit > >> > manner. > >> > > >> > >> Right, should have mentioned that in the commit message. > >> > >> Anyhow, mips has some degree of 64 bit ops even for 32 bits so > >> this becomes more iffy. In particular it does have atomic_add_64. > >> I don't have a good way to test mips atomics and since non-atomic > >> version for powerpc was needed anyway I decided not to try to > >> add one. > >> > > > > I thought the 64-bit stuff was not present in true 32-bit mips at all. > You > > need the lld/scd instructions to do 64-bit atomics which are only > available > > in a 64-bit environment (eg n32 or n64 execution). They throw a fatal > > machine error if you execute them in o32 land. > > > > And I think the proper ifdef for this is defined(mips) && > > !defined(__mips_n64) && !defined(__mips_n32) or something horrible like > > that to not pessimize 64-bit executions. > > > > And powerpc will require something of similar sort (as reported by Mark > MIllard). It gets quite ugly indeed. > > I don't have strong opinion how to express the ifdefs, I think this is > least > bad if sticking to a mi header: > > diff --git a/sys/sys/systm.h b/sys/sys/systm.h > index a1b98c5660c..fab94ee7979 100644 > --- a/sys/sys/systm.h > +++ b/sys/sys/systm.h > @@ -523,7 +523,11 @@ int alloc_unr_specific(struct unrhdr *uh, u_int item); > int alloc_unrl(struct unrhdr *uh); > void free_unr(struct unrhdr *uh, u_int item); > > -#if defined(__mips__) || defined(__powerpc__) > +#if defined(mips) && !defined(__mips_n64) && !defined(__mips_n32) > +#define UNR64_LOCKED > +#endif > + > +#if defined(__powerpc__) && !defined(__powerpc64__) > #define UNR64_LOCKED > #endif > We should move the defining of this to machine/atomic.h as suggested elsewhere. Once it's more than one phrase long, it makes no sense to pollute the MI header with MD stuff like this. Warnerhome | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfqQkgpr3QLiBYaAKjmU=ZtQgnXq8yd8RzPqtsfUTamBDA>