Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1334895 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3] powercap/rapl: reduce ipi calls "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-16 02:00 +0100
    Re: [PATCH v3] powercap/rapl: reduce ipi calls Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 17:10 +0100
      Re: [PATCH v3] powercap/rapl: reduce ipi calls Jacob Pan <jacob.jun.pan@linux.intel.com> - 2016-02-17 19:10 +0100
        Re: [PATCH v3] powercap/rapl: reduce ipi calls Thomas Gleixner <tglx@linutronix.de> - 2016-02-17 19:30 +0100

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

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

[toc] | [next] | [standalone]

#1336542

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

[toc] | [prev] | [next] | [standalone]

#1336618

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.

[toc] | [prev] | [next] | [standalone]

#1336635

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web