diff mbox series

[v2] PCI: Speed up device init by parsing capabilities all at once

Message ID 2A631B59-B3BD-4B3C-9DDD-795B7843333B@vmware.com (mailing list archive)
State Changes Requested
Headers show
Series [v2] PCI: Speed up device init by parsing capabilities all at once | expand

Commit Message

Vikash Bansal Jan. 28, 2022, 4:51 a.m. UTC 
In the current implementation, the PCI capability list is parsed from
the beginning to find each capability, which results in a large number
of redundant PCI reads.

Instead, we can parse the complete list just once, store it in the
pci_dev structure, and get the offset of each capability directly from
the pci_dev structure.

This implementation improves pci devices initialization time  by ~2-3%
(from 270ms to 261ms) in case of bare metal and 7-8% (From 201ms to 184ms)
in case of VM running on ESXi.

It also adds a memory overhead of 20bytes (value of PCI_CAP_ID_MAX) per
PCI device.

Ran pahole for pci_dev structure. This patch is not adding any padding
bytes.

Signed-off-by: Vikash Bansal <bvikas@vmware.com>
---

Changes in v2:
  - Ran pahole tool.
  - Modified comments to add "clock time".
  - Removed comments before call to pci_find_all_capabilities.

drivers/pci/pci.c   | 43 ++++++++++++++++++++++++++++++++++++-------
drivers/pci/probe.c |  1 +
include/linux/pci.h |  2 ++
3 files changed, 39 insertions(+), 7 deletions(-)

--
2.30.0

Comments

Bjorn Helgaas Feb. 2, 2022, 9:35 p.m. UTC | #1 
On Fri, Jan 28, 2022 at 04:51:43AM +0000, Vikash Bansal wrote:
> In the current implementation, the PCI capability list is parsed from
> the beginning to find each capability, which results in a large number
> of redundant PCI reads.
> 
> Instead, we can parse the complete list just once, store it in the
> pci_dev structure, and get the offset of each capability directly from
> the pci_dev structure.
> 
> This implementation improves pci devices initialization time  by ~2-3%
> (from 270ms to 261ms) in case of bare metal and 7-8% (From 201ms to 184ms)
> in case of VM running on ESXi.
> 
> It also adds a memory overhead of 20bytes (value of PCI_CAP_ID_MAX) per
> PCI device.
> 
> Ran pahole for pci_dev structure. This patch is not adding any padding
> bytes.

I like the idea of this.

I guess the corresponding thing could be done for extended
capabilities as well, but they're not as common, there are more of
them, and maybe the benefit wouldn't be worth it.

More comments below.

> Signed-off-by: Vikash Bansal <bvikas@vmware.com>
> ---
> 
> Changes in v2:
>   - Ran pahole tool.
>   - Modified comments to add "clock time".
>   - Removed comments before call to pci_find_all_capabilities.
> 
> drivers/pci/pci.c   | 43 ++++++++++++++++++++++++++++++++++++-------
> drivers/pci/probe.c |  1 +
> include/linux/pci.h |  2 ++
> 3 files changed, 39 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 9ecce435fb3f..b361788bcc27 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -468,6 +468,41 @@ static u8 __pci_bus_find_cap_start(struct pci_bus *bus,
> 	return 0;
> }
> 
> +
> +/**
> + * pci_find_all_capabilities - Read all capabilities
> + * @dev: the PCI device
> + *
> + * Read all capabilities and store offsets in cap_off
> + * array in pci_dev structure.
> + */
> +void pci_find_all_capabilities(struct pci_dev *dev)
> +{
> +	int ttl = PCI_FIND_CAP_TTL;
> +	u16 ent;
> +	u8 pos;
> +	u8 id;
> +
> +	pos = __pci_bus_find_cap_start(dev->bus, dev->devfn, dev->hdr_type);
> +	if (!pos)
> +		return;
> +	pci_bus_read_config_byte(dev->bus, dev->devfn, pos, &pos);
> +	while (ttl--) {
> +		if (pos < 0x40)
> +			break;
> +		pos &= ~3;
> +		pci_bus_read_config_word(dev->bus, dev->devfn, pos, &ent);
> +		id = ent & 0xff;
> +		if (id == 0xff)
> +			break;
> +
> +		/* Read first instance of capability */
> +		if (!(dev->cap_off[id]))
> +			dev->cap_off[id] = pos;
> +		pos = (ent >> 8);
> +	}
> +}

With the current capability infrastructure, one has to specify the
capability of interest up front, so there's no way to iterate directly
through all the capabilities.  Hence this code that looks a lot like
__pci_find_next_cap_ttl(), which knows the capability to look for and
ignores any others.

> +
> /**
>   * pci_find_capability - query for devices' capabilities
>   * @dev: PCI device to query
> @@ -489,13 +524,7 @@ static u8 __pci_bus_find_cap_start(struct pci_bus *bus,
>   */
> u8 pci_find_capability(struct pci_dev *dev, int cap)
> {
> -	u8 pos;
> -
> -	pos = __pci_bus_find_cap_start(dev->bus, dev->devfn, dev->hdr_type);
> -	if (pos)
> -		pos = __pci_find_next_cap(dev->bus, dev->devfn, pos, cap);
> -
> -	return pos;
> +	return dev->cap_off[cap];

This may index past the end of the cap_off[] table.  We have no
control over the callers, so no way to prevent them from reading
arbitrary memory here.

Here's an alternate proposal:

  - Leave pci_find_capability() completely alone.

  - Don't add the cap_off[] table.  This only works for Capabilities
    (not Extended Capabilities), depends on PCI_CAP_ID_MAX (which is
    not fixed and could change in the future), and duplicates existing
    data like dev->pcie_cap, dev->msi_cap, dev->msix_cap, etc.

  - Add pci_find_all_capabilities() (I'd probably call it
    "pci_scan_capabilities()"), but extend it to iterate through both
    kinds of capabilities (probably a helper function for each kind)
    and fill in dev->pcie_cap, dev->msi_cap, dev->aer_cap, etc as it
    finds them.

  - Follow up with patches that remove calls to pci_find_capability()
    and pci_find_ext_capability() for the capabilities we've already
    located.

> }
> EXPORT_SYMBOL(pci_find_capability);
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 17a969942d37..b2fa5b2c42f6 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1831,6 +1831,7 @@ int pci_setup_device(struct pci_dev *dev)
> 	dev->hdr_type = hdr_type & 0x7f;
> 	dev->multifunction = !!(hdr_type & 0x80);
> 	dev->error_state = pci_channel_io_normal;
> +	pci_find_all_capabilities(dev);
> 	set_pcie_port_type(dev);
> 
> 	pci_set_of_node(dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8253a5413d7c..abcf7fdc4c98 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -335,6 +335,7 @@ struct pci_dev {
> 	unsigned int	class;		/* 3 bytes: (base,sub,prog-if) */
> 	u8		revision;	/* PCI revision, low byte of class word */
> 	u8		hdr_type;	/* PCI header type (`multi' flag masked out) */
> +	u8              cap_off[PCI_CAP_ID_MAX]; /* Offsets of all pci capabilities */
> #ifdef CONFIG_PCIEAER
> 	u16		aer_cap;	/* AER capability offset */
> 	struct aer_stats *aer_stats;	/* AER stats for this device */
> @@ -1140,6 +1141,7 @@ void pci_sort_breadthfirst(void);
> 
> u8 pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap);
> u8 pci_find_capability(struct pci_dev *dev, int cap);
> +void pci_find_all_capabilities(struct pci_dev *dev);

Shouldn't be exposed outside drivers/pci/; could move to
drivers/pci/pci.h.

> u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap);
> u8 pci_find_ht_capability(struct pci_dev *dev, int ht_cap);
> u8 pci_find_next_ht_capability(struct pci_dev *dev, u8 pos, int ht_cap);
> --
> 2.30.0
>
Vikash Bansal Feb. 3, 2022, 5:52 a.m. UTC | #2 
On 03/02/22, 3:05 AM, "Bjorn Helgaas" <helgaas@kernel.org> wrote:

    > On Fri, Jan 28, 2022 at 04:51:43AM +0000, Vikash Bansal wrote:

    Note: I am switching employer so from tomorrow onwards my VMWare ID
    "bvikas@vmware.com" will not be reachable. I will be reachable on my
    Personal id "ymcavikas@gmail.com". You may see new patches on this thread
    From one of my colleague from in VMWare.
    
    More comments below.

    >> In the current implementation, the PCI capability list is parsed from
    >> the beginning to find each capability, which results in a large number
    >> of redundant PCI reads.
    >> 
    >> Instead, we can parse the complete list just once, store it in the
    >> pci_dev structure, and get the offset of each capability directly from
    >> the pci_dev structure.
    >> 
    >> This implementation improves pci devices initialization time  by ~2-3%
    >> (from 270ms to 261ms) in case of bare metal and 7-8% (From 201ms to 184ms)
    >> in case of VM running on ESXi.
    >> 
    >> It also adds a memory overhead of 20bytes (value of PCI_CAP_ID_MAX) per
    >> PCI device.
    >> 
    >> Ran pahole for pci_dev structure. This patch is not adding any padding
    >> bytes.
    >
    > I like the idea of this.
    >
    > I guess the corresponding thing could be done for extended
    > capabilities as well, but they're not as common, there are more of
    > them, and maybe the benefit wouldn't be worth it.
    >
    > More comments below.
    >

    You are right, this thing can be done for extended capability.
    We can have similar patch for extended capability also in future.

    More comment below.

    >> Signed-off-by: Vikash Bansal <bvikas@vmware.com>
    >> ---
    >> 
    >> Changes in v2:
    >>   - Ran pahole tool.
    >>   - Modified comments to add "clock time".
    >>   - Removed comments before call to pci_find_all_capabilities.
    >> 
    >> drivers/pci/pci.c   | 43 ++++++++++++++++++++++++++++++++++++-------
    >> drivers/pci/probe.c |  1 +
    >> include/linux/pci.h |  2 ++
    >> 3 files changed, 39 insertions(+), 7 deletions(-)
    >> 
    >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
    >> index 9ecce435fb3f..b361788bcc27 100644
    >> --- a/drivers/pci/pci.c
    >> +++ b/drivers/pci/pci.c
    >> @@ -468,6 +468,41 @@ static u8 __pci_bus_find_cap_start(struct pci_bus *bus,
    >> 	return 0;
    >> }
    >> 
    >> +
    >> +/**
    >> + * pci_find_all_capabilities - Read all capabilities
    >> + * @dev: the PCI device
    >> + *
    >> + * Read all capabilities and store offsets in cap_off
    >> + * array in pci_dev structure.
    >> + */
    >> +void pci_find_all_capabilities(struct pci_dev *dev)
    >> +{
    >> +	int ttl = PCI_FIND_CAP_TTL;
    >> +	u16 ent;
    >> +	u8 pos;
    >> +	u8 id;
    >> +
    >> +	pos = __pci_bus_find_cap_start(dev->bus, dev->devfn, dev->hdr_type);
    >> +	if (!pos)
    >> +		return;
    >> +	pci_bus_read_config_byte(dev->bus, dev->devfn, pos, &pos);
    >> +	while (ttl--) {
    >> +		if (pos < 0x40)
    >> +			break;
    >> +		pos &= ~3;
    >> +		pci_bus_read_config_word(dev->bus, dev->devfn, pos, &ent);
    >> +		id = ent & 0xff;
    >> +		if (id == 0xff)
    >> +			break;
    >> +
    >> +		/* Read first instance of capability */
    >> +		if (!(dev->cap_off[id]))
    >> +			dev->cap_off[id] = pos;
    >> +		pos = (ent >> 8);
    >> +	}
    >> +}
    >
    > With the current capability infrastructure, one has to specify the
    > capability of interest up front, so there's no way to iterate directly
    > through all the capabilities.  Hence this code that looks a lot like
    > __pci_find_next_cap_ttl(), which knows the capability to look for and
    > ignores any others.
    >

    Yes, I have taken login for pci_find_all_capabilities from "__pci_find_next_cap_tt ".
    
    More comments below.

    >> +
    >> /**
    >>   * pci_find_capability - query for devices' capabilities
    >>   * @dev: PCI device to query
    >> @@ -489,13 +524,7 @@ static u8 __pci_bus_find_cap_start(struct pci_bus *bus,
    >>   */
    >> u8 pci_find_capability(struct pci_dev *dev, int cap)
    >> {
    >> -	u8 pos;
    >> -
    >> -	pos = __pci_bus_find_cap_start(dev->bus, dev->devfn, dev->hdr_type);
    >> -	if (pos)
    >> -		pos = __pci_find_next_cap(dev->bus, dev->devfn, pos, cap);
    >> -
    >> -	return pos;
    >> +	return dev->cap_off[cap];
    >
    > This may index past the end of the cap_off[] table.  We have no
    > Control over the callers, so no way to prevent them from reading
    > arbitrary memory here.
    >

    We can add below condition to prevent this

     u8 pci_find_capability(struct pci_dev *dev, int cap)
     {
     +	if(cap > PCI_CAP_ID_MAX)
     +		return 0;
      	return dev->cap_off[cap];
     }

    More comments below.

    > Here's an alternate proposal:
    >
    >  - Leave pci_find_capability() completely alone.
    >
    >  - Don't add the cap_off[] table.  This only works for Capabilities
    >   (not Extended Capabilities), depends on PCI_CAP_ID_MAX (which is
    >    not fixed and could change in the future), and duplicates existing
    >    data like dev->pcie_cap, dev->msi_cap, dev->msix_cap, etc.
    >
    >  - Add pci_find_all_capabilities() (I'd probably call it
    >    "pci_scan_capabilities()"), but extend it to iterate through both
    >    kinds of capabilities (probably a helper function for each kind)
    >    and fill in dev->pcie_cap, dev->msi_cap, dev->aer_cap, etc as it
    >    finds them.
    >
    >  - Follow up with patches that remove calls to pci_find_capability()
    >    and pci_find_ext_capability() for the capabilities we've already
    >    located.
    >

    I thought of having solution more generic. Using dev->pcie_cap,
    dev->msi_cap, dev->aer_cap will take care of only 3 capabilities.
    cap_off[] array will handle all equal to PCI_CAP_ID_MAX.

    More comments below.

    >> }
    >> EXPORT_SYMBOL(pci_find_capability);
    >> 
    >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
    >> index 17a969942d37..b2fa5b2c42f6 100644
    >> --- a/drivers/pci/probe.c
    >> +++ b/drivers/pci/probe.c
    >> @@ -1831,6 +1831,7 @@ int pci_setup_device(struct pci_dev *dev)
    >> 	dev->hdr_type = hdr_type & 0x7f;
    >> 	dev->multifunction = !!(hdr_type & 0x80);
    >> 	dev->error_state = pci_channel_io_normal;
    >> +	pci_find_all_capabilities(dev);
    >> 	set_pcie_port_type(dev);
    >> 
    >> 	pci_set_of_node(dev);
    >> diff --git a/include/linux/pci.h b/include/linux/pci.h
    >> index 8253a5413d7c..abcf7fdc4c98 100644
    >> --- a/include/linux/pci.h
    >> +++ b/include/linux/pci.h
    >> @@ -335,6 +335,7 @@ struct pci_dev {
    >> 	unsigned int	class;		/* 3 bytes: (base,sub,prog-if) */
    >> 	u8		revision;	/* PCI revision, low byte of class word */
    >> 	u8		hdr_type;	/* PCI header type (`multi' flag masked out) */
    >> +	u8              cap_off[PCI_CAP_ID_MAX]; /* Offsets of all pci capabilities */
    >> #ifdef CONFIG_PCIEAER
    >> 	u16		aer_cap;	/* AER capability offset */
    >> 	struct aer_stats *aer_stats;	/* AER stats for this device */
    >> @@ -1140,6 +1141,7 @@ void pci_sort_breadthfirst(void);
    >> 
    >> u8 pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap);
    >> u8 pci_find_capability(struct pci_dev *dev, int cap);
    >> +void pci_find_all_capabilities(struct pci_dev *dev);
    >
    > Shouldn't be exposed outside drivers/pci/; could move to
    > drivers/pci/pci.h.
    >

    Will change this.

    >> u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap);
    >> u8 pci_find_ht_capability(struct pci_dev *dev, int ht_cap);
    >> u8 pci_find_next_ht_capability(struct pci_dev *dev, u8 pos, int ht_cap);
    >> --
    >> 2.30.0
    >>
diff mbox series

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 9ecce435fb3f..b361788bcc27 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -468,6 +468,41 @@  static u8 __pci_bus_find_cap_start(struct pci_bus *bus,
	return 0;
}

+
+/**
+ * pci_find_all_capabilities - Read all capabilities
+ * @dev: the PCI device
+ *
+ * Read all capabilities and store offsets in cap_off
+ * array in pci_dev structure.
+ */
+void pci_find_all_capabilities(struct pci_dev *dev)
+{
+	int ttl = PCI_FIND_CAP_TTL;
+	u16 ent;
+	u8 pos;
+	u8 id;
+
+	pos = __pci_bus_find_cap_start(dev->bus, dev->devfn, dev->hdr_type);
+	if (!pos)
+		return;
+	pci_bus_read_config_byte(dev->bus, dev->devfn, pos, &pos);
+	while (ttl--) {
+		if (pos < 0x40)
+			break;
+		pos &= ~3;
+		pci_bus_read_config_word(dev->bus, dev->devfn, pos, &ent);
+		id = ent & 0xff;
+		if (id == 0xff)
+			break;
+
+		/* Read first instance of capability */
+		if (!(dev->cap_off[id]))
+			dev->cap_off[id] = pos;
+		pos = (ent >> 8);
+	}
+}
+
/**
  * pci_find_capability - query for devices' capabilities
  * @dev: PCI device to query
@@ -489,13 +524,7 @@  static u8 __pci_bus_find_cap_start(struct pci_bus *bus,
  */
u8 pci_find_capability(struct pci_dev *dev, int cap)
{
-	u8 pos;
-
-	pos = __pci_bus_find_cap_start(dev->bus, dev->devfn, dev->hdr_type);
-	if (pos)
-		pos = __pci_find_next_cap(dev->bus, dev->devfn, pos, cap);
-
-	return pos;
+	return dev->cap_off[cap];
}
EXPORT_SYMBOL(pci_find_capability);

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 17a969942d37..b2fa5b2c42f6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1831,6 +1831,7 @@  int pci_setup_device(struct pci_dev *dev)
	dev->hdr_type = hdr_type & 0x7f;
	dev->multifunction = !!(hdr_type & 0x80);
	dev->error_state = pci_channel_io_normal;
+	pci_find_all_capabilities(dev);
	set_pcie_port_type(dev);

	pci_set_of_node(dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8253a5413d7c..abcf7fdc4c98 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -335,6 +335,7 @@  struct pci_dev {
	unsigned int	class;		/* 3 bytes: (base,sub,prog-if) */
	u8		revision;	/* PCI revision, low byte of class word */
	u8		hdr_type;	/* PCI header type (`multi' flag masked out) */
+	u8              cap_off[PCI_CAP_ID_MAX]; /* Offsets of all pci capabilities */
#ifdef CONFIG_PCIEAER
	u16		aer_cap;	/* AER capability offset */
	struct aer_stats *aer_stats;	/* AER stats for this device */
@@ -1140,6 +1141,7 @@  void pci_sort_breadthfirst(void);

u8 pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap);
u8 pci_find_capability(struct pci_dev *dev, int cap);
+void pci_find_all_capabilities(struct pci_dev *dev);
u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap);
u8 pci_find_ht_capability(struct pci_dev *dev, int ht_cap);
u8 pci_find_next_ht_capability(struct pci_dev *dev, u8 pos, int ht_cap);