[PATCH v5 0/8] Rate limit AER logs

[PATCH v5 0/8] Rate limit AER logs

[Jon Pan-Doh]

Proposal
========

When using native AER, spammy devices can flood kernel logs with AER errors
and slow/stall execution. Add per-device per-error-severity ratelimits
for more robust error logging. Allow userspace to configure ratelimits
via sysfs knobs.

Motivation
==========

Inconsistent PCIe error handling, exacerbated at datacenter scale (myriad
of devices), affects repairabilitiy flows for fleet operators.

Exposing PCIe errors/debug info in-band for a userspace daemon (e.g.
rasdaemon) to collect/pass on to repairability services will allow for
more predictable repair flows and decrease machine downtime.

Background
==========

AER error spam has been observed many times, both publicly (e.g. [1], [2],
[3]) and privately. While it usually occurs with correctable errors, it can
happen with uncorrectable errors (e.g. during new HW bringup). 

There have been previous attempts to add ratelimits to AER logs ([4],
[5]). The most recent attempt[5] has many similarities with the proposed
approach.

Patch organization
==================
1-5 AER logging cleanup
6-8 Ratelimits and sysfs knobs

Outstanding work
================
Cleanup:
- Consolidate aer_print_error() and pci_print_error() path[6]

Roadmap:
- IRQ ratelimiting

v5:
- Handle multi-error AER by evaluating ratelimits once and storing result
- Reword/rename commit messages/functions/variable
v4:
- Fix bug where trace not emitted with malformed aer_err_info
- Extend ratelimit to malformed aer_err_info
- Update commit messages with patch motivation
- Squash AER sysfs filename change (Patch 8)
v3:
- Ratelimit aer_print_port_info() (drop Patch 1)
- Add ratelimit enable toggle
- Move trace outside of ratelimit
- Split log level (Patch 2) into two
- More descriptive documentation/sysfs naming
v2:
- Rebased on top of pci/aer (6.14.rc-1)
- Split series into log and IRQ ratelimits (defer patch 5)
- Dropped patch 8 (Move AER sysfs)
- Added log level cleanup patch[7] from Karolina's series
- Fixed bug where dpc errors didn't increment counters
- "X callbacks suppressed" message on ratelimit release -> immediately
- Separate documentation into own patch

[1] https://bugzilla.kernel.org/show_bug.cgi?id=215027
[2] https://bugzilla.kernel.org/show_bug.cgi?id=201517
[3] https://bugzilla.kernel.org/show_bug.cgi?id=196183
[4] https://lore.kernel.org/linux-pci/20230606035442.2886343-2-grundler@chromium.org/
[5] https://lore.kernel.org/linux-pci/cover.1736341506.git.karolina.stolarek@oracle.com/
[6] https://lore.kernel.org/linux-pci/8bcb8c9a7b38ce3bdaca5a64fe76f08b0b337511.1742202797.git.karolina.stolarek@oracle.com/
[7] https://lore.kernel.org/linux-pci/edd77011aafad4c0654358a26b4e538d0c5a321d.1736341506.git.karolina.stolarek@oracle.com/

Jon Pan-Doh (6):
  PCI/AER: Move AER stat collection out of __aer_print_error()
  PCI/AER: Rename aer_print_port_info() to aer_printrp_info()
  PCI/AER: Rename struct aer_stats to aer_report
  PCI/AER: Introduce ratelimit for error logs
  PCI/AER: Add ratelimits to PCI AER Documentation
  PCI/AER: Add sysfs attributes for log ratelimits

Karolina Stolarek (2):
  PCI/AER: Check log level once and propagate down
  PCI/AER: Make all pci_print_aer() log levels depend on error type

 ...es-aer_stats => sysfs-bus-pci-devices-aer} |  34 +++
 Documentation/PCI/pcieaer-howto.rst           |  16 +-
 drivers/pci/pci-sysfs.c                       |   1 +
 drivers/pci/pci.h                             |   6 +-
 drivers/pci/pcie/aer.c                        | 282 +++++++++++++-----
 drivers/pci/pcie/dpc.c                        |   3 +-
 include/linux/pci.h                           |   2 +-
 7 files changed, 270 insertions(+), 74 deletions(-)
 rename Documentation/ABI/testing/{sysfs-bus-pci-devices-aer_stats => sysfs-bus-pci-devices-aer} (77%)

-- 
2.49.0.395.g12beb8f557-goog

[PATCH v5 1/8] PCI/AER: Check log level once and propagate down

[Jon Pan-Doh]

From: Karolina Stolarek <karolina.stolarek@oracle.com>

When reporting an AER error, we check its type multiple times
to determine the log level for each message. Do this check only
in the top-level functions (aer_isr_one_error(), pci_print_aer()) and
propagate the result down the call chain.

Signed-off-by: Karolina Stolarek <karolina.stolarek@oracle.com>
Signed-off-by: Jon Pan-Doh <pandoh@google.com>
Reported-by: Sargun Dhillon <sargun@meta.com>
Acked-by: Paul E. McKenney <paulmck@kernel.org>
Reviewed-by: Jon Pan-Doh <pandoh@google.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pci.h      |  2 +-
 drivers/pci/pcie/aer.c | 34 +++++++++++++++++-----------------
 drivers/pci/pcie/dpc.c |  2 +-
 3 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index b8911d1e10dc..75985b96ecc1 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -551,7 +551,7 @@ struct aer_err_info {
 };
 
 int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
-void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
+void aer_print_error(struct pci_dev *dev, struct aer_err_info *info, const char *level);
 
 int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
 		      unsigned int tlp_len, bool flit,
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 9cff7069577e..45629e1ea058 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -670,20 +670,18 @@ static void pci_rootport_aer_stats_incr(struct pci_dev *pdev,
 }
 
 static void __aer_print_error(struct pci_dev *dev,
-			      struct aer_err_info *info)
+			      struct aer_err_info *info,
+			      const char *level)
 {
 	const char **strings;
 	unsigned long status = info->status & ~info->mask;
-	const char *level, *errmsg;
+	const char *errmsg;
 	int i;
 
-	if (info->severity == AER_CORRECTABLE) {
+	if (info->severity == AER_CORRECTABLE)
 		strings = aer_correctable_error_string;
-		level = KERN_WARNING;
-	} else {
+	else
 		strings = aer_uncorrectable_error_string;
-		level = KERN_ERR;
-	}
 
 	for_each_set_bit(i, &status, 32) {
 		errmsg = strings[i];
@@ -696,11 +694,11 @@ static void __aer_print_error(struct pci_dev *dev,
 	pci_dev_aer_stats_incr(dev, info);
 }
 
-void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
+void aer_print_error(struct pci_dev *dev, struct aer_err_info *info,
+		     const char *level)
 {
 	int layer, agent;
 	int id = pci_dev_id(dev);
-	const char *level;
 
 	if (!info->status) {
 		pci_err(dev, "PCIe Bus Error: severity=%s, type=Inaccessible, (Unregistered Agent ID)\n",
@@ -711,8 +709,6 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
 	layer = AER_GET_LAYER_ERROR(info->severity, info->status);
 	agent = AER_GET_AGENT(info->severity, info->status);
 
-	level = (info->severity == AER_CORRECTABLE) ? KERN_WARNING : KERN_ERR;
-
 	aer_printk(level, dev, "PCIe Bus Error: severity=%s, type=%s, (%s)\n",
 		   aer_error_severity_string[info->severity],
 		   aer_error_layer[layer], aer_agent_string[agent]);
@@ -720,7 +716,7 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
 	aer_printk(level, dev, "  device [%04x:%04x] error status/mask=%08x/%08x\n",
 		   dev->vendor, dev->device, info->status, info->mask);
 
-	__aer_print_error(dev, info);
+	__aer_print_error(dev, info, level);
 
 	if (info->tlp_header_valid)
 		pcie_print_tlp_log(dev, &info->tlp, dev_fmt("  "));
@@ -765,15 +761,18 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,
 {
 	int layer, agent, tlp_header_valid = 0;
 	u32 status, mask;
+	const char *level;
 	struct aer_err_info info;
 
 	if (aer_severity == AER_CORRECTABLE) {
 		status = aer->cor_status;
 		mask = aer->cor_mask;
+		level = KERN_WARNING;
 	} else {
 		status = aer->uncor_status;
 		mask = aer->uncor_mask;
 		tlp_header_valid = status & AER_LOG_TLP_MASKS;
+		level = KERN_ERR;
 	}
 
 	layer = AER_GET_LAYER_ERROR(aer_severity, status);
@@ -786,7 +785,7 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,
 	info.first_error = PCI_ERR_CAP_FEP(aer->cap_control);
 
 	pci_err(dev, "aer_status: 0x%08x, aer_mask: 0x%08x\n", status, mask);
-	__aer_print_error(dev, &info);
+	__aer_print_error(dev, &info, level);
 	pci_err(dev, "aer_layer=%s, aer_agent=%s\n",
 		aer_error_layer[layer], aer_agent_string[agent]);
 
@@ -1257,14 +1256,15 @@ int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
 	return 1;
 }
 
-static inline void aer_process_err_devices(struct aer_err_info *e_info)
+static inline void aer_process_err_devices(struct aer_err_info *e_info,
+					   const char *level)
 {
 	int i;
 
 	/* Report all before handle them, not to lost records by reset etc. */
 	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
 		if (aer_get_device_error_info(e_info->dev[i], e_info))
-			aer_print_error(e_info->dev[i], e_info);
+			aer_print_error(e_info->dev[i], e_info, level);
 	}
 	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
 		if (aer_get_device_error_info(e_info->dev[i], e_info))
@@ -1300,7 +1300,7 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
 		aer_print_port_info(pdev, &e_info);
 
 		if (find_source_device(pdev, &e_info))
-			aer_process_err_devices(&e_info);
+			aer_process_err_devices(&e_info, KERN_WARNING);
 	}
 
 	if (e_src->status & PCI_ERR_ROOT_UNCOR_RCV) {
@@ -1319,7 +1319,7 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
 		aer_print_port_info(pdev, &e_info);
 
 		if (find_source_device(pdev, &e_info))
-			aer_process_err_devices(&e_info);
+			aer_process_err_devices(&e_info, KERN_ERR);
 	}
 }
 
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index df42f15c9829..9e4c9ac737a7 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -289,7 +289,7 @@ void dpc_process_error(struct pci_dev *pdev)
 	else if (reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_UNCOR &&
 		 dpc_get_aer_uncorrect_severity(pdev, &info) &&
 		 aer_get_device_error_info(pdev, &info)) {
-		aer_print_error(pdev, &info);
+		aer_print_error(pdev, &info, KERN_ERR);
 		pci_aer_clear_nonfatal_status(pdev);
 		pci_aer_clear_fatal_status(pdev);
 	}
-- 
2.49.0.395.g12beb8f557-goog

[PATCH v5 2/8] PCI/AER: Make all pci_print_aer() log levels depend on error type

[Jon Pan-Doh]

From: Karolina Stolarek <karolina.stolarek@oracle.com>

Some existing logs in pci_print_aer() log with error severity
by default. Convert them to depend on error type (consistent
with rest of AER logging).

Signed-off-by: Karolina Stolarek <karolina.stolarek@oracle.com>
Signed-off-by: Jon Pan-Doh <pandoh@google.com>
Reported-by: Sargun Dhillon <sargun@meta.com>
Acked-by: Paul E. McKenney <paulmck@kernel.org>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pcie/aer.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 45629e1ea058..3116b4678081 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -784,14 +784,14 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,
 	info.mask = mask;
 	info.first_error = PCI_ERR_CAP_FEP(aer->cap_control);
 
-	pci_err(dev, "aer_status: 0x%08x, aer_mask: 0x%08x\n", status, mask);
+	aer_printk(level, dev, "aer_status: 0x%08x, aer_mask: 0x%08x\n", status, mask);
 	__aer_print_error(dev, &info, level);
-	pci_err(dev, "aer_layer=%s, aer_agent=%s\n",
-		aer_error_layer[layer], aer_agent_string[agent]);
+	aer_printk(level, dev, "aer_layer=%s, aer_agent=%s\n",
+		   aer_error_layer[layer], aer_agent_string[agent]);
 
 	if (aer_severity != AER_CORRECTABLE)
-		pci_err(dev, "aer_uncor_severity: 0x%08x\n",
-			aer->uncor_severity);
+		aer_printk(level, dev, "aer_uncor_severity: 0x%08x\n",
+			   aer->uncor_severity);
 
 	if (tlp_header_valid)
 		pcie_print_tlp_log(dev, &aer->header_log, dev_fmt("  "));
-- 
2.49.0.395.g12beb8f557-goog

[PATCH v5 3/8] PCI/AER: Move AER stat collection out of __aer_print_error()

[Jon Pan-Doh]

Decouple stat collection from internal AER print functions, so the
ratelimit does not impact the error counters. The stats collection is no
longer buried in nested functions, simplifying the function flow.

AERs from ghes or cxl drivers are a minor exception. Stat collection occurs
in pci_print_aer(), an external interface, as that is where aer_err_info
is populated.

Signed-off-by: Jon Pan-Doh <pandoh@google.com>
Reported-by: Sargun Dhillon <sargun@meta.com>
Acked-by: Paul E. McKenney <paulmck@kernel.org>
---
 drivers/pci/pci.h      |  1 +
 drivers/pci/pcie/aer.c | 10 ++++++----
 drivers/pci/pcie/dpc.c |  1 +
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 75985b96ecc1..9d63d32f041c 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -551,6 +551,7 @@ struct aer_err_info {
 };
 
 int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
+void pci_dev_aer_stats_incr(struct pci_dev *pdev, struct aer_err_info *info);
 void aer_print_error(struct pci_dev *dev, struct aer_err_info *info, const char *level);
 
 int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 3116b4678081..e5db1fdd8421 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -617,8 +617,7 @@ const struct attribute_group aer_stats_attr_group = {
 	.is_visible = aer_stats_attrs_are_visible,
 };
 
-static void pci_dev_aer_stats_incr(struct pci_dev *pdev,
-				   struct aer_err_info *info)
+void pci_dev_aer_stats_incr(struct pci_dev *pdev, struct aer_err_info *info)
 {
 	unsigned long status = info->status & ~info->mask;
 	int i, max = -1;
@@ -691,7 +690,6 @@ static void __aer_print_error(struct pci_dev *dev,
 		aer_printk(level, dev, "   [%2d] %-22s%s\n", i, errmsg,
 				info->first_error == i ? " (First)" : "");
 	}
-	pci_dev_aer_stats_incr(dev, info);
 }
 
 void aer_print_error(struct pci_dev *dev, struct aer_err_info *info,
@@ -784,6 +782,8 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,
 	info.mask = mask;
 	info.first_error = PCI_ERR_CAP_FEP(aer->cap_control);
 
+	pci_dev_aer_stats_incr(dev, &info);
+
 	aer_printk(level, dev, "aer_status: 0x%08x, aer_mask: 0x%08x\n", status, mask);
 	__aer_print_error(dev, &info, level);
 	aer_printk(level, dev, "aer_layer=%s, aer_agent=%s\n",
@@ -1263,8 +1263,10 @@ static inline void aer_process_err_devices(struct aer_err_info *e_info,
 
 	/* Report all before handle them, not to lost records by reset etc. */
 	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
-		if (aer_get_device_error_info(e_info->dev[i], e_info))
+		if (aer_get_device_error_info(e_info->dev[i], e_info)) {
+			pci_dev_aer_stats_incr(e_info->dev[i], e_info);
 			aer_print_error(e_info->dev[i], e_info, level);
+		}
 	}
 	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
 		if (aer_get_device_error_info(e_info->dev[i], e_info))
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 9e4c9ac737a7..81cd6e8ff3a4 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -289,6 +289,7 @@ void dpc_process_error(struct pci_dev *pdev)
 	else if (reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_UNCOR &&
 		 dpc_get_aer_uncorrect_severity(pdev, &info) &&
 		 aer_get_device_error_info(pdev, &info)) {
+		pci_dev_aer_stats_incr(pdev, &info);
 		aer_print_error(pdev, &info, KERN_ERR);
 		pci_aer_clear_nonfatal_status(pdev);
 		pci_aer_clear_fatal_status(pdev);
-- 
2.49.0.395.g12beb8f557-goog

[PATCH v5 4/8] PCI/AER: Rename aer_print_port_info() to aer_printrp_info()

[Jon Pan-Doh]

Update function/param names to be more descriptive. This is a
preparatory patch for when source devices are iterated through
to institue rate limits.

Signed-off-by: Jon Pan-Doh <pandoh@google.com>
Reported-by: Sargun Dhillon <sargun@meta.com>
---
 drivers/pci/pcie/aer.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index e5db1fdd8421..3c63a6963608 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -727,15 +727,15 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info,
 			info->severity, info->tlp_header_valid, &info->tlp);
 }
 
-static void aer_print_port_info(struct pci_dev *dev, struct aer_err_info *info)
+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;
 
-	pci_info(dev, "%s%s error message received from %04x:%02x:%02x.%d\n",
+	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(dev->bus), bus, PCI_SLOT(devfn),
+		 pci_domain_nr(rp->bus), bus, PCI_SLOT(devfn),
 		 PCI_FUNC(devfn));
 }
 
@@ -1299,7 +1299,7 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
 			e_info.multi_error_valid = 1;
 		else
 			e_info.multi_error_valid = 0;
-		aer_print_port_info(pdev, &e_info);
+		aer_print_rp_info(pdev, &e_info);
 
 		if (find_source_device(pdev, &e_info))
 			aer_process_err_devices(&e_info, KERN_WARNING);
@@ -1318,7 +1318,7 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
 		else
 			e_info.multi_error_valid = 0;
 
-		aer_print_port_info(pdev, &e_info);
+		aer_print_rp_info(pdev, &e_info);
 
 		if (find_source_device(pdev, &e_info))
 			aer_process_err_devices(&e_info, KERN_ERR);
-- 
2.49.0.395.g12beb8f557-goog

[PATCH v5 5/8] PCI/AER: Rename struct aer_stats to aer_report

[Jon Pan-Doh]

Update name to reflect the broader definition of structs/variables that
are stored (e.g. ratelimits). This is a preparatory patch for adding rate
limit support.

Signed-off-by: Karolina Stolarek <karolina.stolarek@oracle.com>
Signed-off-by: Jon Pan-Doh <pandoh@google.com>
Reported-by: Sargun Dhillon <sargun@meta.com>
Acked-by: Paul E. McKenney <paulmck@kernel.org>
Reviewed-by: Karolina Stolarek <karolina.stolarek@oracle.com>
---
 drivers/pci/pcie/aer.c | 50 +++++++++++++++++++++---------------------
 include/linux/pci.h    |  2 +-
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 3c63a6963608..f657edca8769 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -54,11 +54,11 @@ struct aer_rpc {
 	DECLARE_KFIFO(aer_fifo, struct aer_err_source, AER_ERROR_SOURCES_MAX);
 };
 
-/* AER stats for the device */
-struct aer_stats {
+/* AER report for the device */
+struct aer_report {
 
 	/*
-	 * Fields for all AER capable devices. They indicate the errors
+	 * Stats for all AER capable devices. They indicate the errors
 	 * "as seen by this device". Note that this may mean that if an
 	 * end point is causing problems, the AER counters may increment
 	 * at its link partner (e.g. root port) because the errors will be
@@ -80,7 +80,7 @@ struct aer_stats {
 	u64 dev_total_nonfatal_errs;
 
 	/*
-	 * Fields for Root ports & root complex event collectors only, these
+	 * Stats for Root ports & root complex event collectors only, these
 	 * indicate the total number of ERR_COR, ERR_FATAL, and ERR_NONFATAL
 	 * messages received by the root port / event collector, INCLUDING the
 	 * ones that are generated internally (by the rootport itself)
@@ -377,7 +377,7 @@ void pci_aer_init(struct pci_dev *dev)
 	if (!dev->aer_cap)
 		return;
 
-	dev->aer_stats = kzalloc(sizeof(struct aer_stats), GFP_KERNEL);
+	dev->aer_report = kzalloc(sizeof(*dev->aer_report), GFP_KERNEL);
 
 	/*
 	 * We save/restore PCI_ERR_UNCOR_MASK, PCI_ERR_UNCOR_SEVER,
@@ -398,8 +398,8 @@ void pci_aer_init(struct pci_dev *dev)
 
 void pci_aer_exit(struct pci_dev *dev)
 {
-	kfree(dev->aer_stats);
-	dev->aer_stats = NULL;
+	kfree(dev->aer_report);
+	dev->aer_report = NULL;
 }
 
 #define AER_AGENT_RECEIVER		0
@@ -537,10 +537,10 @@ static const char *aer_agent_string[] = {
 {									\
 	unsigned int i;							\
 	struct pci_dev *pdev = to_pci_dev(dev);				\
-	u64 *stats = pdev->aer_stats->stats_array;			\
+	u64 *stats = pdev->aer_report->stats_array;			\
 	size_t len = 0;							\
 									\
-	for (i = 0; i < ARRAY_SIZE(pdev->aer_stats->stats_array); i++) {\
+	for (i = 0; i < ARRAY_SIZE(pdev->aer_report->stats_array); i++) {\
 		if (strings_array[i])					\
 			len += sysfs_emit_at(buf, len, "%s %llu\n",	\
 					     strings_array[i],		\
@@ -551,7 +551,7 @@ static const char *aer_agent_string[] = {
 					     i, stats[i]);		\
 	}								\
 	len += sysfs_emit_at(buf, len, "TOTAL_%s %llu\n", total_string,	\
-			     pdev->aer_stats->total_field);		\
+			     pdev->aer_report->total_field);		\
 	return len;							\
 }									\
 static DEVICE_ATTR_RO(name)
@@ -572,7 +572,7 @@ aer_stats_dev_attr(aer_dev_nonfatal, dev_nonfatal_errs,
 		     char *buf)						\
 {									\
 	struct pci_dev *pdev = to_pci_dev(dev);				\
-	return sysfs_emit(buf, "%llu\n", pdev->aer_stats->field);	\
+	return sysfs_emit(buf, "%llu\n", pdev->aer_report->field);	\
 }									\
 static DEVICE_ATTR_RO(name)
 
@@ -599,7 +599,7 @@ static umode_t aer_stats_attrs_are_visible(struct kobject *kobj,
 	struct device *dev = kobj_to_dev(kobj);
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	if (!pdev->aer_stats)
+	if (!pdev->aer_report)
 		return 0;
 
 	if ((a == &dev_attr_aer_rootport_total_err_cor.attr ||
@@ -622,25 +622,25 @@ void pci_dev_aer_stats_incr(struct pci_dev *pdev, struct aer_err_info *info)
 	unsigned long status = info->status & ~info->mask;
 	int i, max = -1;
 	u64 *counter = NULL;
-	struct aer_stats *aer_stats = pdev->aer_stats;
+	struct aer_report *aer_report = pdev->aer_report;
 
-	if (!aer_stats)
+	if (!aer_report)
 		return;
 
 	switch (info->severity) {
 	case AER_CORRECTABLE:
-		aer_stats->dev_total_cor_errs++;
-		counter = &aer_stats->dev_cor_errs[0];
+		aer_report->dev_total_cor_errs++;
+		counter = &aer_report->dev_cor_errs[0];
 		max = AER_MAX_TYPEOF_COR_ERRS;
 		break;
 	case AER_NONFATAL:
-		aer_stats->dev_total_nonfatal_errs++;
-		counter = &aer_stats->dev_nonfatal_errs[0];
+		aer_report->dev_total_nonfatal_errs++;
+		counter = &aer_report->dev_nonfatal_errs[0];
 		max = AER_MAX_TYPEOF_UNCOR_ERRS;
 		break;
 	case AER_FATAL:
-		aer_stats->dev_total_fatal_errs++;
-		counter = &aer_stats->dev_fatal_errs[0];
+		aer_report->dev_total_fatal_errs++;
+		counter = &aer_report->dev_fatal_errs[0];
 		max = AER_MAX_TYPEOF_UNCOR_ERRS;
 		break;
 	}
@@ -652,19 +652,19 @@ void pci_dev_aer_stats_incr(struct pci_dev *pdev, struct aer_err_info *info)
 static void pci_rootport_aer_stats_incr(struct pci_dev *pdev,
 				 struct aer_err_source *e_src)
 {
-	struct aer_stats *aer_stats = pdev->aer_stats;
+	struct aer_report *aer_report = pdev->aer_report;
 
-	if (!aer_stats)
+	if (!aer_report)
 		return;
 
 	if (e_src->status & PCI_ERR_ROOT_COR_RCV)
-		aer_stats->rootport_total_cor_errs++;
+		aer_report->rootport_total_cor_errs++;
 
 	if (e_src->status & PCI_ERR_ROOT_UNCOR_RCV) {
 		if (e_src->status & PCI_ERR_ROOT_FATAL_RCV)
-			aer_stats->rootport_total_fatal_errs++;
+			aer_report->rootport_total_fatal_errs++;
 		else
-			aer_stats->rootport_total_nonfatal_errs++;
+			aer_report->rootport_total_nonfatal_errs++;
 	}
 }
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e4bf67bf8172..900edb6f8f62 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -346,7 +346,7 @@ struct pci_dev {
 	u8		hdr_type;	/* PCI header type (`multi' flag masked out) */
 #ifdef CONFIG_PCIEAER
 	u16		aer_cap;	/* AER capability offset */
-	struct aer_stats *aer_stats;	/* AER stats for this device */
+	struct aer_report *aer_report;	/* AER report for this device */
 #endif
 #ifdef CONFIG_PCIEPORTBUS
 	struct rcec_ea	*rcec_ea;	/* RCEC cached endpoint association */
-- 
2.49.0.395.g12beb8f557-goog

[PATCH v5 6/8] PCI/AER: Introduce ratelimit for error logs

[Jon Pan-Doh]

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.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/gong.chen/aer-inject.git

Signed-off-by: Jon Pan-Doh <pandoh@google.com>
Reported-by: Sargun Dhillon <sargun@meta.com>
Acked-by: Paul E. McKenney <paulmck@kernel.org>
---
 drivers/pci/pci.h      |  4 +-
 drivers/pci/pcie/aer.c | 87 ++++++++++++++++++++++++++++++++----------
 drivers/pci/pcie/dpc.c |  2 +-
 3 files changed, 71 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 9d63d32f041c..f709290e9e00 100644
--- 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];
 	int error_dev_num;
 
 	unsigned int id:16;
@@ -552,7 +553,8 @@ struct aer_err_info {
 
 int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
 void pci_dev_aer_stats_incr(struct pci_dev *pdev, struct aer_err_info *info);
-void aer_print_error(struct pci_dev *dev, struct aer_err_info *info, const char *level);
+void aer_print_error(struct pci_dev *dev, struct aer_err_info *info,
+		     const char *level, bool ratelimited);
 
 int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
 		      unsigned int tlp_len, bool flit,
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index f657edca8769..e0f526960134 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -28,6 +28,7 @@
 #include <linux/interrupt.h>
 #include <linux/delay.h>
 #include <linux/kfifo.h>
+#include <linux/ratelimit.h>
 #include <linux/slab.h>
 #include <acpi/apei.h>
 #include <acpi/ghes.h>
@@ -88,6 +89,10 @@ struct aer_report {
 	u64 rootport_total_cor_errs;
 	u64 rootport_total_fatal_errs;
 	u64 rootport_total_nonfatal_errs;
+
+	/* Ratelimits for errors */
+	struct ratelimit_state cor_log_ratelimit;
+	struct ratelimit_state uncor_log_ratelimit;
 };
 
 #define AER_LOG_TLP_MASKS		(PCI_ERR_UNC_POISON_TLP|	\
@@ -379,6 +384,11 @@ void pci_aer_init(struct pci_dev *dev)
 
 	dev->aer_report = kzalloc(sizeof(*dev->aer_report), GFP_KERNEL);
 
+	ratelimit_state_init(&dev->aer_report->cor_log_ratelimit,
+			     DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST);
+	ratelimit_state_init(&dev->aer_report->uncor_log_ratelimit,
+			     DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST);
+
 	/*
 	 * We save/restore PCI_ERR_UNCOR_MASK, PCI_ERR_UNCOR_SEVER,
 	 * PCI_ERR_COR_MASK, and PCI_ERR_CAP.  Root and Root Complex Event
@@ -668,6 +678,18 @@ static void pci_rootport_aer_stats_incr(struct pci_dev *pdev,
 	}
 }
 
+static bool aer_ratelimited(struct pci_dev *dev, unsigned int severity)
+{
+	struct ratelimit_state *ratelimit;
+
+	if (severity == AER_CORRECTABLE)
+		ratelimit = &dev->aer_report->cor_log_ratelimit;
+	else
+		ratelimit = &dev->aer_report->uncor_log_ratelimit;
+
+	return !__ratelimit(ratelimit);
+}
+
 static void __aer_print_error(struct pci_dev *dev,
 			      struct aer_err_info *info,
 			      const char *level)
@@ -693,11 +715,17 @@ static void __aer_print_error(struct pci_dev *dev,
 }
 
 void aer_print_error(struct pci_dev *dev, struct aer_err_info *info,
-		     const char *level)
+		     const char *level, bool ratelimited)
 {
 	int layer, agent;
 	int id = pci_dev_id(dev);
 
+	trace_aer_event(dev_name(&dev->dev), (info->status & ~info->mask),
+			info->severity, info->tlp_header_valid, &info->tlp);
+
+	if (ratelimited)
+		return;
+
 	if (!info->status) {
 		pci_err(dev, "PCIe Bus Error: severity=%s, type=Inaccessible, (Unregistered Agent ID)\n",
 			aer_error_severity_string[info->severity]);
@@ -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;
+		}
+	}
+
+	if (!ratelimited)
+		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));
 }
 
 #ifdef CONFIG_ACPI_APEI_PCIEAER
@@ -784,6 +822,12 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,
 
 	pci_dev_aer_stats_incr(dev, &info);
 
+	trace_aer_event(dev_name(&dev->dev), (status & ~mask),
+			aer_severity, tlp_header_valid, &aer->header_log);
+
+	if (aer_ratelimited(dev, aer_severity))
+		return;
+
 	aer_printk(level, dev, "aer_status: 0x%08x, aer_mask: 0x%08x\n", status, mask);
 	__aer_print_error(dev, &info, level);
 	aer_printk(level, dev, "aer_layer=%s, aer_agent=%s\n",
@@ -795,9 +839,6 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,
 
 	if (tlp_header_valid)
 		pcie_print_tlp_log(dev, &aer->header_log, dev_fmt("  "));
-
-	trace_aer_event(dev_name(&dev->dev), (status & ~mask),
-			aer_severity, tlp_header_valid, &aer->header_log);
 }
 EXPORT_SYMBOL_NS_GPL(pci_print_aer, "CXL");
 
@@ -808,8 +849,12 @@ EXPORT_SYMBOL_NS_GPL(pci_print_aer, "CXL");
  */
 static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
 {
-	if (e_info->error_dev_num < AER_MAX_MULTI_ERR_DEVICES) {
-		e_info->dev[e_info->error_dev_num] = pci_dev_get(dev);
+	int dev_idx = e_info->error_dev_num;
+	unsigned int severity = e_info->severity;
+
+	if (dev_idx < AER_MAX_MULTI_ERR_DEVICES) {
+		e_info->dev[dev_idx] = pci_dev_get(dev);
+		e_info->ratelimited[dev_idx] = aer_ratelimited(dev, severity);
 		e_info->error_dev_num++;
 		return 0;
 	}
@@ -1265,7 +1310,8 @@ static inline void aer_process_err_devices(struct aer_err_info *e_info,
 	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
 		if (aer_get_device_error_info(e_info->dev[i], e_info)) {
 			pci_dev_aer_stats_incr(e_info->dev[i], e_info);
-			aer_print_error(e_info->dev[i], e_info, level);
+			aer_print_error(e_info->dev[i], e_info, level,
+					e_info->ratelimited[i]);
 		}
 	}
 	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
@@ -1299,10 +1345,11 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
 			e_info.multi_error_valid = 1;
 		else
 			e_info.multi_error_valid = 0;
-		aer_print_rp_info(pdev, &e_info);
 
-		if (find_source_device(pdev, &e_info))
+		if (find_source_device(pdev, &e_info)) {
+			aer_print_rp_info(pdev, &e_info);
 			aer_process_err_devices(&e_info, KERN_WARNING);
+		}
 	}
 
 	if (e_src->status & PCI_ERR_ROOT_UNCOR_RCV) {
@@ -1318,10 +1365,10 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
 		else
 			e_info.multi_error_valid = 0;
 
-		aer_print_rp_info(pdev, &e_info);
-
-		if (find_source_device(pdev, &e_info))
+		if (find_source_device(pdev, &e_info)) {
+			aer_print_rp_info(pdev, &e_info);
 			aer_process_err_devices(&e_info, KERN_ERR);
+		}
 	}
 }
 
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 81cd6e8ff3a4..42a36df4a651 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -290,7 +290,7 @@ void dpc_process_error(struct pci_dev *pdev)
 		 dpc_get_aer_uncorrect_severity(pdev, &info) &&
 		 aer_get_device_error_info(pdev, &info)) {
 		pci_dev_aer_stats_incr(pdev, &info);
-		aer_print_error(pdev, &info, KERN_ERR);
+		aer_print_error(pdev, &info, KERN_ERR, false);
 		pci_aer_clear_nonfatal_status(pdev);
 		pci_aer_clear_fatal_status(pdev);
 	}
-- 
2.49.0.395.g12beb8f557-goog

[PATCH v5 7/8] PCI/AER: Add ratelimits to PCI AER Documentation

[Jon Pan-Doh]

Add ratelimits section for rationale and defaults.

Signed-off-by: Karolina Stolarek <karolina.stolarek@oracle.com>
Signed-off-by: Jon Pan-Doh <pandoh@google.com>
Reported-by: Sargun Dhillon <sargun@meta.com>
Acked-by: Paul E. McKenney <paulmck@kernel.org>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 Documentation/PCI/pcieaer-howto.rst | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/PCI/pcieaer-howto.rst b/Documentation/PCI/pcieaer-howto.rst
index f013f3b27c82..896d2a232a90 100644
--- a/Documentation/PCI/pcieaer-howto.rst
+++ b/Documentation/PCI/pcieaer-howto.rst
@@ -85,6 +85,17 @@ In the example, 'Requester ID' means the ID of the device that sent
 the error message to the Root Port. Please refer to PCIe specs for other
 fields.
 
+AER Ratelimits
+--------------
+
+Since error messages can be generated for each transaction, we may see
+large volumes of errors reported. To prevent spammy devices from flooding
+the console/stalling execution, messages are throttled by device and error
+type (correctable vs. uncorrectable).
+
+AER uses the default ratelimit of DEFAULT_RATELIMIT_BURST (10 events) over
+DEFAULT_RATELIMIT_INTERVAL (5 seconds).
+
 AER Statistics / Counters
 -------------------------
 
-- 
2.49.0.395.g12beb8f557-goog

[PATCH v5 8/8] PCI/AER: Add sysfs attributes for log ratelimits

[Jon Pan-Doh]

Allow userspace to read/write log ratelimits per device (including
enable/disable). Create aer/ sysfs directory to store them and any
future aer configs.

Update AER sysfs ABI filename to reflect the broader scope of AER sysfs
attributes (e.g. stats and ratelimits).

Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats ->
Documentation/ABI/testing/sysfs-bus-pci-devices-aer

Tested using aer-inject[1]. Configured correctable log ratelimit to 5.
Sent 6 AER errors. Observed 5 errors logged while AER stats
(cat /sys/bus/pci/devices/<dev>/aer_dev_correctable) shows 6.

Disabled ratelimiting and sent 6 more AER errors. Observed all 6 errors
logged and accounted in AER stats (12 total errors).

[1] https://git.kernel.org/pub/scm/linux/kernel/git/gong.chen/aer-inject.git

Signed-off-by: Karolina Stolarek <karolina.stolarek@oracle.com>
Signed-off-by: Jon Pan-Doh <pandoh@google.com>
Reported-by: Sargun Dhillon <sargun@meta.com>
Acked-by: Paul E. McKenney <paulmck@kernel.org>
---
 ...es-aer_stats => sysfs-bus-pci-devices-aer} | 34 +++++++
 Documentation/PCI/pcieaer-howto.rst           |  5 +-
 drivers/pci/pci-sysfs.c                       |  1 +
 drivers/pci/pci.h                             |  1 +
 drivers/pci/pcie/aer.c                        | 93 +++++++++++++++++++
 5 files changed, 133 insertions(+), 1 deletion(-)
 rename Documentation/ABI/testing/{sysfs-bus-pci-devices-aer_stats => sysfs-bus-pci-devices-aer} (77%)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats b/Documentation/ABI/testing/sysfs-bus-pci-devices-aer
similarity index 77%
rename from Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
rename to Documentation/ABI/testing/sysfs-bus-pci-devices-aer
index d1f67bb81d5d..771204197b71 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
+++ b/Documentation/ABI/testing/sysfs-bus-pci-devices-aer
@@ -117,3 +117,37 @@ Date:		July 2018
 KernelVersion:	4.19.0
 Contact:	linux-pci@vger.kernel.org, rajatja@google.com
 Description:	Total number of ERR_NONFATAL messages reported to rootport.
+
+PCIe AER ratelimits
+-------------------
+
+These attributes show up under all the devices that are AER capable.
+They represent configurable ratelimits of logs per error type.
+
+See Documentation/PCI/pcieaer-howto.rst for more info on ratelimits.
+
+What:		/sys/bus/pci/devices/<dev>/aer/ratelimit_log_enable
+Date:		March 2025
+KernelVersion:	6.15.0
+Contact:	linux-pci@vger.kernel.org, pandoh@google.com
+Description:	Writing 1/0 enables/disables AER log ratelimiting. Reading
+		gets whether or not AER is currently enabled. Enabled by
+		default.
+
+What:		/sys/bus/pci/devices/<dev>/aer/ratelimit_burst_cor_log
+Date:		March 2025
+KernelVersion:	6.15.0
+Contact:	linux-pci@vger.kernel.org, pandoh@google.com
+Description:	Ratelimit burst for correctable error logs. Writing a value
+		changes the number of errors (burst) allowed per interval
+		(5 second window) before ratelimiting. Reading gets the
+		current ratelimit burst.
+
+What:		/sys/bus/pci/devices/<dev>/aer/ratelimit_burst_uncor_log
+Date:		March 2025
+KernelVersion:	6.15.0
+Contact:	linux-pci@vger.kernel.org, pandoh@google.com
+Description:	Ratelimit burst for uncorrectable error logs. Writing a
+		value changes the number of errors (burst) allowed per
+		interval (5 second window) before ratelimiting. Reading
+		gets the current ratelimit burst.
diff --git a/Documentation/PCI/pcieaer-howto.rst b/Documentation/PCI/pcieaer-howto.rst
index 896d2a232a90..043cdb3194be 100644
--- a/Documentation/PCI/pcieaer-howto.rst
+++ b/Documentation/PCI/pcieaer-howto.rst
@@ -96,12 +96,15 @@ type (correctable vs. uncorrectable).
 AER uses the default ratelimit of DEFAULT_RATELIMIT_BURST (10 events) over
 DEFAULT_RATELIMIT_INTERVAL (5 seconds).
 
+Ratelimits are exposed in the form of sysfs attributes and configurable.
+See Documentation/ABI/testing/sysfs-bus-pci-devices-aer.
+
 AER Statistics / Counters
 -------------------------
 
 When PCIe AER errors are captured, the counters / statistics are also exposed
 in the form of sysfs attributes which are documented at
-Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats
+Documentation/ABI/testing/sysfs-bus-pci-devices-aer.
 
 Developer Guide
 ===============
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index b46ce1a2c554..16de3093294e 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1801,6 +1801,7 @@ const struct attribute_group *pci_dev_attr_groups[] = {
 	&pcie_dev_attr_group,
 #ifdef CONFIG_PCIEAER
 	&aer_stats_attr_group,
+	&aer_attr_group,
 #endif
 #ifdef CONFIG_PCIEASPM
 	&aspm_ctrl_attr_group,
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index f709290e9e00..a356640fdb3f 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -891,6 +891,7 @@ void pci_no_aer(void);
 void pci_aer_init(struct pci_dev *dev);
 void pci_aer_exit(struct pci_dev *dev);
 extern const struct attribute_group aer_stats_attr_group;
+extern const struct attribute_group aer_attr_group;
 void pci_aer_clear_fatal_status(struct pci_dev *dev);
 int pci_aer_clear_status(struct pci_dev *dev);
 int pci_aer_raw_clear_status(struct pci_dev *dev);
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index e0f526960134..ee3c285c26a6 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -627,6 +627,99 @@ const struct attribute_group aer_stats_attr_group = {
 	.is_visible = aer_stats_attrs_are_visible,
 };
 
+/*
+ * Ratelimit enable toggle
+ * 0: disabled with ratelimit.interval = 0
+ * 1: enabled with ratelimit.interval = nonzero
+ */
+static ssize_t ratelimit_log_enable_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	bool enable = pdev->aer_report->cor_log_ratelimit.interval != 0;
+
+	return sysfs_emit(buf, "%d\n", enable);
+}
+
+static ssize_t ratelimit_log_enable_store(struct device *dev,
+					  struct device_attribute *attr,
+					  const char *buf, size_t count)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	bool enable;
+	int interval;
+
+	if (kstrtobool(buf, &enable) < 0)
+		return -EINVAL;
+
+	if (enable)
+		interval = DEFAULT_RATELIMIT_INTERVAL;
+	else
+		interval = 0;
+
+	pdev->aer_report->cor_log_ratelimit.interval = interval;
+	pdev->aer_report->uncor_log_ratelimit.interval = interval;
+
+	return count;
+}
+static DEVICE_ATTR_RW(ratelimit_log_enable);
+
+#define aer_ratelimit_burst_attr(name, ratelimit)			\
+	static ssize_t							\
+	name##_show(struct device *dev, struct device_attribute *attr,	\
+		    char *buf)						\
+{									\
+	struct pci_dev *pdev = to_pci_dev(dev);				\
+									\
+	return sysfs_emit(buf, "%d\n",					\
+			  pdev->aer_report->ratelimit.burst);		\
+}									\
+									\
+	static ssize_t							\
+	name##_store(struct device *dev, struct device_attribute *attr,	\
+		     const char *buf, size_t count)			\
+{									\
+	struct pci_dev *pdev = to_pci_dev(dev);				\
+	int burst;							\
+									\
+	if (kstrtoint(buf, 0, &burst) < 0)				\
+		return -EINVAL;						\
+									\
+	pdev->aer_report->ratelimit.burst = burst;			\
+									\
+	return count;							\
+}									\
+static DEVICE_ATTR_RW(name)
+
+aer_ratelimit_burst_attr(ratelimit_burst_cor_log, cor_log_ratelimit);
+aer_ratelimit_burst_attr(ratelimit_burst_uncor_log, uncor_log_ratelimit);
+
+static struct attribute *aer_attrs[] = {
+	&dev_attr_ratelimit_log_enable.attr,
+	&dev_attr_ratelimit_burst_cor_log.attr,
+	&dev_attr_ratelimit_burst_uncor_log.attr,
+	NULL
+};
+
+static umode_t aer_attrs_are_visible(struct kobject *kobj,
+				     struct attribute *a, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (!pdev->aer_report)
+		return 0;
+
+	return a->mode;
+}
+
+const struct attribute_group aer_attr_group = {
+	.name = "aer",
+	.attrs = aer_attrs,
+	.is_visible = aer_attrs_are_visible,
+};
+
 void pci_dev_aer_stats_incr(struct pci_dev *pdev, struct aer_err_info *info)
 {
 	unsigned long status = info->status & ~info->mask;
-- 
2.49.0.395.g12beb8f557-goog

Re: [PATCH v5 4/8] PCI/AER: Rename aer_print_port_info() to aer_printrp_info()

[Karolina Stolarek]

On 21/03/2025 02:58, Jon Pan-Doh wrote:
> Update function/param names to be more descriptive. This is a
> preparatory patch for when source devices are iterated through
> to institue rate limits.

This commit description doesn't provide enough information on why and 
how we make the names more descriptive. With this change, we want to 
make it clear that this function logs information about the Root Port 
that received an error message, not just a generic PCIe Port.

Also, there's a typo in the subject:
s/aer_printrp_info()/aer_print_rp_info()/

I'm fine with the code changes but I'd like to see the new commit 
description before giving the r-b.

All the best,
Karolina

> 
> Signed-off-by: Jon Pan-Doh <pandoh@google.com>
> Reported-by: Sargun Dhillon <sargun@meta.com>
> ---
>   drivers/pci/pcie/aer.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index e5db1fdd8421..3c63a6963608 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -727,15 +727,15 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info,
>   			info->severity, info->tlp_header_valid, &info->tlp);
>   }
>   
> -static void aer_print_port_info(struct pci_dev *dev, struct aer_err_info *info)
> +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;
>   
> -	pci_info(dev, "%s%s error message received from %04x:%02x:%02x.%d\n",
> +	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(dev->bus), bus, PCI_SLOT(devfn),
> +		 pci_domain_nr(rp->bus), bus, PCI_SLOT(devfn),
>   		 PCI_FUNC(devfn));
>   }
>   
> @@ -1299,7 +1299,7 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
>   			e_info.multi_error_valid = 1;
>   		else
>   			e_info.multi_error_valid = 0;
> -		aer_print_port_info(pdev, &e_info);
> +		aer_print_rp_info(pdev, &e_info);
>   
>   		if (find_source_device(pdev, &e_info))
>   			aer_process_err_devices(&e_info, KERN_WARNING);
> @@ -1318,7 +1318,7 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
>   		else
>   			e_info.multi_error_valid = 0;
>   
> -		aer_print_port_info(pdev, &e_info);
> +		aer_print_rp_info(pdev, &e_info);
>   
>   		if (find_source_device(pdev, &e_info))
>   			aer_process_err_devices(&e_info, KERN_ERR);

Re: [PATCH v5 6/8] PCI/AER: Introduce ratelimit for error logs

[Karolina Stolarek]

On 21/03/2025 02:58, Jon Pan-Doh wrote:
 >
>   
>   void aer_print_error(struct pci_dev *dev, struct aer_err_info *info,
> -		     const char *level)
> +		     const char *level, bool ratelimited)

Ideally, we would like to be able to extract the "ratelimited" flag from 
the aer_err_info struct, with no need for extra parameters in this function.

>   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;
> +		}
> +	}

(please correct me if I'm misreading the patch)

It looks like we ratelimit the Root Port logs based on the source device 
that generated the message, and the actual errors in 
aer_process_err_devices() use their own ratelimits. As you noted in one 
of your emails, there might be the case where we report errors but 
there's no information about the Root Port that issued the interrupt 
we're handling.

The way I understood the suggestion in 20250320202913.GA1097165@bhelgaas 
is that we evaluate the ratelimit of the Root Port or Downstream Port, 
save it in aer_err_info, and use it in aer_print_rp_info() and 
aer_print_error(). I'm worried that one noisy device under a Root Port 
could hit a ratelimit and hide everything else

A fair (and complicated) solution would be to check the ratelimits of 
all devices in the Error Message to see if there is at least one that 
can be reported. If so, use that ratelimit when printing both the Root 
Port info and the error details from that device.

This is to say that if we keep aer_print_rp_info() (which was decided a 
couple emails ago), we should print it before any error coming from that 
Root Port is reported.

All the best,
Karolina

Re: [PATCH v5 6/8] PCI/AER: Introduce ratelimit for error logs

[Jon Pan-Doh]

On Fri, Mar 21, 2025 at 6:46 AM Karolina Stolarek
<karolina.stolarek@oracle.com> wrote:
>
> On 21/03/2025 02:58, Jon Pan-Doh wrote:
> >   void aer_print_error(struct pci_dev *dev, struct aer_err_info *info,
> > -                  const char *level)
> > +                  const char *level, bool ratelimited)
>
> Ideally, we would like to be able to extract the "ratelimited" flag from
> the aer_err_info struct, with no need for extra parameters in this function.

I considered this, but pci_print_aer() and dpc_process_aer() both call
aer_print_error directly without populating info->dev[]. I thought it
was easier/cleaner to pass it in vs. populate info->dev[] and then
read it.

> (please correct me if I'm misreading the patch)
>
> It looks like we ratelimit the Root Port logs based on the source device
> that generated the message, and the actual errors in
> aer_process_err_devices() use their own ratelimits. As you noted in one
> of your emails, there might be the case where we report errors but
> there's no information about the Root Port that issued the interrupt
> we're handling.

Your understanding is correct. I think the edge case described is
acceptable behavior:

1. Multiple errors arrive where the first source device is ratelimited
2. Root port log and first source device log are not printed
3. Other error source device(s) logs are printed

The root port message is of the form:
<root port> (Multiple) <severity> error message received from <first
source device>

If the root port log is printed, this could cause confusion as:
- root port log mentions first source device only
- source device logs mention other source device(s) but not
ratelimited first source device

There's an argument that there is still value in the root port log as
you can infer that the subsequent source device logs are under that
same root port. I don't think it's that useful (biased as I advocated
for removing root port logs to begin with :))

> The way I understood the suggestion in 20250320202913.GA1097165@bhelgaas
> is that we evaluate the ratelimit of the Root Port or Downstream Port,
> save it in aer_err_info, and use it in aer_print_rp_info() and
> aer_print_error(). I'm worried that one noisy device under a Root Port
> could hit a ratelimit and hide everything else

This is not a 100% translation of Bjorn's suggestion as I share your
concern (1 spammy device ratelimits and hides other devices).

> A fair (and complicated) solution would be to check the ratelimits of
> all devices in the Error Message to see if there is at least one that
> can be reported. If so, use that ratelimit when printing both the Root
> Port info and the error details from that device.

I mentioned this as well[1], albeit briefly. I'd opt for this if my
initial solution isn't satisfactory. You could make it more
complicated by substituting the source device, if it is ratelimited,
to a non-ratelimited one. However, it's not good as it changes the
default expectation that the root port log would have the source ID.

[1] https://lore.kernel.org/linux-pci/CAMC_AXWQg53uNLsZizxEsOf_3C2gv=vBdAAeMek1AmnTnMmDAw@mail.gmail.com/

Thanks,
Jon

Re: [PATCH v5 4/8] PCI/AER: Rename aer_print_port_info() to aer_printrp_info()

[Jon Pan-Doh]

On Fri, Mar 21, 2025 at 6:39 AM Karolina Stolarek
<karolina.stolarek@oracle.com> wrote:
> This commit description doesn't provide enough information on why and
> how we make the names more descriptive. With this change, we want to
> make it clear that this function logs information about the Root Port
> that received an error message, not just a generic PCIe Port.
>
> Also, there's a typo in the subject:
> s/aer_printrp_info()/aer_print_rp_info()/
>
> I'm fine with the code changes but I'd like to see the new commit
> description before giving the r-b.

Ack. Will add more detail/fix typos (incl. s/institue/institute) in
v6. Something like:

PCI/AER: Rename aer_print_port_info() to aer_print_rp_info()

    Update function/param names to be more descriptive. Make it clear that
    this function logs error information from a root port vs. generic PCIe
    port.

    This is a preparatory patch for instituting rate limits on logs. It
    frees up the dev variable for source device iteration.

Thanks,
Jon

Re: [PATCH v5 5/8] PCI/AER: Rename struct aer_stats to aer_report

[Bjorn Helgaas]

On Thu, Mar 20, 2025 at 06:58:03PM -0700, Jon Pan-Doh wrote:
> Update name to reflect the broader definition of structs/variables that
> are stored (e.g. ratelimits). This is a preparatory patch for adding rate
> limit support.
> 
> Signed-off-by: Karolina Stolarek <karolina.stolarek@oracle.com>
> Signed-off-by: Jon Pan-Doh <pandoh@google.com>
> Reported-by: Sargun Dhillon <sargun@meta.com>

What did Sargun report?  Is there a bug fix in here?  Can we include a
URL to whatever Sargun reported?

Re: [PATCH v5 5/8] PCI/AER: Rename struct aer_stats to aer_report

[Jon Pan-Doh]

On Fri, Mar 21, 2025 at 3:01 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> What did Sargun report?  Is there a bug fix in here?  Can we include a
> URL to whatever Sargun reported?

Paul added Reported-By and Acked-By tags[1] for the series which I
applied to each patch. Checkpatch mildly complained about not having a
Closes tag for Reported-By.

Sargun/Paul, do you have a Closes/URL tag?

Thanks,
Jon

Re: [PATCH v5 5/8] PCI/AER: Rename struct aer_stats to aer_report

[Paul E. McKenney]

On Fri, Mar 21, 2025 at 05:01:15PM -0500, Bjorn Helgaas wrote:
> On Thu, Mar 20, 2025 at 06:58:03PM -0700, Jon Pan-Doh wrote:
> > Update name to reflect the broader definition of structs/variables that
> > are stored (e.g. ratelimits). This is a preparatory patch for adding rate
> > limit support.
> > 
> > Signed-off-by: Karolina Stolarek <karolina.stolarek@oracle.com>
> > Signed-off-by: Jon Pan-Doh <pandoh@google.com>
> > Reported-by: Sargun Dhillon <sargun@meta.com>
> 
> What did Sargun report?  Is there a bug fix in here?  Can we include a
> URL to whatever Sargun reported?

He reported RCU CPU stall warnings and CSD-lock warnings internally
within Meta, so sorry, no useful URL.

I did put together a series of hacks that fix the problem: (1) Disabling
__aer_print_error() entirely, (2) Disabling __aer_print_error() printk()
and sysfs, (3) Disabling only __aer_print_error() printk(), and finally
(4) Throttling __aer_print_error() printk().  Jon's patch looks to
cover my #4 plus it looks to allow run-time control of the throttling.
So my patch is strictly a stop-the-bleeding measure for Meta's fleet
while this patch series makes its way upstream.

I do plan to look at Jon's patch in more detail when he posts the next
version.

Fair enough?

							Thanx, Paul

Re: [PATCH v5 5/8] PCI/AER: Rename struct aer_stats to aer_report

[Paul E. McKenney]

On Fri, Mar 21, 2025 at 03:15:26PM -0700, Jon Pan-Doh wrote:
> On Fri, Mar 21, 2025 at 3:01 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > What did Sargun report?  Is there a bug fix in here?  Can we include a
> > URL to whatever Sargun reported?
> 
> Paul added Reported-By and Acked-By tags[1] for the series which I
> applied to each patch. Checkpatch mildly complained about not having a
> Closes tag for Reported-By.
> 
> Sargun/Paul, do you have a Closes/URL tag?

The URL of this email?

Perhaps more productively, I hope that we can run tests on this series,
but it will take some time.

							Thanx, Paul

Re: [PATCH v5 5/8] PCI/AER: Rename struct aer_stats to aer_report

[Bjorn Helgaas]

On Fri, Mar 21, 2025 at 03:16:48PM -0700, Paul E. McKenney wrote:
> On Fri, Mar 21, 2025 at 05:01:15PM -0500, Bjorn Helgaas wrote:
> > On Thu, Mar 20, 2025 at 06:58:03PM -0700, Jon Pan-Doh wrote:
> > > Update name to reflect the broader definition of structs/variables that
> > > are stored (e.g. ratelimits). This is a preparatory patch for adding rate
> > > limit support.
> > > 
> > > Signed-off-by: Karolina Stolarek <karolina.stolarek@oracle.com>
> > > Signed-off-by: Jon Pan-Doh <pandoh@google.com>
> > > Reported-by: Sargun Dhillon <sargun@meta.com>
> > 
> > What did Sargun report?  Is there a bug fix in here?  Can we include a
> > URL to whatever Sargun reported?
> 
> He reported RCU CPU stall warnings and CSD-lock warnings internally
> within Meta, so sorry, no useful URL.

Oh, I see now how this happened via your ack email, Paul.  So I think
it would make sense for Jon to add Sargun's reported-by to "[PATCH v5
6/8] PCI/AER: Introduce ratelimit for error logs", along with a line
in the commit log connecting Sargun with the RCU CPU stall warnings,
etc., because that's the patch that actually addresses those warnings.

I wouldn't add it to the other patches because it's just confusing.

Bjorn

Re: [PATCH v5 5/8] PCI/AER: Rename struct aer_stats to aer_report

[Paul E. McKenney]

On Fri, Mar 21, 2025 at 05:39:30PM -0500, Bjorn Helgaas wrote:
> On Fri, Mar 21, 2025 at 03:16:48PM -0700, Paul E. McKenney wrote:
> > On Fri, Mar 21, 2025 at 05:01:15PM -0500, Bjorn Helgaas wrote:
> > > On Thu, Mar 20, 2025 at 06:58:03PM -0700, Jon Pan-Doh wrote:
> > > > Update name to reflect the broader definition of structs/variables that
> > > > are stored (e.g. ratelimits). This is a preparatory patch for adding rate
> > > > limit support.
> > > > 
> > > > Signed-off-by: Karolina Stolarek <karolina.stolarek@oracle.com>
> > > > Signed-off-by: Jon Pan-Doh <pandoh@google.com>
> > > > Reported-by: Sargun Dhillon <sargun@meta.com>
> > > 
> > > What did Sargun report?  Is there a bug fix in here?  Can we include a
> > > URL to whatever Sargun reported?
> > 
> > He reported RCU CPU stall warnings and CSD-lock warnings internally
> > within Meta, so sorry, no useful URL.
> 
> Oh, I see now how this happened via your ack email, Paul.  So I think
> it would make sense for Jon to add Sargun's reported-by to "[PATCH v5
> 6/8] PCI/AER: Introduce ratelimit for error logs", along with a line
> in the commit log connecting Sargun with the RCU CPU stall warnings,
> etc., because that's the patch that actually addresses those warnings.
> 
> I wouldn't add it to the other patches because it's just confusing.

Works for me!

							Thanx, Paul

Re: [PATCH v5 8/8] PCI/AER: Add sysfs attributes for log ratelimits

[Krzysztof Wilczyński]

Hello,

A few small comments.

> +/*
> + * Ratelimit enable toggle
> + * 0: disabled with ratelimit.interval = 0
> + * 1: enabled with ratelimit.interval = nonzero
> + */

Move the above comment so it hugs ratelimit_log_enable_store().  Also,
perhaps "Ratelimit enable toggle:", while at it, and feel free to indent
both choices for readability.

That sad, I am not sure if we really  need to explain here how this
particular store() function works.

> +static ssize_t ratelimit_log_enable_show(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	bool enable = pdev->aer_report->cor_log_ratelimit.interval != 0;
> +
> +	return sysfs_emit(buf, "%d\n", enable);
> +}

Perhaps "status" or "enabled" for the variable name above.

> +static ssize_t ratelimit_log_enable_store(struct device *dev,
> +					  struct device_attribute *attr,
> +					  const char *buf, size_t count)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	bool enable;
> +	int interval;

I would love if we could add the following here:

	if (!capable(CAP_SYS_ADMIN))
		return -EPERM;

To ensure that only privileged user also sporting a proper set capabilities
can modify this attribute.

> +	if (kstrtobool(buf, &enable) < 0)
> +		return -EINVAL;
> +
> +	if (enable)
> +		interval = DEFAULT_RATELIMIT_INTERVAL;
> +	else
> +		interval = 0;
> +
> +	pdev->aer_report->cor_log_ratelimit.interval = interval;
> +	pdev->aer_report->uncor_log_ratelimit.interval = interval;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(ratelimit_log_enable);

[...]
> +	static ssize_t							\
> +	name##_store(struct device *dev, struct device_attribute *attr,	\
> +		     const char *buf, size_t count)			\
> +{									\
> +	struct pci_dev *pdev = to_pci_dev(dev);				\
> +	int burst;							\

Same as earlier comment.  Add the following:

	if (!capable(CAP_SYS_ADMIN))
		return -EPERM;

> +	if (kstrtoint(buf, 0, &burst) < 0)				\
> +		return -EINVAL;						\
> +									\
> +	pdev->aer_report->ratelimit.burst = burst;			\
> +									\
> +	return count;							\
> +}									\
> +static DEVICE_ATTR_RW(name)

Thank you!

	Krzysztof

Re: [PATCH v5 6/8] PCI/AER: Introduce ratelimit for error logs

[Paul E. McKenney]

On Thu, Mar 20, 2025 at 06:58:04PM -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.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/gong.chen/aer-inject.git
> 
> Signed-off-by: Jon Pan-Doh <pandoh@google.com>
> Reported-by: Sargun Dhillon <sargun@meta.com>
> Acked-by: Paul E. McKenney <paulmck@kernel.org>

Just taking a closer look.  There are some things that I would do
differently, but I don't see any show-stoppers here.

I don't see this patch series in -next, which is fine:  This is not
so urgent that I would want to drive it into the current merge window.
I am hoping that it goes into -next as soon as v6.14-rc1 comes out.

> ---
>  drivers/pci/pci.h      |  4 +-
>  drivers/pci/pcie/aer.c | 87 ++++++++++++++++++++++++++++++++----------
>  drivers/pci/pcie/dpc.c |  2 +-
>  3 files changed, 71 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 9d63d32f041c..f709290e9e00 100644
> --- 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 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?

>  	int error_dev_num;
>  
>  	unsigned int id:16;
> @@ -552,7 +553,8 @@ struct aer_err_info {
>  
>  int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
>  void pci_dev_aer_stats_incr(struct pci_dev *pdev, struct aer_err_info *info);
> -void aer_print_error(struct pci_dev *dev, struct aer_err_info *info, const char *level);
> +void aer_print_error(struct pci_dev *dev, struct aer_err_info *info,
> +		     const char *level, bool ratelimited);

And here you do pass it in.

>  int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
>  		      unsigned int tlp_len, bool flit,
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index f657edca8769..e0f526960134 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -28,6 +28,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/delay.h>
>  #include <linux/kfifo.h>
> +#include <linux/ratelimit.h>
>  #include <linux/slab.h>
>  #include <acpi/apei.h>
>  #include <acpi/ghes.h>
> @@ -88,6 +89,10 @@ struct aer_report {
>  	u64 rootport_total_cor_errs;
>  	u64 rootport_total_fatal_errs;
>  	u64 rootport_total_nonfatal_errs;
> +
> +	/* Ratelimits for errors */
> +	struct ratelimit_state cor_log_ratelimit;
> +	struct ratelimit_state uncor_log_ratelimit;

Separate per-device rate limiting for correctable and uncorrectable
errors, very good!

>  };
>  
>  #define AER_LOG_TLP_MASKS		(PCI_ERR_UNC_POISON_TLP|	\
> @@ -379,6 +384,11 @@ void pci_aer_init(struct pci_dev *dev)
>  
>  	dev->aer_report = kzalloc(sizeof(*dev->aer_report), GFP_KERNEL);
>  
> +	ratelimit_state_init(&dev->aer_report->cor_log_ratelimit,
> +			     DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST);
> +	ratelimit_state_init(&dev->aer_report->uncor_log_ratelimit,
> +			     DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST);
> +
>  	/*
>  	 * We save/restore PCI_ERR_UNCOR_MASK, PCI_ERR_UNCOR_SEVER,
>  	 * PCI_ERR_COR_MASK, and PCI_ERR_CAP.  Root and Root Complex Event
> @@ -668,6 +678,18 @@ static void pci_rootport_aer_stats_incr(struct pci_dev *pdev,
>  	}
>  }
>  
> +static bool aer_ratelimited(struct pci_dev *dev, unsigned int severity)
> +{
> +	struct ratelimit_state *ratelimit;
> +
> +	if (severity == AER_CORRECTABLE)
> +		ratelimit = &dev->aer_report->cor_log_ratelimit;
> +	else
> +		ratelimit = &dev->aer_report->uncor_log_ratelimit;
> +
> +	return !__ratelimit(ratelimit);
> +}

Initialization and single is-it-ratelimited function, good.

> +
>  static void __aer_print_error(struct pci_dev *dev,
>  			      struct aer_err_info *info,
>  			      const char *level)
> @@ -693,11 +715,17 @@ static void __aer_print_error(struct pci_dev *dev,
>  }
>  
>  void aer_print_error(struct pci_dev *dev, struct aer_err_info *info,
> -		     const char *level)
> +		     const char *level, bool ratelimited)
>  {
>  	int layer, agent;
>  	int id = pci_dev_id(dev);
>  
> +	trace_aer_event(dev_name(&dev->dev), (info->status & ~info->mask),
> +			info->severity, info->tlp_header_valid, &info->tlp);

Unconditional tracing, as before.  I cannot imagine that moving this to
the top of the function hurts anything.

> +	if (ratelimited)
> +		return;
> +
>  	if (!info->status) {
>  		pci_err(dev, "PCIe Bus Error: severity=%s, type=Inaccessible, (Unregistered Agent ID)\n",
>  			aer_error_severity_string[info->severity]);
> @@ -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?

> +	if (!ratelimited)
> +		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));
>  }
>  
>  #ifdef CONFIG_ACPI_APEI_PCIEAER
> @@ -784,6 +822,12 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,
>  
>  	pci_dev_aer_stats_incr(dev, &info);
>  
> +	trace_aer_event(dev_name(&dev->dev), (status & ~mask),
> +			aer_severity, tlp_header_valid, &aer->header_log);
> +
> +	if (aer_ratelimited(dev, aer_severity))
> +		return;

PCIe suffices for our use case, but symmetry is not a bad thing.

>  	aer_printk(level, dev, "aer_status: 0x%08x, aer_mask: 0x%08x\n", status, mask);
>  	__aer_print_error(dev, &info, level);
>  	aer_printk(level, dev, "aer_layer=%s, aer_agent=%s\n",
> @@ -795,9 +839,6 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,
>  
>  	if (tlp_header_valid)
>  		pcie_print_tlp_log(dev, &aer->header_log, dev_fmt("  "));
> -
> -	trace_aer_event(dev_name(&dev->dev), (status & ~mask),
> -			aer_severity, tlp_header_valid, &aer->header_log);
>  }
>  EXPORT_SYMBOL_NS_GPL(pci_print_aer, "CXL");
>  
> @@ -808,8 +849,12 @@ EXPORT_SYMBOL_NS_GPL(pci_print_aer, "CXL");
>   */
>  static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
>  {
> -	if (e_info->error_dev_num < AER_MAX_MULTI_ERR_DEVICES) {
> -		e_info->dev[e_info->error_dev_num] = pci_dev_get(dev);
> +	int dev_idx = e_info->error_dev_num;
> +	unsigned int severity = e_info->severity;
> +
> +	if (dev_idx < AER_MAX_MULTI_ERR_DEVICES) {
> +		e_info->dev[dev_idx] = pci_dev_get(dev);
> +		e_info->ratelimited[dev_idx] = aer_ratelimited(dev, severity);
>  		e_info->error_dev_num++;
>  		return 0;
>  	}
> @@ -1265,7 +1310,8 @@ static inline void aer_process_err_devices(struct aer_err_info *e_info,
>  	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
>  		if (aer_get_device_error_info(e_info->dev[i], e_info)) {
>  			pci_dev_aer_stats_incr(e_info->dev[i], e_info);
> -			aer_print_error(e_info->dev[i], e_info, level);
> +			aer_print_error(e_info->dev[i], e_info, level,
> +					e_info->ratelimited[i]);
>  		}
>  	}
>  	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
> @@ -1299,10 +1345,11 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
>  			e_info.multi_error_valid = 1;
>  		else
>  			e_info.multi_error_valid = 0;
> -		aer_print_rp_info(pdev, &e_info);
>  
> -		if (find_source_device(pdev, &e_info))
> +		if (find_source_device(pdev, &e_info)) {
> +			aer_print_rp_info(pdev, &e_info);
>  			aer_process_err_devices(&e_info, KERN_WARNING);
> +		}
>  	}

And here we are in the interrupts handler.

>  	if (e_src->status & PCI_ERR_ROOT_UNCOR_RCV) {
> @@ -1318,10 +1365,10 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
>  		else
>  			e_info.multi_error_valid = 0;
>  
> -		aer_print_rp_info(pdev, &e_info);
> -
> -		if (find_source_device(pdev, &e_info))
> +		if (find_source_device(pdev, &e_info)) {
> +			aer_print_rp_info(pdev, &e_info);
>  			aer_process_err_devices(&e_info, KERN_ERR);
> +		}
>  	}
>  }
>  
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 81cd6e8ff3a4..42a36df4a651 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -290,7 +290,7 @@ void dpc_process_error(struct pci_dev *pdev)
>  		 dpc_get_aer_uncorrect_severity(pdev, &info) &&
>  		 aer_get_device_error_info(pdev, &info)) {
>  		pci_dev_aer_stats_incr(pdev, &info);
> -		aer_print_error(pdev, &info, KERN_ERR);
> +		aer_print_error(pdev, &info, KERN_ERR, false);

And we don't throttle DPC errors.  No opinion on this one.

>  		pci_aer_clear_nonfatal_status(pdev);
>  		pci_aer_clear_fatal_status(pdev);
>  	}
> -- 
> 2.49.0.395.g12beb8f557-goog
> 

Re: [PATCH v5 6/8] PCI/AER: Introduce ratelimit for error logs

[Jon Pan-Doh]

On Tue, Mar 25, 2025 at 10:17 AM Paul E. McKenney <paulmck@kernel.org> 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@google.com/

Thanks,
Jon

Re: [PATCH v5 8/8] PCI/AER: Add sysfs attributes for log ratelimits

[Jon Pan-Doh]

On Sun, Mar 23, 2025 at 5:21 AM Krzysztof Wilczyński <kw@linux.com> wrote:
> That sad, I am not sure if we really  need to explain here how this
> particular store() function works.

Will remove in v6 since it made sense to others w/o comment.

> > +static ssize_t ratelimit_log_enable_show(struct device *dev,
> > +                                      struct device_attribute *attr,
> > +                                      char *buf)
> > +{
> > +     struct pci_dev *pdev = to_pci_dev(dev);
> > +     bool enable = pdev->aer_report->cor_log_ratelimit.interval != 0;
> > +
> > +     return sysfs_emit(buf, "%d\n", enable);
> > +}
>
> Perhaps "status" or "enabled" for the variable name above.

s/enable/enabled in v6.

> > +static ssize_t ratelimit_log_enable_store(struct device *dev,
> > +                                       struct device_attribute *attr,
> > +                                       const char *buf, size_t count)
> > +{
> > +     struct pci_dev *pdev = to_pci_dev(dev);
> > +     bool enable;
> > +     int interval;
>
> I would love if we could add the following here:
>
>         if (!capable(CAP_SYS_ADMIN))
>                 return -EPERM;
>
> To ensure that only privileged user also sporting a proper set capabilities
> can modify this attribute.
> [...]
> > +     static ssize_t                                                  \
> > +     name##_store(struct device *dev, struct device_attribute *attr, \
> > +                  const char *buf, size_t count)                     \
> > +{                                                                    \
> > +     struct pci_dev *pdev = to_pci_dev(dev);                         \
> > +     int burst;                                                      \
>
> Same as earlier comment.  Add the following:
>
>         if (!capable(CAP_SYS_ADMIN))
>                 return -EPERM;

Will add in v6.

Thanks,
Jon

Re: [PATCH v5 6/8] PCI/AER: Introduce ratelimit for error logs

[Bjorn Helgaas]

On Thu, Mar 20, 2025 at 06:58:04PM -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).

> +static bool aer_ratelimited(struct pci_dev *dev, unsigned int severity)
> +{
> +	struct ratelimit_state *ratelimit;
> +
> +	if (severity == AER_CORRECTABLE)
> +		ratelimit = &dev->aer_report->cor_log_ratelimit;
> +	else
> +		ratelimit = &dev->aer_report->uncor_log_ratelimit;
> +
> +	return !__ratelimit(ratelimit);
> +}

I found the __ratelimit() return values a little confusing (1 == print
the message, 0 == don't print), so this is appealing because it's less
confusing by itself.

But I think we should name this "aer_ratelimit()" and return the
result of __ratelimit() without inverting it so it works the same way
as __ratelimit() and similar wrappers like ata_ratelimit(),
net_ratelimit(), drbd_ratelimit().

Re: [PATCH v5 6/8] PCI/AER: Introduce ratelimit for error logs

[Jon Pan-Doh]

On Mon, Mar 31, 2025 at 11:48 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> I found the __ratelimit() return values a little confusing (1 == print
> the message, 0 == don't print), so this is appealing because it's less
> confusing by itself.
>
> But I think we should name this "aer_ratelimit()" and return the
> result of __ratelimit() without inverting it so it works the same way
> as __ratelimit() and similar wrappers like ata_ratelimit(),
> net_ratelimit(), drbd_ratelimit().

Ack. Caught between readability and consistency :).

> On Thu, Mar 20, 2025 at 06:58:04PM -0700, Jon Pan-Doh 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];

s/ratelimited/ratelimit here as well? Should it store aer_ratelimit()
or !aer_ratelimit()?

Thanks,
Jon

Re: [PATCH v5 6/8] PCI/AER: Introduce ratelimit for error logs

[Bjorn Helgaas]

On Mon, Mar 31, 2025 at 05:30:50PM -0700, Jon Pan-Doh wrote:
> On Mon, Mar 31, 2025 at 11:48 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > I found the __ratelimit() return values a little confusing (1 == print
> > the message, 0 == don't print), so this is appealing because it's less
> > confusing by itself.
> >
> > But I think we should name this "aer_ratelimit()" and return the
> > result of __ratelimit() without inverting it so it works the same way
> > as __ratelimit() and similar wrappers like ata_ratelimit(),
> > net_ratelimit(), drbd_ratelimit().
> 
> Ack. Caught between readability and consistency :).
> 
> > On Thu, Mar 20, 2025 at 06:58:04PM -0700, Jon Pan-Doh 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];
> 
> s/ratelimited/ratelimit here as well? Should it store aer_ratelimit()
> or !aer_ratelimit()?

I'm in favor of avoiding negation when possible, so I would name it
"ratelimit" with the semantic of "1 == print", even though that seems
a little backwards to me.  But I think it will make sense to people
who read ratelimiting in other areas.

Re: [PATCH v5 6/8] PCI/AER: Introduce ratelimit for error logs

[Paul E. McKenney]

On Thu, Mar 27, 2025 at 03:49:12PM -0700, Jon Pan-Doh wrote:
> On Tue, Mar 25, 2025 at 10:17 AM Paul E. McKenney <paulmck@kernel.org> 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).

I traced out the call trees in v6.14 and got the following:

------------------------------------------------------------------------

Call Tree for aer_print_port_info().  (This is the old name, with Jon’s
patch applied this is aer_print_rp_info().)

	aer_isr() is a threaded interrupt handler.
		aer_isr() invokes aer_isr_one_error().
			aer_isr_one_error() invokes aer_print_port_info().


Call Tree for aer_print_error()

	aer_isr() is a threaded interrupt handler.
		aer_isr() invokes aer_isr_one_error().
			aer_isr_one_error() invokes aer_process_err_devices().
				aer_process_err_devices() invokes aer_print_error().

	dpc_handler() is a threaded interrupt handler.
		dpc_handler() invokes dpc_process_error().
			dpc_process_error() invokes aer_print_error()

	edr_handle_event() is an ACPI notifier.
		edr_handle_event() invokes dpc_process_error().
			dpc_process_error() invokes aer_print_error().

------------------------------------------------------------------------

So ___ratelimit() could be invoked in aer_isr_one_error() and the result
could be passed down directly to aer_print_port_info() on the one hand
and to aer_print_error() via aer_process_err_devices() on the other.

And ___ratelimit() could be invoked in dpc_process_error() and the
result passed directly to aer_print_error().

This would permit the ratelimit_state structures to be placed in
the portion of the pci_dev structure under #ifdef CONFIG_PCIEAER,
avoiding the need to search the enclosing structure.

Presumably using a helper function that invokes __ratelimit() for
CONFIG_PCIEAER=y kernels and just returns true otherwise.

Or am I missing something here?  (Quite possibly your root-port points
below...)

> > > @@ -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@google.com/

OK, I see that aer_isr_one_error() invokes aer_print_port_info(),
and only then invokes find_source_device(), and only after that invokes
aer_process_err_devices().  And it appears that aer_process_err_devices()
iterates over the devices and chooses one, which it passes to
aer_print_error().

Is the logging by aer_print_port_info() and by the subsequent call
to aer_print_error() to be throttled as a group, but specific to the
non-root device passed to aer_print_error()?  Or should the logging by
aer_print_port_info() be throttled specific to the root device with the
subsequent call to aer_print_error() throttled separately specific to
the non-root device?

Or have I lost the thread completely?

							Thanx, Paul

Re: [PATCH v5 6/8] PCI/AER: Introduce ratelimit for error logs

[Karolina Stolarek]

On 21/03/2025 19:41, Jon Pan-Doh wrote:
> On Fri, Mar 21, 2025 at 6:46 AM Karolina Stolarek
> <karolina.stolarek@oracle.com> wrote:
>>
>> On 21/03/2025 02:58, Jon Pan-Doh wrote:
>>>    void aer_print_error(struct pci_dev *dev, struct aer_err_info *info,
>>> -                  const char *level)
>>> +                  const char *level, bool ratelimited)
>>
>> Ideally, we would like to be able to extract the "ratelimited" flag from
>> the aer_err_info struct, with no need for extra parameters in this function.
> 
> I considered this, but pci_print_aer() and dpc_process_aer() both call
> aer_print_error directly without populating info->dev[]. I thought it
> was easier/cleaner to pass it in vs. populate info->dev[] and then
> read it.

We decided to not rate limit DPC and with my patch, eventually, 
hopefully, landing upstream, we will stop caring about pci_print_aer() 
altogether.

>> It looks like we ratelimit the Root Port logs based on the source device
>> that generated the message, and the actual errors in
>> aer_process_err_devices() use their own ratelimits. As you noted in one
>> of your emails, there might be the case where we report errors but
>> there's no information about the Root Port that issued the interrupt
>> we're handling.
> 
> Your understanding is correct. I think the edge case described is
> acceptable behavior:
> 
> 1. Multiple errors arrive where the first source device is ratelimited
> 2. Root port log and first source device log are not printed
> 3. Other error source device(s) logs are printed

I find it inconsistent. I won't block the series on the basis of this 
change but wanted to point out that such a thing can happen.

(...)

>> The way I understood the suggestion in 20250320202913.GA1097165@bhelgaas
>> is that we evaluate the ratelimit of the Root Port or Downstream Port,
>> save it in aer_err_info, and use it in aer_print_rp_info() and
>> aer_print_error(). I'm worried that one noisy device under a Root Port
>> could hit a ratelimit and hide everything else
> 
> This is not a 100% translation of Bjorn's suggestion as I share your
> concern (1 spammy device ratelimits and hides other devices).

I understand.

>> A fair (and complicated) solution would be to check the ratelimits of
>> all devices in the Error Message to see if there is at least one that
>> can be reported. If so, use that ratelimit when printing both the Root
>> Port info and the error details from that device.
> 
> I mentioned this as well[1], albeit briefly. I'd opt for this if my
> initial solution isn't satisfactory. You could make it more
> complicated by substituting the source device, if it is ratelimited,
> to a non-ratelimited one. However, it's not good as it changes the
> default expectation that the root port log would have the source ID.

Oh yes, that sounds overly complicated. I have some doubts about missing 
Root Port logs in that specific case (even if they are confusing and may 
point to the ratelimited source), but let's go with the series as it is.

All the best,
Karolina

Re: [PATCH v5 6/8] PCI/AER: Introduce ratelimit for error logs

[Bjorn Helgaas]

On Thu, Mar 20, 2025 at 06:58:04PM -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).

Sorry for the long delay getting back to this.  Obviously this series
will need to be rebased to v6.15-rc1.

> +++ 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];

What would you think about this with related changes below:

  int ratelimit[AER_MAX_MULTI_ERR_DEVICES];
  int combined_ratelimit;

> +static bool aer_ratelimited(struct pci_dev *dev, unsigned int severity)
> +{
> +	struct ratelimit_state *ratelimit;
> +
> +	if (severity == AER_CORRECTABLE)
> +		ratelimit = &dev->aer_report->cor_log_ratelimit;
> +	else
> +		ratelimit = &dev->aer_report->uncor_log_ratelimit;
> +
> +	return !__ratelimit(ratelimit);

IMO this will fit better with other ratelimit users if we return int
with:

  return __ratelimit(ratelimit);

>  void aer_print_error(struct pci_dev *dev, struct aer_err_info *info,
> -		     const char *level)
> +		     const char *level, bool ratelimited)
>  {
>  	int layer, agent;
>  	int id = pci_dev_id(dev);
>  
> +	trace_aer_event(dev_name(&dev->dev), (info->status & ~info->mask),
> +			info->severity, info->tlp_header_valid, &info->tlp);

Maybe move the trace_aer_event() call up to aer_process_err_devices(),
where it would be next to the pci_dev_aer_stats_incr()?  Then
aer_print_error() would be pure printing.

The e_info->ratelimit[i] test could go in aer_process_err_devices() as
well, so you wouldn't have to pass it in to aer_print_error().

>  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;
> +		}
> +	}

If add_error_device() sets info->combined_ratelimit (as below), you
could drop the loop above and do this:

  if (info->combined_ratelimit)
    pci_info(rp, "...");

The combined_ratelimit check could go up in aer_isr_one_error() and
this function would also be pure printing.

I guess this and aer_print_error() could go either way: the ratelimit
check inside the function or in the caller.  If you do the check
inside aer_print_error(), you have to pass in ratelimit because you
don't know which element of the info->ratelimit[] table to look at,
which I guess is an argument for doing the check in the callers.

> +	if (!ratelimited)
> +		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));
>  }

>  
>  #ifdef CONFIG_ACPI_APEI_PCIEAER
> @@ -784,6 +822,12 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,
>  
>  	pci_dev_aer_stats_incr(dev, &info);
>  
> +	trace_aer_event(dev_name(&dev->dev), (status & ~mask),
> +			aer_severity, tlp_header_valid, &aer->header_log);
> +
> +	if (aer_ratelimited(dev, aer_severity))
> +		return;
> +
>  	aer_printk(level, dev, "aer_status: 0x%08x, aer_mask: 0x%08x\n", status, mask);
>  	__aer_print_error(dev, &info, level);
>  	aer_printk(level, dev, "aer_layer=%s, aer_agent=%s\n",
> @@ -795,9 +839,6 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,
>  
>  	if (tlp_header_valid)
>  		pcie_print_tlp_log(dev, &aer->header_log, dev_fmt("  "));
> -
> -	trace_aer_event(dev_name(&dev->dev), (status & ~mask),
> -			aer_severity, tlp_header_valid, &aer->header_log);
>  }
>  EXPORT_SYMBOL_NS_GPL(pci_print_aer, "CXL");
>  
> @@ -808,8 +849,12 @@ EXPORT_SYMBOL_NS_GPL(pci_print_aer, "CXL");
>   */
>  static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
>  {
> -	if (e_info->error_dev_num < AER_MAX_MULTI_ERR_DEVICES) {
> -		e_info->dev[e_info->error_dev_num] = pci_dev_get(dev);
> +	int dev_idx = e_info->error_dev_num;
> +	unsigned int severity = e_info->severity;
> +
> +	if (dev_idx < AER_MAX_MULTI_ERR_DEVICES) {
> +		e_info->dev[dev_idx] = pci_dev_get(dev);
> +		e_info->ratelimited[dev_idx] = aer_ratelimited(dev, severity);

If we have info to print for this device (ratelimit==1), we should
also print the Root Port header.  I think this would be simpler than
combining the device ratelimits in aer_print_rp_info():

  int ratelimit = aer_ratelimit(dev, severity);
  e_info->ratelimited[dev_idx] = ratelimit;
  e_info->combined_ratelimit |= ratelimit;

>  		e_info->error_dev_num++;
>  		return 0;
>  	}
> @@ -1265,7 +1310,8 @@ static inline void aer_process_err_devices(struct aer_err_info *e_info,
>  	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
>  		if (aer_get_device_error_info(e_info->dev[i], e_info)) {
>  			pci_dev_aer_stats_incr(e_info->dev[i], e_info);
> -			aer_print_error(e_info->dev[i], e_info, level);
> +			aer_print_error(e_info->dev[i], e_info, level,
> +					e_info->ratelimited[i]);
>  		}
>  	}
>  	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
> @@ -1299,10 +1345,11 @@ static void aer_isr_one_error(struct aer_rpc *rpc,

Tangent: I'm a little queasy about how e_info is an uninitialized
stack variable in aer_isr_one_error().  There are hints that we know
about this, e.g., the "Must reset in this function" comment in
find_source_device(), but I would feel a lot better about this if we
just cleared it out.

>  			e_info.multi_error_valid = 1;
>  		else
>  			e_info.multi_error_valid = 0;
> -		aer_print_rp_info(pdev, &e_info);
>  
> -		if (find_source_device(pdev, &e_info))
> +		if (find_source_device(pdev, &e_info)) {
> +			aer_print_rp_info(pdev, &e_info);
>  			aer_process_err_devices(&e_info, KERN_WARNING);
> +		}

Previously we always printed the RP info ("error message received
from").  Now we only print the RP info if we found a downstream device
with error info.

I think we should print the RP info even if we can't find the
downstream device (maybe it's broken, was yanked out, powered off,
etc), e.g., maybe something like this:

    if (find_source_device(pdev, &e_info)) {
	if (e_info.combined_ratelimit)
	    aer_print_rp_info(pdev, &e_info);
	aer_process_err_devices(&e_info, KERN_WARNING);
    } else {
	if (aer_ratelimit(pdev, AER_CORRECTABLE))
	    aer_print_rp_info(pdev, &e_info);
    }

The idea is:

  - we print the RP info if any downstream device info will be
    printed, and the downstream info is ratelimited based on the
    device it came from, and

  - if we don't find downstream error info, we ratelimit printing the
    RP info based on the RP itself.

Bjorn

Re: [PATCH v5 1/8] PCI/AER: Check log level once and propagate down

[Bjorn Helgaas]

On Thu, Mar 20, 2025 at 06:57:59PM -0700, Jon Pan-Doh wrote:
> From: Karolina Stolarek <karolina.stolarek@oracle.com>
> 
> When reporting an AER error, we check its type multiple times
> to determine the log level for each message. Do this check only
> in the top-level functions (aer_isr_one_error(), pci_print_aer()) and
> propagate the result down the call chain.
> 
> Signed-off-by: Karolina Stolarek <karolina.stolarek@oracle.com>
> Signed-off-by: Jon Pan-Doh <pandoh@google.com>
> Reported-by: Sargun Dhillon <sargun@meta.com>
> Acked-by: Paul E. McKenney <paulmck@kernel.org>
> Reviewed-by: Jon Pan-Doh <pandoh@google.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  drivers/pci/pci.h      |  2 +-
>  drivers/pci/pcie/aer.c | 34 +++++++++++++++++-----------------
>  drivers/pci/pcie/dpc.c |  2 +-
>  3 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index b8911d1e10dc..75985b96ecc1 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -551,7 +551,7 @@ struct aer_err_info {
>  };
>  
>  int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
> -void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
> +void aer_print_error(struct pci_dev *dev, struct aer_err_info *info, const char *level);

I'm (finally) getting back to this series because it really needs to
make v6.16.

It would definitely be nice to determine the log level once instead of
several times, but I'm not sure I like passing "level" through the
whole chain because it seems like a lot of change to get that benefit:

  - it changes the prototype for __aer_print_error(),
    aer_print_error(), and aer_process_err_devices()

  - it removes the info->severity test from aer_print_error(), but
    leaves it in __aer_print_error() and pci_print_aer(), which need
    it for other reasons

All these functions take a pointer to a struct aer_err_info, and if we
want to compute the log level once, maybe we could stash the result in
struct aer_err_info, similar to what we did with ratelimited[] here:
https://lore.kernel.org/all/20250321015806.954866-7-pandoh@google.com/

I'm rebasing this series to v6.15-rc1 and will post a v6 proposal
soon.

>  int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
>  		      unsigned int tlp_len, bool flit,
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 9cff7069577e..45629e1ea058 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -670,20 +670,18 @@ static void pci_rootport_aer_stats_incr(struct pci_dev *pdev,
>  }
>  
>  static void __aer_print_error(struct pci_dev *dev,
> -			      struct aer_err_info *info)
> +			      struct aer_err_info *info,
> +			      const char *level)
>  {
>  	const char **strings;
>  	unsigned long status = info->status & ~info->mask;
> -	const char *level, *errmsg;
> +	const char *errmsg;
>  	int i;
>  
> -	if (info->severity == AER_CORRECTABLE) {
> +	if (info->severity == AER_CORRECTABLE)
>  		strings = aer_correctable_error_string;
> -		level = KERN_WARNING;
> -	} else {
> +	else
>  		strings = aer_uncorrectable_error_string;
> -		level = KERN_ERR;
> -	}
>  
>  	for_each_set_bit(i, &status, 32) {
>  		errmsg = strings[i];
> @@ -696,11 +694,11 @@ static void __aer_print_error(struct pci_dev *dev,
>  	pci_dev_aer_stats_incr(dev, info);
>  }
>  
> -void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
> +void aer_print_error(struct pci_dev *dev, struct aer_err_info *info,
> +		     const char *level)
>  {
>  	int layer, agent;
>  	int id = pci_dev_id(dev);
> -	const char *level;
>  
>  	if (!info->status) {
>  		pci_err(dev, "PCIe Bus Error: severity=%s, type=Inaccessible, (Unregistered Agent ID)\n",
> @@ -711,8 +709,6 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
>  	layer = AER_GET_LAYER_ERROR(info->severity, info->status);
>  	agent = AER_GET_AGENT(info->severity, info->status);
>  
> -	level = (info->severity == AER_CORRECTABLE) ? KERN_WARNING : KERN_ERR;
> -
>  	aer_printk(level, dev, "PCIe Bus Error: severity=%s, type=%s, (%s)\n",
>  		   aer_error_severity_string[info->severity],
>  		   aer_error_layer[layer], aer_agent_string[agent]);
> @@ -720,7 +716,7 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
>  	aer_printk(level, dev, "  device [%04x:%04x] error status/mask=%08x/%08x\n",
>  		   dev->vendor, dev->device, info->status, info->mask);
>  
> -	__aer_print_error(dev, info);
> +	__aer_print_error(dev, info, level);
>  
>  	if (info->tlp_header_valid)
>  		pcie_print_tlp_log(dev, &info->tlp, dev_fmt("  "));
> @@ -765,15 +761,18 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,
>  {
>  	int layer, agent, tlp_header_valid = 0;
>  	u32 status, mask;
> +	const char *level;
>  	struct aer_err_info info;
>  
>  	if (aer_severity == AER_CORRECTABLE) {
>  		status = aer->cor_status;
>  		mask = aer->cor_mask;
> +		level = KERN_WARNING;
>  	} else {
>  		status = aer->uncor_status;
>  		mask = aer->uncor_mask;
>  		tlp_header_valid = status & AER_LOG_TLP_MASKS;
> +		level = KERN_ERR;
>  	}
>  
>  	layer = AER_GET_LAYER_ERROR(aer_severity, status);
> @@ -786,7 +785,7 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,
>  	info.first_error = PCI_ERR_CAP_FEP(aer->cap_control);
>  
>  	pci_err(dev, "aer_status: 0x%08x, aer_mask: 0x%08x\n", status, mask);
> -	__aer_print_error(dev, &info);
> +	__aer_print_error(dev, &info, level);
>  	pci_err(dev, "aer_layer=%s, aer_agent=%s\n",
>  		aer_error_layer[layer], aer_agent_string[agent]);
>  
> @@ -1257,14 +1256,15 @@ int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
>  	return 1;
>  }
>  
> -static inline void aer_process_err_devices(struct aer_err_info *e_info)
> +static inline void aer_process_err_devices(struct aer_err_info *e_info,
> +					   const char *level)
>  {
>  	int i;
>  
>  	/* Report all before handle them, not to lost records by reset etc. */
>  	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
>  		if (aer_get_device_error_info(e_info->dev[i], e_info))
> -			aer_print_error(e_info->dev[i], e_info);
> +			aer_print_error(e_info->dev[i], e_info, level);
>  	}
>  	for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
>  		if (aer_get_device_error_info(e_info->dev[i], e_info))
> @@ -1300,7 +1300,7 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
>  		aer_print_port_info(pdev, &e_info);
>  
>  		if (find_source_device(pdev, &e_info))
> -			aer_process_err_devices(&e_info);
> +			aer_process_err_devices(&e_info, KERN_WARNING);
>  	}
>  
>  	if (e_src->status & PCI_ERR_ROOT_UNCOR_RCV) {
> @@ -1319,7 +1319,7 @@ static void aer_isr_one_error(struct aer_rpc *rpc,
>  		aer_print_port_info(pdev, &e_info);
>  
>  		if (find_source_device(pdev, &e_info))
> -			aer_process_err_devices(&e_info);
> +			aer_process_err_devices(&e_info, KERN_ERR);
>  	}
>  }
>  
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index df42f15c9829..9e4c9ac737a7 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -289,7 +289,7 @@ void dpc_process_error(struct pci_dev *pdev)
>  	else if (reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_UNCOR &&
>  		 dpc_get_aer_uncorrect_severity(pdev, &info) &&
>  		 aer_get_device_error_info(pdev, &info)) {
> -		aer_print_error(pdev, &info);
> +		aer_print_error(pdev, &info, KERN_ERR);
>  		pci_aer_clear_nonfatal_status(pdev);
>  		pci_aer_clear_fatal_status(pdev);
>  	}
> -- 
> 2.49.0.395.g12beb8f557-goog
> 

Re: [PATCH v5 5/8] PCI/AER: Rename struct aer_stats to aer_report

[Bjorn Helgaas]

On Fri, Mar 21, 2025 at 05:39:30PM -0500, Bjorn Helgaas wrote:
> On Fri, Mar 21, 2025 at 03:16:48PM -0700, Paul E. McKenney wrote:
> > On Fri, Mar 21, 2025 at 05:01:15PM -0500, Bjorn Helgaas wrote:
> > > On Thu, Mar 20, 2025 at 06:58:03PM -0700, Jon Pan-Doh wrote:
> > > > Update name to reflect the broader definition of structs/variables that
> > > > are stored (e.g. ratelimits). This is a preparatory patch for adding rate
> > > > limit support.
> > > > 
> > > > Signed-off-by: Karolina Stolarek <karolina.stolarek@oracle.com>
> > > > Signed-off-by: Jon Pan-Doh <pandoh@google.com>
> > > > Reported-by: Sargun Dhillon <sargun@meta.com>
> > > 
> > > What did Sargun report?  Is there a bug fix in here?  Can we include a
> > > URL to whatever Sargun reported?
> > 
> > He reported RCU CPU stall warnings and CSD-lock warnings internally
> > within Meta, so sorry, no useful URL.

CSD?  I guess knowing what CSD is isn't completely essential here, but
I haven't a clue what it means.  Something to do with IPI and getting
another CPU to run a function?  Is CSD an acronym for something?

Bjorn

Re: [PATCH v5 5/8] PCI/AER: Rename struct aer_stats to aer_report

[Paul E. McKenney]

On Thu, May 01, 2025 at 05:02:59PM -0500, Bjorn Helgaas wrote:
> On Fri, Mar 21, 2025 at 05:39:30PM -0500, Bjorn Helgaas wrote:
> > On Fri, Mar 21, 2025 at 03:16:48PM -0700, Paul E. McKenney wrote:
> > > On Fri, Mar 21, 2025 at 05:01:15PM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Mar 20, 2025 at 06:58:03PM -0700, Jon Pan-Doh wrote:
> > > > > Update name to reflect the broader definition of structs/variables that
> > > > > are stored (e.g. ratelimits). This is a preparatory patch for adding rate
> > > > > limit support.
> > > > > 
> > > > > Signed-off-by: Karolina Stolarek <karolina.stolarek@oracle.com>
> > > > > Signed-off-by: Jon Pan-Doh <pandoh@google.com>
> > > > > Reported-by: Sargun Dhillon <sargun@meta.com>
> > > > 
> > > > What did Sargun report?  Is there a bug fix in here?  Can we include a
> > > > URL to whatever Sargun reported?
> > > 
> > > He reported RCU CPU stall warnings and CSD-lock warnings internally
> > > within Meta, so sorry, no useful URL.
> 
> CSD?  I guess knowing what CSD is isn't completely essential here, but
> I haven't a clue what it means.  Something to do with IPI and getting
> another CPU to run a function?  Is CSD an acronym for something?

Yeah, it is a bit obscure, apologies!

It is "CSD" as in the csd_lock() function, and it stands for "call single
data", as in the ->csd field of the call_function_data structure, which
is of type call_single_data_t.  All in the service of smp_call_function()
and friends, mostly in the file kernel/smp.c.

							Thanx, Paul

Re: [PATCH v5 1/8] PCI/AER: Check log level once and propagate down

[Karolina Stolarek]

On 01/05/2025 23:43, Bjorn Helgaas wrote:
> 
> I'm (finally) getting back to this series because it really needs to
> make v6.16.
> 
> It would definitely be nice to determine the log level once instead of
> several times, but I'm not sure I like passing "level" through the
> whole chain because it seems like a lot of change to get that benefit:
> 
>    - it changes the prototype for __aer_print_error(),
>      aer_print_error(), and aer_process_err_devices()
> 
>    - it removes the info->severity test from aer_print_error(), but
>      leaves it in __aer_print_error() and pci_print_aer(), which need
>      it for other reasons
> 
> All these functions take a pointer to a struct aer_err_info, and if we
> want to compute the log level once, maybe we could stash the result in
> struct aer_err_info, similar to what we did with ratelimited[] here:
> https://lore.kernel.org/all/20250321015806.954866-7-pandoh@google.com/

I think that would be a good compromise between these two approaches.

> I'm rebasing this series to v6.15-rc1 and will post a v6 proposal
> soon.

Do you plan to include changes suggested in the thread or just rebase 
the series?

Also, it's still unclear to me how to approach the sysfs patch, both in 
the context of the ratelimit refactor (which, some of it, is in the next 
tree)[1] and the value that should be exposed in the attribute. We have 
control only over the burst but not the interval. When we deal with high 
rates of errors, we may want to increase the time window to see if the 
flood is out of ordinary or is it constant.

All the best,
Karolina
-------------------------------------------------
[1] - 
https://lore.kernel.org/all/b0883f20-c337-40bb-b564-c535a162bf54@paulmck-laptop/

Re: [PATCH v5 1/8] PCI/AER: Check log level once and propagate down

[Bjorn Helgaas]

On Mon, May 05, 2025 at 11:30:58AM +0200, Karolina Stolarek wrote:
> On 01/05/2025 23:43, Bjorn Helgaas wrote:
> > 
> > I'm (finally) getting back to this series because it really needs to
> > make v6.16.
> > 
> > It would definitely be nice to determine the log level once instead of
> > several times, but I'm not sure I like passing "level" through the
> > whole chain because it seems like a lot of change to get that benefit:
> > 
> >    - it changes the prototype for __aer_print_error(),
> >      aer_print_error(), and aer_process_err_devices()
> > 
> >    - it removes the info->severity test from aer_print_error(), but
> >      leaves it in __aer_print_error() and pci_print_aer(), which need
> >      it for other reasons
> > 
> > All these functions take a pointer to a struct aer_err_info, and if we
> > want to compute the log level once, maybe we could stash the result in
> > struct aer_err_info, similar to what we did with ratelimited[] here:
> > https://lore.kernel.org/all/20250321015806.954866-7-pandoh@google.com/
> 
> I think that would be a good compromise between these two approaches.
> 
> > I'm rebasing this series to v6.15-rc1 and will post a v6 proposal
> > soon.
> 
> Do you plan to include changes suggested in the thread or just rebase the
> series?

Yes.

> Also, it's still unclear to me how to approach the sysfs patch, both in the
> context of the ratelimit refactor (which, some of it, is in the next
> tree)[1] and the value that should be exposed in the attribute. We have
> control only over the burst but not the interval. When we deal with high
> rates of errors, we may want to increase the time window to see if the flood
> is out of ordinary or is it constant.

Unclear to me, too.  Might have to revisit that.

> [1] - https://lore.kernel.org/all/b0883f20-c337-40bb-b564-c535a162bf54@paulmck-laptop/
> 

Re: [PATCH v5 1/8] PCI/AER: Check log level once and propagate down

[Karolina Stolarek]

On 05/05/2025 19:43, Bjorn Helgaas wrote:
> On Mon, May 05, 2025 at 11:30:58AM +0200, Karolina Stolarek wrote:
>> On 01/05/2025 23:43, Bjorn Helgaas wrote:
>>>
>>> I'm (finally) getting back to this series because it really needs to
>>> make v6.16.
>>>
>>> It would definitely be nice to determine the log level once instead of
>>> several times, but I'm not sure I like passing "level" through the
>>> whole chain because it seems like a lot of change to get that benefit:
>>>
>>>     - it changes the prototype for __aer_print_error(),
>>>       aer_print_error(), and aer_process_err_devices()
>>>
>>>     - it removes the info->severity test from aer_print_error(), but
>>>       leaves it in __aer_print_error() and pci_print_aer(), which need
>>>       it for other reasons
>>>
>>> All these functions take a pointer to a struct aer_err_info, and if we
>>> want to compute the log level once, maybe we could stash the result in
>>> struct aer_err_info, similar to what we did with ratelimited[] here:
>>> https://lore.kernel.org/all/20250321015806.954866-7-pandoh@google.com/
>>
>> I think that would be a good compromise between these two approaches.
>>
>>> I'm rebasing this series to v6.15-rc1 and will post a v6 proposal
>>> soon.
>>
>> Do you plan to include changes suggested in the thread or just rebase the
>> series?
> 
> Yes.
> 
>> Also, it's still unclear to me how to approach the sysfs patch, both in the
>> context of the ratelimit refactor (which, some of it, is in the next
>> tree)[1] and the value that should be exposed in the attribute. We have
>> control only over the burst but not the interval. When we deal with high
>> rates of errors, we may want to increase the time window to see if the flood
>> is out of ordinary or is it constant.
> 
> Unclear to me, too.  Might have to revisit that.

I understand. Let me know if you need any help with the series.

All the best,
Karolina

> 
>> [1] - https://lore.kernel.org/all/b0883f20-c337-40bb-b564-c535a162bf54@paulmck-laptop/
>>