On Tue, Mar 25, 2025 at 10:17 AM Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -533,6 +533,7 @@ static inline bool pci_dev_test_and_set_removed(struct pci_dev *dev)
> >
> > struct aer_err_info {
> > struct pci_dev *dev[AER_MAX_MULTI_ERR_DEVICES];
> > + bool ratelimited[AER_MAX_MULTI_ERR_DEVICES];
>
> In my stop-the-bleeding patch, I pass this as an argument to the functions
> needing it, but this works fine too. Yes, this approach does consume
> a bit more storage, but I doubt that it is enough to matter.
The reason for the boolean array is that we want to eval/use the same
ratelimit for two log functions (aer_print_rp_info() and
aer_print_error()). In past versions[1], I removed aer_print_rp_info()
which simplified the ratelimit eval (i.e. directly eval within
aer_print_error()). However, others found it helpful to keep the root
port logs as it directly showed the linkage from interrupt on root
port -> error source device(s).
> The main concern is that either all information for a given error is
> printed or none of it is, to avoid confusion. (There will of course be
> the possibility of partial drops due to buffer overruns further down
> the console-log pipeline, but no need for additional opportunities
> for confusion.)
>
> For this boolean array to provide this property, the error path for a
> given device must be single threaded, for example, only one interrupt
> handler at a time. Is this the case?
I believe so. AER uses threaded irq where interrupt processing is done
by storing/reading info from a FIFO (i.e. serializes handling).
> > @@ -722,21 +750,31 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info,
> > out:
> > if (info->id && info->error_dev_num > 1 && info->id == id)
> > pci_err(dev, " Error of this Agent is reported first\n");
> > -
> > - trace_aer_event(dev_name(&dev->dev), (info->status & ~info->mask),
> > - info->severity, info->tlp_header_valid, &info->tlp);
> > }
> >
> > static void aer_print_rp_info(struct pci_dev *rp, struct aer_err_info *info)
> > {
> > u8 bus = info->id >> 8;
> > u8 devfn = info->id & 0xff;
> > + struct pci_dev *dev;
> > + bool ratelimited = false;
> > + int i;
> >
> > - pci_info(rp, "%s%s error message received from %04x:%02x:%02x.%d\n",
> > - info->multi_error_valid ? "Multiple " : "",
> > - aer_error_severity_string[info->severity],
> > - pci_domain_nr(rp->bus), bus, PCI_SLOT(devfn),
> > - PCI_FUNC(devfn));
> > + /* extract endpoint device ratelimit */
> > + for (i = 0; i < info->error_dev_num; i++) {
> > + dev = info->dev[i];
> > + if (info->id == pci_dev_id(dev)) {
> > + ratelimited = info->ratelimited[i];
> > + break;
> > + }
> > + }
>
> I cannot resist noting that passing a "ratelimited" argument to this
> function would make it unnecessary to search this array. This would
> require doing rate-limiting checks in aer_isr_one_error(), which looks
> like it should work. Then again, I do not claim to be familiar with
> this AER code.
>
> The "ratelimited" arguments would need to be added to
> aer_print_port_info(), aer_process_err_devices(), and aer_print_error().
> Maybe some that I have missed. Or is there some handoff to
> softirq or workqueues that I missed?
We are not ratelimiting the root port, but the source device that
generated the interrupt on the root port. Thus, we have to search the
array at some point. We could bake this into the topology traversal
(link PCI ID with pci_dev) as another param to aer_error_info, but the
array is <= 8 (i.e. small).
The root port itself can generate AER notifications. Ratelimiting by
both its own errors as well as downstream devices will most likely
mask its own errors.
[1] https://lore.kernel.org/linux-pci/20250214023543.992372-2-pandoh@xxxxxxxxxx/
Thanks,
Jon
