From: Bjorn Helgaas <helgaas@kernel.org> To: Alex_Gagniuc@Dellteam.com Cc: mr.nuke.me@gmail.com, linux-pci@vger.kernel.org, bhelgaas@google.com, keith.busch@intel.com, Austin.Bolen@dell.com, Shyam.Iyer@dell.com, ariel.elior@cavium.com, everest-linux-l2@cavium.com, davem@davemloft.net, michael.chan@broadcom.com, ganeshgr@chelsio.com, jeffrey.t.kirsher@intel.com, tariqt@mellanox.com, saeedm@mellanox.com, leon@kernel.org, jakub.kicinski@netronome.com, dirk.vandermerwe@netronome.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, intel-wired-lan@lists.osuosl.org, linux-rdma@vger.kernel.org, oss-drivers@netronome.com, stephen@networkplumber.org, mj@ucw.cz, Alex Williamson <alex.williamson@redhat.com> Subject: Re: [PATCH 1/9] PCI: sysfs: Export available PCIe bandwidth Date: Thu, 4 Oct 2018 15:45:49 -0500 [thread overview] Message-ID: <20181004204549.GI120535@bhelgaas-glaptop.roam.corp.google.com> (raw) In-Reply-To: <f9fbc8babbb344eca593a3f8b0d096d9@ausx13mps321.AMER.DELL.COM> [+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 > >> > > >
WARNING: multiple messages have this Message-ID (diff)
From: Bjorn Helgaas <helgaas@kernel.org> To: intel-wired-lan@osuosl.org Subject: [Intel-wired-lan] [PATCH 1/9] PCI: sysfs: Export available PCIe bandwidth Date: Thu, 4 Oct 2018 15:45:49 -0500 [thread overview] Message-ID: <20181004204549.GI120535@bhelgaas-glaptop.roam.corp.google.com> (raw) In-Reply-To: <f9fbc8babbb344eca593a3f8b0d096d9@ausx13mps321.AMER.DELL.COM> [+cc Alex (VC mentioned below)] On Wed, Oct 03, 2018 at 10:00:19PM +0000, Alex_Gagniuc at 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 > >> > > >
next prev parent reply other threads:[~2018-10-04 20:45 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-09-03 18:02 [PATCH 0/9] Export PCIe bandwidth via sysfs Alexandru Gagniuc 2018-09-03 18:02 ` Alexandru Gagniuc 2018-09-03 18:02 ` [PATCH 1/9] PCI: sysfs: Export available PCIe bandwidth Alexandru Gagniuc 2018-09-05 7:26 ` Stephen Hemminger 2018-09-05 7:26 ` [Intel-wired-lan] " Stephen Hemminger 2018-09-05 13:52 ` Alex G. 2018-10-03 21:30 ` Bjorn Helgaas 2018-10-03 21:30 ` [Intel-wired-lan] " Bjorn Helgaas 2018-10-03 21:30 ` Bjorn Helgaas 2018-10-03 21:30 ` Bjorn Helgaas 2018-10-03 22:00 ` Alex_Gagniuc 2018-10-03 22:00 ` [Intel-wired-lan] " Alex_Gagniuc 2018-10-03 22:00 ` Alex_Gagniuc 2018-10-04 20:45 ` Bjorn Helgaas [this message] 2018-10-04 20:45 ` [Intel-wired-lan] " Bjorn Helgaas 2018-10-08 21:09 ` Alex_Gagniuc 2018-10-08 21:09 ` [Intel-wired-lan] " Alex_Gagniuc 2018-10-08 21:09 ` Alex_Gagniuc 2018-09-03 18:02 ` [PATCH 2/9] bnx2x: Do not call pcie_print_link_status() Alexandru Gagniuc 2018-09-03 18:02 ` [PATCH 3/9] bnxt_en: " Alexandru Gagniuc 2018-09-03 18:02 ` [PATCH 4/9] cxgb4: " Alexandru Gagniuc 2018-09-03 18:02 ` [PATCH 5/9] fm10k: " Alexandru Gagniuc 2018-09-03 18:02 ` [PATCH 6/9] ixgbe: " Alexandru Gagniuc 2018-09-03 18:02 ` [PATCH 7/9] net/mlx4: " Alexandru Gagniuc 2018-09-04 12:51 ` Leon Romanovsky 2018-09-04 12:51 ` [Intel-wired-lan] " Leon Romanovsky 2018-09-03 18:02 ` [PATCH 8/9] net/mlx5: " Alexandru Gagniuc 2018-09-04 12:51 ` Leon Romanovsky 2018-09-04 12:51 ` [Intel-wired-lan] " Leon Romanovsky 2018-09-03 18:02 ` [PATCH 9/9] nfp: " Alexandru Gagniuc
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20181004204549.GI120535@bhelgaas-glaptop.roam.corp.google.com \ --to=helgaas@kernel.org \ --cc=Alex_Gagniuc@Dellteam.com \ --cc=Austin.Bolen@dell.com \ --cc=Shyam.Iyer@dell.com \ --cc=alex.williamson@redhat.com \ --cc=ariel.elior@cavium.com \ --cc=bhelgaas@google.com \ --cc=davem@davemloft.net \ --cc=dirk.vandermerwe@netronome.com \ --cc=everest-linux-l2@cavium.com \ --cc=ganeshgr@chelsio.com \ --cc=intel-wired-lan@lists.osuosl.org \ --cc=jakub.kicinski@netronome.com \ --cc=jeffrey.t.kirsher@intel.com \ --cc=keith.busch@intel.com \ --cc=leon@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=linux-rdma@vger.kernel.org \ --cc=michael.chan@broadcom.com \ --cc=mj@ucw.cz \ --cc=mr.nuke.me@gmail.com \ --cc=netdev@vger.kernel.org \ --cc=oss-drivers@netronome.com \ --cc=saeedm@mellanox.com \ --cc=stephen@networkplumber.org \ --cc=tariqt@mellanox.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.