[PATCH][PCI]: Introduce pci_find_capability_cached and make MSI use it

[PATCH][PCI]: Introduce pci_find_capability_cached and make MSI use it

[Arnaldo Carvalho de Melo]

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

Re: [PATCH][PCI]: Introduce pci_find_capability_cached and make MSI use it

[Arnaldo Carvalho de Melo]

Em Thu, May 15, 2008 at 01:04:26PM -0300, Arnaldo Carvalho de Melo escreveu:
> 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*

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

Thanks,

- Arnaldo

Re: [PATCH][PCI]: Introduce pci_find_capability_cached and make MSI use it

[Matthew Wilcox]

On Thu, May 15, 2008 at 01:04:26PM -0300, Arnaldo Carvalho de Melo wrote:
> 	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.

-- 
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."

Re: [PATCH][PCI]: Introduce pci_find_capability_cached and make MSI use it

[Arnaldo Carvalho de Melo]

Em Thu, May 15, 2008 at 11:04:07AM -0600, Matthew Wilcox escreveu:
> On Thu, May 15, 2008 at 01:04:26PM -0300, Arnaldo Carvalho de Melo wrote:
> > 	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

Re: [PATCH][PCI]: Introduce pci_find_capability_cached and make MSI use it

[Jesse Barnes]

On Thursday, May 15, 2008 10:10 am Arnaldo Carvalho de Melo wrote:
> Em Thu, May 15, 2008 at 11:04:07AM -0600, Matthew Wilcox escreveu:
> > On Thu, May 15, 2008 at 01:04:26PM -0300, Arnaldo Carvalho de Melo wrote:
> > > 	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.

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

Re: [PATCH][PCI]: Introduce pci_find_capability_cached and make MSI use it

[Arnaldo Carvalho de Melo]

Em Fri, May 16, 2008 at 10:44:34AM -0700, Jesse Barnes escreveu:
> On Thursday, May 15, 2008 10:10 am Arnaldo Carvalho de Melo wrote:
> > Em Thu, May 15, 2008 at 11:04:07AM -0600, Matthew Wilcox escreveu:
> > > On Thu, May 15, 2008 at 01:04:26PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > 	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.
> 
> 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

Re: [PATCH][PCI]: Introduce pci_find_capability_cached and make MSI use it

[Jesse Barnes]

On Friday, May 16, 2008 11:33 am Arnaldo Carvalho de Melo wrote:
> > 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

Re: [PATCH][PCI]: Introduce pci_find_capability_cached and make MSI use it

[Arnaldo Carvalho de Melo]

Em Fri, May 16, 2008 at 12:01:48PM -0700, Jesse Barnes escreveu:
> On Friday, May 16, 2008 11:33 am Arnaldo Carvalho de Melo wrote:
> > > 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).

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

[PATCH] msi: skip calling pci_find_capability from msi_set_mask_bits

[Hidetoshi Seto]

Jesse Barnes wrote:
> 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?


> 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 <seto.hidetoshi@jp.fujitsu.com>
---
 drivers/pci/msi.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 8c61304..ccb1974 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -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:
-- 
1.5.4.3

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

[Jesse Barnes]

On Sunday, May 18, 2008 9:48 pm Hidetoshi Seto wrote:
> Jesse Barnes wrote:
> > 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?
>
> > 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 <seto.hidetoshi@jp.fujitsu.com>

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

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

[Arnaldo Carvalho de Melo]

Em Mon, May 19, 2008 at 01:11:45PM -0700, Jesse Barnes escreveu:
> On Sunday, May 18, 2008 9:48 pm Hidetoshi Seto wrote:
> > Jesse Barnes wrote:
> > > 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?
> >
> > > 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 <seto.hidetoshi@jp.fujitsu.com>
> 
> 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

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

[Arnaldo Carvalho de Melo]

Em Mon, May 19, 2008 at 01:11:45PM -0700, Jesse Barnes escreveu:
> On Sunday, May 18, 2008 9:48 pm Hidetoshi Seto wrote:
> > Jesse Barnes wrote:
> > > 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?
> >
> > > 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 <seto.hidetoshi@jp.fujitsu.com>
> 
> 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 <acme@redhat.com>

- Arnaldo

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

[Jesse Barnes]

On Tuesday, May 20, 2008 12:56 pm Arnaldo Carvalho de Melo wrote:
> Em Mon, May 19, 2008 at 01:11:45PM -0700, Jesse Barnes escreveu:
> > On Sunday, May 18, 2008 9:48 pm Hidetoshi Seto wrote:
> > > Jesse Barnes wrote:
> > > > 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?
> > >
> > > > 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 <seto.hidetoshi@jp.fujitsu.com>
> >
> > 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 <acme@redhat.com>

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

Jesse