Re: [PATCH v3 5/8] PCI/AER: Introduce ratelimit for error logs

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Mar 19, 2025 at 01:40:46AM -0700, Jon Pan-Doh wrote:
> Spammy devices can flood kernel logs with AER errors and slow/stall
> execution. Add per-device ratelimits for AER correctable and uncorrectable
> errors that use the kernel defaults (10 per 5s).
> 
> Tested using aer-inject[1]. Sent 11 AER errors. Observed 10 errors logged
> while AER stats (cat /sys/bus/pci/devices/<dev>/aer_dev_correctable) show
> true count of 11.

I think this is really on the right track.  A few minor comments
below.

> @@ -697,6 +711,7 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info,
>  {
>  	int layer, agent;
>  	int id = pci_dev_id(dev);
> +	struct ratelimit_state *ratelimit;
>  
>  	if (!info->status) {
>  		pci_err(dev, "PCIe Bus Error: severity=%s, type=Inaccessible, (Unregistered Agent ID)\n",
> @@ -704,6 +719,17 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info,
>  		goto out;
>  	}
>  
> +	if (info->severity == AER_CORRECTABLE)
> +		ratelimit = &dev->aer_report->cor_log_ratelimit;
> +	else
> +		ratelimit = &dev->aer_report->uncor_log_ratelimit;
> +
> +	trace_aer_event(dev_name(&dev->dev), (info->status & ~info->mask),
> +			info->severity, info->tlp_header_valid, &info->tlp);
> +
> +	if (!__ratelimit(ratelimit))
> +		return;

  - I think the ratelimit lookup and __ratelimit() call should be
    together since there's no need for trace_aer_event() to be in the
    middle.

  - The lookup and __ratelimit() calls are repeated and are probably
    worth factoring out into something like this:

      static int aer_ratelimit(struct pci_dev *dev, unsigned int severity)

  - Previously we *always* called trace_aer_event(), but now we don't
    in the !info->status case.  Maybe an unintentional change?  I
    think we should call trace_aer_event() always, or change that in a
    separate patch if we need to.  This would always have been simpler
    if trace_aer_event() had been the very first thing in the
    function.

  - The !info->status case message is not rate-limited.  Seems like
    maybe it should be?

>  	layer = AER_GET_LAYER_ERROR(info->severity, info->status);
>  	agent = AER_GET_AGENT(info->severity, info->status);
>  
> @@ -722,21 +748,33 @@ 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);
>  }