| Message ID | 20210616221329.1909276-1-srinivas.pandruvada@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [1/2] platform/x86: ISST: Optimize CPU to PCI device mapping | expand |
Hi Srinivas, On 6/17/21 12:13 AM, Srinivas Pandruvada wrote: > It was observed that some of the high performance benchmarks are spending > more time in kernel depending on which CPU package they are executing. > The difference is significant and benchmark scores varies more than 10%. > These benchmarks adjust class of service to improve thread performance > which run in parallel. This class of service change causes access to > MMIO region of Intel Speed Select PCI devices depending on the CPU > package they are executing. > > This mapping from CPU to PCI device instance uses a standard Linux PCI > interface "pci_get_domain_bus_and_slot()". This function does a linear > search to get to a PCI device. Since these platforms have 100+ PCI > devices, this search can be expensive in fast path for benchmarks. > > Since the device and function of PCI device is fixed for Intel > Speed Select PCI devices, the CPU to PCI device information can be cached > at the same time when bus number for the CPU is read. In this way during > runtime the cached information can be used. This improves performance > of these benchmarks significantly. > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > --- > .../intel_speed_select_if/isst_if_common.c | 29 +++++++++++++++---- > 1 file changed, 24 insertions(+), 5 deletions(-) > > diff --git a/drivers/platform/x86/intel_speed_select_if/isst_if_common.c b/drivers/platform/x86/intel_speed_select_if/isst_if_common.c > index 0c2aa22c7a12..aedb8310214c 100644 > --- a/drivers/platform/x86/intel_speed_select_if/isst_if_common.c > +++ b/drivers/platform/x86/intel_speed_select_if/isst_if_common.c > @@ -281,11 +281,27 @@ static int isst_if_get_platform_info(void __user *argp) > struct isst_if_cpu_info { > /* For BUS 0 and BUS 1 only, which we need for PUNIT interface */ > int bus_info[2]; > + struct pci_dev *pci_dev[2]; > int punit_cpu_id; > }; > > static struct isst_if_cpu_info *isst_cpu_info; > > +static struct pci_dev *_isst_if_get_pci_dev(int cpu, int bus_no, int dev, int fn) > +{ > + int bus_number; > + > + if (bus_no < 0 || bus_no > 1 || cpu < 0 || cpu >= nr_cpu_ids || > + cpu >= num_possible_cpus()) > + return NULL; > + > + bus_number = isst_cpu_info[cpu].bus_info[bus_no]; > + if (bus_number < 0) > + return NULL; > + > + return pci_get_domain_bus_and_slot(0, bus_number, PCI_DEVFN(dev, fn)); > +} > + > /** > * isst_if_get_pci_dev() - Get the PCI device instance for a CPU > * @cpu: Logical CPU number. > @@ -300,17 +316,18 @@ static struct isst_if_cpu_info *isst_cpu_info; > */ > struct pci_dev *isst_if_get_pci_dev(int cpu, int bus_no, int dev, int fn) > { > - int bus_number; > + struct pci_dev *pci_dev; > > if (bus_no < 0 || bus_no > 1 || cpu < 0 || cpu >= nr_cpu_ids || > cpu >= num_possible_cpus()) > return NULL; > > - bus_number = isst_cpu_info[cpu].bus_info[bus_no]; > - if (bus_number < 0) > - return NULL; > + pci_dev = isst_cpu_info[cpu].pci_dev[bus_no]; If the _isst_if_get_pci_dev() call below fails, then pci_dev might end up getting set to NULL here. > > - return pci_get_domain_bus_and_slot(0, bus_number, PCI_DEVFN(dev, fn)); > + if (pci_dev->devfn == PCI_DEVFN(dev, fn)) And then this would lead to a NULL ptr deref, I've replaced this the above if with: if (pci_dev && pci_dev->devfn == PCI_DEVFN(dev, fn)) to avoid this. I've applied this series with the above change to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > + return pci_dev; > + > + return _isst_if_get_pci_dev(cpu, bus_no, dev, fn); > } > EXPORT_SYMBOL_GPL(isst_if_get_pci_dev); > > @@ -327,6 +344,8 @@ static int isst_if_cpu_online(unsigned int cpu) > } else { > isst_cpu_info[cpu].bus_info[0] = data & 0xff; > isst_cpu_info[cpu].bus_info[1] = (data >> 8) & 0xff; > + isst_cpu_info[cpu].pci_dev[0] = _isst_if_get_pci_dev(cpu, 0, 0, 1); > + isst_cpu_info[cpu].pci_dev[1] = _isst_if_get_pci_dev(cpu, 1, 30, 1); > } > > ret = rdmsrl_safe(MSR_THREAD_ID_INFO, &data); >
Hi Hans, On Thu, 2021-06-17 at 13:37 +0200, Hans de Goede wrote: > > [...] > Hi Srinivas, > > On 6/17/21 12:13 AM, Srinivas Pandruvada wrote: > > > > - bus_number = isst_cpu_info[cpu].bus_info[bus_no]; > > - if (bus_number < 0) > > - return NULL; > > + pci_dev = isst_cpu_info[cpu].pci_dev[bus_no]; > > If the _isst_if_get_pci_dev() call below fails, then pci_dev might > end up getting set to NULL here. > > > > > - return pci_get_domain_bus_and_slot(0, bus_number, > > PCI_DEVFN(dev, fn)); > > + if (pci_dev->devfn == PCI_DEVFN(dev, fn)) > > And then this would lead to a NULL ptr deref, I've replaced this > the above if with: > > if (pci_dev && pci_dev->devfn == PCI_DEVFN(dev, fn)) > > to avoid this. Looks good. Thanks for doing the change. -Srinivas > > I've applied this series with the above change > to my review-hans branch: > https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans > > Note it will show up in my review-hans branch once I've pushed my > local branch there, which might take a while. > > Once I've run some tests on this branch the patches there will be > added to the platform-drivers-x86/for-next branch and eventually > will be included in the pdx86 pull-request to Linus for the next > merge-window. > > Regards, > > Hans > > > > > > > + return pci_dev; > > + > > + return _isst_if_get_pci_dev(cpu, bus_no, dev, fn); > > } > > EXPORT_SYMBOL_GPL(isst_if_get_pci_dev); > > > > @@ -327,6 +344,8 @@ static int isst_if_cpu_online(unsigned int cpu) > > } else { > > isst_cpu_info[cpu].bus_info[0] = data & 0xff; > > isst_cpu_info[cpu].bus_info[1] = (data >> 8) & 0xff; > > + isst_cpu_info[cpu].pci_dev[0] = > > _isst_if_get_pci_dev(cpu, 0, 0, 1); > > + isst_cpu_info[cpu].pci_dev[1] = > > _isst_if_get_pci_dev(cpu, 1, 30, 1); > > } > > > > ret = rdmsrl_safe(MSR_THREAD_ID_INFO, &data); > >
diff --git a/drivers/platform/x86/intel_speed_select_if/isst_if_common.c b/drivers/platform/x86/intel_speed_select_if/isst_if_common.c index 0c2aa22c7a12..aedb8310214c 100644 --- a/drivers/platform/x86/intel_speed_select_if/isst_if_common.c +++ b/drivers/platform/x86/intel_speed_select_if/isst_if_common.c @@ -281,11 +281,27 @@ static int isst_if_get_platform_info(void __user *argp) struct isst_if_cpu_info { /* For BUS 0 and BUS 1 only, which we need for PUNIT interface */ int bus_info[2]; + struct pci_dev *pci_dev[2]; int punit_cpu_id; }; static struct isst_if_cpu_info *isst_cpu_info; +static struct pci_dev *_isst_if_get_pci_dev(int cpu, int bus_no, int dev, int fn) +{ + int bus_number; + + if (bus_no < 0 || bus_no > 1 || cpu < 0 || cpu >= nr_cpu_ids || + cpu >= num_possible_cpus()) + return NULL; + + bus_number = isst_cpu_info[cpu].bus_info[bus_no]; + if (bus_number < 0) + return NULL; + + return pci_get_domain_bus_and_slot(0, bus_number, PCI_DEVFN(dev, fn)); +} + /** * isst_if_get_pci_dev() - Get the PCI device instance for a CPU * @cpu: Logical CPU number. @@ -300,17 +316,18 @@ static struct isst_if_cpu_info *isst_cpu_info; */ struct pci_dev *isst_if_get_pci_dev(int cpu, int bus_no, int dev, int fn) { - int bus_number; + struct pci_dev *pci_dev; if (bus_no < 0 || bus_no > 1 || cpu < 0 || cpu >= nr_cpu_ids || cpu >= num_possible_cpus()) return NULL; - bus_number = isst_cpu_info[cpu].bus_info[bus_no]; - if (bus_number < 0) - return NULL; + pci_dev = isst_cpu_info[cpu].pci_dev[bus_no]; - return pci_get_domain_bus_and_slot(0, bus_number, PCI_DEVFN(dev, fn)); + if (pci_dev->devfn == PCI_DEVFN(dev, fn)) + return pci_dev; + + return _isst_if_get_pci_dev(cpu, bus_no, dev, fn); } EXPORT_SYMBOL_GPL(isst_if_get_pci_dev); @@ -327,6 +344,8 @@ static int isst_if_cpu_online(unsigned int cpu) } else { isst_cpu_info[cpu].bus_info[0] = data & 0xff; isst_cpu_info[cpu].bus_info[1] = (data >> 8) & 0xff; + isst_cpu_info[cpu].pci_dev[0] = _isst_if_get_pci_dev(cpu, 0, 0, 1); + isst_cpu_info[cpu].pci_dev[1] = _isst_if_get_pci_dev(cpu, 1, 30, 1); } ret = rdmsrl_safe(MSR_THREAD_ID_INFO, &data);
It was observed that some of the high performance benchmarks are spending more time in kernel depending on which CPU package they are executing. The difference is significant and benchmark scores varies more than 10%. These benchmarks adjust class of service to improve thread performance which run in parallel. This class of service change causes access to MMIO region of Intel Speed Select PCI devices depending on the CPU package they are executing. This mapping from CPU to PCI device instance uses a standard Linux PCI interface "pci_get_domain_bus_and_slot()". This function does a linear search to get to a PCI device. Since these platforms have 100+ PCI devices, this search can be expensive in fast path for benchmarks. Since the device and function of PCI device is fixed for Intel Speed Select PCI devices, the CPU to PCI device information can be cached at the same time when bus number for the CPU is read. In this way during runtime the cached information can be used. This improves performance of these benchmarks significantly. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> --- .../intel_speed_select_if/isst_if_common.c | 29 +++++++++++++++---- 1 file changed, 24 insertions(+), 5 deletions(-)