Message-ID: <20090929184650.GF3958@sequoia.sous-sol.org>
Date:	Tue, 29 Sep 2009 11:46:50 -0700
From:	Chris Wright <chrisw@...s-sol.org>
To:	"Kay, Allen M" <allen.m.kay@...el.com>
Cc:	Chris Wright <chrisw@...s-sol.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"jbarnes@...tuousgeek.org" <jbarnes@...tuousgeek.org>,
	"matthew@....cx" <matthew@....cx>
Subject: Re: [PATCH ACS v3 1/1]

* Kay, Allen M (allen.m.kay@...el.com) wrote:
> >
> >This may negatively impact p2p traffic throughput for devices that don't
> >need it.  Have you considered this impact or attempted to measure it?
> >
> 
> As far as I know, there is no existing PCIe devices that have ACS capable PCIe switches.  This means this patch will not impact existing P2P devices.  On the NHM platform I tested this patch on, only root ports support ACS which has no material impact on PCIe transactions since whatever upstream traffic root port sees is already forwarded to the root complex anyways.

BTW, I tested it as well...works as advertised ;-)  I did not generate
any errors to see if AER is working, did you?

> As for future devices that does have ACS capable PCIe switches, this patch can cause potential P2P performance issue as you indicated.  Although PCI IOV SIG has yet to make a decision on this issue, it would be reasonable to expect this problem can be mitigated with ATS capable devices.  For example, it would be reasonable to expect translated addresses can be routed directly to the peer device while un-translated addresses would have to be routed to the root complex.

This means adding direct translated P2P support, no?  And what does the
device cache when the IOMMU is in PT mode?  I'm mainly voicing concern
about the non-IOV case (i.e. common case) that this impacts by enabling
as a default.

> By the way, PLX technology announced first such switch on 8/26.  We will be take a look at these devices as soon as we get hold of these in our lab.

Or multifunction devices, but any testing is good.

> >An alternative approach would be to enable this during device assignment.
> >
> 
> I have indeed spent some time playing around with a patch that does this.  There are some potential drawbacks. Given that PCI is already enabled at the time of device assignment, enabling P2P upstream forwarding might disrupt in flight PCIe transactions.  In addition, this means we need separate patches for enabling ACS for KVM and Xen as device assignment for KVM and Xen do not share code paths.

I hadn't considered in-flight transactions.  The device should be
quiesced and reset before assignment, but that doesn't account for
other devices effected by intermediate downstream port ACS changes.
It's also not entirely clear what to do on de-assignment.  Would be a
bit odd, but could be driven from userspace.

> >Also, there is no checking that the relevant path through the topology has
> >the right capabilties.  Is there any reason you left that out?  It would
> >certainly simplify the filtering logic, for example.  
> >
> 
> Do you mean enable p2p forwarding on all upstream PCIe switches only if all of them are ACS capable?  I can see this can potentially simplify filtering software to just check the lowest level PCIe switch.

Yeah, and the RC requirements too, of course.

> This appears to be a trade-off between whether we want put the complexity in Linux PCI driver or in the user mode filtering code.  In my mind, if we take the view that the device filtering software is the ultimate authority in determining whether a device is assignable, it probably should not trust the host to always do the right thing from virtualization standpoint.  If a paranoid filtering software always checks the entire path from the device to the root complex anyways, it might be reasonable to simplify the code in the kernel.

The reason I mention it is not just filtering, but can create a platform
w/ undefined behaviour w/out checking.

> >And given some states result in undefined behavior, perhaps it makes sense to check
> >while enabling ACS.
> >
> 
> By "undefined behavior", do you mean when there a mix of ACS and non-ACS capable PCIe switches and P2P upstream forwarding is enabled in ACS capable PCIe switches?  I would expect the aggregate behavior is the same as no P2P upstream forwarding.

Yes, that's what I mean.

> Let's say we have a configuration where the lowest PCIe switch is ACS capable and it has P2P upstream forwarding enabled.  However, the PCIe switch just above it is not ACS capable.
> 
> I would expect the following behavior:
> 
> 1) P2P transaction is forwarded upstream by the ACS capable PCIe switch
> 2) non-ACS capable switch sends the transaction back
> 3) ACS capable switch sends the transaction to the peer device.
> 
> The aggregate result is the transaction behaved as if all the switches are not ACS capable.

Right, although it's implementation specific what actually happens.
May not matter much, I just don't know what switch vendors will do.

> > I'd call it pci_enable_acs...in fact, the kdoc above tries something close to that ;-)
> >
> 
> No problem, I can change the code to incorporate this once we have an agreement on other items.

thanks,
-chris
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists