Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CANEJEGsE6KS484iSLkKV8hx2nNThZGfaaz+u+R-A3X5nRev6Gg@mail.gmail.com>
 <ZGT6sTOtk+WY3aYt@bhelgaas>
In-Reply-To: <ZGT6sTOtk+WY3aYt@bhelgaas>
From:   Grant Grundler <grundler@chromium.org>
Date:   Wed, 17 May 2023 14:02:43 -0700
Message-ID: <CANEJEGv8yxcYmrn4dsc0GCrcMGSFJNoJ=-VUvTjPLCVug+X29w@mail.gmail.com>
Subject: Re: [PATCHv2 pci-next 2/2] PCI/AER: Rate limit the reporting of the
 correctable errors
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Grant Grundler <grundler@chromium.org>,
        Rajat Jain <rajatja@chromium.org>,
        Rajat Khandelwal <rajat.khandelwal@linux.intel.com>,
        linux-pci@vger.kernel.org,
        Mahesh J Salgaonkar <mahesh@linux.ibm.com>,
        linux-kernel@vger.kernel.org,
        "Oliver O 'Halloran" <oohall@gmail.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        linuxppc-dev@lists.ozlabs.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, May 17, 2023 at 9:03=E2=80=AFAM Bjorn Helgaas <helgaas@kernel.org> =
wrote:
>
> On Fri, Apr 07, 2023 at 04:46:03PM -0700, Grant Grundler wrote:
> > On Fri, Apr 7, 2023 at 12:46=E2=80=AFPM Bjorn Helgaas <helgaas@kernel.o=
rg> wrote:
> > > On Fri, Apr 07, 2023 at 11:53:27AM -0700, Grant Grundler wrote:
> > > > On Thu, Apr 6, 2023 at 12:50=E2=80=AFPM Bjorn Helgaas <helgaas@kern=
el.org>
> > > wrote:
> > > > > On Fri, Mar 17, 2023 at 10:51:09AM -0700, Grant Grundler wrote:
> > > > > > From: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
> > > > > >
> > > > > > There are many instances where correctable errors tend to inund=
ate
> > > > > > the message buffer. We observe such instances during thunderbol=
t PCIe
> > > > > > tunneling.
> > > > ...
> > >
> > > > > >               if (info->severity =3D=3D AER_CORRECTABLE)
> > > > > > -                     pci_info(dev, "   [%2d] %-22s%s\n", i, er=
rmsg,
> > > > > > -                             info->first_error =3D=3D i ? " (F=
irst)" :
> > > "");
> > > > > > +                     pci_info_ratelimited(dev, "   [%2d]
> > > %-22s%s\n", i, errmsg,
> > > > > > +                                          info->first_error =
=3D=3D i ?
> > > " (First)" : "");
> > > > >
> > > > > I don't think this is going to reliably work the way we want.  We=
 have
> > > > > a bunch of pci_info_ratelimited() calls, and each caller has its =
own
> > > > > ratelimit_state data.  Unless we call pci_info_ratelimited() exac=
tly
> > > > > the same number of times for each error, the ratelimit counters w=
ill
> > > > > get out of sync and we'll end up printing fragments from error A =
mixed
> > > > > with fragments from error B.
> > > >
> > > > Ok - what I'm reading between the lines here is the output should b=
e
> > > > emitted in one step, not multiple pci_info_ratelimited() calls. if =
the
> > > > code built an output string (using sprintnf()), and then called
> > > > pci_info_ratelimited() exactly once at the bottom, would that be
> > > > sufficient?
> > > >
> > > > > I think we need to explicitly manage the ratelimiting ourselves,
> > > > > similar to print_hmi_event_info() or print_extlog_rcd().  Then we=
 can
> > > > > have a *single* ratelimit_state, and we can check it once to dete=
rmine
> > > > > whether to log this correctable error.
> > > >
> > > > Is the rate limiting per call location or per device? From above, I
> > > > understood rate limiting is "per call location".  If the code only
> > > > has one call location, it should achieve the same goal, right?
> > >
> > > Rate-limiting is per call location, so yes, if we only have one call
> > > location, that would solve it.  It would also have the nice property
> > > that all the output would be atomic so it wouldn't get mixed with
> > > other stuff, and it might encourage us to be a little less wordy in
> > > the output.
> > >
> >
> > +1 to all of those reasons. Especially reducing the number of lines out=
put.
> >
> > I'm going to be out for the next week. If someone else (Rajat Kendalwal
> > maybe?) wants to rework this to use one call location it should be fair=
ly
> > straight forward. If not, I'll tackle this when I'm back (in 2 weeks
> > essentially).
>
> Ping?  Really hoping to merge this for v6.5.

Sorry - I forgot about this... I'll take a shot at it. Should have
something by this evening.

cheers,
grant

>
> Bjorn