pcie_bus_config settings. What exactly do each setting mean..

pcie_bus_config settings. What exactly do each setting mean..

[Raj, Ashok]

Hi Bjorn

I was trying to fix pcie_write_mrrs() since its seems to not follow PCIe
SIG recommendations. 

It appears to that 010 (512b) is the default and 4096b is the max spec
allowed limit.

Current code seems to match MRRS and MPS, which seem to be completely
different purposes. 

But trying to fix i got confused with all these pcie_bus_config_types.

enum pcie_bus_config_types {
        PCIE_BUS_TUNE_OFF,      /* Don't touch MPS at all */
        PCIE_BUS_DEFAULT,       /* Ensure MPS matches upstream bridge */
        PCIE_BUS_SAFE,          /* Use largest MPS boot-time devices support */
        PCIE_BUS_PERFORMANCE,   /* Use MPS and MRRS for best performance */
        PCIE_BUS_PEER2PEER,     /* Set MPS = 128 for all devices */
};

Not sure what the difference between BUS_TUNE_OFF and BUS_DEFAULT is.

MPS matching upstream bridge is required in all cases right?
BUS_SAFE/BUS_PERFORMANCE? Not sure why that is special for BUS_DEFAULT.

Wouldn't it be simple to say:

BUS_DEFAULT : Just use BIOS settings, no change to anything.

BUS_SAFE: Says Choose largest MPS boot-time settings? What does that
actually mean? Why isn't that BUS_PERFORMANCE?

P2P: Choose smallest setting makes sense.

I think only 3 settings make sense.

BUS_DEFAULT == TUNE_OFF : Choose BIOS values, don't touch anything
BUS_SAFE == BUS_PERFORMANCE : Should actually be the default.
BUS_PEER2PEER - Same as now

Do we really have value for the other settings? Maybe for BUS_DEFAULT we
should call it BUS_BIOS (to indicate real meaning)

BUS_PERFORMANCE should be the default setting for the system.

BUS_P2P if someone needs to configure for p2p.

Cheers,
Ashok

Re: pcie_bus_config settings. What exactly do each setting mean..

[Bjorn Helgaas]

On Tue, Jun 02, 2020 at 08:23:54PM -0700, Raj, Ashok wrote:
> Hi Bjorn
> 
> I was trying to fix pcie_write_mrrs() since it seems to not follow PCIe
> SIG recommendations. 
> 
> It appears to that 010 (512b) is the default and 4096b is the max spec
> allowed limit.
> 
> Current code seems to match MRRS and MPS, which seem to be completely
> different purposes. 

Yes, I agree that MRRS and MPS have different purposes, and from a
hardware point of view, there's no reason MRRS should be related to
MPS.

The reason Linux tries to set MRRS == MPS is so we can set MPS on
shared paths to be higher than the smallest MPS of the leaves.  For
the leaf with a small MPS, we set MRRS == MPS and assume that is
enough to limit the size of TLPs it receives.  That assumption is true
for *reads* initiated by the leaf, but not it's necessarily true for
*writes* to the leaf, e.g., peer-to-peer writes.

There's a little more explanation here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b03e7495a862

> But trying to fix i got confused with all these pcie_bus_config_types.
> 
> enum pcie_bus_config_types {
>         PCIE_BUS_TUNE_OFF,      /* Don't touch MPS at all */
>         PCIE_BUS_DEFAULT,       /* Ensure MPS matches upstream bridge */
>         PCIE_BUS_SAFE,          /* Use largest MPS boot-time devices support */
>         PCIE_BUS_PERFORMANCE,   /* Use MPS and MRRS for best performance */
>         PCIE_BUS_PEER2PEER,     /* Set MPS = 128 for all devices */
> };
> 
> Not sure what the difference between BUS_TUNE_OFF and BUS_DEFAULT is.

I'm not sure either, but there is a case in pci_configure_mps() where
we test for PCIE_BUS_TUNE_OFF but not BUS_DEFAULT, so it's not
completely obvious that they're identical.

> MPS matching upstream bridge is required in all cases right?

The MRRS trick above is intended to relax this requirement.

> BUS_SAFE/BUS_PERFORMANCE? Not sure why that is special for BUS_DEFAULT.
> 
> Wouldn't it be simple to say:
> 
> BUS_DEFAULT : Just use BIOS settings, no change to anything.
> 
> BUS_SAFE: Says Choose largest MPS boot-time settings? What does that
> actually mean? Why isn't that BUS_PERFORMANCE?
> 
> P2P: Choose smallest setting makes sense.
> 
> I think only 3 settings make sense.
> 
> BUS_DEFAULT == TUNE_OFF : Choose BIOS values, don't touch anything
> BUS_SAFE == BUS_PERFORMANCE : Should actually be the default.
> BUS_PEER2PEER - Same as now
> 
> Do we really have value for the other settings? Maybe for BUS_DEFAULT we
> should call it BUS_BIOS (to indicate real meaning)
> 
> BUS_PERFORMANCE should be the default setting for the system.
> 
> BUS_P2P if someone needs to configure for p2p.

I would love it if somebody reworked this a bit.  I think it's a
little confusing right now.

Bjorn