* [PATCH v2] PCI: Speed up device init by parsing capabilities all at once @ 2022-01-28 4:51 Vikash Bansal 2022-02-02 21:35 ` Bjorn Helgaas 0 siblings, 1 reply; 6+ messages in thread From: Vikash Bansal @ 2022-01-28 4:51 UTC (permalink / raw) To: bhelgaas Cc: linux-pci, linux-kernel, gregkh, Srivatsa Bhat, srivatsa, Alexey Makhalov, Srinidhi Rao, Vikash Bansal, Anish Swaminathan, Vasavi Sirnapalli, Ajay Kaher 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(-) 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); -- 2.30.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] PCI: Speed up device init by parsing capabilities all at once 2022-01-28 4:51 [PATCH v2] PCI: Speed up device init by parsing capabilities all at once Vikash Bansal @ 2022-02-02 21:35 ` Bjorn Helgaas 2022-02-03 5:52 ` Vikash Bansal 0 siblings, 1 reply; 6+ messages in thread From: Bjorn Helgaas @ 2022-02-02 21:35 UTC (permalink / raw) To: Vikash Bansal Cc: bhelgaas, linux-pci, linux-kernel, gregkh, Srivatsa Bhat, srivatsa, Alexey Makhalov, Srinidhi Rao, Anish Swaminathan, Vasavi Sirnapalli, Ajay Kaher 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] PCI: Speed up device init by parsing capabilities all at once 2022-02-02 21:35 ` Bjorn Helgaas @ 2022-02-03 5:52 ` Vikash Bansal 2022-02-23 11:34 ` [PATCH v3] " Ajay Kaher 0 siblings, 1 reply; 6+ messages in thread From: Vikash Bansal @ 2022-02-03 5:52 UTC (permalink / raw) To: Bjorn Helgaas Cc: bhelgaas, linux-pci, linux-kernel, gregkh, Srivatsa Bhat, srivatsa, Alexey Makhalov, Srinidhi Rao, Anish Swaminathan, Vasavi Sirnapalli, Ajay Kaher, ymcavikas 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 >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3] PCI: Speed up device init by parsing capabilities all at once 2022-02-03 5:52 ` Vikash Bansal @ 2022-02-23 11:34 ` Ajay Kaher 2022-05-05 11:27 ` Ajay Kaher 0 siblings, 1 reply; 6+ messages in thread From: Ajay Kaher @ 2022-02-23 11:34 UTC (permalink / raw) To: bhelgaas, helgaas, gregkh Cc: ymcavikas, akaher, amakhalov, anishs, linux-kernel, linux-pci, srinidhir, srivatsa, srivatsab, vsirnapalli, Vikash Bansal From: Vikash Bansal <bvikas@vmware.com> 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> Signed-off-by: Ajay Kaher <akaher@vmware.com> --- Changes in v3: - Added check before access cap_off from pci_find_capability(). - Moved declaration of pci_find_all_capabilities() to drivers/pci/pci.h. 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 | 44 +++++++++++++++++++++++++++++++++++++------- drivers/pci/pci.h | 1 + drivers/pci/probe.c | 1 + include/linux/pci.h | 1 + 4 files changed, 40 insertions(+), 7 deletions(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 3d2fb39..cf54811 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -469,6 +469,40 @@ static u8 __pci_bus_find_cap_start(struct pci_bus *bus, } /** + * 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 * @cap: capability code @@ -489,13 +523,9 @@ 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; + if(cap >= PCI_CAP_ID_MAX) + return 0; + return dev->cap_off[cap]; } EXPORT_SYMBOL(pci_find_capability); diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 3d60cab..3cb70d5 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -87,6 +87,7 @@ bool pci_bridge_d3_possible(struct pci_dev *dev); void pci_bridge_d3_update(struct pci_dev *dev); void pci_bridge_wait_for_secondary_bus(struct pci_dev *dev); void pci_bridge_reconfigure_ltr(struct pci_dev *dev); +void pci_find_all_capabilities(struct pci_dev *dev); static inline void pci_wakeup_event(struct pci_dev *dev) { diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 087d365..d75e1fa 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1839,6 +1839,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 18a75c8..bd61cee 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -326,6 +326,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 */ -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] PCI: Speed up device init by parsing capabilities all at once 2022-02-23 11:34 ` [PATCH v3] " Ajay Kaher @ 2022-05-05 11:27 ` Ajay Kaher 0 siblings, 0 replies; 6+ messages in thread From: Ajay Kaher @ 2022-05-05 11:27 UTC (permalink / raw) To: bhelgaas, helgaas, gregkh Cc: ymcavikas, Alexey Makhalov, Anish Swaminathan, linux-kernel, linux-pci, Srinidhi Rao, srivatsa, Srivatsa Bhat, Vasavi Sirnapalli On 23/02/22, 5:20 PM, "Ajay Kaher" <akaher@vmware.com> wrote: > From: Vikash Bansal <bvikas@vmware.com> Bjorn, as per your guidance we sent v3 (hope it's as per your expectation). Please let us know if you are looking for more improvement or wanted us to re-sent this patch. -Ajay > 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> > Signed-off-by: Ajay Kaher <akaher@vmware.com> > > --- > > Changes in v3: > - Added check before access cap_off from pci_find_capability(). > - Moved declaration of pci_find_all_capabilities() to drivers/pci/pci.h. > > 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 | 44 +++++++++++++++++++++++++++++++++++++------- > drivers/pci/pci.h | 1 + > drivers/pci/probe.c | 1 + > include/linux/pci.h | 1 + > 4 files changed, 40 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 3d2fb39..cf54811 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -469,6 +469,40 @@ static u8 __pci_bus_find_cap_start(struct pci_bus *bus, > } > > /** > + * 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 > * @cap: capability code > @@ -489,13 +523,9 @@ 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; > + if(cap >= PCI_CAP_ID_MAX) > + return 0; > + return dev->cap_off[cap]; > } > EXPORT_SYMBOL(pci_find_capability); > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 3d60cab..3cb70d5 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -87,6 +87,7 @@ bool pci_bridge_d3_possible(struct pci_dev *dev); > void pci_bridge_d3_update(struct pci_dev *dev); > void pci_bridge_wait_for_secondary_bus(struct pci_dev *dev); > void pci_bridge_reconfigure_ltr(struct pci_dev *dev); > +void pci_find_all_capabilities(struct pci_dev *dev); > > static inline void pci_wakeup_event(struct pci_dev *dev) > { > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 087d365..d75e1fa 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1839,6 +1839,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 18a75c8..bd61cee 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -326,6 +326,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 */ > -- > 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <1642881286-31024-1-git-send-email-bvikas@vmware.com>]
* Re: [PATCH v2] PCI: Speed up device init by parsing capabilities all at once [not found] <1642881286-31024-1-git-send-email-bvikas@vmware.com> @ 2022-01-24 10:35 ` Greg KH 0 siblings, 0 replies; 6+ messages in thread From: Greg KH @ 2022-01-24 10:35 UTC (permalink / raw) To: Vikash Bansal Cc: bhelgaas, linux-pci, linux-kernel, srivatsab, srivatsa, amakhalov, srinidhir, anishs, vsirnapalli, akaher On Sat, Jan 22, 2022 at 11:54:46AM -0800, 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. > > Run pahole for pci_dev structure. This patch is not adding any padding > bytes. > > Signed-off-by: Vikash Bansal <bvikas@vmware.com> > --- > 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); > + } > +} > + > /** > * 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); > -- > 2.30.0 > Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - This looks like a new version of a previously submitted patch, but you did not list below the --- line any changes from the previous version. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/SubmittingPatches for what needs to be done here to properly describe this. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-05 11:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-28 4:51 [PATCH v2] PCI: Speed up device init by parsing capabilities all at once Vikash Bansal
2022-02-02 21:35 ` Bjorn Helgaas
2022-02-03 5:52 ` Vikash Bansal
2022-02-23 11:34 ` [PATCH v3] " Ajay Kaher
2022-05-05 11:27 ` Ajay Kaher
[not found] <1642881286-31024-1-git-send-email-bvikas@vmware.com>
2022-01-24 10:35 ` [PATCH v2] " Greg KH
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).