Date: Thu, 17 Mar 2011 13:02:14 -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: <201103171302.16038.jkim@FreeBSD.org> In-Reply-To: <20110317234320.E1128@besplex.bde.org> References: <201103161609.p2GG98q6097329@svn.freebsd.org> <20110317234320.E1128@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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?
Jung-uk Kim
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201103171302.16038.jkim>