Re: [PATCH] Use maximum latency when determining L1/L0s ASPM v2

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Oct 07, 2020 at 01:31:48PM +0200, Ian Kumlien wrote:
> On Mon, Oct 5, 2020 at 9:09 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Mon, Oct 05, 2020 at 08:38:55PM +0200, Ian Kumlien wrote:
> > > On Mon, Oct 5, 2020 at 8:31 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > >
> > > > On Mon, Aug 03, 2020 at 04:58:32PM +0200, Ian Kumlien wrote:
> > > > > Changes:
> > > > > * Handle L0s correclty as well, making it per direction
> > > > > * Moved the switch cost in to the if statement since a non L1 switch has
> > > > >   no additional cost.
> > > > >
> > > > > For L0s:
> > > > > We sumarize the entire latency per direction to see if it's acceptable
> > > > > for the PCIe endpoint.
> > > > >
> > > > > If it's not, we clear the link for the path that had too large latency.
> > > > >
> > > > > For L1:
> > > > > Currently we check the maximum latency of upstream and downstream
> > > > > per link, not the maximum for the path
> > > > >
> > > > > This would work if all links have the same latency, but:
> > > > > endpoint -> c -> b -> a -> root  (in the order we walk the path)
> > > > >
> > > > > If c or b has the higest latency, it will not register
> > > > >
> > > > > Fix this by maintaining the maximum latency value for the path
> > > > >
> > > > > This change fixes a regression introduced (but not caused) by:
> > > > > 66ff14e59e8a (PCI/ASPM: Allow ASPM on links to PCIe-to-PCI/PCI-X Bridges)
> > > > >
> > > > > Signed-off-by: Ian Kumlien <ian.kumlien@gmail.com>
> > > >
> > > > I'm not sure where we're at with this.  If we can come up with:
> > > >
> > > >   - "lspci -vv" for the entire affected hierarchy before the fix
> > > >
> > > >   - specific identification of incorrect configuration per spec
> > > >
> > > >   - patch that fixes that specific misconfiguration
> > > >
> > > >   - "lspci -vv" for the entire affected hierarchy after the fix
> > > >
> > > > then we have something to work with.  It doesn't have to (and should
> > > > not) fix all the problems at once.
> > >
> > > So detail the changes on my specific machine and then mention
> > > 5.4.1.2.2 of the pci spec
> > > detailing the exit from PCIe ASPM L1?
> >
> > Like I said, I need to see the current ASPM configuration, a note
> > about what is wrong with it (this probably involves a comparison with
> > what the spec says it *should* be), and the configuration after the
> > patch showing that it's now fixed.
> >
> > > Basically writing a better changelog for the first patch?
> > >
> > > Any comments on the L0s patch?
> >
> > Not yet.  When it's packaged up in mergeable form I'll review it.  I
> > just don't have time to extract everything myself.
> 
> So, did it like this, since I don't think the output from my system
> actually is important.
> (I added some descriptive text that is loosely based on the spec)

I think the before/after lspci from your system *is* important.
Concrete examples are a big help.

The commit log below looks accurate, but of course needs to be
attached to a specific patch.  Can you please post the complete patch
with commit log all by itself so it's not buried in the middle of this
long thread?

> ----
> Use maximum latency when determining L1 ASPM
> 
> Make pcie_aspm_check_latency comply with the PCIe spec, specifically:
> "5.4.1.2.2. Exit from the L1 State"
> 
> Which makes it clear that each switch is required to initiate a
> transition within 1μs from receiving it, accumulating this latency and
> then we have to wait for the slowest link along the path before
> entering L0 state from L1.
> 
> The current code doesn't take the maximum latency into account.
> 
> From the example:
>    +----------------+
>    |                |
>    |  Root complex  |
>    |                |
>    |    +-----+     |
>    |    |32 μs|     |
>    +----------------+
>            |
>            |  Link 1
>            |
>    +----------------+
>    |     |8 μs|     |
>    |     +----+     |
>    |    Switch A    |
>    |     +----+     |
>    |     |8 μs|     |
>    +----------------+
>            |
>            |  Link 2
>            |
>    +----------------+
>    |    |32 μs|     |
>    |    +-----+     |
>    |    Switch B    |
>    |    +-----+     |
>    |    |32 μs|     |
>    +----------------+
>            |
>            |  Link 3
>            |
>    +----------------+
>    |     |8μs|      |
>    |     +---+      |
>    |   Endpoint C   |
>    |                |
>    |                |
>    +----------------+
> 
> Links 1, 2 and 3 are all in L1 state - endpoint C initiates the
> transition to L0 at time T. Since switch B takes 32 μs to exit L1 on
> it's ports, Link 3 will transition to L0 at T+32 (longest time
> considering T+8 for endpoint C and T+32 for switch B).
> 
> Switch B is required to initiate a transition from the L1 state on it's
> upstream port after no more than 1 μs from the beginning of the
> transition from L1 state on the downstream port. Therefore, transition from
> L1 to L0 will begin on link 2 at T+1, this will cascade up the path.
> 
> The path will exit L1 at T+34.
> 
> Signed-off-by: Ian Kumlien <ian.kumlien@gmail.com>
> ----
> 
> > > > > ---
> > > > >  drivers/pci/pcie/aspm.c | 41 ++++++++++++++++++++++++++---------------
> > > > >  1 file changed, 26 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > > > > index b17e5ffd31b1..bc512e217258 100644
> > > > > --- a/drivers/pci/pcie/aspm.c
> > > > > +++ b/drivers/pci/pcie/aspm.c
> > > > > @@ -434,7 +434,8 @@ static void pcie_get_aspm_reg(struct pci_dev *pdev,
> > > > >
> > > > >  static void pcie_aspm_check_latency(struct pci_dev *endpoint)
> > > > >  {
> > > > > -     u32 latency, l1_switch_latency = 0;
> > > > > +     u32 latency, l1_max_latency = 0, l1_switch_latency = 0,
> > > > > +             l0s_latency_up = 0, l0s_latency_dw = 0;
> > > > >       struct aspm_latency *acceptable;
> > > > >       struct pcie_link_state *link;
> > > > >
> > > > > @@ -447,15 +448,22 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
> > > > >       acceptable = &link->acceptable[PCI_FUNC(endpoint->devfn)];
> > > > >
> > > > >       while (link) {
> > > > > -             /* Check upstream direction L0s latency */
> > > > > -             if ((link->aspm_capable & ASPM_STATE_L0S_UP) &&
> > > > > -                 (link->latency_up.l0s > acceptable->l0s))
> > > > > -                     link->aspm_capable &= ~ASPM_STATE_L0S_UP;
> > > > > -
> > > > > -             /* Check downstream direction L0s latency */
> > > > > -             if ((link->aspm_capable & ASPM_STATE_L0S_DW) &&
> > > > > -                 (link->latency_dw.l0s > acceptable->l0s))
> > > > > -                     link->aspm_capable &= ~ASPM_STATE_L0S_DW;
> > > > > +             if (link->aspm_capable & ASPM_STATE_L0S) {
> > > > > +                     /* Check upstream direction L0s latency */
> > > > > +                     if (link->aspm_capable & ASPM_STATE_L0S_UP) {
> > > > > +                             l0s_latency_up += link->latency_up.l0s;
> > > > > +                             if (l0s_latency_up > acceptable->l0s)
> > > > > +                                     link->aspm_capable &= ~ASPM_STATE_L0S_UP;
> > > > > +                     }
> > > > > +
> > > > > +                     /* Check downstream direction L0s latency */
> > > > > +                     if (link->aspm_capable & ASPM_STATE_L0S_DW) {
> > > > > +                             l0s_latency_dw += link->latency_dw.l0s;
> > > > > +                             if (l0s_latency_dw > acceptable->l0s)
> > > > > +                                     link->aspm_capable &= ~ASPM_STATE_L0S_DW;
> > > > > +                     }
> > > > > +             }
> > > > > +
> > > > >               /*
> > > > >                * Check L1 latency.
> > > > >                * Every switch on the path to root complex need 1
> > > > > @@ -469,11 +477,14 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
> > > > >                * L1 exit latencies advertised by a device include L1
> > > > >                * substate latencies (and hence do not do any check).
> > > > >                */
> > > > > -             latency = max_t(u32, link->latency_up.l1, link->latency_dw.l1);
> > > > > -             if ((link->aspm_capable & ASPM_STATE_L1) &&
> > > > > -                 (latency + l1_switch_latency > acceptable->l1))
> > > > > -                     link->aspm_capable &= ~ASPM_STATE_L1;
> > > > > -             l1_switch_latency += 1000;
> > > > > +             if (link->aspm_capable & ASPM_STATE_L1) {
> > > > > +                     latency = max_t(u32, link->latency_up.l1, link->latency_dw.l1);
> > > > > +                     l1_max_latency = max_t(u32, latency, l1_max_latency);
> > > > > +                     if (l1_max_latency + l1_switch_latency > acceptable->l1)
> > > > > +                             link->aspm_capable &= ~ASPM_STATE_L1;
> > > > > +
> > > > > +                     l1_switch_latency += 1000;
> > > > > +             }
> > > > >
> > > > >               link = link->parent;
> > > > >       }
> > > > > --
> > > > > 2.28.0
> > > > >