[PATCH 0/9] Export PCIe bandwidth via sysfs

[PATCH 0/9] Export PCIe bandwidth via sysfs

[Alexandru Gagniuc]

This is a follow-on series to
Commit 2d1ce5ec2117 ("PCI: Check for PCIe Link downtraining")

The remaining issues was that some pcie drivers print link status directly,
sometimes resulting in duplicate system log messages with degraded links.

From my understanding, the maintainers of these drivers are fine with
removing the duplicate prints as long as the bandwidth information is
readily available. sysfs seemed to be the consensus.

Example:

	$ cat /sys/bus/pci/devices/0000:b1:00.0/available_bandwidth
	7.876 Gb/s


Alexandru Gagniuc (9):
  PCI: sysfs: Export available PCIe bandwidth
  bnx2x: Do not call pcie_print_link_status()
  bnxt_en: Do not call pcie_print_link_status()
  cxgb4: Do not call pcie_print_link_status()
  fm10k: Do not call pcie_print_link_status()
  ixgbe: Do not call pcie_print_link_status()
  net/mlx4: Do not call pcie_print_link_status()
  net/mlx5: Do not call pcie_print_link_status()
  nfp: Do not call pcie_print_link_status()

 .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  |  1 -
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  1 -
 .../net/ethernet/chelsio/cxgb4/cxgb4_main.c   |  3 ---
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c  |  3 ---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 26 -------------------
 drivers/net/ethernet/mellanox/mlx4/main.c     |  7 -----
 .../net/ethernet/mellanox/mlx5/core/main.c    |  4 ---
 .../netronome/nfp/nfpcore/nfp6000_pcie.c      |  1 -
 drivers/pci/pci-sysfs.c                       | 13 ++++++++++
 9 files changed, 13 insertions(+), 46 deletions(-)

-- 
2.17.1

[PATCH 1/9] PCI: sysfs: Export available PCIe bandwidth

[Alexandru Gagniuc]

For certain bandwidth-critical devices (e.g. multi-port network cards)
it is useful to know the available bandwidth to the root complex. This
information is only available via the system log, which doesn't
account for link degradation after probing.

With a sysfs attribute, we can computes the bandwidth on-demand, and
will detect degraded links.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/pci/pci-sysfs.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 9ecfe13157c0..6658e927b1f5 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -218,6 +218,18 @@ static ssize_t current_link_width_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(current_link_width);
 
+static ssize_t available_bandwidth_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	u32 bw_avail;
+
+	bw_avail = pcie_bandwidth_available(pci_dev, NULL, NULL, NULL);
+
+	return sprintf(buf, "%u.%03u Gb/s\n", bw_avail / 1000, bw_avail % 1000);
+}
+static DEVICE_ATTR_RO(available_bandwidth);
+
 static ssize_t secondary_bus_number_show(struct device *dev,
 					 struct device_attribute *attr,
 					 char *buf)
@@ -786,6 +798,7 @@ static struct attribute *pcie_dev_attrs[] = {
 	&dev_attr_current_link_width.attr,
 	&dev_attr_max_link_width.attr,
 	&dev_attr_max_link_speed.attr,
+	&dev_attr_available_bandwidth.attr,
 	NULL,
 };
 
-- 
2.17.1

[PATCH 2/9] bnx2x: Do not call pcie_print_link_status()

[Alexandru Gagniuc]

This is now done by the PCI core to warn of sub-optimal bandwidth.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 71362b7f6040..9bd0852d9a66 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -14100,7 +14100,6 @@ static int bnx2x_init_one(struct pci_dev *pdev,
 	       board_info[ent->driver_data].name,
 	       (CHIP_REV(bp) >> 12) + 'A', (CHIP_METAL(bp) >> 4),
 	       dev->base_addr, bp->pdev->irq, dev->dev_addr);
-	pcie_print_link_status(bp->pdev);
 
 	bnx2x_register_phc(bp);
 
-- 
2.17.1

[PATCH 3/9] bnxt_en: Do not call pcie_print_link_status()

[Alexandru Gagniuc]

This is now done by the PCI core to warn of sub-optimal bandwidth.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 8bb1e38b1681..246f33d2cc9c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -9046,7 +9046,6 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	netdev_info(dev, "%s found at mem %lx, node addr %pM\n",
 		    board_info[ent->driver_data].name,
 		    (long)pci_resource_start(pdev, 0), dev->dev_addr);
-	pcie_print_link_status(pdev);
 
 	return 0;
 
-- 
2.17.1

[PATCH 4/9] cxgb4: Do not call pcie_print_link_status()

[Alexandru Gagniuc]

This is now done by the PCI core to warn of sub-optimal bandwidth.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 961e3087d1d3..1deb68c99a63 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -5782,9 +5782,6 @@ static int init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 			free_msix_info(adapter);
 	}
 
-	/* check for PCI Express bandwidth capabiltites */
-	pcie_print_link_status(pdev);
-
 	err = init_rss(adapter);
 	if (err)
 		goto out_free_dev;
-- 
2.17.1

[PATCH 5/9] fm10k: Do not call pcie_print_link_status()

[Alexandru Gagniuc]

This is now done by the PCI core to warn of sub-optimal bandwidth.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/net/ethernet/intel/fm10k/fm10k_pci.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
index 15071e4adb98..079fd3c884ea 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_pci.c
@@ -2224,9 +2224,6 @@ static int fm10k_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* kick off service timer now, even when interface is down */
 	mod_timer(&interface->service_timer, (HZ * 2) + jiffies);
 
-	/* print warning for non-optimal configurations */
-	pcie_print_link_status(interface->pdev);
-
 	/* report MAC address for logging */
 	dev_info(&pdev->dev, "%pM\n", netdev->dev_addr);
 
-- 
2.17.1

[PATCH 6/9] ixgbe: Do not call pcie_print_link_status()

[Alexandru Gagniuc]

This is now done by the PCI core to warn of sub-optimal bandwidth.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 26 -------------------
 1 file changed, 26 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 9a23d33a47ed..9663419e0ceb 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -241,28 +241,6 @@ static inline bool ixgbe_pcie_from_parent(struct ixgbe_hw *hw)
 	}
 }
 
-static void ixgbe_check_minimum_link(struct ixgbe_adapter *adapter,
-				     int expected_gts)
-{
-	struct ixgbe_hw *hw = &adapter->hw;
-	struct pci_dev *pdev;
-
-	/* Some devices are not connected over PCIe and thus do not negotiate
-	 * speed. These devices do not have valid bus info, and thus any report
-	 * we generate may not be correct.
-	 */
-	if (hw->bus.type == ixgbe_bus_type_internal)
-		return;
-
-	/* determine whether to use the parent device */
-	if (ixgbe_pcie_from_parent(&adapter->hw))
-		pdev = adapter->pdev->bus->parent->self;
-	else
-		pdev = adapter->pdev;
-
-	pcie_print_link_status(pdev);
-}
-
 static void ixgbe_service_event_schedule(struct ixgbe_adapter *adapter)
 {
 	if (!test_bit(__IXGBE_DOWN, &adapter->state) &&
@@ -10792,10 +10770,6 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		break;
 	}
 
-	/* don't check link if we failed to enumerate functions */
-	if (expected_gts > 0)
-		ixgbe_check_minimum_link(adapter, expected_gts);
-
 	err = ixgbe_read_pba_string_generic(hw, part_str, sizeof(part_str));
 	if (err)
 		strlcpy(part_str, "Unknown", sizeof(part_str));
-- 
2.17.1

[PATCH 7/9] net/mlx4: Do not call pcie_print_link_status()

[Alexandru Gagniuc]

This is now done by the PCI core to warn of sub-optimal bandwidth.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/net/ethernet/mellanox/mlx4/main.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
index d2d59444f562..9902fa3a2c13 100644
--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -3525,13 +3525,6 @@ static int mlx4_load_one(struct pci_dev *pdev, int pci_dev_data,
 		}
 	}
 
-	/* check if the device is functioning at its maximum possible speed.
-	 * No return code for this call, just warn the user in case of PCI
-	 * express device capabilities are under-satisfied by the bus.
-	 */
-	if (!mlx4_is_slave(dev))
-		pcie_print_link_status(dev->persist->pdev);
-
 	/* In master functions, the communication channel must be initialized
 	 * after obtaining its address from fw */
 	if (mlx4_is_master(dev)) {
-- 
2.17.1

[PATCH 8/9] net/mlx5: Do not call pcie_print_link_status()

[Alexandru Gagniuc]

This is now done by the PCI core to warn of sub-optimal bandwidth.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/main.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index cf3e4a659052..888af98694f8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -1056,10 +1056,6 @@ static int mlx5_load_one(struct mlx5_core_dev *dev, struct mlx5_priv *priv,
 	dev_info(&pdev->dev, "firmware version: %d.%d.%d\n", fw_rev_maj(dev),
 		 fw_rev_min(dev), fw_rev_sub(dev));
 
-	/* Only PFs hold the relevant PCIe information for this query */
-	if (mlx5_core_is_pf(dev))
-		pcie_print_link_status(dev->pdev);
-
 	/* on load removing any previous indication of internal error, device is
 	 * up
 	 */
-- 
2.17.1

[PATCH 9/9] nfp: Do not call pcie_print_link_status()

[Alexandru Gagniuc]

This is now done by the PCI core to warn of sub-optimal bandwidth.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c
index c8d0b1016a64..87dde0f787e9 100644
--- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c
+++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c
@@ -1328,7 +1328,6 @@ struct nfp_cpp *nfp_cpp_from_nfp6000_pcie(struct pci_dev *pdev)
 	/*  Finished with card initialization. */
 	dev_info(&pdev->dev,
 		 "Netronome Flow Processor NFP4000/NFP6000 PCIe Card Probe\n");
-	pcie_print_link_status(pdev);
 
 	nfp = kzalloc(sizeof(*nfp), GFP_KERNEL);
 	if (!nfp) {
-- 
2.17.1

Re: [PATCH 7/9] net/mlx4: Do not call pcie_print_link_status()

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]

On Mon, Sep 03, 2018 at 01:02:34PM -0500, Alexandru Gagniuc wrote:
> This is now done by the PCI core to warn of sub-optimal bandwidth.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  drivers/net/ethernet/mellanox/mlx4/main.c | 7 -------
>  1 file changed, 7 deletions(-)
>

Thanks,
Acked-by: Leon Romanovsky <leonro@mellanox.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 8/9] net/mlx5: Do not call pcie_print_link_status()

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]

On Mon, Sep 03, 2018 at 01:02:35PM -0500, Alexandru Gagniuc wrote:
> This is now done by the PCI core to warn of sub-optimal bandwidth.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/main.c | 4 ----
>  1 file changed, 4 deletions(-)
>

Thanks,
Acked-by: Leon Romanovsky <leonro@mellanox.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/9] PCI: sysfs: Export available PCIe bandwidth

[Stephen Hemminger]

On Mon,  3 Sep 2018 13:02:28 -0500
Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:

> For certain bandwidth-critical devices (e.g. multi-port network cards)
> it is useful to know the available bandwidth to the root complex. This
> information is only available via the system log, which doesn't
> account for link degradation after probing.
> 
> With a sysfs attribute, we can computes the bandwidth on-demand, and
> will detect degraded links.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>

In other places (like /sys/class/net/eth0/speed) only the raw value is printed
without suffix.  The general convention in sysfs is that it should be one value
per file and in more raw format.  So why not just print it in bits/sec without
suffix?

Re: [PATCH 1/9] PCI: sysfs: Export available PCIe bandwidth

[Alex G.]

On 09/05/2018 02:26 AM, Stephen Hemminger wrote:
> On Mon,  3 Sep 2018 13:02:28 -0500
> Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
> 
>> For certain bandwidth-critical devices (e.g. multi-port network cards)
>> it is useful to know the available bandwidth to the root complex. This
>> information is only available via the system log, which doesn't
>> account for link degradation after probing.
>>
>> With a sysfs attribute, we can computes the bandwidth on-demand, and
>> will detect degraded links.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> 
> In other places (like /sys/class/net/eth0/speed) only the raw value is printed
> without suffix.  The general convention in sysfs is that it should be one value
> per file and in more raw format.  So why not just print it in bits/sec without
> suffix?

I wanted to be consistent with other PCIe exports that use units.For 
example:

/sys/devices/pci0000:3a/0000:3a:00.0/0000:3b:00.0/0000:3c:05.0/max_link_speed:8 
GT/s
/sys/devices/pci0000:3a/0000:3a:00.0/0000:3b:00.0/0000:3c:05.0/current_link_speed:2.5 
GT/s
/sys/bus/pci/slots/182/cur_bus_speed:2.5 GT/s PCIe
/sys/bus/pci/slots/182/max_bus_speed:8.0 GT/s PCIe

Re: [PATCH 1/9] PCI: sysfs: Export available PCIe bandwidth

[Bjorn Helgaas]

[+cc Stephen, Martin (for possible lspci changes)]

Hi Alexandru,

On Mon, Sep 03, 2018 at 01:02:28PM -0500, Alexandru Gagniuc wrote:
> For certain bandwidth-critical devices (e.g. multi-port network cards)
> it is useful to know the available bandwidth to the root complex. This
> information is only available via the system log, which doesn't
> account for link degradation after probing.
> 
> With a sysfs attribute, we can computes the bandwidth on-demand, and
> will detect degraded links.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  drivers/pci/pci-sysfs.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 9ecfe13157c0..6658e927b1f5 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -218,6 +218,18 @@ static ssize_t current_link_width_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(current_link_width);
>  
> +static ssize_t available_bandwidth_show(struct device *dev,
> +				       struct device_attribute *attr, char *buf)
> +{
> +	struct pci_dev *pci_dev = to_pci_dev(dev);
> +	u32 bw_avail;
> +
> +	bw_avail = pcie_bandwidth_available(pci_dev, NULL, NULL, NULL);
> +
> +	return sprintf(buf, "%u.%03u Gb/s\n", bw_avail / 1000, bw_avail % 1000);
> +}
> +static DEVICE_ATTR_RO(available_bandwidth);

Help me understand this.  We already have these sysfs attributes:

  max_link_speed          # eg, 16 GT/s
  max_link_width          # eg, 8
  current_link_speed      # eg, 16 GT/s
  current_link_width      # eg, 8

so I think the raw materials are already exposed.

The benefits I see for this new file are that

  - pcie_bandwidth_available() does the work of traversing up the
    tree, doing the computations (link width * speed, reduced by
    encoding overhead), and finding the minimum, and

  - it re-traverses the path every time we look at it, while the
    boot-time check is a one-time event.

In principle this could all be done in user space with the attributes
that are already exported.  There's some precedent for things like
this in lspci, e.g., "NUMA node" [1], and lspci might even be a more
user-friendly place for users to look for this, as opposed to
searching through sysfs.

[1] https://git.kernel.org/pub/scm/utils/pciutils/pciutils.git/commit/?id=90ec4a6d0ae8

>  static ssize_t secondary_bus_number_show(struct device *dev,
>  					 struct device_attribute *attr,
>  					 char *buf)
> @@ -786,6 +798,7 @@ static struct attribute *pcie_dev_attrs[] = {
>  	&dev_attr_current_link_width.attr,
>  	&dev_attr_max_link_width.attr,
>  	&dev_attr_max_link_speed.attr,
> +	&dev_attr_available_bandwidth.attr,
>  	NULL,
>  };
>  
> -- 
> 2.17.1
> 

Re: [PATCH 1/9] PCI: sysfs: Export available PCIe bandwidth

[Alex_Gagniuc]

On 10/03/2018 04:31 PM, Bjorn Helgaas wrote:
> 
> [EXTERNAL EMAIL]
> Please report any suspicious attachments, links, or requests for sensitive information.
> 
> 
> [+cc Stephen, Martin (for possible lspci changes)]
> 
> Hi Alexandru,
> 
> On Mon, Sep 03, 2018 at 01:02:28PM -0500, Alexandru Gagniuc wrote:
>> For certain bandwidth-critical devices (e.g. multi-port network cards)
>> it is useful to know the available bandwidth to the root complex. This
>> information is only available via the system log, which doesn't
>> account for link degradation after probing.
>>
>> With a sysfs attribute, we can computes the bandwidth on-demand, and
>> will detect degraded links.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> ---
>>   drivers/pci/pci-sysfs.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index 9ecfe13157c0..6658e927b1f5 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -218,6 +218,18 @@ static ssize_t current_link_width_show(struct device *dev,
>>   }
>>   static DEVICE_ATTR_RO(current_link_width);
>>   
>> +static ssize_t available_bandwidth_show(struct device *dev,
>> +				       struct device_attribute *attr, char *buf)
>> +{
>> +	struct pci_dev *pci_dev = to_pci_dev(dev);
>> +	u32 bw_avail;
>> +
>> +	bw_avail = pcie_bandwidth_available(pci_dev, NULL, NULL, NULL);
>> +
>> +	return sprintf(buf, "%u.%03u Gb/s\n", bw_avail / 1000, bw_avail % 1000);
>> +}
>> +static DEVICE_ATTR_RO(available_bandwidth);
> 
> Help me understand this.  We already have these sysfs attributes:
> 
>    max_link_speed          # eg, 16 GT/s
>    max_link_width          # eg, 8
>    current_link_speed      # eg, 16 GT/s
>    current_link_width      # eg, 8
> 
> so I think the raw materials are already exposed.
> > The benefits I see for this new file are that
> 
>    - pcie_bandwidth_available() does the work of traversing up the
>      tree, doing the computations (link width * speed, reduced by
>      encoding overhead), and finding the minimum, and
> 
>    - it re-traverses the path every time we look at it, while the
>      boot-time check is a one-time event.
> 
> In principle this could all be done in user space with the attributes
> that are already exported.  There's some precedent for things like
> this in lspci, e.g., "NUMA node" [1], and lspci might even be a more
> user-friendly place for users to look for this, as opposed to
> searching through sysfs.

Parsing the endpoint to root port bandwidth is, in principle, possible 
from userspace. It's just that in practice it's very clumsy to do, and, 
as you pointed out, not that reliable.

I understand it's not information that all users would jump in the air 
to know. However, it was important enough for certain use cases, that 
the kernel already has a very reliable way to calculate it.

It seems to me that the most elegant way is to let the kernel tell us, 
since the kernel already has this facility. To quote one of the texts 
under Documentation/, it is an elegant way to "avoid reinventing kernel 
wheels in userspace".

Alex

> [1] https://git.kernel.org/pub/scm/utils/pciutils/pciutils.git/commit/?id=90ec4a6d0ae8
> 
>>   static ssize_t secondary_bus_number_show(struct device *dev,
>>   					 struct device_attribute *attr,
>>   					 char *buf)
>> @@ -786,6 +798,7 @@ static struct attribute *pcie_dev_attrs[] = {
>>   	&dev_attr_current_link_width.attr,
>>   	&dev_attr_max_link_width.attr,
>>   	&dev_attr_max_link_speed.attr,
>> +	&dev_attr_available_bandwidth.attr,
>>   	NULL,
>>   };
>>   
>> -- 
>> 2.17.1
>>
> 

Re: [PATCH 1/9] PCI: sysfs: Export available PCIe bandwidth

[Bjorn Helgaas]

[+cc Alex (VC mentioned below)]

On Wed, Oct 03, 2018 at 10:00:19PM +0000, Alex_Gagniuc@Dellteam.com wrote:
> On 10/03/2018 04:31 PM, Bjorn Helgaas wrote:
> > On Mon, Sep 03, 2018 at 01:02:28PM -0500, Alexandru Gagniuc wrote:
> >> For certain bandwidth-critical devices (e.g. multi-port network cards)
> >> it is useful to know the available bandwidth to the root complex. This
> >> information is only available via the system log, which doesn't
> >> account for link degradation after probing.
> >>
> >> With a sysfs attribute, we can computes the bandwidth on-demand, and
> >> will detect degraded links.
> >>
> >> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> >> ---
> >>   drivers/pci/pci-sysfs.c | 13 +++++++++++++
> >>   1 file changed, 13 insertions(+)
> >>
> >> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> >> index 9ecfe13157c0..6658e927b1f5 100644
> >> --- a/drivers/pci/pci-sysfs.c
> >> +++ b/drivers/pci/pci-sysfs.c
> >> @@ -218,6 +218,18 @@ static ssize_t current_link_width_show(struct device *dev,
> >>   }
> >>   static DEVICE_ATTR_RO(current_link_width);
> >>   
> >> +static ssize_t available_bandwidth_show(struct device *dev,
> >> +				       struct device_attribute *attr, char *buf)
> >> +{
> >> +	struct pci_dev *pci_dev = to_pci_dev(dev);
> >> +	u32 bw_avail;
> >> +
> >> +	bw_avail = pcie_bandwidth_available(pci_dev, NULL, NULL, NULL);
> >> +
> >> +	return sprintf(buf, "%u.%03u Gb/s\n", bw_avail / 1000, bw_avail % 1000);
> >> +}
> >> +static DEVICE_ATTR_RO(available_bandwidth);
> > 
> > Help me understand this.  We already have these sysfs attributes:
> > 
> >    max_link_speed          # eg, 16 GT/s
> >    max_link_width          # eg, 8
> >    current_link_speed      # eg, 16 GT/s
> >    current_link_width      # eg, 8
> > 
> > so I think the raw materials are already exposed.
> >
> > The benefits I see for this new file are that
> > 
> >    - pcie_bandwidth_available() does the work of traversing up the
> >      tree, doing the computations (link width * speed, reduced by
> >      encoding overhead), and finding the minimum, and
> > 
> >    - it re-traverses the path every time we look at it, while the
> >      boot-time check is a one-time event.
> > 
> > In principle this could all be done in user space with the attributes
> > that are already exported.  There's some precedent for things like
> > this in lspci, e.g., "NUMA node" [1], and lspci might even be a more
> > user-friendly place for users to look for this, as opposed to
> > searching through sysfs.
> 
> Parsing the endpoint to root port bandwidth is, in principle, possible 
> from userspace. It's just that in practice it's very clumsy to do, and, 
> as you pointed out, not that reliable.

I don't remember the reliability issue.  Was that in a previous
conversation?  AFAICT, using current_link_speed and current_link_width
should be as reliable as pcie_bandwidth_available() because they're
both based on reading PCI_EXP_LNKSTA.

This patch exposes only the available bandwidth, not the limiting
device, link speed, or link width.  Maybe that extra information isn't
important in this context.  Of course, it's easy to derive using
current_link_speed and current_link_width, but if we do that, there's
really no benefit to adding a new file.

Linux doesn't really support Virtual Channels (VC) yet, and I don't
know much about it, but it seems like Quality-of-Service features like
VC could affect this idea of available bandwidth.

Since we already expose the necessary information, and we might throw
additional things like VC into the mix, I'm a little hesitant about
adding things to sysfs because they're very difficult to change later.  

> I understand it's not information that all users would jump in the air 
> to know. However, it was important enough for certain use cases, that 
> the kernel already has a very reliable way to calculate it.
> 
> It seems to me that the most elegant way is to let the kernel tell us, 
> since the kernel already has this facility. To quote one of the texts 
> under Documentation/, it is an elegant way to "avoid reinventing kernel 
> wheels in userspace".
> 
> Alex
> 
> > [1] https://git.kernel.org/pub/scm/utils/pciutils/pciutils.git/commit/?id=90ec4a6d0ae8
> > 
> >>   static ssize_t secondary_bus_number_show(struct device *dev,
> >>   					 struct device_attribute *attr,
> >>   					 char *buf)
> >> @@ -786,6 +798,7 @@ static struct attribute *pcie_dev_attrs[] = {
> >>   	&dev_attr_current_link_width.attr,
> >>   	&dev_attr_max_link_width.attr,
> >>   	&dev_attr_max_link_speed.attr,
> >> +	&dev_attr_available_bandwidth.attr,
> >>   	NULL,
> >>   };
> >>   
> >> -- 
> >> 2.17.1
> >>
> > 
> 

Re: [PATCH 1/9] PCI: sysfs: Export available PCIe bandwidth

[Alex_Gagniuc]

On 10/04/2018 03:45 PM, Bjorn Helgaas wrote:
> 
> [EXTERNAL EMAIL]
> Please report any suspicious attachments, links, or requests for sensitive information.
> 
> 
> [+cc Alex (VC mentioned below)]
> 
> On Wed, Oct 03, 2018 at 10:00:19PM +0000, Alex_Gagniuc@Dellteam.com wrote:
>> On 10/03/2018 04:31 PM, Bjorn Helgaas wrote:
>>> On Mon, Sep 03, 2018 at 01:02:28PM -0500, Alexandru Gagniuc wrote:
>>>> For certain bandwidth-critical devices (e.g. multi-port network cards)
>>>> it is useful to know the available bandwidth to the root complex. This
>>>> information is only available via the system log, which doesn't
>>>> account for link degradation after probing.
>>>>
>>>> With a sysfs attribute, we can computes the bandwidth on-demand, and
>>>> will detect degraded links.
>>>>
>>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>>>> ---
>>>>    drivers/pci/pci-sysfs.c | 13 +++++++++++++
>>>>    1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>>>> index 9ecfe13157c0..6658e927b1f5 100644
>>>> --- a/drivers/pci/pci-sysfs.c
>>>> +++ b/drivers/pci/pci-sysfs.c
>>>> @@ -218,6 +218,18 @@ static ssize_t current_link_width_show(struct device *dev,
>>>>    }
>>>>    static DEVICE_ATTR_RO(current_link_width);
>>>>    
>>>> +static ssize_t available_bandwidth_show(struct device *dev,
>>>> +				       struct device_attribute *attr, char *buf)
>>>> +{
>>>> +	struct pci_dev *pci_dev = to_pci_dev(dev);
>>>> +	u32 bw_avail;
>>>> +
>>>> +	bw_avail = pcie_bandwidth_available(pci_dev, NULL, NULL, NULL);
>>>> +
>>>> +	return sprintf(buf, "%u.%03u Gb/s\n", bw_avail / 1000, bw_avail % 1000);
>>>> +}
>>>> +static DEVICE_ATTR_RO(available_bandwidth);
>>>
>>> Help me understand this.  We already have these sysfs attributes:
>>>
>>>     max_link_speed          # eg, 16 GT/s
>>>     max_link_width          # eg, 8
>>>     current_link_speed      # eg, 16 GT/s
>>>     current_link_width      # eg, 8
>>>
>>> so I think the raw materials are already exposed.
>>>
>>> The benefits I see for this new file are that
>>>
>>>     - pcie_bandwidth_available() does the work of traversing up the
>>>       tree, doing the computations (link width * speed, reduced by
>>>       encoding overhead), and finding the minimum, and
>>>
>>>     - it re-traverses the path every time we look at it, while the
>>>       boot-time check is a one-time event.
>>>
>>> In principle this could all be done in user space with the attributes
>>> that are already exported.  There's some precedent for things like
>>> this in lspci, e.g., "NUMA node" [1], and lspci might even be a more
>>> user-friendly place for users to look for this, as opposed to
>>> searching through sysfs.
>>
>> Parsing the endpoint to root port bandwidth is, in principle, possible
>> from userspace. It's just that in practice it's very clumsy to do, and,
>> as you pointed out, not that reliable.
> 
> I don't remember the reliability issue.  Was that in a previous
> conversation?  AFAICT, using current_link_speed and current_link_width
> should be as reliable as pcie_bandwidth_available() because they're
> both based on reading PCI_EXP_LNKSTA.
> 
> This patch exposes only the available bandwidth, not the limiting
> device, link speed, or link width.  Maybe that extra information isn't
> important in this context.  Of course, it's easy to derive using
> current_link_speed and current_link_width, but if we do that, there's
> really no benefit to adding a new file.
> 
> Linux doesn't really support Virtual Channels (VC) yet, and I don't
> know much about it, but it seems like Quality-of-Service features like
> VC could affect this idea of available bandwidth.
> 
> Since we already expose the necessary information, and we might throw
> additional things like VC into the mix, I'm a little hesitant about
> adding things to sysfs because they're very difficult to change later.

I understand your concerns with VC and crazy PCIe outliers. 
'available_bandwidth' is meant to mean physical bandwidth, rather than 
"what comcast gives you". I see now the possibility for confusion.
My motivation is to save drivers from the hassle of having to call 
pcie_print_link_status(), and prevent the rare duplicate syslog messages.

I prefer re-using the kernel logic over doing it over again from 
userspace, but I won't insist over your doubts.

Alex

>> I understand it's not information that all users would jump in the air
>> to know. However, it was important enough for certain use cases, that
>> the kernel already has a very reliable way to calculate it.
>>
>> It seems to me that the most elegant way is to let the kernel tell us,
>> since the kernel already has this facility. To quote one of the texts
>> under Documentation/, it is an elegant way to "avoid reinventing kernel
>> wheels in userspace".
>>
>> Alex
>>
>>> [1] https://git.kernel.org/pub/scm/utils/pciutils/pciutils.git/commit/?id=90ec4a6d0ae8
>>>
>>>>    static ssize_t secondary_bus_number_show(struct device *dev,
>>>>    					 struct device_attribute *attr,
>>>>    					 char *buf)
>>>> @@ -786,6 +798,7 @@ static struct attribute *pcie_dev_attrs[] = {
>>>>    	&dev_attr_current_link_width.attr,
>>>>    	&dev_attr_max_link_width.attr,
>>>>    	&dev_attr_max_link_speed.attr,
>>>> +	&dev_attr_available_bandwidth.attr,
>>>>    	NULL,
>>>>    };
>>>>    
>>>> -- 
>>>> 2.17.1
>>>>
>>>
>>
>