Re: [net-next 10/10] net/mlx5e: Add support for PCI relaxed ordering

From: Aya Levin <ayal@mellanox.com>
To: Bjorn Helgaas <helgaas@kernel.org>, Jakub Kicinski <kuba@kernel.org>
Cc: Saeed Mahameed <saeedm@mellanox.com>,
	"mkubecek@suse.cz" <mkubecek@suse.cz>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Tariq Toukan <tariqt@mellanox.com>,
	linux-pci@vger.kernel.org,
	Alexander Duyck <alexander.h.duyck@linux.intel.com>
Subject: Re: [net-next 10/10] net/mlx5e: Add support for PCI relaxed ordering
Date: Mon, 29 Jun 2020 12:32:44 +0300	[thread overview]
Message-ID: <ca121a18-8c11-5830-9840-51f353c3ddd2@mellanox.com> (raw)
In-Reply-To: <20200626201254.GA2932090@bjorn-Precision-5520>

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.