[PATCH v2 0/2] Reduce IPI calls for remote msr access

[PATCH v2 0/2] Reduce IPI calls for remote msr access

[Jacob Pan]

IPI calls to access MSRs on remote CPUs are expensive. So add a lib call
for read/modify/write access and use it in Intel RAPL driver.

Changes since v1:
	(based on feedback from Thomas Gleixner)
	- remove duplicated !CONFIG_SMP code
	- rename variables
	- added a check for local CPU before searching for active CPU oni
	  the package

Jacob Pan (2):
  x86/msr: add on cpu read/modify/write function
  powercap/rapl: reduce ipi calls

 arch/x86/include/asm/msr.h    | 13 ++++++++
 arch/x86/lib/msr-smp.c        | 34 +++++++++++++++++++
 arch/x86/lib/msr.c            | 18 ++++++++++
 drivers/powercap/intel_rapl.c | 77 +++++++++++++++++++++++--------------------
 4 files changed, 107 insertions(+), 35 deletions(-)

-- 
1.9.1

[PATCH v2 1/2] x86/msr: add on cpu read/modify/write function

[Jacob Pan]

Remote CPU read/modify/write is often needed but currently without
a lib call. This patch adds an API to perform on CPU safe read/modify/write
so that callers don't have to invent such function.

Based on initial code from:
Peter Zijlstra <peterz@infradead.org>

Suggested-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 arch/x86/include/asm/msr.h | 13 +++++++++++++
 arch/x86/lib/msr-smp.c     | 34 ++++++++++++++++++++++++++++++++++
 arch/x86/lib/msr.c         | 18 ++++++++++++++++++
 3 files changed, 65 insertions(+)

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 77d8b28..c771abd 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -27,6 +27,13 @@ struct msr_info {
 	int err;
 };
 
+struct msr_action {
+	u32 msr_no;
+	u64 clear_mask;
+	u64 set_mask;
+	int err;
+};
+
 struct msr_regs_info {
 	u32 *regs;
 	int err;
@@ -244,6 +251,7 @@ struct msr *msrs_alloc(void);
 void msrs_free(struct msr *msrs);
 int msr_set_bit(u32 msr, u8 bit);
 int msr_clear_bit(u32 msr, u8 bit);
+int msr_rmwl_safe(u32 msr_no, u64 clear_mask, u64 set_mask);
 
 #ifdef CONFIG_SMP
 int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h);
@@ -258,6 +266,7 @@ int rdmsrl_safe_on_cpu(unsigned int cpu, u32 msr_no, u64 *q);
 int wrmsrl_safe_on_cpu(unsigned int cpu, u32 msr_no, u64 q);
 int rdmsr_safe_regs_on_cpu(unsigned int cpu, u32 regs[8]);
 int wrmsr_safe_regs_on_cpu(unsigned int cpu, u32 regs[8]);
+int rmwmsrl_safe_on_cpu(unsigned int cpu, u32 msr_no, u64 clear_mask, u64 set_mask);
 #else  /*  CONFIG_SMP  */
 static inline int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h)
 {
@@ -314,6 +323,10 @@ static inline int wrmsr_safe_regs_on_cpu(unsigned int cpu, u32 regs[8])
 {
 	return wrmsr_safe_regs(regs);
 }
+static inline int rmwmsrl_safe_on_cpu(unsigned int cpu, u32 msr_no, u64 clear_mask, u64 set_mask)
+{
+	return msr_rmwl_safe(msr_no, clear_mask, set_mask);
+}
 #endif  /* CONFIG_SMP */
 #endif /* __ASSEMBLY__ */
 #endif /* _ASM_X86_MSR_H */
diff --git a/arch/x86/lib/msr-smp.c b/arch/x86/lib/msr-smp.c
index 518532e..468d891 100644
--- a/arch/x86/lib/msr-smp.c
+++ b/arch/x86/lib/msr-smp.c
@@ -221,6 +221,40 @@ int rdmsrl_safe_on_cpu(unsigned int cpu, u32 msr_no, u64 *q)
 }
 EXPORT_SYMBOL(rdmsrl_safe_on_cpu);
 
+static void remote_rmwmsrl_safe(void *info)
+{
+	struct msr_action *ma = info;
+
+	ma->err = msr_rmwl_safe(ma->msr_no, ma->clear_mask, ma->set_mask);
+}
+
+/**
+ * rmwmsrl_safe_on_cpu: Perform a read/modify/write msr transaction on cpu
+ *
+ * @cpu:  target cpu
+ * @msr:  msr number
+ * @clear_mask: bitmask to change
+ * @set_mask: bits value for the mask
+ *
+ * Returns zero for success, a negative number on error.
+ */
+int rmwmsrl_safe_on_cpu(unsigned int cpu, u32 msr, u64 clear_mask, u64 set_mask)
+{
+	int err;
+	struct msr_action ma;
+
+	memset(&ma, 0, sizeof(ma));
+
+	ma.msr_no = msr;
+	ma.clear_mask = clear_mask;
+	ma.set_mask = set_mask;
+
+	err = smp_call_function_single(cpu, remote_rmwmsrl_safe, &ma, 1);
+
+	return err ? err : ma.err;
+}
+EXPORT_SYMBOL(rmwmsrl_safe_on_cpu);
+
 /*
  * These variants are significantly slower, but allows control over
  * the entire 32-bit GPR set.
diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c
index 4362373..6e12e8d 100644
--- a/arch/x86/lib/msr.c
+++ b/arch/x86/lib/msr.c
@@ -108,3 +108,21 @@ int msr_clear_bit(u32 msr, u8 bit)
 {
 	return __flip_bit(msr, bit, false);
 }
+
+int msr_rmwl_safe(u32 msr_no, u64 clear_mask, u64 set_mask)
+{
+	int err;
+	u64 val;
+
+	err = rdmsrl_safe(msr_no, &val);
+	if (err)
+		goto out;
+
+	val &= ~clear_mask;
+	val |= set_mask;
+
+	err = wrmsrl_safe(msr_no, val);
+
+out:
+	return err;
+}
-- 
1.9.1

[PATCH v2 2/2] powercap/rapl: reduce ipi calls

[Jacob Pan]

Reduce remote CPU calls for MSR access by combining read
modify write into one function. Also optimize search for active CPU on
package.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/powercap/intel_rapl.c | 77 +++++++++++++++++++++++--------------------
 1 file changed, 42 insertions(+), 35 deletions(-)

diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c
index 48747c2..b34d6a6 100644
--- a/drivers/powercap/intel_rapl.c
+++ b/drivers/powercap/intel_rapl.c
@@ -263,7 +263,11 @@ static struct rapl_package *find_package_by_id(int id)
 /* caller to ensure CPU hotplug lock is held */
 static int find_active_cpu_on_package(int package_id)
 {
-	int i;
+	/* try to avoid remote cpu call, use raw since we are preemptible */
+	int i = raw_smp_processor_id();
+
+	if (topology_physical_package_id(i) == package_id)
+		return i;
 
 	for_each_online_cpu(i) {
 		if (topology_physical_package_id(i) == package_id)
@@ -805,30 +809,18 @@ static int rapl_write_data_raw(struct rapl_domain *rd,
 			enum rapl_primitives prim,
 			unsigned long long value)
 {
-	u64 msr_val;
-	u32 msr;
 	struct rapl_primitive_info *rp = &rpi[prim];
 	int cpu;
+	u64 bits;
 
 	cpu = find_active_cpu_on_package(rd->package_id);
 	if (cpu < 0)
 		return cpu;
-	msr = rd->msrs[rp->id];
-	if (rdmsrl_safe_on_cpu(cpu, msr, &msr_val)) {
-		dev_dbg(&rd->power_zone.dev,
-			"failed to read msr 0x%x on cpu %d\n", msr, cpu);
-		return -EIO;
-	}
-	value = rapl_unit_xlate(rd, rd->package_id, rp->unit, value, 1);
-	msr_val &= ~rp->mask;
-	msr_val |= value << rp->shift;
-	if (wrmsrl_safe_on_cpu(cpu, msr, msr_val)) {
-		dev_dbg(&rd->power_zone.dev,
-			"failed to write msr 0x%x on cpu %d\n", msr, cpu);
-		return -EIO;
-	}
 
-	return 0;
+	bits = rapl_unit_xlate(rd, rd->package_id, rp->unit, value, 1);
+	bits |= bits << rp->shift;
+
+	return rmwmsrl_safe_on_cpu(cpu, rd->msrs[rp->id], rp->mask, bits);
 }
 
 /*
@@ -893,6 +885,21 @@ static int rapl_check_unit_atom(struct rapl_package *rp, int cpu)
 	return 0;
 }
 
+static void power_limit_irq_save_cpu(void *info)
+{
+	u32 l, h = 0;
+	struct rapl_package *rp = (struct rapl_package *)info;
+
+	/* save the state of PLN irq mask bit before disabling it */
+	rdmsr_safe(MSR_IA32_PACKAGE_THERM_INTERRUPT, &l, &h);
+	if (!(rp->power_limit_irq & PACKAGE_PLN_INT_SAVED)) {
+		rp->power_limit_irq = l & PACKAGE_THERM_INT_PLN_ENABLE;
+		rp->power_limit_irq |= PACKAGE_PLN_INT_SAVED;
+	}
+	l &= ~PACKAGE_THERM_INT_PLN_ENABLE;
+	wrmsr_safe(MSR_IA32_PACKAGE_THERM_INTERRUPT, l, h);
+}
+
 
 /* REVISIT:
  * When package power limit is set artificially low by RAPL, LVT
@@ -906,7 +913,6 @@ static int rapl_check_unit_atom(struct rapl_package *rp, int cpu)
 
 static void package_power_limit_irq_save(int package_id)
 {
-	u32 l, h = 0;
 	int cpu;
 	struct rapl_package *rp;
 
@@ -920,20 +926,27 @@ static void package_power_limit_irq_save(int package_id)
 	cpu = find_active_cpu_on_package(package_id);
 	if (cpu < 0)
 		return;
-	/* save the state of PLN irq mask bit before disabling it */
-	rdmsr_safe_on_cpu(cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT, &l, &h);
-	if (!(rp->power_limit_irq & PACKAGE_PLN_INT_SAVED)) {
-		rp->power_limit_irq = l & PACKAGE_THERM_INT_PLN_ENABLE;
-		rp->power_limit_irq |= PACKAGE_PLN_INT_SAVED;
-	}
-	l &= ~PACKAGE_THERM_INT_PLN_ENABLE;
-	wrmsr_on_cpu(cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT, l, h);
+	smp_call_function_single(cpu, power_limit_irq_save_cpu, rp, 1);
+}
+
+static void power_limit_irq_restore_cpu(void *info)
+{
+	u32 l, h = 0;
+	struct rapl_package *rp = (struct rapl_package *)info;
+
+	rdmsr_safe(MSR_IA32_PACKAGE_THERM_INTERRUPT, &l, &h);
+
+	if (rp->power_limit_irq & PACKAGE_THERM_INT_PLN_ENABLE)
+		l |= PACKAGE_THERM_INT_PLN_ENABLE;
+	else
+		l &= ~PACKAGE_THERM_INT_PLN_ENABLE;
+
+	wrmsr_safe(MSR_IA32_PACKAGE_THERM_INTERRUPT, l, h);
 }
 
 /* restore per package power limit interrupt enable state */
 static void package_power_limit_irq_restore(int package_id)
 {
-	u32 l, h;
 	int cpu;
 	struct rapl_package *rp;
 
@@ -951,14 +964,8 @@ static void package_power_limit_irq_restore(int package_id)
 	/* irq enable state not saved, nothing to restore */
 	if (!(rp->power_limit_irq & PACKAGE_PLN_INT_SAVED))
 		return;
-	rdmsr_safe_on_cpu(cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT, &l, &h);
-
-	if (rp->power_limit_irq & PACKAGE_THERM_INT_PLN_ENABLE)
-		l |= PACKAGE_THERM_INT_PLN_ENABLE;
-	else
-		l &= ~PACKAGE_THERM_INT_PLN_ENABLE;
 
-	wrmsr_on_cpu(cpu, MSR_IA32_PACKAGE_THERM_INTERRUPT, l, h);
+	smp_call_function_single(cpu, power_limit_irq_restore_cpu, rp, 1);
 }
 
 static void set_floor_freq_default(struct rapl_domain *rd, bool mode)
-- 
1.9.1

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

[Thomas Gleixner]

On Tue, 12 Jan 2016, Jacob Pan wrote:
> @@ -805,30 +809,18 @@ static int rapl_write_data_raw(struct rapl_domain *rd,
>  			enum rapl_primitives prim,
>  			unsigned long long value)
>  {
> -	u64 msr_val;
> -	u32 msr;
>  	struct rapl_primitive_info *rp = &rpi[prim];
>  	int cpu;
> +	u64 bits;
>  
>  	cpu = find_active_cpu_on_package(rd->package_id);
>  	if (cpu < 0)
>  		return cpu;
> -	msr = rd->msrs[rp->id];
> -	if (rdmsrl_safe_on_cpu(cpu, msr, &msr_val)) {
> -		dev_dbg(&rd->power_zone.dev,
> -			"failed to read msr 0x%x on cpu %d\n", msr, cpu);
> -		return -EIO;
> -	}
> -	value = rapl_unit_xlate(rd, rd->package_id, rp->unit, value, 1);
> -	msr_val &= ~rp->mask;
> -	msr_val |= value << rp->shift;
> -	if (wrmsrl_safe_on_cpu(cpu, msr, msr_val)) {
> -		dev_dbg(&rd->power_zone.dev,
> -			"failed to write msr 0x%x on cpu %d\n", msr, cpu);
> -		return -EIO;
> -	}
>  
> -	return 0;
> +	bits = rapl_unit_xlate(rd, rd->package_id, rp->unit, value, 1);
> +	bits |= bits << rp->shift;
> +
> +	return rmwmsrl_safe_on_cpu(cpu, rd->msrs[rp->id], rp->mask, bits);

So here you actually use that new (misnomed) function, but for 

> +static void power_limit_irq_save_cpu(void *info)

and

> +static void power_limit_irq_restore_cpu(void *info)

you use a bog standard smp function call. What's the benefit of adding that
rmw function over a bog standard smp function call if you can only use it for
one instance of the same pattern?

Boris asked you the same question here

      https://lkml.kernel.org/r/20151220152749.GA29805@pd.tnic

but you decided to ignore it.

Thanks,

	tglx

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

[Jacob Pan]

On Wed, 13 Jan 2016 10:47:26 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:

> So here you actually use that new (misnomed) function, but for 
> 
> > +static void power_limit_irq_save_cpu(void *info)  
> 
> and
> 
> > +static void power_limit_irq_restore_cpu(void *info)  
> 
> you use a bog standard smp function call. What's the benefit of
> adding that rmw function over a bog standard smp function call if you
> can only use it for one instance of the same pattern?
> 
> Boris asked you the same question here
> 
>       https://lkml.kernel.org/r/20151220152749.GA29805@pd.tnic
> 
> but you decided to ignore it.
+Borislav,

Thanks for bring this out. I didn't mean to ignore. I thought my point
was stated in the commit message there was no point of going back and
forth. Read-Modify-Write is quite common, not just for RAPL could be
used by future code. Sorry if I wasn't clear.

Jacob

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

[Borislav Petkov]

On Wed, Jan 13, 2016 at 08:21:13AM -0800, Jacob Pan wrote:
> Thanks for bring this out. I didn't mean to ignore. I thought my point
> was stated in the commit message there was no point of going back and
> forth. Read-Modify-Write is quite common, not just for RAPL could be
> used by future code. Sorry if I wasn't clear.

But it also shows that it doesn't suffice for all your needs. So why add
it?

You can much better define your own functions which do all the MSR
handling you require and call them with smp_call_function_*.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

[Jacob Pan]

On Wed, 13 Jan 2016 17:36:10 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Wed, Jan 13, 2016 at 08:21:13AM -0800, Jacob Pan wrote:
> > Thanks for bring this out. I didn't mean to ignore. I thought my
> > point was stated in the commit message there was no point of going
> > back and forth. Read-Modify-Write is quite common, not just for
> > RAPL could be used by future code. Sorry if I wasn't clear.
> 
> But it also shows that it doesn't suffice for all your needs. So why
> add it?
> 
> You can much better define your own functions which do all the MSR
> handling you require and call them with smp_call_function_*.
> 
yeah, that is what I did in the original patch.
https://lkml.org/lkml/2015/12/7/1090

Then i was suggested to add a rmw msr api for the common good :), I
think it is a good idea since such operation is not limited to RAPL
driver. Other register access APIs such as regmap have more complete
selections.


Jacob

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

[Borislav Petkov]

On Wed, Jan 13, 2016 at 09:51:24AM -0800, Jacob Pan wrote:
> yeah, that is what I did in the original patch.
> https://lkml.org/lkml/2015/12/7/1090
> 
> Then i was suggested to add a rmw msr api for the common good :), I
> think it is a good idea since such operation is not limited to RAPL
> driver.

Others are...?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

[Jacob Pan]

On Wed, 13 Jan 2016 19:04:12 +0100
Borislav Petkov <bp@alien8.de> wrote:

> > Then i was suggested to add a rmw msr api for the common good :), I
> > think it is a good idea since such operation is not limited to RAPL
> > driver.  
> 
> Others are...?

nothing for the _safe version right now. Quick scan through the code
I see the non safe version has a couple can be converted to rmw.
e.g.

static int cpufreq_p4_setdc(unsigned int cpu, unsigned int newstate)
{
...

	rdmsr_on_cpu(cpu, MSR_IA32_THERM_CONTROL, &l, &h);
	if (newstate == DC_DISABLE) {
		pr_debug("CPU#%d disabling modulation\n", cpu);
		wrmsr_on_cpu(cpu, MSR_IA32_THERM_CONTROL, l & ~(1<<4),
	h); } else {
		pr_debug("CPU#%d setting duty cycle to %d%%\n",
			cpu, ((125 * newstate) / 10));
		/* bits 63 - 5	: reserved
		 * bit  4	: enable/disable
		 * bits 3-1	: duty cycle
		 * bit  0	: reserved
		 */
		l = (l & ~14);
		l = l | (1<<4) | ((newstate & 0x7)<<1);
		wrmsr_on_cpu(cpu, MSR_IA32_THERM_CONTROL, l, h);
	} 


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);

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

[Borislav Petkov]

On Wed, Jan 13, 2016 at 10:21:38AM -0800, Jacob Pan wrote:
> static int cpufreq_p4_setdc(unsigned int cpu, unsigned int newstate)
> {
> ...
> 
> 	rdmsr_on_cpu(cpu, MSR_IA32_THERM_CONTROL, &l, &h);
> 	if (newstate == DC_DISABLE) {
> 		pr_debug("CPU#%d disabling modulation\n", cpu);
> 		wrmsr_on_cpu(cpu, MSR_IA32_THERM_CONTROL, l & ~(1<<4),
> 	h); } else {
> 		pr_debug("CPU#%d setting duty cycle to %d%%\n",
> 			cpu, ((125 * newstate) / 10));
> 		/* bits 63 - 5	: reserved
> 		 * bit  4	: enable/disable
> 		 * bits 3-1	: duty cycle
> 		 * bit  0	: reserved
> 		 */
> 		l = (l & ~14);
> 		l = l | (1<<4) | ((newstate & 0x7)<<1);
> 		wrmsr_on_cpu(cpu, MSR_IA32_THERM_CONTROL, l, h);
> 	} 

This cannot be converted because you need to do the stuff between the
rdmsr_on_cpu() and wrmsr_on_cpu() calls.

> 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.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

[Jacob Pan]

On Wed, 13 Jan 2016 20:16:22 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Wed, Jan 13, 2016 at 10:21:38AM -0800, Jacob Pan wrote:
> > static int cpufreq_p4_setdc(unsigned int cpu, unsigned int newstate)
> > {
> > ...
> > 
> > 	rdmsr_on_cpu(cpu, MSR_IA32_THERM_CONTROL, &l, &h);
> > 	if (newstate == DC_DISABLE) {
> > 		pr_debug("CPU#%d disabling modulation\n", cpu);
> > 		wrmsr_on_cpu(cpu, MSR_IA32_THERM_CONTROL, l &
> > ~(1<<4), h); } else {
> > 		pr_debug("CPU#%d setting duty cycle to %d%%\n",
> > 			cpu, ((125 * newstate) / 10));
> > 		/* bits 63 - 5	: reserved
> > 		 * bit  4	: enable/disable
> > 		 * bits 3-1	: duty cycle
> > 		 * bit  0	: reserved
> > 		 */
> > 		l = (l & ~14);
> > 		l = l | (1<<4) | ((newstate & 0x7)<<1);
> > 		wrmsr_on_cpu(cpu, MSR_IA32_THERM_CONTROL, l, h);
> > 	}   
> 
> This cannot be converted because you need to do the stuff between the
> rdmsr_on_cpu() and wrmsr_on_cpu() calls.
> 
it can be converted if move the below if statement outside read/write
pair. 
 	if (newstate == DC_DISABLE) {

> > 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?

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

[Borislav Petkov]

On Wed, Jan 13, 2016 at 12:10:03PM -0800, Jacob Pan wrote:
> > > 	rdmsr_on_cpu(cpu, MSR_IA32_THERM_CONTROL, &l, &h);
> > > 	if (newstate == DC_DISABLE) {
> > > 		pr_debug("CPU#%d disabling modulation\n", cpu);
> > > 		wrmsr_on_cpu(cpu, MSR_IA32_THERM_CONTROL, l &
> > > ~(1<<4), h); } else {
> > > 		pr_debug("CPU#%d setting duty cycle to %d%%\n",
> > > 			cpu, ((125 * newstate) / 10));
> > > 		/* bits 63 - 5	: reserved
> > > 		 * bit  4	: enable/disable
> > > 		 * bits 3-1	: duty cycle
> > > 		 * bit  0	: reserved
> > > 		 */
> > > 		l = (l & ~14);
> > > 		l = l | (1<<4) | ((newstate & 0x7)<<1);
> > > 		wrmsr_on_cpu(cpu, MSR_IA32_THERM_CONTROL, l, h);
> > > 	}   
> > 
> > This cannot be converted because you need to do the stuff between the
> > rdmsr_on_cpu() and wrmsr_on_cpu() calls.
> > 
> it can be converted if move the below if statement outside read/write
> pair. 
>  	if (newstate == DC_DISABLE) {

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.

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.

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.

So thanks, but no thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

[Srinivas Pandruvada]

On Wed, 2016-01-13 at 22:26 +0100, Borislav Petkov wrote:

[Cut]

> 
> 	rmwmsrl_safe_on_cpu(policy->cpu, MSR_IA32_PERF_CTL,
> 			    INTEL_PERF_CTL_MASK,
> 			    (u32)sfi_cpufreq_array[next_perf_state].ctr
> l_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.
> 
> So thanks, but no thanks.
> 
I agree, in some cases it will not make much sense to use read-
modify_write calls, the user may decide whether it makes sense or not.
But such interface is not new to Linux kernel:

regmap_update_bits(), which is referenced for 346 times.

Are you saying that any such calls are not useful?

Thanks,
Srinivas

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

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 984 bytes --]

On Wed, 13 Jan 2016, Srinivas Pandruvada wrote:
> On Wed, 2016-01-13 at 22:26 +0100, Borislav Petkov wrote:
> > 	rmwmsrl_safe_on_cpu(policy->cpu, MSR_IA32_PERF_CTL,
> > 			    INTEL_PERF_CTL_MASK,
> > 			    (u32)sfi_cpufreq_array[next_perf_state].ctr
> > l_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.
> > 
> > So thanks, but no thanks.
> > 
> I agree, in some cases it will not make much sense to use read-
> modify_write calls, the user may decide whether it makes sense or not.
> But such interface is not new to Linux kernel:
> 
> regmap_update_bits(), which is referenced for 346 times.
> 
> Are you saying that any such calls are not useful?

There are certainly cases when such calls are useful. And those cases are when
we have a sufficiently big occurence of similar code which is sufficiently
complex to justify the library code and the export.

Thanks,

	tglx

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

[Jacob Pan]

On Wed, 13 Jan 2016 23:02:33 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Wed, 13 Jan 2016, Srinivas Pandruvada wrote:
> > On Wed, 2016-01-13 at 22:26 +0100, Borislav Petkov wrote:
> > > 	rmwmsrl_safe_on_cpu(policy->cpu, MSR_IA32_PERF_CTL,
> > > 			    INTEL_PERF_CTL_MASK,
> > > 			    (u32)sfi_cpufreq_array[next_perf_state].ctr
> > > l_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.
> > > 
> > > So thanks, but no thanks.
> > > 
> > I agree, in some cases it will not make much sense to use read-
> > modify_write calls, the user may decide whether it makes sense or
> > not. But such interface is not new to Linux kernel:
> > 
> > regmap_update_bits(), which is referenced for 346 times.
> > 
> > Are you saying that any such calls are not useful?
> 
> There are certainly cases when such calls are useful. And those cases
> are when we have a sufficiently big occurence of similar code which
> is sufficiently complex to justify the library code and the export.
> 
The balance of pros and cons depends on the number of occurrence. The
lib call overhead is constant where saving from the callers are
multiplied. Anyway, I will go back to my original code until we have
enough callers to tip the balance.

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

[H. Peter Anvin]

On 01/13/16 14:11, Jacob Pan wrote:
>
> The balance of pros and cons depends on the number of occurrence. The
> lib call overhead is constant where saving from the callers are
> multiplied. Anyway, I will go back to my original code until we have
> enough callers to tip the balance.
> 

The thing about premature librarization is that it can cause very
unnatural code to end up being written.  Until it is clear what the APIs
we actually need are, we shouldn't force them into a mold.  Of course,
it often makes sense to make these *local* APIs that, if useful, can get
globalized.

	-hpa

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

[Borislav Petkov]

On Wed, Jan 13, 2016 at 01:54:43PM -0800, Srinivas Pandruvada wrote:
> regmap_update_bits(), which is referenced for 346 times.

I'm at a loss as to why people keep talking about regmap.

$ git grep regmap_update_bits arch/x86/
$

> Are you saying that any such calls are not useful?

It seems I have to repeat myself ad absurdum so let me summarize: I
don't have anything against useful APIs. Those are not.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

[Srinivas Pandruvada]

On Wed, 2016-01-13 at 23:16 +0100, Borislav Petkov wrote:
> On Wed, Jan 13, 2016 at 01:54:43PM -0800, Srinivas Pandruvada wrote:
> > regmap_update_bits(), which is referenced for 346 times.
> 
> I'm at a loss as to why people keep talking about regmap.

Thomas Gleixner, understood the context and he gave a satisfactory
reply.

Thanks,
Srinivas

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

[Jacob Pan]

On Wed, 13 Jan 2016 22:26:02 +0100
Borislav Petkov <bp@alien8.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.

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

[Borislav Petkov]

On Wed, Jan 13, 2016 at 02:20:07PM -0800, Jacob Pan wrote:
> To me the caller code became more readable.

Not to me.

> I think you are referring the function name being not readable, which
> is separate of this conversion.

You think wrong.

I typed in the example and commented right under it. The old code is
much more understandable than the new code. The old code shows what bits
get set and cleared, the new code uses masks, sometimes hidden behind
defines, which people have to look up to understand what's going on. And
then look at the function definition to know which arg which is. And so
on and so on...

But I'm going to stop wasting my time with this now. I gave you enough
arguments against.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

[Thomas Gleixner]

On Wed, 13 Jan 2016, Jacob Pan wrote:
> On Wed, 13 Jan 2016 20:16:22 +0100
> Borislav Petkov <bp@alien8.de> wrote:
> 
> > On Wed, Jan 13, 2016 at 10:21:38AM -0800, Jacob Pan wrote:
> > > static int cpufreq_p4_setdc(unsigned int cpu, unsigned int newstate)
> > > {
> > > ...
> > > 
> > > 	rdmsr_on_cpu(cpu, MSR_IA32_THERM_CONTROL, &l, &h);
> > > 	if (newstate == DC_DISABLE) {
> > > 		pr_debug("CPU#%d disabling modulation\n", cpu);
> > > 		wrmsr_on_cpu(cpu, MSR_IA32_THERM_CONTROL, l &
> > > ~(1<<4), h); } else {
> > > 		pr_debug("CPU#%d setting duty cycle to %d%%\n",
> > > 			cpu, ((125 * newstate) / 10));
> > > 		/* bits 63 - 5	: reserved
> > > 		 * bit  4	: enable/disable
> > > 		 * bits 3-1	: duty cycle
> > > 		 * bit  0	: reserved
> > > 		 */
> > > 		l = (l & ~14);
> > > 		l = l | (1<<4) | ((newstate & 0x7)<<1);
> > > 		wrmsr_on_cpu(cpu, MSR_IA32_THERM_CONTROL, l, h);
> > > 	}   
> > 
> > This cannot be converted because you need to do the stuff between the
> > rdmsr_on_cpu() and wrmsr_on_cpu() calls.
> > 
> it can be converted if move the below if statement outside read/write
> pair. 
>  	if (newstate == DC_DISABLE) {
> 
> > > 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?

And what's the actual saving over a simple function which does that rdmsr,
modify, wrmsr thing and call it via smp_call_function like you did for 2 of 3
places in the rapl driver?

The amount of IPIs is the same. The amount of saved code is questionable. Lets
look at your usecase:

@@ -805,30 +809,18 @@ static int rapl_write_data_raw(struct rapl_domain *rd,
                        enum rapl_primitives prim,
                        unsigned long long value)
 {
-       u64 msr_val;
-       u32 msr;
        struct rapl_primitive_info *rp = &rpi[prim];
        int cpu;
+       u64 bits;
 
        cpu = find_active_cpu_on_package(rd->package_id);
        if (cpu < 0)
                return cpu;
-       msr = rd->msrs[rp->id];
-       if (rdmsrl_safe_on_cpu(cpu, msr, &msr_val)) {
-               dev_dbg(&rd->power_zone.dev,
-                       "failed to read msr 0x%x on cpu %d\n", msr, cpu);
-               return -EIO;
-       }
-       value = rapl_unit_xlate(rd, rd->package_id, rp->unit, value, 1);
-       msr_val &= ~rp->mask;
-       msr_val |= value << rp->shift;
-       if (wrmsrl_safe_on_cpu(cpu, msr, msr_val)) {
-               dev_dbg(&rd->power_zone.dev,
-                       "failed to write msr 0x%x on cpu %d\n", msr, cpu);
-               return -EIO;
-       }
 
-       return 0;
+       bits = rapl_unit_xlate(rd, rd->package_id, rp->unit, value, 1);
+       bits |= bits << rp->shift;
+
+       return rmwmsrl_safe_on_cpu(cpu, rd->msrs[rp->id], rp->mask, bits);
 }

So that has:	5 insertions and 17 deletions

And the library code adds 65 lines including an export. So the text size
balance of this is:

Mainline:
text	data	bss  dec     hex   filename
 5021	1008    0     6029   178d  ../build/arch/x86/lib/built-in.o
10870	1040	24   11934   2e9e  ../build/drivers/powercap/intel_rapl.o

Your patch (Just the above part which uses the lib stuff)
 5385	1008	0     6393   18f9  ../build/arch/x86/lib/built-in.o
10838	1040    24   11902   2e7e  ../build/drivers/powercap/intel_rapl.o
-----
+ 332

Now you have two more possible candidates, which require another 65 lines of
different library code and lets assume another 364 bytes of library code. So
lets further assume the above examples safe us like the rapl one 32 bytes
each, then the net damage is: 632 byte extra text size.

So what exactly is the point of this exercise?

Thanks,

	tglx