| Message ID | 20240822204120.3634-1-wei.huang2@amd.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | PCIe TPH and cache direct injection support | expand |
Hi Bjorn, I have incorporated Jakub's feedback on patch 11 (i.e. bnxt.c) in my new revision. Do you have additional comments or suggestions on V4 to include in the next spin? Thanks, -Wei On 8/22/24 3:41 PM, Wei Huang wrote: > Hi All, > > TPH (TLP Processing Hints) is a PCIe feature that allows endpoint > devices to provide optimization hints for requests that target memory > space. These hints, in a format called steering tag (ST), are provided > in the requester's TLP headers and allow the system hardware, including > the Root Complex, to optimize the utilization of platform resources > for the requests. > > Upcoming AMD hardware implement a new Cache Injection feature that > leverages TPH. Cache Injection allows PCIe endpoints to inject I/O > Coherent DMA writes directly into an L2 within the CCX (core complex) > closest to the CPU core that will consume it. This technology is aimed > at applications requiring high performance and low latency, such as > networking and storage applications. > > This series introduces generic TPH support in Linux, allowing STs to be > retrieved and used by PCIe endpoint drivers as needed. As a > demonstration, it includes an example usage in the Broadcom BNXT driver. > When running on Broadcom NICs with the appropriate firmware, it shows > substantial memory bandwidth savings and better network bandwidth using > real-world benchmarks. This solution is vendor-neutral and implemented > based on industry standards (PCIe Spec and PCI FW Spec). > > V3->V4: > * Rebase on top of the latest pci/next tree (tag: 6.11-rc1) > * Add new API functioins to query/enable/disable TPH support > * Make pcie_tph_set_st() completely independent from pcie_tph_get_cpu_st() > * Rewrite bnxt.c based on new APIs > * Remove documentation for now due to constantly changing API > * Remove pci=notph, but keep pci=nostmode with better flow (Bjorn) > * Lots of code rewrite in tph.c & pci-tph.h with cleaner interface (Bjorn) > * Add TPH save/restore support (Paul Luse and Lukas Wunner) > > V2->V3: > * Rebase on top of pci/next tree (tag: pci-v6.11-changes) > * Redefine PCI TPH registers (pci_regs.h) without breaking uapi > * Fix commit subjects/messages for kernel options (Jonathan and Bjorn) > * Break API functions into three individual patches for easy review > * Rewrite lots of code in tph.c/tph.h based (Jonathan and Bjorn) > > V1->V2: > * Rebase on top of pci.git/for-linus (6.10-rc1) > * Address mismatched data types reported by Sparse (Sparse check passed) > * Add pcie_tph_intr_vec_supported() for checking IRQ mode support > * Skip bnxt affinity notifier registration if > pcie_tph_intr_vec_supported()=false > * Minor fixes in bnxt driver (i.e. warning messages) > > Manoj Panicker (1): > bnxt_en: Add TPH support in BNXT driver > > Michael Chan (1): > bnxt_en: Pass NQ ID to the FW when allocating RX/RX AGG rings > > Paul Luse (1): > PCI/TPH: Add save/restore support for TPH > > Wei Huang (9): > PCI: Introduce PCIe TPH support framework > PCI: Add TPH related register definition > PCI/TPH: Add pcie_tph_modes() to query TPH modes > PCI/TPH: Add pcie_enable_tph() to enable TPH > PCI/TPH: Add pcie_disable_tph() to disable TPH > PCI/TPH: Add pcie_tph_enabled() to check TPH state > PCI/TPH: Add pcie_tph_set_st_entry() to set ST tag > PCI/TPH: Add pcie_tph_get_cpu_st() to get ST tag > PCI/TPH: Add pci=nostmode to force TPH No ST Mode > > .../admin-guide/kernel-parameters.txt | 3 + > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 86 ++- > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 4 + > drivers/pci/pci.c | 4 + > drivers/pci/pci.h | 12 + > drivers/pci/pcie/Kconfig | 11 + > drivers/pci/pcie/Makefile | 1 + > drivers/pci/pcie/tph.c | 563 ++++++++++++++++++ > drivers/pci/probe.c | 1 + > include/linux/pci-tph.h | 48 ++ > include/linux/pci.h | 7 + > include/uapi/linux/pci_regs.h | 38 +- > 12 files changed, 768 insertions(+), 10 deletions(-) > create mode 100644 drivers/pci/pcie/tph.c > create mode 100644 include/linux/pci-tph.h >
On Thu, Aug 22, 2024 at 03:41:08PM -0500, Wei Huang wrote: > Hi All, > > TPH (TLP Processing Hints) is a PCIe feature that allows endpoint > devices to provide optimization hints for requests that target memory > space. These hints, in a format called steering tag (ST), are provided > in the requester's TLP headers and allow the system hardware, including > the Root Complex, to optimize the utilization of platform resources > for the requests. > > Upcoming AMD hardware implement a new Cache Injection feature that > leverages TPH. Cache Injection allows PCIe endpoints to inject I/O > Coherent DMA writes directly into an L2 within the CCX (core complex) > closest to the CPU core that will consume it. This technology is aimed > at applications requiring high performance and low latency, such as > networking and storage applications. Thanks for this example, it's a great intro. Suggest adding something similar to a patch commit log, since the cover letter is harder to find after this appears in git. > This series introduces generic TPH support in Linux, allowing STs to be > retrieved and used by PCIe endpoint drivers as needed. As a > demonstration, it includes an example usage in the Broadcom BNXT driver. > When running on Broadcom NICs with the appropriate firmware, it shows > substantial memory bandwidth savings and better network bandwidth using > real-world benchmarks. This solution is vendor-neutral and implemented > based on industry standards (PCIe Spec and PCI FW Spec). > > V3->V4: > * Rebase on top of the latest pci/next tree (tag: 6.11-rc1) No need to rebase to pci/next; pci/main is where it will be applied. But it currently applies cleanly to either, so no problem. > * Add new API functioins to query/enable/disable TPH support > * Make pcie_tph_set_st() completely independent from pcie_tph_get_cpu_st() > * Rewrite bnxt.c based on new APIs > * Remove documentation for now due to constantly changing API I'd like to see this documentation included. And updated if the API changes, of course. > * Remove pci=notph, but keep pci=nostmode with better flow (Bjorn) This seems backward to me. I think "pci=notph" makes sense as a way to completely disable the TPH feature in case a user trips over a hardware or driver defect. But "pci=nostmode" is advertised as a way to quantify the benefit of Steering Tags, and that seems like it's of interest to developers but not users. So my advice would be to keep "pci=notph" and drop "pci=nostmode". Bjorn
On 9/4/24 13:49, Bjorn Helgaas wrote: > On Thu, Aug 22, 2024 at 03:41:08PM -0500, Wei Huang wrote: >> Hi All, >> >> TPH (TLP Processing Hints) is a PCIe feature that allows endpoint >> devices to provide optimization hints for requests that target memory >> space. These hints, in a format called steering tag (ST), are provided >> in the requester's TLP headers and allow the system hardware, including >> the Root Complex, to optimize the utilization of platform resources >> for the requests. >> >> Upcoming AMD hardware implement a new Cache Injection feature that >> leverages TPH. Cache Injection allows PCIe endpoints to inject I/O >> Coherent DMA writes directly into an L2 within the CCX (core complex) >> closest to the CPU core that will consume it. This technology is aimed >> at applications requiring high performance and low latency, such as >> networking and storage applications. > > Thanks for this example, it's a great intro. Suggest adding something > similar to a patch commit log, since the cover letter is harder to > find after this appears in git. I'll incorporate some of these descriptions into the TPH patches where relevant. Additionally, I'll enhance the commit log for bnxt.c (patch 11) with examples of the benefits. > >> This series introduces generic TPH support in Linux, allowing STs to be >> retrieved and used by PCIe endpoint drivers as needed. As a >> demonstration, it includes an example usage in the Broadcom BNXT driver. >> When running on Broadcom NICs with the appropriate firmware, it shows >> substantial memory bandwidth savings and better network bandwidth using >> real-world benchmarks. This solution is vendor-neutral and implemented >> based on industry standards (PCIe Spec and PCI FW Spec). >> >> V3->V4: >> * Rebase on top of the latest pci/next tree (tag: 6.11-rc1) > > No need to rebase to pci/next; pci/main is where it will be applied. > But it currently applies cleanly to either, so no problem. Got it, will rebase to pci/main in next spin anyway. > >> * Add new API functioins to query/enable/disable TPH support >> * Make pcie_tph_set_st() completely independent from pcie_tph_get_cpu_st() >> * Rewrite bnxt.c based on new APIs >> * Remove documentation for now due to constantly changing API > > I'd like to see this documentation included. And updated if the API > changes, of course. Will do. > >> * Remove pci=notph, but keep pci=nostmode with better flow (Bjorn) > > This seems backward to me. I think "pci=notph" makes sense as a way > to completely disable the TPH feature in case a user trips over a > hardware or driver defect. > > But "pci=nostmode" is advertised as a way to quantify the benefit of > Steering Tags, and that seems like it's of interest to developers but > not users. > > So my advice would be to keep "pci=notph" and drop "pci=nostmode". OK, I will replace the "nostmode" patch with "notph" in V5. > > Bjorn
On Wed, Sep 04, 2024 at 02:48:30PM -0500, Wei Huang wrote: > On 9/4/24 13:49, Bjorn Helgaas wrote: > > On Thu, Aug 22, 2024 at 03:41:08PM -0500, Wei Huang wrote: > >> Hi All, > >> > >> TPH (TLP Processing Hints) is a PCIe feature that allows endpoint > >> devices to provide optimization hints for requests that target memory > >> space. These hints, in a format called steering tag (ST), are provided > >> in the requester's TLP headers and allow the system hardware, including > >> the Root Complex, to optimize the utilization of platform resources > >> for the requests. > >> > >> Upcoming AMD hardware implement a new Cache Injection feature that > >> leverages TPH. Cache Injection allows PCIe endpoints to inject I/O > >> Coherent DMA writes directly into an L2 within the CCX (core complex) > >> closest to the CPU core that will consume it. This technology is aimed > >> at applications requiring high performance and low latency, such as > >> networking and storage applications. > > > > Thanks for this example, it's a great intro. Suggest adding something > > similar to a patch commit log, since the cover letter is harder to > > find after this appears in git. > > I'll incorporate some of these descriptions into the TPH patches where > relevant. Additionally, I'll enhance the commit log for bnxt.c (patch > 11) with examples of the benefits. Sounds good. Another confusing point that would be helpful to mention is that TPH includes two pieces: Processing Hints and Steering Tags. As far as I can see, the only architected control of Processing Hints (bi-directional, requester, target, target w/ priority) is to enable/disable TPH or Extended TPH. But we *do* have significant software control over the Steering Tags.
On Thu, Aug 22, 2024 at 03:41:08PM -0500, Wei Huang wrote: > Hi All, > > TPH (TLP Processing Hints) is a PCIe feature that allows endpoint > devices to provide optimization hints for requests that target memory > space. These hints, in a format called steering tag (ST), are provided > in the requester's TLP headers and allow the system hardware, including > the Root Complex, to optimize the utilization of platform resources > for the requests. > Paul Luse (1): > PCI/TPH: Add save/restore support for TPH > > Wei Huang (9): > PCI: Introduce PCIe TPH support framework > PCI: Add TPH related register definition > PCI/TPH: Add pcie_tph_modes() to query TPH modes > PCI/TPH: Add pcie_enable_tph() to enable TPH > PCI/TPH: Add pcie_disable_tph() to disable TPH > PCI/TPH: Add pcie_tph_enabled() to check TPH state > PCI/TPH: Add pcie_tph_set_st_entry() to set ST tag > PCI/TPH: Add pcie_tph_get_cpu_st() to get ST tag > PCI/TPH: Add pci=nostmode to force TPH No ST Mode To me, this series would make more sense if we squashed these together: PCI: Introduce PCIe TPH support framework PCI: Add TPH related register definition PCI/TPH: Add pcie_enable_tph() to enable TPH PCI/TPH: Add pcie_disable_tph() to disable TPH PCI/TPH: Add save/restore support for TPH These would add the "minimum viable functionality", e.g., enable TPH just for Processing Hints, with no Steering Tag support at all. Would also include "pci=notph". PCI/TPH: Add pcie_tph_set_st_entry() to set ST tag PCI/TPH: Add pcie_tph_get_cpu_st() to get ST tag And squash these also to add Steering Tag support in a single commit, including enhancing the save/restore. PCI/TPH: Add pcie_tph_modes() to query TPH modes PCI/TPH: Add pcie_tph_enabled() to check TPH state And maybe we can get away without these altogether. I mentioned pcie_tph_modes() elsewhere; seems possibly unnecessary since drivers can just request the mode they want and we'll fail if it's not supported. Drivers should also be able to remember whether they enabled TPH successfully without us having to remind them.
On 9/4/24 15:20, Bjorn Helgaas wrote: > To me, this series would make more sense if we squashed these > together: > > PCI: Introduce PCIe TPH support framework > PCI: Add TPH related register definition > PCI/TPH: Add pcie_enable_tph() to enable TPH > PCI/TPH: Add pcie_disable_tph() to disable TPH > PCI/TPH: Add save/restore support for TPH > > These would add the "minimum viable functionality", e.g., enable TPH > just for Processing Hints, with no Steering Tag support at all. Would > also include "pci=notph". Will do > > PCI/TPH: Add pcie_tph_set_st_entry() to set ST tag > PCI/TPH: Add pcie_tph_get_cpu_st() to get ST tag > > And squash these also to add Steering Tag support in a single commit, > including enhancing the save/restore. Can you elaborate on save/restore enhancement? Assuming that the first combined patch will have save/restore support as suggested. Are you talking about the ST entries save/restore as the enhancements (see below), because the second combined patch deals with ST? st_entry = (u16 *)cap; offset = PCI_TPH_BASE_SIZEOF; num_entries = get_st_table_size(pdev); for (i = 0; i < num_entries; i++) { pci_write_config_word(pdev, pdev->tph_cap + offset, *st_entry++); offset += sizeof(u16); } > > PCI/TPH: Add pcie_tph_modes() to query TPH modes > PCI/TPH: Add pcie_tph_enabled() to check TPH state > > And maybe we can get away without these altogether. I mentioned > pcie_tph_modes() elsewhere; seems possibly unnecessary since drivers > can just request the mode they want and we'll fail if it's not > supported. > > Drivers should also be able to remember whether they enabled TPH > successfully without us having to remind them.
On Thu, Sep 05, 2024 at 10:45:57AM -0500, Wei Huang wrote: > On 9/4/24 15:20, Bjorn Helgaas wrote: > > To me, this series would make more sense if we squashed these > > together: > > > > PCI: Introduce PCIe TPH support framework > > PCI: Add TPH related register definition > > PCI/TPH: Add pcie_enable_tph() to enable TPH > > PCI/TPH: Add pcie_disable_tph() to disable TPH > > PCI/TPH: Add save/restore support for TPH > > > > These would add the "minimum viable functionality", e.g., enable TPH > > just for Processing Hints, with no Steering Tag support at all. Would > > also include "pci=notph". > > > PCI/TPH: Add pcie_tph_set_st_entry() to set ST tag > > PCI/TPH: Add pcie_tph_get_cpu_st() to get ST tag > > > > And squash these also to add Steering Tag support in a single commit, > > including enhancing the save/restore. > > Can you elaborate on save/restore enhancement? Assuming that the first > combined patch will have save/restore support as suggested. Are you > talking about the ST entries save/restore as the enhancements (see > below), because the second combined patch deals with ST? > > st_entry = (u16 *)cap; > offset = PCI_TPH_BASE_SIZEOF; > num_entries = get_st_table_size(pdev); > for (i = 0; i < num_entries; i++) { > pci_write_config_word(pdev, pdev->tph_cap + offset, > *st_entry++); > offset += sizeof(u16); > } I meant that since the first patch knows nothing about ST, it would save/restore TPH control but not the ST entries. The second patch would add ST support and also add save/restore of the ST entries.