linux-kernel - Re: [PATCH v2 2/2] powercap/rapl: reduce ipi calls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160113142007.7c99e826@yairi>
Date:	Wed, 13 Jan 2016 14:20:07 -0800
From:	Jacob Pan <jacob.jun.pan@...ux.intel.com>
To:	Borislav Petkov <bp@...en8.de>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux PM <linux-pm@...r.kernel.org>,
	Rafael Wysocki <rafael.j.wysocki@...el.com>,
	"H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
	X86 Kernel <x86@...nel.org>,
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
	Peter Zijlstra <peterz@...radead.org>,
	jacob.jun.pan@...ux.intel.com
Subject: Re: [PATCH v2 2/2] powercap/rapl: reduce ipi calls

On Wed, 13 Jan 2016 22:26:02 +0100
Borislav Petkov <bp@...en8.de> wrote:

> You mean something like this (I'm having hard time even figuring out
> what goes where):
> 
> 	if (newstate == DC_DISABLE) {
> 		pr_debug("CPU#%d disabling modulation\n", cpu);
> 		rmwmsrl_safe_on_cpu(cpu, MSR_IA32_THERM_CONTROL, (1
> << 4), 0); } else {
> 		pr_debug("CPU#%d setting duty cycle to %d%%\n", cpu,
> ((125 * newstate) / 10)); rmwmsrl_safe_on_cpu(cpu,
> MSR_IA32_THERM_CONTROL, 14, (1 << 4) |	((newstate &
> 0x7)<<1)); }
> 
> Now this is *absolutely* unreadable and hard to use. The previous
> version at least showed what happens to which bits. This call site
> will make everyone go look at the definition of rmwmsrl_safe_on_cpu()
> and see what those last two arguments do actually.
> 
To me the caller code became more readable. I think you are referring
the function name being not readable, which is separate of this
conversion.

> And, again, for the n-th time, this still doesn't work if you need to
> do other stuff between the rdmsr and wrmsr. So your interface will
> cover *some* cases but not all. So people should do
> rmwmsrl_safe_on_cpu() but not always - only if they don't need to do
> stuff between the reads and the writes.
> 
I know, I never disagreed with that :) which is why I am not using it in
the other cases in RAPL driver.
> Hmm, no thanks.
> 
> > > > static int sfi_cpufreq_target(struct cpufreq_policy *policy,
> > > > unsigned int index) {
> > > > ...
> > > > 
> > > > 	rdmsr_on_cpu(policy->cpu, MSR_IA32_PERF_CTL, &lo, &hi);
> > > > 	lo = (lo & ~INTEL_PERF_CTL_MASK) |
> > > > 		((u32)
> > > > sfi_cpufreq_array[next_perf_state].ctrl_val &
> > > > INTEL_PERF_CTL_MASK); wrmsr_on_cpu(policy->cpu,
> > > > MSR_IA32_PERF_CTL, lo, hi);    
> > > 
> > > Ditto.
> > > 
> > > These two examples prove my point, actually.  
> > 
> > same here, it is just clear mask and set mask, why not?  
> 
> Like this?
> 
> 	rmwmsrl_safe_on_cpu(policy->cpu, MSR_IA32_PERF_CTL,
> 			    INTEL_PERF_CTL_MASK,
> 			    (u32)sfi_cpufreq_array[next_perf_state].ctrl_val
> & INTEL_PERF_CTL_MASK);
> 
> Yikes!
> 
> So yes, it can work but it is ugly, hard to parse and use, not generic
> enough, etc, etc.
I don't think the conversion adds extra ugliness. It actually makes it
more clear what is the mask to clear and what bits to set.