On Thu, Mar 20, 2025 at 10:51 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > On Thu, Mar 20, 2025 at 03:56:53PM +0100, Karolina Stolarek wrote: > > On 20/03/2025 09:20, Jon Pan-Doh wrote: > > > + /* > > > + * Ratelimits are doubled as a given error produces 2 logs (root port > > > + * and endpoint) that should be under same ratelimit. > > > + */ > > For these devices, we would call the ratelimit > > just once. I don't have a nice an clean solution for this problem, I just > > wanted to highlight that 1) we don't use the Root Port's ratelimit in > > aer_print_port_info(), 2) we may use the bursts to either print port_info + > > error message or just the message, in different combinations. I think we > > should reword this comment to highlight the fact that we don't check the > > ratelimit once per error, we could do it twice. You're right. I was thinking of amending it to something like: Ratelimits are doubled as a given error notification produces up to 2 logs (1 at root port and 1 at source device) that should be under the same ratelimit. > Very good point. aer_print_rp_info() is always ratelimited based on > the ERR_* Source Identification, but if Multiple ERR_* is set, > aer_print_error() ratelimits based on whatever downstream device we > found that had any error of the same class logged. > > That does seem like a problem. I would propose that we always > ratelimit using the device from Source Identification. I think that's > available in aer_print_error(); we would just use info->id instead of > "dev". Wouldn't you be incorrectly counting the non-source ID devices then? I think this is another reason where removing aer_print_port_info()[1] (only printing port info when failing to get device error info) simplifies things. Of course, we then have to weigh whether the loss of info is less than the ratelimit complexity. > > I'm also thinking -- we are ratelimiting the aer_print_port_info() and > > aer_print_error(). What about the messages in dpc_process_error()? Should we > > check early if DPC was triggered because of an uncorrectable error, and if > > so, ratelimit that? > > Good question. It does seem like the dpc_process_error() messages > should be similarly ratelimited. I think we currently only enable DPC > for fatal errors, and the Downstream Port takes the link down, which > resets the hierarchy below. So (1) we probably won't see storms of > fatal error messages, and (2) it looks like we might not print any > error info from downstream devices, since they're not reachable while > the link is down. I did not expect error storms from DPC so I thought it best to focus on AER. [1] https://lore.kernel.org/linux-pci/CAMC_AXWVOtKh2r4kX6c7jtJwQaEE4KEQsH=uoB1OhczJ=8K2VA@xxxxxxxxxxxxxx/ Thanks, Jon On Thu, Mar 20, 2025 at 10:51 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > On Thu, Mar 20, 2025 at 03:56:53PM +0100, Karolina Stolarek wrote: > > On 20/03/2025 09:20, 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). > > > > + /* > > > + * Ratelimits are doubled as a given error produces 2 logs (root port > > > + * and endpoint) that should be under same ratelimit. > > > + */ > > > > It took me a bit to understand what this comment is about. > > > > When we handle an error message, we first use the source's ratelimit to > > decide if we want to print the port info, and then the actual error. In > > theory, there could be more errors of the same class coming from other > > devices within one message. > > I think this refers to the fact that when we get an AER interrupt from > a Root Port, the RP has a single Requester ID logged in the Source > Identification, but if Multiple ERR_* is set, find_device_iter() may > collect error info from several devices? > > > For these devices, we would call the ratelimit > > just once. I don't have a nice an clean solution for this problem, I just > > wanted to highlight that 1) we don't use the Root Port's ratelimit in > > aer_print_port_info(), 2) we may use the bursts to either print port_info + > > error message or just the message, in different combinations. I think we > > should reword this comment to highlight the fact that we don't check the > > ratelimit once per error, we could do it twice. > > Very good point. aer_print_rp_info() is always ratelimited based on > the ERR_* Source Identification, but if Multiple ERR_* is set, > aer_print_error() ratelimits based on whatever downstream device we > found that had any error of the same class logged. > > E.g., if we had something like this topology: > > 00:1c.0 Root Port to [bus 01-04] > 01:00.0 Switch Upstream Port to [bus 02-04] > 02:00.0 Switch Downstream Port to [bus 03] > 02:00.1 Switch Downstream Port to [bus 04] > 03:00.0 NIC > 04:00.0 NVMe > > where 03:00.0 and 04:00.0 both logged ERR_FATAL, and the Root Port > received the 03:00.0 message first, 03:00.0 would be logged as the > Source Identification, and Multiple ERR_FATAL Received should be set. > The messages related to 00:1c.0 and 03:00.0 would be ratelimited based > on 03:00.0, but aer_print_error() messages related to 04:00.0 would be > ratelimited based on 04:00.0. > > That does seem like a problem. I would propose that we always > ratelimit using the device from Source Identification. I think that's > available in aer_print_error(); we would just use info->id instead of > "dev". > > That does mean we'd have to figure out the pci_dev corresponding to > the Requester ID in info->id. Maybe we could add an > aer_err_info.src_dev pointer, and fill it in when find_device_iter() > finds the right device? > > > Also, I wonder -- do only Endpoints generate error messages? From what I > > understand, that some errors can be detected by intermediary devices. > > Yes, I think any device, including switches between a Root Port and > Endpoint, can detect errors and generate error messages. > > I guess this means the "endpoint" variable in aer_print_port_info() is > probably too specific. IIUC the aer_print_port_info() "dev" parameter > is always a Root Port since it came from aer_rpc.rpd. Naming it "rp" > would make this more obvious and free up "dev" for the source device. > And "aer_print_port_info" itself could be more descriptive, e.g., > "aer_print_rp_info()", since *every* PCIe device has a Port. > > > I'm also thinking -- we are ratelimiting the aer_print_port_info() and > > aer_print_error(). What about the messages in dpc_process_error()? Should we > > check early if DPC was triggered because of an uncorrectable error, and if > > so, ratelimit that? > > Good question. It does seem like the dpc_process_error() messages > should be similarly ratelimited. I think we currently only enable DPC > for fatal errors, and the Downstream Port takes the link down, which > resets the hierarchy below. So (1) we probably won't see storms of > fatal error messages, and (2) it looks like we might not print any > error info from downstream devices, since they're not reachable while > the link is down. > > It does seem like we *should* try to print that info after the link > comes back up, since the log registers are sticky and should survive > the reset. Maybe we do that already and I just missed it. > > This seems like something we could put off a little bit while we deal > with the AER correctable error issue. > > Bjorn
