From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964924AbbLGXY1 (ORCPT ); Mon, 7 Dec 2015 18:24:27 -0500 Received: from mga11.intel.com ([192.55.52.93]:57767 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933012AbbLGXY0 (ORCPT ); Mon, 7 Dec 2015 18:24:26 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,397,1444719600"; d="scan'208";a="866719562" Message-ID: <1449530602.3240.130.camel@spandruv-desk3.jf.intel.com> Subject: Re: [PATCH] powercap/rapl: reduce ipi calls From: Srinivas Pandruvada To: Jacob Pan Cc: LKML , Linux PM , Rafael Wysocki , Peter Zijlstra Date: Mon, 07 Dec 2015 15:23:22 -0800 In-Reply-To: <1449529275-14722-1-git-send-email-jacob.jun.pan@linux.intel.com> References: <1449529275-14722-1-git-send-email-jacob.jun.pan@linux.intel.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4 (3.10.4-4.fc20) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 2015-12-07 at 15:01 -0800, Jacob Pan wrote: > Reduce remote CPU calls for MSR access by combining read > modify write into one function. > > Suggested-by: Peter Zijlstra > Signed-off-by: Jacob Pan > --- > drivers/powercap/intel_rapl.c | 91 +++++++++++++++++++++++++++---------------- > 1 file changed, 58 insertions(+), 33 deletions(-) > > diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c > index cc97f08..620c916 100644 > --- a/drivers/powercap/intel_rapl.c > +++ b/drivers/powercap/intel_rapl.c > @@ -800,33 +800,43 @@ static int rapl_read_data_raw(struct rapl_domain *rd, > return 0; > } > > +struct rapl_msr_action { > + u32 msr; > + unsigned long long value; > + int shift; > + u64 mask; > +}; > + > +static void rapl_write_data_cpu(void *info) > +{ > + u64 msr_val; > + struct rapl_msr_action *ra = (struct rapl_msr_action *)info; > + > + rdmsrl_safe(ra->msr, &msr_val); > + msr_val &= ~ra->mask; > + msr_val |= ra->value << ra->shift; > + wrmsrl_safe(ra->msr, msr_val); What about adding additional common interface wrmsrl_safe_update(), so that everyone can use this? > +} > + > /* 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_msr_action ra; > struct rapl_primitive_info *rp = &rpi[prim]; > int cpu; > > 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; > - } > + > + ra.msr = rd->msrs[rp->id]; > + ra.shift = rp->shift; > + ra.mask = rp->mask; > + ra.value = rapl_unit_xlate(rd, rd->package_id, rp->unit, value, 1); > + smp_call_function_single(cpu, rapl_write_data_cpu, &ra, 1); > > return 0; > } > @@ -893,6 +903,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 +931,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 +944,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 +982,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)