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>
next in thread | previous in thread | raw e-mail | index | archive | help
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. Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfqQkgpr3QLiBYaAKjmU=ZtQgnXq8yd8RzPqtsfUTamBDA>