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

[Ian Kumlien <ian.kumlien@gmail.com>]

Actually, from reading the code, it should theoretically only be up...

since:
        /*
         * Re-read upstream/downstream components' register state
         * after clock configuration
         */
        pcie_get_aspm_reg(parent, &upreg);
        pcie_get_aspm_reg(child, &dwreg);
...

But the max was there before? And also, it fixes actual issues? I'm
off to bed.... :)

On Wed, Sep 23, 2020 at 1:29 AM Ian Kumlien <ian.kumlien@gmail.com> wrote:
>
> On Wed, Sep 23, 2020 at 1:00 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > [+cc Alexander]
> >
> > On Tue, Sep 22, 2020 at 11:02:35PM +0200, Ian Kumlien wrote:
> > > On Tue, Sep 22, 2020 at 10:19 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Mon, Aug 03, 2020 at 04:58:32PM +0200, Ian Kumlien wrote:
> >
> > > > > @@ -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;
> > > > > +             }
> > > >
> > > > This accumulates the 1 usec delays for a Switch to propagate the exit
> > > > transition from its Downstream Port to its Upstream Port, but it
> > > > doesn't accumulate the L1 exit latencies themselves for the entire
> > > > path, does it?  I.e., we don't accumulate "latency" for the whole
> > > > path.  Don't we need that?
> > >
> > > Not for L1's apparently, from what I gather the maximum link latency
> > > is "largest latency" + 1us * number-of-hops
> > >
> > > Ie, just like the comment above states - the L1 total time might be
> > > more but  1us is all that is needed to "start" and that propagates
> > > over the link.
> >
> > Ah, you're right!  I don't think this is clear from the existing code
> > comment, but it *is* clear from the example in sec 5.4.1.2.2 (Figure
> > 5-8) of the spec.
> >
> > > @@ -448,14 +449,18 @@ static void pcie_aspm_check_latency(struct
> > > pci_dev *endpoint)
> > >
> > >         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;
> > > +               if (link->aspm_capable & ASPM_STATE_L0S_UP) {
> > > +                       l0s_latency_up += link->latency_up.l0s;
> >
> > It's pretty clear from sec 5.4.1.2.2 that we *don't* need to
> > accumulate the L1 exit latencies.  Unfortunately sec 5.4.1.1.2 about
> > L0s exit doesn't have such a nice example.
> >
> > The L0s *language* is similar though:
> >
> >   5.4.1.1.2: If the Upstream component is a Switch (i.e., it is not
> >   the Root Complex), then it must initiate a transition on its
> >   Upstream Port Transmit Lanes (if the Upstream Port's Transmit Lanes
> >   are in a low-power state) as soon as it detects an exit from L0s on
> >   any of its Downstream Ports.
>
> "it detects an exit"
>
> >   5.4.1.2.1: A Switch is required to initiate an L1 exit transition on
> >   its Upstream Port Link after no more than 1 μs from the beginning of
> >   an L1 exit transition on any of its Downstream Port Links.  during
> >   L1 exit.
>
> vs "from the beginning of"
>
> So to me, this looks like edge triggering - only sense i could make of
> it would be cumulative
>
> (you should also note that i have no L0s issues, but I suspect that
> the code is wrong currently)
>
> > So a switch must start upstream L0s exit "as soon as" it sees L0s exit
> > on any downstream port, while it must start L1 exit "no more than 1 μs"
> > after seeing an L1 exit.
> >
> > And I really can't tell from the spec whether we need to accumulate
> > the L0s exit latencies or not.  Maybe somebody can clarify this.
> >
> > > commit db3d9c4baf4ab177d87b5cd41f624f5901e7390f
> > > Author: Ian Kumlien <ian.kumlien@gmail.com>
> > > Date:   Sun Jul 26 16:01:15 2020 +0200
> > >
> > >     Use maximum latency when determining L1 ASPM
> > >
> > >     If it's not, we clear the link for the path that had too large latency.
> > >
> > >     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
> > >
> > >     See this bugzilla for more information:
> > >     https://bugzilla.kernel.org/show_bug.cgi?id=208741
> > >
> > >     This fixes an issue for me where my desktops machines maximum bandwidth
> > >     for remote connections dropped from 933 MBit to ~40 MBit.
> > >
> > >     The bug became obvious once we enabled ASPM on all links:
> > >     66ff14e59e8a (PCI/ASPM: Allow ASPM on links to PCIe-to-PCI/PCI-X Bridges)
> >
> > I can't connect the dots here yet.  I don't see a PCIe-to-PCI/PCI-X
> > bridge in your lspci, so I can't figure out why this commit would make
> > a difference for you.
> >
> > IIUC, the problem device is 03:00.0, the Intel I211 NIC.  Here's the
> > path to it:
> >
> >   00:01.2 Root Port              to [bus 01-07]
> >   01:00.0 Switch Upstream Port   to [bus 02-07]
> >   02:03.0 Switch Downstream Port to [bus 03]
> >   03:00.0 Endpoint (Intel I211 NIC)
> >
> > And I think this is the relevant info:
> >
> >                                                     LnkCtl    LnkCtl
> >            ------DevCap-------  ----LnkCap-------  -Before-  -After--
> >   00:01.2                                L1 <32us       L1+       L1-
> >   01:00.0                                L1 <32us       L1+       L1-
> >   02:03.0                                L1 <32us       L1+       L1+
> >   03:00.0  L0s <512ns L1 <64us  L0s <2us L1 <16us  L0s- L1-  L0s- L1-
> >
> > The NIC says it can tolerate at most 512ns of L0s exit latency and at
> > most 64us of L1 exit latency.
> >
> > 02:03.0 doesn't support L0s, and the NIC itself can't exit L0s that
> > fast anyway (it can only do <2us), so L0s should be out of the picture
> > completely.
> >
> > Before your patch, apparently we (or BIOS) enabled L1 on the link from
> > 00:01.2 to 01:00.0, and partially enabled it on the link from 02:03.0
> > to 03:00.0.
>
> According to the spec, this is managed by the OS - which was the
> change introduced...
>
> > It looks like we *should* be able to enable L1 on both links since the
> > exit latency should be <33us (first link starts exit at T=0, completes
> > by T=32; second link starts exit at T=1, completes by T=33), and
> > 03:00.0 can tolerate up to 64us.
> >
> > I guess the effect of your patch is to disable L1 on the 00:01.2 -
> > 01:00.0 link?  And that makes the NIC work better?  I am obviously
> > missing something because I don't understand why the patch does that
> > or why it works better.
>
> It makes it work like normal again, like if i disable ASPM on the nic itself...
>
> I don't know which value that reflects, up or down - since we do max
> of both values and
> it actually disables ASPM.
>
> What we can see is that the first device that passes the threshold is 01:00.0
>
> How can I read more data from PCIe without needing to add kprint...
>
> This is what lspci uses apparently:
> #define  PCI_EXP_LNKCAP_L0S     0x07000 /* L0s Exit Latency */
> #define  PCI_EXP_LNKCAP_L1      0x38000 /* L1 Exit Latency */
>
> But which latencies are those? up or down?
>
> > I added Alexander to cc since it sounds like he's helped debug this,
> > too.
> >
> > >     Signed-off-by: Ian Kumlien <ian.kumlien@gmail.com>
> > >
> > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > > index 253c30cc1967..893b37669087 100644
> > > --- a/drivers/pci/pcie/aspm.c
> > > +++ b/drivers/pci/pcie/aspm.c
> > > @@ -434,7 +434,7 @@ 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;
> > >         struct aspm_latency *acceptable;
> > >         struct pcie_link_state *link;
> > >
> > > @@ -456,10 +456,14 @@ static void pcie_aspm_check_latency(struct
> > > pci_dev *endpoint)
> > >                 if ((link->aspm_capable & ASPM_STATE_L0S_DW) &&
> > >                     (link->latency_dw.l0s > acceptable->l0s))
> > >                         link->aspm_capable &= ~ASPM_STATE_L0S_DW;
> > > +
> > >                 /*
> > >                  * Check L1 latency.
> > > -                * Every switch on the path to root complex need 1
> > > -                * more microsecond for L1. Spec doesn't mention L0s.
> > > +                *
> > > +                * PCIe r5.0, sec 5.4.1.2.2 states:
> > > +                * A Switch is required to initiate an L1 exit transition on its
> > > +                * Upstream Port Link after no more than 1 μs from the
> > > beginning of an
> > > +                * L1 exit transition on any of its Downstream Port Links.
> > >                  *
> > >                  * The exit latencies for L1 substates are not advertised
> > >                  * by a device.  Since the spec also doesn't mention a way
> > > @@ -469,11 +473,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;
> > >         }
> > > ----------------------