From: <Alex_Gagniuc@Dellteam.com>
To: <talgi@mellanox.com>, <mr.nuke.me@gmail.com>,
<jakub.kicinski@netronome.com>
Cc: <linux-pci@vger.kernel.org>, <bhelgaas@google.com>,
<keith.busch@intel.com>, <Austin.Bolen@dell.com>,
<Shyam.Iyer@dell.com>, <jeffrey.t.kirsher@intel.com>,
<ariel.elior@cavium.com>, <michael.chan@broadcom.com>,
<ganeshgr@chelsio.com>, <tariqt@mellanox.com>,
<airlied@gmail.com>, <alexander.deucher@amd.com>,
<mike.marciniszyn@intel.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5] PCI: Check for PCIe downtraining conditions
Date: Mon, 30 Jul 2018 23:26:02 +0000 [thread overview]
Message-ID: <cc8e0ec8fd7c4222a8ef858d3682b18b@ausx13mps321.AMER.DELL.COM> (raw)
In-Reply-To: cf663b45-a6ce-b0d7-c79a-a02940001c20@mellanox.com
On 07/24/2018 08:40 AM, Tal Gilboa wrote:
> On 7/24/2018 2:59 AM, Alex G. wrote:
>>
>>
>> On 07/23/2018 05:14 PM, Jakub Kicinski wrote:
>>> On Tue, 24 Jul 2018 00:52:22 +0300, Tal Gilboa wrote:
>>>> On 7/24/2018 12:01 AM, Jakub Kicinski wrote:
>>>>> On Mon, 23 Jul 2018 15:03:38 -0500, Alexandru Gagniuc wrote:
>>>>>> PCIe downtraining happens when both the device and PCIe port are
>>>>>> capable of a larger bus width or higher speed than negotiated.
>>>>>> Downtraining might be indicative of other problems in the system, and
>>>>>> identifying this from userspace is neither intuitive, nor
>>>>>> straightforward.
>>>>>>
>>>>>> The easiest way to detect this is with pcie_print_link_status(),
>>>>>> since the bottleneck is usually the link that is downtrained. It's not
>>>>>> a perfect solution, but it works extremely well in most cases.
>>>>>>
>>>>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>>>>>> ---
>>>>>>
>>>>>> For the sake of review, I've created a __pcie_print_link_status()
>>>>>> which
>>>>>> takes a 'verbose' argument. If we agree want to go this route, and
>>>>>> update
>>>>>> the users of pcie_print_link_status(), I can split this up in two
>>>>>> patches.
>>>>>> I prefer just printing this information in the core functions, and
>>>>>> letting
>>>>>> drivers not have to worry about this. Though there seems to be
>>>>>> strong for
>>>>>> not going that route, so here it goes:
>>>>>
>>>>> FWIW the networking drivers print PCIe BW because sometimes the network
>>>>> bandwidth is simply over-provisioned on multi port cards, e.g. 80Gbps
>>>>> card on a x8 link.
>>>>>
>>>>> Sorry to bike shed, but currently the networking cards print the info
>>>>> during probe. Would it make sense to move your message closer to probe
>>>>> time? Rather than when device is added. If driver structure is
>>>>> available, we could also consider adding a boolean to struct pci_driver
>>>>> to indicate if driver wants the verbose message? This way we avoid
>>>>> duplicated prints.
>>>>>
>>>>> I have no objection to current patch, it LGTM. Just a thought.
>>>>
>>>> I don't see the reason for having two functions. What's the problem with
>>>> adding the verbose argument to the original function?
>>>
>>> IMHO it's reasonable to keep the default parameter to what 90% of users
>>> want by a means on a wrapper. The non-verbose output is provided by
>>> the core already for all devices.
>>>
>>> What do you think of my proposal above Tal? That would make the extra
>>> wrapper unnecessary since the verbose parameter would be part of the
>>> driver structure, and it would avoid the duplicated output.
>>
>> I see how it might make sense to add another member to the driver
>> struct, but is it worth the extra learning curve? It seems to be
>> something with the potential to confuse new driver developers, and
>> having a very marginal benefit.
>> Although, if that's what people want...
>
> I prefer the wrapper function. Looking at struct pci_driver it would
> seem strange for it to hold a field for controlling verbosity (IMO).
> This is a very (very) specific field in a very general struct.
If people are okay with the wrapper, then I'm not going to update the patch.
Alex
next prev parent reply other threads:[~2018-07-30 23:26 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-04 15:55 [PATCH v3] PCI: Check for PCIe downtraining conditions Alexandru Gagniuc
2018-06-05 12:27 ` Andy Shevchenko
2018-06-05 13:04 ` Andy Shevchenko
2018-07-16 21:17 ` Bjorn Helgaas
2018-07-16 22:28 ` Alex_Gagniuc
2018-07-18 21:53 ` Bjorn Helgaas
2018-07-19 15:46 ` Alex G.
2018-07-23 20:01 ` [PATCH v2] PCI/AER: Do not clear AER bits if we don't own AER Alexandru Gagniuc
2018-07-25 1:24 ` kbuild test robot
2018-07-23 20:03 ` [PATCH v5] PCI: Check for PCIe downtraining conditions Alexandru Gagniuc
2018-07-23 21:01 ` Jakub Kicinski
2018-07-23 21:52 ` Tal Gilboa
2018-07-23 22:14 ` Jakub Kicinski
2018-07-23 23:59 ` Alex G.
2018-07-24 13:39 ` Tal Gilboa
2018-07-30 23:26 ` Alex_Gagniuc [this message]
2018-07-31 6:40 ` Tal Gilboa
2018-07-31 15:10 ` Alex G.
2018-08-05 7:05 ` Tal Gilboa
2018-08-06 18:39 ` Alex_Gagniuc
2018-08-06 19:46 ` Bjorn Helgaas
2018-08-06 23:25 ` [PATCH v6 1/9] " Alexandru Gagniuc
2018-08-06 23:25 ` [PATCH v6 2/9] bnx2x: Do not call pcie_print_link_status() Alexandru Gagniuc
2018-08-06 23:25 ` [PATCH v6 3/9] bnxt_en: " Alexandru Gagniuc
2018-08-06 23:25 ` [PATCH v6 4/9] cxgb4: " Alexandru Gagniuc
2018-08-06 23:25 ` [PATCH v6 5/9] fm10k: " Alexandru Gagniuc
2018-08-07 17:52 ` Jeff Kirsher
2018-08-06 23:25 ` [PATCH v6 6/9] ixgbe: " Alexandru Gagniuc
2018-08-07 17:51 ` Jeff Kirsher
2018-08-06 23:25 ` [PATCH v6 7/9] net/mlx4: " Alexandru Gagniuc
2018-08-08 6:10 ` Leon Romanovsky
2018-08-06 23:25 ` [PATCH v6 8/9] net/mlx5: " Alexandru Gagniuc
2018-08-08 6:08 ` Leon Romanovsky
2018-08-08 14:23 ` Tal Gilboa
2018-08-08 15:41 ` Leon Romanovsky
2018-08-08 15:56 ` Tal Gilboa
2018-08-08 16:33 ` Alex G.
2018-08-08 17:27 ` Leon Romanovsky
2018-08-09 14:02 ` Bjorn Helgaas
2018-08-06 23:25 ` [PATCH v6 9/9] nfp: " Alexandru Gagniuc
2018-08-07 19:44 ` [PATCH v6 1/9] PCI: Check for PCIe downtraining conditions David Miller
2018-08-07 21:41 ` Bjorn Helgaas
2018-07-18 13:38 ` [PATCH v3] " Tal Gilboa
2018-07-19 15:49 ` Alex G.
2018-07-23 5:21 ` Tal Gilboa
2018-07-23 17:01 ` Alex G.
2018-07-23 21:35 ` Tal Gilboa
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=cc8e0ec8fd7c4222a8ef858d3682b18b@ausx13mps321.AMER.DELL.COM \
--to=alex_gagniuc@dellteam.com \
--cc=Austin.Bolen@dell.com \
--cc=Shyam.Iyer@dell.com \
--cc=airlied@gmail.com \
--cc=alexander.deucher@amd.com \
--cc=ariel.elior@cavium.com \
--cc=bhelgaas@google.com \
--cc=ganeshgr@chelsio.com \
--cc=jakub.kicinski@netronome.com \
--cc=jeffrey.t.kirsher@intel.com \
--cc=keith.busch@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=michael.chan@broadcom.com \
--cc=mike.marciniszyn@intel.com \
--cc=mr.nuke.me@gmail.com \
--cc=talgi@mellanox.com \
--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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).