From: Arnaldo Carvalho de Melo <acme@redhat.com>
To: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
linux-rt-users@vger.kernel.org
Subject: [PATCH][PCI]: Introduce pci_find_capability_cached and make MSI use it
Date: Thu, 15 May 2008 13:04:26 -0300 [thread overview]
Message-ID: <20080515160426.GD14846@ghostprotocols.net> (raw)
Hi,
While using the preemptirqsoff ftrace tracer I noticed that
everytime handle_edge_irq is called it needs to mask and unmask MSI, and
that leads to a series of very expensive calls to pci_find_capability,
as can be seen here, with preemption disabled:
<idle>-0 [03] 422.558652: unmask_msi_irq <-handle_edge_irq
<idle>-0 [03] 422.558653: msi_set_mask_bits <-unmask_msi_irq
<idle>-0 [03] 422.558653: msi_set_enable <-msi_set_mask_bits
<idle>-0 [03] 422.558654: pci_find_capability <-msi_set_enable
<idle>-0 [03] 422.558655: __pci_bus_find_cap_start <-pci_find_capability
<idle>-0 [03] 422.558655: pci_bus_read_config_word <-__pci_bus_find_cap_start
<idle>-0 [03] 422.558656: _spin_lock_irqsave <-pci_bus_read_config_word
<idle>-0 [03] 422.558656: add_preempt_count <-_spin_lock_irqsave
<idle>-0 [03] 422.558657: pci_read <-pci_bus_read_config_word
<idle>-0 [03] 422.558657: raw_pci_read <-pci_read
<idle>-0 [03] 422.558658: pci_conf1_read <-raw_pci_read
<idle>-0 [03] 422.558658: _spin_lock_irqsave <-pci_conf1_read
<idle>-0 [03] 422.558659: add_preempt_count <-_spin_lock_irqsave
BZZT! 37us
<idle>-0 [03] 422.558696: _spin_unlock_irqrestore <-pci_conf1_read
<idle>-0 [03] 422.558697: sub_preempt_count <-_spin_unlock_irqrestore
<idle>-0 [03] 422.558697: preempt_schedule <-_spin_unlock_irqrestore
<idle>-0 [03] 422.558698: _spin_unlock_irqrestore <-pci_bus_read_config_word
<idle>-0 [03] 422.558698: sub_preempt_count <-_spin_unlock_irqrestore
<idle>-0 [03] 422.558699: preempt_schedule <-_spin_unlock_irqrestore
<idle>-0 [03] 422.558699: __pci_find_next_cap <-pci_find_capability
<idle>-0 [03] 422.558700: __pci_find_next_cap_ttl <-__pci_find_next_cap
<idle>-0 [03] 422.558700: pci_bus_read_config_byte <-__pci_find_next_cap_ttl
<idle>-0 [03] 422.558701: _spin_lock_irqsave <-pci_bus_read_config_byte
<idle>-0 [03] 422.558701: add_preempt_count <-_spin_lock_irqsave
<idle>-0 [03] 422.558702: pci_read <-pci_bus_read_config_byte
<idle>-0 [03] 422.558702: raw_pci_read <-pci_read
<idle>-0 [03] 422.558703: pci_conf1_read <-raw_pci_read
<idle>-0 [03] 422.558703: _spin_lock_irqsave <-pci_conf1_read
<idle>-0 [03] 422.558704: add_preempt_count <-_spin_lock_irqsave
BZZT! 38us
<idle>-0 [03] 422.558742: _spin_unlock_irqrestore <-pci_conf1_read
<idle>-0 [03] 422.558743: sub_preempt_count <-_spin_unlock_irqrestore
<idle>-0 [03] 422.558743: preempt_schedule <-_spin_unlock_irqrestore
<idle>-0 [03] 422.558744: _spin_unlock_irqrestore <-pci_bus_read_config_byte
<idle>-0 [03] 422.558744: sub_preempt_count <-_spin_unlock_irqrestore
<idle>-0 [03] 422.558745: preempt_schedule <-_spin_unlock_irqrestore
<idle>-0 [03] 422.558745: pci_bus_read_config_byte <-__pci_find_next_cap_ttl
<idle>-0 [03] 422.558746: _spin_lock_irqsave <-pci_bus_read_config_byte
<idle>-0 [03] 422.558746: add_preempt_count <-_spin_lock_irqsave
<idle>-0 [03] 422.558747: pci_read <-pci_bus_read_config_byte
<idle>-0 [03] 422.558747: raw_pci_read <-pci_read
<idle>-0 [03] 422.558748: pci_conf1_read <-raw_pci_read
<idle>-0 [03] 422.558748: _spin_lock_irqsave <-pci_conf1_read
<idle>-0 [03] 422.558749: add_preempt_count <-_spin_lock_irqsave
BZZT! 39us
<idle>-0 [03] 422.558788: _spin_unlock_irqrestore <-pci_conf1_read
<idle>-0 [03] 422.558789: sub_preempt_count <-_spin_unlock_irqrestore
<idle>-0 [03] 422.558789: preempt_schedule <-_spin_unlock_irqrestore
<idle>-0 [03] 422.558790: _spin_unlock_irqrestore <-pci_bus_read_config_byte
<idle>-0 [03] 422.558790: sub_preempt_count <-_spin_unlock_irqrestore
<idle>-0 [03] 422.558791: preempt_schedule <-_spin_unlock_irqrestore
<idle>-0 [03] 422.558791: pci_bus_read_config_byte <-__pci_find_next_cap_ttl
<idle>-0 [03] 422.558792: _spin_lock_irqsave <-pci_bus_read_config_byte
<idle>-0 [03] 422.558792: add_preempt_count <-_spin_lock_irqsave
<idle>-0 [03] 422.558793: pci_read <-pci_bus_read_config_byte
<idle>-0 [03] 422.558793: raw_pci_read <-pci_read
<idle>-0 [03] 422.558794: pci_conf1_read <-raw_pci_read
<idle>-0 [03] 422.558794: _spin_lock_irqsave <-pci_conf1_read
<idle>-0 [03] 422.558795: add_preempt_count <-_spin_lock_irqsave
BZZT! 39us
<idle>-0 [03] 422.558834: _spin_unlock_irqrestore <-pci_conf1_read
<idle>-0 [03] 422.558834: sub_preempt_count <-_spin_unlock_irqrestore
<SNIP many more such BZZT!s>
So I implemented pci_find_capability_cached and made MSI use it
for good measure, please consider applying.
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
diff --git a/Makefile b/Makefile
index 14f34b4..d79fdac 100644
--- a/Makefile
+++ b/Makefile
@@ -1,7 +1,7 @@
VERSION = 2
PATCHLEVEL = 6
SUBLEVEL = 25
-EXTRAVERSION =
+EXTRAVERSION = .pci_cached
NAME = Funky Weasel is Jiggy wit it
# *DOCUMENTATION*
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 8c61304..297f185 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -75,7 +75,7 @@ static void msi_set_enable(struct pci_dev *dev, int enable)
int pos;
u16 control;
- pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
+ pos = pci_find_capability_cached(dev, PCI_CAP_ID_MSI);
if (pos) {
pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &control);
control &= ~PCI_MSI_FLAGS_ENABLE;
@@ -90,7 +90,7 @@ static void msix_set_enable(struct pci_dev *dev, int enable)
int pos;
u16 control;
- pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
+ pos = pci_find_capability_cached(dev, PCI_CAP_ID_MSIX);
if (pos) {
pci_read_config_word(dev, pos + PCI_MSIX_FLAGS, &control);
control &= ~PCI_MSIX_FLAGS_ENABLE;
@@ -352,7 +352,7 @@ static int msi_capability_init(struct pci_dev *dev)
msi_set_enable(dev, 0); /* Ensure msi is disabled as I set it up */
- pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
+ pos = pci_find_capability_cached(dev, PCI_CAP_ID_MSI);
pci_read_config_word(dev, msi_control_reg(pos), &control);
/* MSI Entry Initialization */
entry = alloc_msi_entry();
@@ -426,7 +426,7 @@ static int msix_capability_init(struct pci_dev *dev,
msix_set_enable(dev, 0);/* Ensure msix is disabled as I set it up */
- pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
+ pos = pci_find_capability_cached(dev, PCI_CAP_ID_MSIX);
/* Request & Map MSI-X table region */
pci_read_config_word(dev, msi_control_reg(pos), &control);
nr_entries = multi_msix_capable(control);
@@ -533,7 +533,7 @@ static int pci_msi_check_device(struct pci_dev* dev, int nvec, int type)
if (ret)
return ret;
- if (!pci_find_capability(dev, type))
+ if (!pci_find_capability_cached(dev, type))
return -EINVAL;
return 0;
@@ -667,7 +667,7 @@ int pci_enable_msix(struct pci_dev* dev, struct msix_entry *entries, int nvec)
if (status)
return status;
- pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
+ pos = pci_find_capability_cached(dev, PCI_CAP_ID_MSIX);
pci_read_config_word(dev, msi_control_reg(pos), &control);
nr_entries = multi_msix_capable(control);
if (nvec > nr_entries)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e4548ab..659c93c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -171,6 +171,35 @@ int pci_find_capability(struct pci_dev *dev, int cap)
}
/**
+ * pci_find_capability_cached - query for devices' capabilities, cached version
+ * @dev: PCI device to query
+ * @cap: capability code
+ *
+ * Tell if a device supports a given PCI capability.
+ * Returns the address of the requested capability structure within the
+ * device's PCI configuration space or 0 in case the device does not
+ * support it. Possible values for @cap:
+ *
+ * %PCI_CAP_ID_PM Power Management
+ * %PCI_CAP_ID_AGP Accelerated Graphics Port
+ * %PCI_CAP_ID_VPD Vital Product Data
+ * %PCI_CAP_ID_SLOTID Slot Identification
+ * %PCI_CAP_ID_MSI Message Signalled Interrupts
+ * %PCI_CAP_ID_CHSWP CompactPCI HotSwap
+ * %PCI_CAP_ID_PCIX PCI-X
+ * %PCI_CAP_ID_EXP PCI Express
+ */
+int pci_find_capability_cached(struct pci_dev *dev, int cap)
+{
+ const int idx = cap - 1;
+
+ if (dev->cached_capabilities[idx] == -1)
+ dev->cached_capabilities[idx] = pci_find_capability(dev, cap);
+
+ return dev->cached_capabilities[idx];
+}
+
+/**
* pci_bus_find_capability - query for devices' capabilities
* @bus: the PCI bus to query
* @devfn: PCI device to query
@@ -1680,6 +1709,7 @@ EXPORT_SYMBOL(pcim_enable_device);
EXPORT_SYMBOL(pcim_pin_device);
EXPORT_SYMBOL(pci_disable_device);
EXPORT_SYMBOL(pci_find_capability);
+EXPORT_SYMBOL(pci_find_capability_cached);
EXPORT_SYMBOL(pci_bus_find_capability);
EXPORT_SYMBOL(pci_release_regions);
EXPORT_SYMBOL(pci_request_regions);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 4a55bf3..59338e7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -885,6 +885,7 @@ static void pci_release_bus_bridge_dev(struct device *dev)
struct pci_dev *alloc_pci_dev(void)
{
+ int i;
struct pci_dev *dev;
dev = kzalloc(sizeof(struct pci_dev), GFP_KERNEL);
@@ -893,6 +894,9 @@ struct pci_dev *alloc_pci_dev(void)
INIT_LIST_HEAD(&dev->bus_list);
+ for (i = 0; i < ARRAY_SIZE(dev->cached_capabilities); ++i)
+ dev->cached_capabilities[i] = -1;
+
pci_msi_init_pci_dev(dev);
return dev;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 6d0f93d..b7c4cfb 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -197,6 +197,7 @@ struct pci_dev {
unsigned int msix_enabled:1;
unsigned int is_managed:1;
unsigned int is_pcie:1;
+ int cached_capabilities[PCI_CAP_LIST_NR_ENTRIES]; /* See pci_find_capability_cached */
pci_dev_flags_t dev_flags;
atomic_t enable_cnt; /* pci_enable_device has been called */
@@ -516,6 +517,7 @@ struct pci_dev __deprecated *pci_find_slot(unsigned int bus,
#endif /* CONFIG_PCI_LEGACY */
int pci_find_capability(struct pci_dev *dev, int cap);
+int pci_find_capability_cached(struct pci_dev *dev, int cap);
int pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap);
int pci_find_ext_capability(struct pci_dev *dev, int cap);
int pci_find_ht_capability(struct pci_dev *dev, int ht_cap);
@@ -874,6 +876,11 @@ static inline int pci_find_capability(struct pci_dev *dev, int cap)
return 0;
}
+static inline int pci_find_capability_cached(struct pci_dev *dev, int cap)
+{
+ return 0;
+}
+
static inline int pci_find_next_capability(struct pci_dev *dev, u8 post,
int cap)
{
diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
index c0c1223..60c64fb 100644
--- a/include/linux/pci_regs.h
+++ b/include/linux/pci_regs.h
@@ -210,6 +210,7 @@
#define PCI_CAP_ID_AGP3 0x0E /* AGP Target PCI-PCI bridge */
#define PCI_CAP_ID_EXP 0x10 /* PCI Express */
#define PCI_CAP_ID_MSIX 0x11 /* MSI-X */
+#define PCI_CAP_LIST_NR_ENTRIES PCI_CAP_ID_MSIX
#define PCI_CAP_LIST_NEXT 1 /* Next capability in the list */
#define PCI_CAP_FLAGS 2 /* Capability defined flags (16 bits) */
#define PCI_CAP_SIZEOF 4
next reply other threads:[~2008-05-15 16:04 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-05-15 16:04 Arnaldo Carvalho de Melo [this message]
2008-05-15 17:02 ` [PATCH][PCI]: Introduce pci_find_capability_cached and make MSI use it Arnaldo Carvalho de Melo
2008-05-15 17:04 ` Matthew Wilcox
2008-05-15 17:10 ` Arnaldo Carvalho de Melo
2008-05-16 17:44 ` Jesse Barnes
2008-05-16 18:33 ` Arnaldo Carvalho de Melo
2008-05-16 19:01 ` Jesse Barnes
2008-05-16 19:10 ` Arnaldo Carvalho de Melo
2008-05-19 4:48 ` [PATCH] msi: skip calling pci_find_capability from msi_set_mask_bits Hidetoshi Seto
2008-05-19 20:11 ` Jesse Barnes
2008-05-19 20:26 ` Arnaldo Carvalho de Melo
2008-05-20 19:56 ` Arnaldo Carvalho de Melo
2008-05-20 20:06 ` Jesse Barnes
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20080515160426.GD14846@ghostprotocols.net \
--to=acme@redhat.com \
--cc=jbarnes@virtuousgeek.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-rt-users@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).