Messages in this thread |  | | Subject | Re: [PATCH v5 03/14] PCI: Add pcie_bandwidth_capable() to compute max supported link bandwidth | From | Tal Gilboa <> | Date | Mon, 2 Apr 2018 17:34:06 +0300 |
| |
On 4/2/2018 5:05 PM, Bjorn Helgaas wrote: > On Mon, Apr 02, 2018 at 10:34:58AM +0300, Tal Gilboa wrote: >> On 4/2/2018 3:40 AM, Bjorn Helgaas wrote: >>> On Sun, Apr 01, 2018 at 11:38:53PM +0300, Tal Gilboa wrote: >>>> On 3/31/2018 12:05 AM, Bjorn Helgaas wrote: >>>>> From: Tal Gilboa <talgi@mellanox.com> >>>>> >>>>> Add pcie_bandwidth_capable() to compute the max link bandwidth supported by >>>>> a device, based on the max link speed and width, adjusted by the encoding >>>>> overhead. >>>>> >>>>> The maximum bandwidth of the link is computed as: >>>>> >>>>> max_link_speed * max_link_width * (1 - encoding_overhead) >>>>> >>>>> The encoding overhead is about 20% for 2.5 and 5.0 GT/s links using 8b/10b >>>>> encoding, and about 1.5% for 8 GT/s or higher speed links using 128b/130b >>>>> encoding. >>>>> >>>>> Signed-off-by: Tal Gilboa <talgi@mellanox.com> >>>>> [bhelgaas: adjust for pcie_get_speed_cap() and pcie_get_width_cap() >>>>> signatures, don't export outside drivers/pci] >>>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> >>>>> Reviewed-by: Tariq Toukan <tariqt@mellanox.com> >>>>> --- >>>>> drivers/pci/pci.c | 21 +++++++++++++++++++++ >>>>> drivers/pci/pci.h | 9 +++++++++ >>>>> 2 files changed, 30 insertions(+) >>>>> >>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >>>>> index 43075be79388..9ce89e254197 100644 >>>>> --- a/drivers/pci/pci.c >>>>> +++ b/drivers/pci/pci.c >>>>> @@ -5208,6 +5208,27 @@ enum pcie_link_width pcie_get_width_cap(struct pci_dev *dev) >>>>> return PCIE_LNK_WIDTH_UNKNOWN; >>>>> } >>>>> +/** >>>>> + * pcie_bandwidth_capable - calculates a PCI device's link bandwidth capability >>>>> + * @dev: PCI device >>>>> + * @speed: storage for link speed >>>>> + * @width: storage for link width >>>>> + * >>>>> + * Calculate a PCI device's link bandwidth by querying for its link speed >>>>> + * and width, multiplying them, and applying encoding overhead. >>>>> + */ >>>>> +u32 pcie_bandwidth_capable(struct pci_dev *dev, enum pci_bus_speed *speed, >>>>> + enum pcie_link_width *width) >>>>> +{ >>>>> + *speed = pcie_get_speed_cap(dev); >>>>> + *width = pcie_get_width_cap(dev); >>>>> + >>>>> + if (*speed == PCI_SPEED_UNKNOWN || *width == PCIE_LNK_WIDTH_UNKNOWN) >>>>> + return 0; >>>>> + >>>>> + return *width * PCIE_SPEED2MBS_ENC(*speed); >>>>> +} >>>>> + >>>>> /** >>>>> * pci_select_bars - Make BAR mask from the type of resource >>>>> * @dev: the PCI device for which BAR mask is made >>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h >>>>> index 66738f1050c0..2a50172b9803 100644 >>>>> --- a/drivers/pci/pci.h >>>>> +++ b/drivers/pci/pci.h >>>>> @@ -261,8 +261,17 @@ void pci_disable_bridge_window(struct pci_dev *dev); >>>>> (speed) == PCIE_SPEED_2_5GT ? "2.5 GT/s" : \ >>>>> "Unknown speed") >>>>> +/* PCIe speed to Mb/s with encoding overhead: 20% for gen2, ~1.5% for gen3 */ >>>>> +#define PCIE_SPEED2MBS_ENC(speed) \ >>>> >>>> Missing gen4. >>> >>> I made it "gen3+". I think that's accurate, isn't it? The spec >>> doesn't seem to actually use "gen3" as a specific term, but sec 4.2.2 >>> says rates of 8 GT/s or higher (which I think includes gen3 and gen4) >>> use 128b/130b encoding. >>> >> >> I meant that PCIE_SPEED_16_0GT will return 0 from this macro since it wasn't >> added. Need to return 15754. > > Oh, duh, of course! Sorry for being dense. What about the following? > I included the calculation as opposed to just the magic numbers to try > to make it clear how they're derived. This has the disadvantage of > truncating the result instead of rounding, but I doubt that's > significant in this context. If it is, we could use the magic numbers > and put the computation in a comment.
We can always use DIV_ROUND_UP((speed * enc_nominator), enc_denominator). I think this is confusing and since this introduces a bandwidth limit I would prefer to give a wider limit than a wrong one, even it is by less than 1Mb/s. My vote is for leaving it as you wrote below.
> > Another question: we currently deal in Mb/s, not MB/s. Mb/s has the > advantage of sort of corresponding to the GT/s numbers, but using MB/s > would have the advantage of smaller numbers that match the table here: > https://en.wikipedia.org/wiki/PCI_Express#History_and_revisions, > but I don't know what's most typical in user-facing situations. > What's better?
I don't know what's better but for network devices we measure bandwidth in Gb/s, so presenting bandwidth in MB/s would mean additional calculations. The truth is I would have prefer to use Gb/s instead of Mb/s, but again, don't want to loss up to 1Gb/s.
> > > commit 946435491b35b7782157e9a4d1bd73071fba7709 > Author: Tal Gilboa <talgi@mellanox.com> > Date: Fri Mar 30 08:32:03 2018 -0500 > > PCI: Add pcie_bandwidth_capable() to compute max supported link bandwidth > > Add pcie_bandwidth_capable() to compute the max link bandwidth supported by > a device, based on the max link speed and width, adjusted by the encoding > overhead. > > The maximum bandwidth of the link is computed as: > > max_link_width * max_link_speed * (1 - encoding_overhead) > > 2.5 and 5.0 GT/s links use 8b/10b encoding, which reduces the raw bandwidth > available by 20%; 8.0 GT/s and faster links use 128b/130b encoding, which > reduces it by about 1.5%. > > The result is in Mb/s, i.e., megabits/second, of raw bandwidth. > > Signed-off-by: Tal Gilboa <talgi@mellanox.com> > [bhelgaas: add 16 GT/s, adjust for pcie_get_speed_cap() and > pcie_get_width_cap() signatures, don't export outside drivers/pci] > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > Reviewed-by: Tariq Toukan <tariqt@mellanox.com> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 43075be79388..ff1e72060952 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -5208,6 +5208,28 @@ enum pcie_link_width pcie_get_width_cap(struct pci_dev *dev) > return PCIE_LNK_WIDTH_UNKNOWN; > } > > +/** > + * pcie_bandwidth_capable - calculate a PCI device's link bandwidth capability > + * @dev: PCI device > + * @speed: storage for link speed > + * @width: storage for link width > + * > + * Calculate a PCI device's link bandwidth by querying for its link speed > + * and width, multiplying them, and applying encoding overhead. The result > + * is in Mb/s, i.e., megabits/second of raw bandwidth. > + */ > +u32 pcie_bandwidth_capable(struct pci_dev *dev, enum pci_bus_speed *speed, > + enum pcie_link_width *width) > +{ > + *speed = pcie_get_speed_cap(dev); > + *width = pcie_get_width_cap(dev); > + > + if (*speed == PCI_SPEED_UNKNOWN || *width == PCIE_LNK_WIDTH_UNKNOWN) > + return 0; > + > + return *width * PCIE_SPEED2MBS_ENC(*speed); > +} > + > /** > * pci_select_bars - Make BAR mask from the type of resource > * @dev: the PCI device for which BAR mask is made > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 66738f1050c0..37f9299ed623 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -261,8 +261,18 @@ void pci_disable_bridge_window(struct pci_dev *dev); > (speed) == PCIE_SPEED_2_5GT ? "2.5 GT/s" : \ > "Unknown speed") > > +/* PCIe speed to Mb/s reduced by encoding overhead */ > +#define PCIE_SPEED2MBS_ENC(speed) \ > + ((speed) == PCIE_SPEED_16_0GT ? (16000*(128/130)) : \ > + (speed) == PCIE_SPEED_8_0GT ? (8000*(128/130)) : \ > + (speed) == PCIE_SPEED_5_0GT ? (5000*(8/10)) : \ > + (speed) == PCIE_SPEED_2_5GT ? (2500*(8/10)) : \ > + 0) > + > enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev); > enum pcie_link_width pcie_get_width_cap(struct pci_dev *dev); > +u32 pcie_bandwidth_capable(struct pci_dev *dev, enum pci_bus_speed *speed, > + enum pcie_link_width *width); > > /* Single Root I/O Virtualization */ > struct pci_sriov { >
|  |