Re: [PATCH 2/5] powernow-k8: Add core performance boost support

[Borislav Petkov <bp@amd64.org>]

(Adding hpa to Cc)

From: Thomas Renninger <trenn@suse.de>
Date: Tue, Mar 23, 2010 at 12:17:16PM +0100

> > +/* core performance boost */
> > +static bool cpb_capable, cpb_disabled;
> Whatabout using a cpufeature (arch/x86/include/asm/cpufeature.h)
> instead of cpb_capable. Then people could see this feature in 
> /proc/cpuinfo and other code parts could check for it easily if
> needed later.

I don't have a problem with that per se. It's just that /proc/cpuinfo
is a widely used interface and, AFAIR, changing it is not taken that
lightly.

Peter, what do you think?

> It could already be set in arch/x86/kernel/cpu/amd.c and
> powernow-k8 could use cpu_has(cpu, X86_FEATURE_CPB);

I'd still like to cache the cpb_capable value locally instead of getting
x86_cpuinfo percpu var and querying it. Especially if this happens often
and not only at driver init.

> Instead of cpb_disabled, I'd use cpb_enabled. Checking for
> !cpb_disabled whether it's enabled, is ugly to read.

Fair enough, cpb_disabled reflects the bit semantics in the MSR but why
not, don't matter which to me.

[..]

> > +		_cpb_toggle_msrs(t);
> > +		printk(KERN_INFO PFX "Core Boosting enabled.\n");
> Always printk on every toggle?
> That should not happen often and a user might want to get noticed if
> an app does this behind his back -> should be fine w/ or w/o, just not 
> sure whether it's intended.

Well, actually, this should be on by default and the user or an app
shouldn't be fidgeting with it all the time. It's there only for
benchmarking purposes so that you can disable it when you really have
to. But I guess it actually is going to get used if its there so maybe
we should have to rethink our approach. Hmm...

-- 
Regards/Gruss,
Boris.

--
Advanced Micro Devices, Inc.
Operating Systems Research Center