| Message ID | 1438208335-19457-3-git-send-email-keith.busch@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
[+cc Mike, linux-rdma] On Wed, Jul 29, 2015 at 04:18:54PM -0600, Keith Busch wrote: > From: Dave Jiang <dave.jiang@intel.com> > > This is in perperation of un-exporting the pcie_set_mps() function > symbol. A driver should not be changing the MPS as that is the > responsibility of the PCI subsystem. Please explain the implications of removing this code. Does this affect performance of the device? If so, how do we get that performance back? I also cc'd the QIB maintainers for you: QIB DRIVER M: Mike Marciniszyn <infinipath@intel.com> L: linux-rdma@vger.kernel.org F: drivers/infiniband/hw/qib/ > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/infiniband/hw/qib/qib_pcie.c | 27 +-------------------------- > 1 file changed, 1 insertion(+), 26 deletions(-) > > diff --git a/drivers/infiniband/hw/qib/qib_pcie.c b/drivers/infiniband/hw/qib/qib_pcie.c > index 4758a38..b8a2dcd 100644 > --- a/drivers/infiniband/hw/qib/qib_pcie.c > +++ b/drivers/infiniband/hw/qib/qib_pcie.c > @@ -557,12 +557,11 @@ static void qib_tune_pcie_coalesce(struct qib_devdata *dd) > */ > static int qib_pcie_caps; > module_param_named(pcie_caps, qib_pcie_caps, int, S_IRUGO); > -MODULE_PARM_DESC(pcie_caps, "Max PCIe tuning: Payload (0..3), ReadReq (4..7)"); > +MODULE_PARM_DESC(pcie_caps, "Max PCIe tuning: ReadReq (4..7)"); > > static void qib_tune_pcie_caps(struct qib_devdata *dd) > { > struct pci_dev *parent; > - u16 rc_mpss, rc_mps, ep_mpss, ep_mps; > u16 rc_mrrs, ep_mrrs, max_mrrs; > > /* Find out supported and configured values for parent (root) */ > @@ -575,30 +574,6 @@ static void qib_tune_pcie_caps(struct qib_devdata *dd) > if (!pci_is_pcie(parent) || !pci_is_pcie(dd->pcidev)) > return; > > - rc_mpss = parent->pcie_mpss; > - rc_mps = ffs(pcie_get_mps(parent)) - 8; > - /* Find out supported and configured values for endpoint (us) */ > - ep_mpss = dd->pcidev->pcie_mpss; > - ep_mps = ffs(pcie_get_mps(dd->pcidev)) - 8; > - > - /* Find max payload supported by root, endpoint */ > - if (rc_mpss > ep_mpss) > - rc_mpss = ep_mpss; > - > - /* If Supported greater than limit in module param, limit it */ > - if (rc_mpss > (qib_pcie_caps & 7)) > - rc_mpss = qib_pcie_caps & 7; > - /* If less than (allowed, supported), bump root payload */ > - if (rc_mpss > rc_mps) { > - rc_mps = rc_mpss; > - pcie_set_mps(parent, 128 << rc_mps); > - } > - /* If less than (allowed, supported), bump endpoint payload */ > - if (rc_mpss > ep_mps) { > - ep_mps = rc_mpss; > - pcie_set_mps(dd->pcidev, 128 << ep_mps); > - } > - > /* > * Now the Read Request size. > * No field for max supported, but PCIe spec limits it to 4096, > -- > 1.7.10.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-08-17 at 17:30 -0500, Bjorn Helgaas wrote: > [+cc Mike, linux-rdma] > > On Wed, Jul 29, 2015 at 04:18:54PM -0600, Keith Busch wrote: > > From: Dave Jiang <dave.jiang@intel.com> > > > > This is in perperation of un-exporting the pcie_set_mps() function > > symbol. A driver should not be changing the MPS as that is the > > responsibility of the PCI subsystem. > > Please explain the implications of removing this code. Does this > affect > performance of the device? If so, how do we get that performance > back? Honestly I don't know. But at the same time I think the driver shouldn't be touching the MPS at all. Shouldn't that be left to the PCIe subsystem and rely on the PCIe subsystem to set this to a sane value? > > I also cc'd the QIB maintainers for you: > > QIB DRIVER > M: Mike Marciniszyn <infinipath@intel.com> > L: linux-rdma@vger.kernel.org > F: drivers/infiniband/hw/qib/ > > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > --- > > drivers/infiniband/hw/qib/qib_pcie.c | 27 +--------------------- > > ----- > > 1 file changed, 1 insertion(+), 26 deletions(-) > > > > diff --git a/drivers/infiniband/hw/qib/qib_pcie.c > > b/drivers/infiniband/hw/qib/qib_pcie.c > > index 4758a38..b8a2dcd 100644 > > --- a/drivers/infiniband/hw/qib/qib_pcie.c > > +++ b/drivers/infiniband/hw/qib/qib_pcie.c > > @@ -557,12 +557,11 @@ static void qib_tune_pcie_coalesce(struct > > qib_devdata *dd) > > */ > > static int qib_pcie_caps; > > module_param_named(pcie_caps, qib_pcie_caps, int, S_IRUGO); > > -MODULE_PARM_DESC(pcie_caps, "Max PCIe tuning: Payload (0..3), > > ReadReq (4..7)"); > > +MODULE_PARM_DESC(pcie_caps, "Max PCIe tuning: ReadReq (4..7)"); > > > > static void qib_tune_pcie_caps(struct qib_devdata *dd) > > { > > struct pci_dev *parent; > > - u16 rc_mpss, rc_mps, ep_mpss, ep_mps; > > u16 rc_mrrs, ep_mrrs, max_mrrs; > > > > /* Find out supported and configured values for parent > > (root) */ > > @@ -575,30 +574,6 @@ static void qib_tune_pcie_caps(struct > > qib_devdata *dd) > > if (!pci_is_pcie(parent) || !pci_is_pcie(dd->pcidev)) > > return; > > > > - rc_mpss = parent->pcie_mpss; > > - rc_mps = ffs(pcie_get_mps(parent)) - 8; > > - /* Find out supported and configured values for endpoint > > (us) */ > > - ep_mpss = dd->pcidev->pcie_mpss; > > - ep_mps = ffs(pcie_get_mps(dd->pcidev)) - 8; > > - > > - /* Find max payload supported by root, endpoint */ > > - if (rc_mpss > ep_mpss) > > - rc_mpss = ep_mpss; > > - > > - /* If Supported greater than limit in module param, limit > > it */ > > - if (rc_mpss > (qib_pcie_caps & 7)) > > - rc_mpss = qib_pcie_caps & 7; > > - /* If less than (allowed, supported), bump root payload */ > > - if (rc_mpss > rc_mps) { > > - rc_mps = rc_mpss; > > - pcie_set_mps(parent, 128 << rc_mps); > > - } > > - /* If less than (allowed, supported), bump endpoint > > payload */ > > - if (rc_mpss > ep_mps) { > > - ep_mps = rc_mpss; > > - pcie_set_mps(dd->pcidev, 128 << ep_mps); > > - } > > - > > /* > > * Now the Read Request size. > > * No field for max supported, but PCIe spec limits it to > > 4096,
On Mon, Aug 17, 2015 at 5:50 PM, Jiang, Dave <dave.jiang@intel.com> wrote: > On Mon, 2015-08-17 at 17:30 -0500, Bjorn Helgaas wrote: >> [+cc Mike, linux-rdma] >> >> On Wed, Jul 29, 2015 at 04:18:54PM -0600, Keith Busch wrote: >> > From: Dave Jiang <dave.jiang@intel.com> >> > >> > This is in perperation of un-exporting the pcie_set_mps() function >> > symbol. A driver should not be changing the MPS as that is the >> > responsibility of the PCI subsystem. >> >> Please explain the implications of removing this code. Does this >> affect >> performance of the device? If so, how do we get that performance >> back? > > Honestly I don't know. But at the same time I think the driver > shouldn't be touching the MPS at all. Shouldn't that be left to the > PCIe subsystem and rely on the PCIe subsystem to set this to a sane > value? Yes, I think in principle the PCI core should own this, but I also don't want to introduce a performance regression, so I think we need to understand whether there's a problem, and if there is, fix it. >> I also cc'd the QIB maintainers for you: >> >> QIB DRIVER >> M: Mike Marciniszyn <infinipath@intel.com> >> L: linux-rdma@vger.kernel.org >> F: drivers/infiniband/hw/qib/ >> >> > Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> > --- >> > drivers/infiniband/hw/qib/qib_pcie.c | 27 +--------------------- >> > ----- >> > 1 file changed, 1 insertion(+), 26 deletions(-) >> > >> > diff --git a/drivers/infiniband/hw/qib/qib_pcie.c >> > b/drivers/infiniband/hw/qib/qib_pcie.c >> > index 4758a38..b8a2dcd 100644 >> > --- a/drivers/infiniband/hw/qib/qib_pcie.c >> > +++ b/drivers/infiniband/hw/qib/qib_pcie.c >> > @@ -557,12 +557,11 @@ static void qib_tune_pcie_coalesce(struct >> > qib_devdata *dd) >> > */ >> > static int qib_pcie_caps; >> > module_param_named(pcie_caps, qib_pcie_caps, int, S_IRUGO); >> > -MODULE_PARM_DESC(pcie_caps, "Max PCIe tuning: Payload (0..3), >> > ReadReq (4..7)"); >> > +MODULE_PARM_DESC(pcie_caps, "Max PCIe tuning: ReadReq (4..7)"); >> > >> > static void qib_tune_pcie_caps(struct qib_devdata *dd) >> > { >> > struct pci_dev *parent; >> > - u16 rc_mpss, rc_mps, ep_mpss, ep_mps; >> > u16 rc_mrrs, ep_mrrs, max_mrrs; >> > >> > /* Find out supported and configured values for parent >> > (root) */ >> > @@ -575,30 +574,6 @@ static void qib_tune_pcie_caps(struct >> > qib_devdata *dd) >> > if (!pci_is_pcie(parent) || !pci_is_pcie(dd->pcidev)) >> > return; >> > >> > - rc_mpss = parent->pcie_mpss; >> > - rc_mps = ffs(pcie_get_mps(parent)) - 8; >> > - /* Find out supported and configured values for endpoint >> > (us) */ >> > - ep_mpss = dd->pcidev->pcie_mpss; >> > - ep_mps = ffs(pcie_get_mps(dd->pcidev)) - 8; >> > - >> > - /* Find max payload supported by root, endpoint */ >> > - if (rc_mpss > ep_mpss) >> > - rc_mpss = ep_mpss; >> > - >> > - /* If Supported greater than limit in module param, limit >> > it */ >> > - if (rc_mpss > (qib_pcie_caps & 7)) >> > - rc_mpss = qib_pcie_caps & 7; >> > - /* If less than (allowed, supported), bump root payload */ >> > - if (rc_mpss > rc_mps) { >> > - rc_mps = rc_mpss; >> > - pcie_set_mps(parent, 128 << rc_mps); >> > - } >> > - /* If less than (allowed, supported), bump endpoint >> > payload */ >> > - if (rc_mpss > ep_mps) { >> > - ep_mps = rc_mpss; >> > - pcie_set_mps(dd->pcidev, 128 << ep_mps); >> > - } >> > - >> > /* >> > * Now the Read Request size. >> > * No field for max supported, but PCIe spec limits it to >> > 4096, -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 17, 2015 at 07:06:10PM -0500, Bjorn Helgaas wrote: > On Mon, Aug 17, 2015 at 5:50 PM, Jiang, Dave <dave.jiang@intel.com> wrote: > > On Mon, 2015-08-17 at 17:30 -0500, Bjorn Helgaas wrote: > >> [+cc Mike, linux-rdma] > >> > >> On Wed, Jul 29, 2015 at 04:18:54PM -0600, Keith Busch wrote: > >> > From: Dave Jiang <dave.jiang@intel.com> > >> > > >> > This is in perperation of un-exporting the pcie_set_mps() function > >> > symbol. A driver should not be changing the MPS as that is the > >> > responsibility of the PCI subsystem. > >> > >> Please explain the implications of removing this code. Does this > >> affect > >> performance of the device? If so, how do we get that performance > >> back? > > > > Honestly I don't know. But at the same time I think the driver > > shouldn't be touching the MPS at all. Shouldn't that be left to the > > PCIe subsystem and rely on the PCIe subsystem to set this to a sane > > value? > > Yes, I think in principle the PCI core should own this, but I also > don't want to introduce a performance regression, so I think we need > to understand whether there's a problem, and if there is, fix it. Making sure Mike is CC'd directly.. Mike: I see this has been cut and pasted to HFI1 too, I would be disappointed if HFI needs it as well. :( FWIW, I totally agree with the above. MPS/MRS and related have to do with root port capability, switches in the path and any platform bugs.. I'm not sure why a driver would ever want to mess with this, and since this code doesn't walk the bus toward the root port, it is technically wrong, right? I also find it strange that qib_pcie_caps defaults to zero which means the 'tuning' reduces the payload size to the minimums (??) > >> > /* > >> > * Now the Read Request size. > >> > * No field for max supported, but PCIe spec limits it to > >> > 4096, .. it has been a bit since I looked at this, but IIRC, MRRS is also something that should not be touched by a driver? Jason -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/hw/qib/qib_pcie.c b/drivers/infiniband/hw/qib/qib_pcie.c index 4758a38..b8a2dcd 100644 --- a/drivers/infiniband/hw/qib/qib_pcie.c +++ b/drivers/infiniband/hw/qib/qib_pcie.c @@ -557,12 +557,11 @@ static void qib_tune_pcie_coalesce(struct qib_devdata *dd) */ static int qib_pcie_caps; module_param_named(pcie_caps, qib_pcie_caps, int, S_IRUGO); -MODULE_PARM_DESC(pcie_caps, "Max PCIe tuning: Payload (0..3), ReadReq (4..7)"); +MODULE_PARM_DESC(pcie_caps, "Max PCIe tuning: ReadReq (4..7)"); static void qib_tune_pcie_caps(struct qib_devdata *dd) { struct pci_dev *parent; - u16 rc_mpss, rc_mps, ep_mpss, ep_mps; u16 rc_mrrs, ep_mrrs, max_mrrs; /* Find out supported and configured values for parent (root) */ @@ -575,30 +574,6 @@ static void qib_tune_pcie_caps(struct qib_devdata *dd) if (!pci_is_pcie(parent) || !pci_is_pcie(dd->pcidev)) return; - rc_mpss = parent->pcie_mpss; - rc_mps = ffs(pcie_get_mps(parent)) - 8; - /* Find out supported and configured values for endpoint (us) */ - ep_mpss = dd->pcidev->pcie_mpss; - ep_mps = ffs(pcie_get_mps(dd->pcidev)) - 8; - - /* Find max payload supported by root, endpoint */ - if (rc_mpss > ep_mpss) - rc_mpss = ep_mpss; - - /* If Supported greater than limit in module param, limit it */ - if (rc_mpss > (qib_pcie_caps & 7)) - rc_mpss = qib_pcie_caps & 7; - /* If less than (allowed, supported), bump root payload */ - if (rc_mpss > rc_mps) { - rc_mps = rc_mpss; - pcie_set_mps(parent, 128 << rc_mps); - } - /* If less than (allowed, supported), bump endpoint payload */ - if (rc_mpss > ep_mps) { - ep_mps = rc_mpss; - pcie_set_mps(dd->pcidev, 128 << ep_mps); - } - /* * Now the Read Request size. * No field for max supported, but PCIe spec limits it to 4096,