On 6/26/2020 11:12 PM, Bjorn Helgaas wrote:
On Wed, Jun 24, 2020 at 10:22:58AM -0700, Jakub Kicinski wrote:
On Wed, 24 Jun 2020 10:34:40 +0300 Aya Levin wrote:
I think Michal will rightly complain that this does not belong in
private flags any more. As (/if?) ARM deployments take a foothold
in DC this will become a common setting for most NICs.
Initially we used pcie_relaxed_ordering_enabled() to
programmatically enable this on/off on boot but this seems to
introduce some degradation on some Intel CPUs since the Intel Faulty
CPUs list is not up to date. Aya is discussing this with Bjorn.
Adding Bjorn Helgaas
I see. Simply using pcie_relaxed_ordering_enabled() and blacklisting
bad CPUs seems far nicer from operational perspective. Perhaps Bjorn
will chime in. Pushing the validation out to the user is not a great
solution IMHO.
I'm totally lost, but maybe it doesn't matter because it looks like
David has pulled this series already.
There probably *should* be a PCI core interface to enable RO, but
there isn't one today.
pcie_relaxed_ordering_enabled() doesn't *enable* anything. All it
does is tell you whether RO is already enabled.
This patch ([net-next 10/10] net/mlx5e: Add support for PCI relaxed
ordering) apparently adds a knob to control RO, but I can't connect
the dots. It doesn't touch PCI_EXP_DEVCTL_RELAX_EN, and that symbol
doesn't occur anywhere in drivers/net except tg3, myri10ge, and niu.
And this whole series doesn't contain PCI_EXP_DEVCTL_RELAX_EN or
pcie_relaxed_ordering_enabled().
I wanted to turn on RO on the ETH driver based on
pcie_relaxed_ordering_enabled().
From my experiments I see that pcie_relaxed_ordering_enabled() return
true on Intel(R) Xeon(R) CPU E5-2650 v3 @ 2.30GHz. This CPU is from
Haswell series which is known to have bug in RO implementation. In this
case, I expected pcie_relaxed_ordering_enabled() to return false,
shouldn't it?
In addition, we are worried about future bugs in new CPUs which may
result in performance degradation while using RO, as long as the
function pcie_relaxed_ordering_enabled() will return true for these
CPUs. That's why we thought of adding the feature on our card with
default off and enable the user to set it.
I do have a couple emails from Aya, but they didn't include a patch
and I haven't quite figured out what the question was.
So until we figure this out, will keep this off by default.
for the private flags we want to keep them for performance analysis as
we do with all other mlx5 special performance features and flags.
