diff mbox

[2/3] QIB: Removing usage of pcie_set_mps()

Message ID 1438208335-19457-3-git-send-email-keith.busch@intel.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Keith Busch July 29, 2015, 10:18 p.m. UTC 
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.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/infiniband/hw/qib/qib_pcie.c |   27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)

Comments

Bjorn Helgaas Aug. 17, 2015, 10:30 p.m. UTC | #1 
[+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
Dave Jiang Aug. 17, 2015, 10:50 p.m. UTC | #2 
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,
Bjorn Helgaas Aug. 18, 2015, 12:06 a.m. UTC | #3 
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
Jason Gunthorpe Aug. 18, 2015, 1:49 a.m. UTC | #4 
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 mbox

Patch

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,