Date: Thu, 17 Mar 2011 13:09:38 -0400 From: Jung-uk Kim <jkim@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r219698 - head/sys/i386/include Message-ID: <201103171309.42283.jkim@FreeBSD.org> In-Reply-To: <201103171302.16038.jkim@FreeBSD.org> References: <201103161609.p2GG98q6097329@svn.freebsd.org> <20110317234320.E1128@besplex.bde.org> <201103171302.16038.jkim@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday 17 March 2011 01:02 pm, Jung-uk Kim wrote:
> On Thursday 17 March 2011 08:55 am, Bruce Evans wrote:
> > On Wed, 16 Mar 2011, Jung-uk Kim wrote:
> > > Log:
> > > Rework r219679. Always check CPU class at run-time to make it
> > > predictable. Unfortunately, it pulls in <machine/cputypes.h>
> > > but it is small enough and namespace pollution is minimal, I
> > > hope.
> > >
> > > Pointed out by: bde
> >
> > Well, I don't like the namespace pollution, and the old code
> > carefully avoided it by using the tsc_present global.
> >
> > > Modified: head/sys/i386/include/cpu.h
> > > ===============================================================
> > >== ============= --- head/sys/i386/include/cpu.h Wed Mar 16
> > > 12:40:58 2011 (r219697) +++ head/sys/i386/include/cpu.h Wed Mar
> > > 16 16:09:08 2011 (r219698) @@ -39,6 +39,7 @@
> > > /*
> > > * Definitions unique to i386 cpu support.
> > > */
> > > +#include <machine/cputypes.h>
> > > #include <machine/psl.h>
> > > #include <machine/frame.h>
> > > #include <machine/segments.h>
> > > @@ -69,14 +70,13 @@ void swi_vm(void *);
> > > static __inline uint64_t
> > > get_cyclecount(void)
> > > {
> > > -#if defined(I486_CPU) || defined(KLD_MODULE)
> > > struct bintime bt;
> > >
> > > - binuptime(&bt);
> > > - return ((uint64_t)bt.sec << 56 | bt.frac >> 8);
> > > -#else
> > > + if (cpu_class == CPUCLASS_486) {
> > > + binuptime(&bt);
> > > + return ((uint64_t)bt.sec << 56 | bt.frac >> 8);
> > > + }
> > > return (rdtsc());
> > > -#endif
> > > }
> > >
> > > #endif
> >
> > cpu_class shouldn't be used for anything, and might not be able
> > to correctly classify whether the CPU has a TSC. The cpu feature
> > for this should be used. Using it directly with the correct
> > includes would give considerably more namespace pollution
> > (md_var.h for cpu_feature and specialreg.h for CPUID_TSC).
> >
> > Although I said that tsc_present should go, it seems to be the
> > best thing to use here. Rename it __cpu_feature_TSC to reflect
> > that it is exactly (cpu_feature & CPUID_TSC) and put underscores
> > in its name so as to start being even more careful about
> > namespace pollution in cpu.h. Or redeclare cpu_feature and
> > CPUID_TSC.
>
> Actually, I think this function must be move to machdep.c as a real
> function unless there is any serious objection. There is no reason
> to pollute this file for awful things like this. I know it was
> done because of performance reason in the past (such as CPU ticker,
> performance measurement, I guess.) but it is rarely used now and it
> does only one serious thing, i.e., early entropy seeding. There is
> a minor usage in SCTP for debugging but moving it to machdep.c
> won't hurt much, I think. What do you think?
Why can't we just use srandom(cpu_ticks()) in the first place? I
wonder...
Jung-uk Kim
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201103171309.42283.jkim>