[PATCH v3] powercap/rapl: reduce ipi calls

[PATCH v3] 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 such that remote CPU is not used if the current CPU is already
on the target package.

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

diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c
index 48747c2..2dcd95f 100644
--- a/drivers/powercap/intel_rapl.c
+++ b/drivers/powercap/intel_rapl.c
@@ -133,6 +133,12 @@ struct rapl_domain_data {
 	unsigned long timestamp;
 };
 
+struct msrl_action {
+	u32 msr_no;
+	u64 clear_mask;
+	u64 set_mask;
+	int err;
+};
 
 #define	DOMAIN_STATE_INACTIVE           BIT(0)
 #define	DOMAIN_STATE_POWER_LIMIT_SET    BIT(1)
@@ -263,7 +269,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)
@@ -800,35 +810,62 @@ static int rapl_read_data_raw(struct rapl_domain *rd,
 	return 0;
 }
 
+
+static int msrl_update_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;
+}
+
+static void msrl_update_func(void *info)
+{
+	struct msrl_action *ma = info;
+
+	ma->err = msrl_update_safe(ma->msr_no, ma->clear_mask, ma->set_mask);
+}
+
 /* Similar use of primitive info in the read counterpart */
 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;
+	struct msrl_action ma;
+	int ret;
 
 	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;
+	memset(&ma, 0, sizeof(ma));
+
+	ma.msr_no = rd->msrs[rp->id];
+	ma.clear_mask = rp->mask;
+	ma.set_mask = bits;
+
+	ret = smp_call_function_single(cpu, msrl_update_func, &ma, 1);
+	if (ret)
+		WARN_ON_ONCE(ret);
+	else
+		ret = ma.err;
+
+	return ret;
 }
 
 /*
@@ -893,6 +930,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 +958,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 +971,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 +1009,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 v3] powercap/rapl: reduce ipi calls

[Rafael J. Wysocki]

On Friday, January 15, 2016 11:04:13 AM Jacob Pan wrote:
> Reduce remote CPU calls for MSR access by combining read
> modify write into one function. Also optimize search for active CPU on
> package such that remote CPU is not used if the current CPU is already
> on the target package.
> 
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>

I'm wondering if there are any comments on this one?

> ---
>  drivers/powercap/intel_rapl.c | 122 ++++++++++++++++++++++++++++++------------
>  1 file changed, 87 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c
> index 48747c2..2dcd95f 100644
> --- a/drivers/powercap/intel_rapl.c
> +++ b/drivers/powercap/intel_rapl.c
> @@ -133,6 +133,12 @@ struct rapl_domain_data {
>  	unsigned long timestamp;
>  };
>  
> +struct msrl_action {
> +	u32 msr_no;
> +	u64 clear_mask;
> +	u64 set_mask;
> +	int err;
> +};
>  
>  #define	DOMAIN_STATE_INACTIVE           BIT(0)
>  #define	DOMAIN_STATE_POWER_LIMIT_SET    BIT(1)
> @@ -263,7 +269,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)
> @@ -800,35 +810,62 @@ static int rapl_read_data_raw(struct rapl_domain *rd,
>  	return 0;
>  }
>  
> +
> +static int msrl_update_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;
> +}
> +
> +static void msrl_update_func(void *info)
> +{
> +	struct msrl_action *ma = info;
> +
> +	ma->err = msrl_update_safe(ma->msr_no, ma->clear_mask, ma->set_mask);
> +}
> +
>  /* Similar use of primitive info in the read counterpart */
>  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;
> +	struct msrl_action ma;
> +	int ret;
>  
>  	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;
> +	memset(&ma, 0, sizeof(ma));
> +
> +	ma.msr_no = rd->msrs[rp->id];
> +	ma.clear_mask = rp->mask;
> +	ma.set_mask = bits;
> +
> +	ret = smp_call_function_single(cpu, msrl_update_func, &ma, 1);
> +	if (ret)
> +		WARN_ON_ONCE(ret);
> +	else
> +		ret = ma.err;
> +
> +	return ret;
>  }
>  
>  /*
> @@ -893,6 +930,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 +958,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 +971,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 +1009,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)
> 

Re: [PATCH v3] powercap/rapl: reduce ipi calls

[Thomas Gleixner]

On Tue, 16 Feb 2016, Rafael J. Wysocki wrote:
> On Friday, January 15, 2016 11:04:13 AM Jacob Pan wrote:
> > Reduce remote CPU calls for MSR access by combining read
> > modify write into one function. Also optimize search for active CPU on
> > package such that remote CPU is not used if the current CPU is already
> > on the target package.
> > 
> > Suggested-by: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> 
> I'm wondering if there are any comments on this one?

Yes.
 
> > ---
> >  drivers/powercap/intel_rapl.c | 122 ++++++++++++++++++++++++++++++------------
> >  1 file changed, 87 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c
> > index 48747c2..2dcd95f 100644
> > --- a/drivers/powercap/intel_rapl.c
> > +++ b/drivers/powercap/intel_rapl.c
> > @@ -133,6 +133,12 @@ struct rapl_domain_data {
> >  	unsigned long timestamp;
> >  };
> >  
> > +struct msrl_action {
> > +	u32 msr_no;
> > +	u64 clear_mask;
> > +	u64 set_mask;
> > +	int err;
> > +};
> >  
> >  #define	DOMAIN_STATE_INACTIVE           BIT(0)
> >  #define	DOMAIN_STATE_POWER_LIMIT_SET    BIT(1)
> > @@ -263,7 +269,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)

This is still crap. We really want a proper handling for that which does
not require these silly for_each_cpu() loops.

That code already has a hotplug notifier, so you can track one online cpu in
the package like we do in cqm and other facilities.

The first online cpu of a package sets itself as 'owner'. On unplug it looks
up:

	rpl->cpu = cpumask_any_but(topology_core_cpumask(cpu), cpu);

So if this is the last cpu then rpl->cpu becomes nr_cpu_ids.

So that whole nonsense can be avoided completely.

Thanks,

	tglx

Re: [PATCH v3] powercap/rapl: reduce ipi calls

[Jacob Pan]

On Wed, 17 Feb 2016 17:04:41 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:

> > >  
> > >  	for_each_online_cpu(i) {
> > >  		if (topology_physical_package_id(i) ==
> > > package_id)  
> 
> This is still crap. We really want a proper handling for that which
> does not require these silly for_each_cpu() loops.
> 
> That code already has a hotplug notifier, so you can track one online
> cpu in the package like we do in cqm and other facilities.
> 
> The first online cpu of a package sets itself as 'owner'. On unplug
> it looks up:
> 
> 	rpl->cpu = cpumask_any_but(topology_core_cpumask(cpu), cpu);
> 
> So if this is the last cpu then rpl->cpu becomes nr_cpu_ids.
> 
> So that whole nonsense can be avoided completely.
ok, that would work nicely, similar to cqm_pick_event_reader().
thanks for the tip. just fyi, unlike perf cqm, rapl runtime change is
expected to be low frequency.

Re: [PATCH v3] powercap/rapl: reduce ipi calls

[Thomas Gleixner]

On Wed, 17 Feb 2016, Jacob Pan wrote:
> On Wed, 17 Feb 2016 17:04:41 +0100 (CET)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > > >  
> > > >  	for_each_online_cpu(i) {
> > > >  		if (topology_physical_package_id(i) ==
> > > > package_id)  
> > 
> > This is still crap. We really want a proper handling for that which
> > does not require these silly for_each_cpu() loops.
> > 
> > That code already has a hotplug notifier, so you can track one online
> > cpu in the package like we do in cqm and other facilities.
> > 
> > The first online cpu of a package sets itself as 'owner'. On unplug
> > it looks up:
> > 
> > 	rpl->cpu = cpumask_any_but(topology_core_cpumask(cpu), cpu);
> > 
> > So if this is the last cpu then rpl->cpu becomes nr_cpu_ids.
> > 
> > So that whole nonsense can be avoided completely.
> ok, that would work nicely, similar to cqm_pick_event_reader().
> thanks for the tip. just fyi, unlike perf cqm, rapl runtime change is
> expected to be low frequency.

That still does not justify crap like this. And please use the version of pick
reader which I posted a couple of hours ago.

       http://marc.info/?l=linux-kernel&m=145572542221627&w=2

Thanks,

	tglx