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

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

[Jiang, Dave]

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,

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

[Bjorn Helgaas]

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,

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

[Jason Gunthorpe]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >> >
> >> > 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-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html