Skip to contentSkip to conversation

: Introduce pci_find_capability_cached and make MSI use it

PATCH In review Arnaldo Carvalho de Melo 4 participants 13 messages May 15, 2008 – May 20, 2008

Subsystems (2)
Kbuild PCI
lore
Share & tools
lore

Arnaldo Carvalho de Melo

author
#0 lore raw

PATCH 1/1 : Introduce pci_find_capability_cached and make MSI use it

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 <[email protected]>
Makefile +1 -1
index 14f34b4..d79fdac 100644
@@ -1,7 +1,7 @@
 VERSION = 2
 PATCHLEVEL = 6
 SUBLEVEL = 25
-EXTRAVERSION =
+EXTRAVERSION = .pci_cached
 NAME = Funky Weasel is Jiggy wit it
 
 # *DOCUMENTATION*
drivers/pci/msi.c +6 -6
index 8c61304..297f185 100644
@@ -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)
drivers/pci/pci.c +30 -0
index e4548ab..659c93c 100644
@@ -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);
drivers/pci/probe.c +4 -0
index 4a55bf3..59338e7 100644
@@ -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;
include/linux/pci.h +7 -0
index 6d0f93d..b7c4cfb 100644
@@ -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)
 {
include/linux/pci_regs.h +1 -0
index c0c1223..60c64fb 100644
@@ -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
author
#1 lore raw

Em Thu, May 15, 2008 at 01:04:26PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 91 lines

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 <[email protected]>
Makefile +0 -0
index 14f34b4..d79fdac 100644

Makefile line 7

Quoted 8 lines
Makefile +1 -1
@@ -1,7 +1,7 @@
 VERSION = 2
 PATCHLEVEL = 6
 SUBLEVEL = 25
-EXTRAVERSION =
+EXTRAVERSION = .pci_cached
 NAME = Funky Weasel is Jiggy wit it
 
 # *DOCUMENTATION*

Ouch, left this in, do you want another patch or can you just remove this bit?

Thanks,

- Arnaldo

#2 lore raw
In reply to Arnaldo Carvalho de Melo
Quoted 2 lines · Arnaldo Carvalho de Melo

So I implemented pci_find_capability_cached and made MSI use it
for good measure, please consider applying.

As I told you on IRC, this is just the MSI code being complete crap. It should be caching the offset itself. We shouldn't have this extra array in the struct pci_dev just because MSI is broken.

Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
author
#3 lore raw

Em Thu, May 15, 2008 at 11:04:07AM -0600, Matthew Wilcox escreveu:

Quoted 7 lines · Arnaldo Carvalho de Melo
Quoted 2 lines

So I implemented pci_find_capability_cached and made MSI use it
for good measure, please consider applying.

As I told you on IRC, this is just the MSI code being complete crap. It should be caching the offset itself. We shouldn't have this extra array in the struct pci_dev just because MSI is broken.

Well, we can certainly do that, its just that I did this first and thought that perhaps there could be some other users, but I see that 44 extra bytes per pci_dev can be a pain if the only one to reap benefits is MSI, can't you think of any other users? I couldn't detect any so far in my admitedly limited testing.

- Arnaldo

#4 lore raw
Quoted 9 lines · Arnaldo Carvalho de Melo

Em Thu, May 15, 2008 at 11:04:07AM -0600, Matthew Wilcox escreveu:

Quoted 7 lines · Arnaldo Carvalho de Melo
Quoted 2 lines

So I implemented pci_find_capability_cached and made MSI use it
for good measure, please consider applying.

As I told you on IRC, this is just the MSI code being complete crap. It should be caching the offset itself. We shouldn't have this extra array in the struct pci_dev just because MSI is broken.

Quoted 5 lines

Well, we can certainly do that, its just that I did this first and thought that perhaps there could be some other users, but I see that 44 extra bytes per pci_dev can be a pain if the only one to reap benefits is MSI, can't you think of any other users? I couldn't detect any so far in my admitedly limited testing.

There are a few other common cap checks, but I don't think they compare to MSI in terms of latency sensitivity (though I didn't audit all the CAP_ID_EXP checks, there are quite a few of those).

Since we know MSI is a problem, let's just go with fixing that for now. If we find that other caps are also causing problems we can revisit caching all of them; the patch is simple enough.

Thanks,
Jesse

author
#5 lore raw

Em Fri, May 16, 2008 at 10:44:34AM -0700, Jesse Barnes escreveu:

Quoted 16 lines · Arnaldo Carvalho de Melo
Quoted 9 lines

Em Thu, May 15, 2008 at 11:04:07AM -0600, Matthew Wilcox escreveu:

Quoted 7 lines · Arnaldo Carvalho de Melo
Quoted 2 lines

So I implemented pci_find_capability_cached and made MSI use it
for good measure, please consider applying.

As I told you on IRC, this is just the MSI code being complete crap. It should be caching the offset itself. We shouldn't have this extra array in the struct pci_dev just because MSI is broken.

Quoted 5 lines

Well, we can certainly do that, its just that I did this first and thought that perhaps there could be some other users, but I see that 44 extra bytes per pci_dev can be a pain if the only one to reap benefits is MSI, can't you think of any other users? I couldn't detect any so far in my admitedly limited testing.

Quoted 7 lines

There are a few other common cap checks, but I don't think they compare to MSI in terms of latency sensitivity (though I didn't audit all the CAP_ID_EXP checks, there are quite a few of those).

Since we know MSI is a problem, let's just go with fixing that for now. If we find that other caps are also causing problems we can revisit caching all of them; the patch is simple enough.

Do you want me to submit another patch or can you cook up one?

- Arnaldo

#6 lore raw
Quoted 9 lines · Arnaldo Carvalho de Melo
Quoted 7 lines

There are a few other common cap checks, but I don't think they compare to MSI in terms of latency sensitivity (though I didn't audit all the CAP_ID_EXP checks, there are quite a few of those).

Since we know MSI is a problem, let's just go with fixing that for now. If we find that other caps are also causing problems we can revisit caching all of them; the patch is simple enough.

Do you want me to submit another patch or can you cook up one?

I can hack it up, probably not until Monday though (I'll be out of town this weekend).

Jesse

author
#7 lore raw

Em Fri, May 16, 2008 at 12:01:48PM -0700, Jesse Barnes escreveu:

Quoted 5 lines · Arnaldo Carvalho de Melo
Quoted 4 lines
Quoted 4 lines

There are a few other common cap checks, but I don't think they compare to MSI in terms of latency sensitivity (though I didn't audit all the CAP_ID_EXP checks, there are quite a few of those).

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Since we know MSI is a problem, let's just go with fixing that for now. If we find that other caps are also causing problems we can revisit caching all of them; the patch is simple enough.

Do you want me to submit another patch or can you cook up one?

I can hack it up, probably not until Monday though (I'll be out of town this weekend).

OK, for now this patch should be in the next rt patchset, we'll drop it when you get The Right Thing merged 8)

- Arnaldo

#8 lore raw

PATCH 1/1 msi: skip calling pci_find_capability from msi_set_mask_bits

In reply to Jesse Barnes
Quoted 3 lines · Jesse Barnes

Since we know MSI is a problem, let's just go with fixing that for now. If we find that other caps are also causing problems we can revisit caching all of them; the patch is simple enough.

Humm...
I suppose it can be more simple. How about this patch?

Quoted 2 lines

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

The position of MSI capability is already cached in the msi_desc when we enter the msi_set_mask_bits(). Use it instead.

Signed-off-by Hidetoshi Seto <[email protected]>
---
 drivers/pci/msi.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)
drivers/pci/msi.c +8 -4
index 8c61304..ccb1974 100644
@@ -70,12 +70,10 @@ arch_teardown_msi_irqs(struct pci_dev *dev)
 	}
 }
 
-static void msi_set_enable(struct pci_dev *dev, int enable)
+static void __msi_set_enable(struct pci_dev *dev, int pos, int enable)
 {
-	int pos;
 	u16 control;
 
-	pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
 	if (pos) {
 		pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &control);
 		control &= ~PCI_MSI_FLAGS_ENABLE;

@@ -85,6 +83,11 @@ static void msi_set_enable(struct pci_dev *dev, int enable)
 	}
 }
 
+static void msi_set_enable(struct pci_dev *dev, int enable)
+{
+	__msi_set_enable(dev, pci_find_capability(dev, PCI_CAP_ID_MSI), enable);
+}
+
 static void msix_set_enable(struct pci_dev *dev, int enable)
 {
 	int pos;

@@ -141,7 +144,8 @@ static void msi_set_mask_bits(unsigned int irq, u32 mask, u32 flag)
 			mask_bits |= flag & mask;
 			pci_write_config_dword(entry->dev, pos, mask_bits);
 		} else {
-			msi_set_enable(entry->dev, !flag);
+			__msi_set_enable(entry->dev, entry->msi_attrib.pos,
+					 !flag);
 		}
 		break;
 	case PCI_CAP_ID_MSIX:
Signature · 1 line
1.5.4.3
#9 lore raw

Re: [PATCH] msi: skip calling pci_find_capability from msi_set_mask_bits

Quoted 8 lines · Hidetoshi Seto
Quoted 3 lines · Jesse Barnes

Since we know MSI is a problem, let's just go with fixing that for now. If we find that other caps are also causing problems we can revisit caching all of them; the patch is simple enough.

Humm...
I suppose it can be more simple. How about this patch?

Quoted 7 lines
Quoted 2 lines

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

The position of MSI capability is already cached in the msi_desc when we enter the msi_set_mask_bits(). Use it instead.

Signed-off-by Hidetoshi Seto <[email protected]>

Yeah, this looks really nice. It should also fix Arnaldo's latency problem, and really looks like a bug fix for the MSI code more than anything.

Arnaldo, can you take a look & test and ack/nack?

Thanks,
Jesse

author
#10 lore raw

Re: [PATCH] msi: skip calling pci_find_capability from msi_set_mask_bits

Em Mon, May 19, 2008 at 01:11:45PM -0700, Jesse Barnes escreveu:

Quoted 15 lines · Hidetoshi Seto
Quoted 14 lines
Quoted 3 lines · Jesse Barnes

Since we know MSI is a problem, let's just go with fixing that for now. If we find that other caps are also causing problems we can revisit caching all of them; the patch is simple enough.

Humm...
I suppose it can be more simple. How about this patch?

Quoted 2 lines

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

The position of MSI capability is already cached in the msi_desc when we enter the msi_set_mask_bits(). Use it instead.

Quoted 6 lines
Quoted 1 line
Signed-off-by Hidetoshi Seto <[email protected]>

Yeah, this looks really nice. It should also fix Arnaldo's latency problem, and really looks like a bug fix for the MSI code more than anything.

Arnaldo, can you take a look & test and ack/nack?

I'll do that now

- Arnaldo

author Acked-by
#11 lore raw

Re: [PATCH] msi: skip calling pci_find_capability from msi_set_mask_bits

In reply to Jesse Barnes

Em Mon, May 19, 2008 at 01:11:45PM -0700, Jesse Barnes escreveu:

Quoted 15 lines · Hidetoshi Seto
Quoted 14 lines
Quoted 3 lines · Jesse Barnes

Since we know MSI is a problem, let's just go with fixing that for now. If we find that other caps are also causing problems we can revisit caching all of them; the patch is simple enough.

Humm...
I suppose it can be more simple. How about this patch?

Quoted 2 lines

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

The position of MSI capability is already cached in the msi_desc when we enter the msi_set_mask_bits(). Use it instead.

Quoted 6 lines
Quoted 1 line
Signed-off-by Hidetoshi Seto <[email protected]>

Yeah, this looks really nice. It should also fix Arnaldo's latency problem, and really looks like a bug fix for the MSI code more than anything.

Arnaldo, can you take a look & test and ack/nack?

Tested, works as expected, thanks.

Acked-by Arnaldo Carvalho de Melo <[email protected]>

- Arnaldo

#12 lore raw

Re: [PATCH] msi: skip calling pci_find_capability from msi_set_mask_bits

Quoted 23 lines · Arnaldo Carvalho de Melo

Em Mon, May 19, 2008 at 01:11:45PM -0700, Jesse Barnes escreveu:

Quoted 22 lines · Hidetoshi Seto
Quoted 8 lines
Quoted 3 lines · Jesse Barnes

Since we know MSI is a problem, let's just go with fixing that for now. If we find that other caps are also causing problems we can revisit caching all of them; the patch is simple enough.

Humm...
I suppose it can be more simple. How about this patch?

Quoted 8 lines
Quoted 3 lines

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

The position of MSI capability is already cached in the msi_desc when we enter the msi_set_mask_bits(). Use it instead.

Signed-off-by Hidetoshi Seto <[email protected]>

Yeah, this looks really nice. It should also fix Arnaldo's latency problem, and really looks like a bug fix for the MSI code more than anything.

Quoted 5 lines
Quoted 1 line

Arnaldo, can you take a look & test and ack/nack?

Tested, works as expected, thanks.

Acked-by Arnaldo Carvalho de Melo <[email protected]>

Great, thanks a lot for testing (and getting us on track to fix this in the first place!).

Jesse