Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1360164 > unrolled thread

[PATCH v2 0/2] iommu/vt-d: Fault logging improvements

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/2] iommu/vt-d: Fault logging improvements Alex Williamson <alex.williamson@redhat.com> - 2016-03-17 21:20 +0100
    [PATCH v2 1/2] iommu/vt-d: Ratelimit fault handler Alex Williamson <alex.williamson@redhat.com> - 2016-03-17 21:20 +0100
      Re: [PATCH v2 1/2] iommu/vt-d: Ratelimit fault handler Joe Perches <joe@perches.com> - 2016-03-17 21:40 +0100
        Re: [PATCH v2 1/2] iommu/vt-d: Ratelimit fault handler Alex Williamson <alex.williamson@redhat.com> - 2016-03-17 21:50 +0100
    Re: [PATCH v2 0/2] iommu/vt-d: Fault logging improvements Joe Perches <joe@perches.com> - 2016-03-17 21:30 +0100

#1360164 — [PATCH v2 0/2] iommu/vt-d: Fault logging improvements

Ratelimit and improve formatting.

v2:

 - Use a single ratelimit state as suggested by Joe Perches, except
   I chose to move it up to dmar_fault() so that it includes the
   "handling fault status reg" pr_err and we can avoid collecting
   entries for logging if we don't plan to print them.
 - Added reformatting changes suggested by Joe Perches.
 - While there is clearly more that could be done with disabling
   fault handling for specific context entries on storm and sending
   errors to drivers, this makes a marked improvement on its own.

Thanks,
Alex

---

Alex Williamson (2):
      iommu/vt-d: Ratelimit fault handler
      iommu/vt-d: Improve fault handler error messages


 drivers/iommu/dmar.c |   47 +++++++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 20 deletions(-)

[toc] | [next] | [standalone]

#1360165 — [PATCH v2 1/2] iommu/vt-d: Ratelimit fault handler

Fault rates can easily overwhelm the console and make the system
unresponsive.  Ratelimit to allow an opportunity for maintenance.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/iommu/dmar.c |   33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 8ffd756..8f8bfff 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -1602,10 +1602,17 @@ irqreturn_t dmar_fault(int irq, void *dev_id)
 	int reg, fault_index;
 	u32 fault_status;
 	unsigned long flag;
+	bool ratelimited;
+	static DEFINE_RATELIMIT_STATE(rs,
+				      DEFAULT_RATELIMIT_INTERVAL,
+				      DEFAULT_RATELIMIT_BURST);
+
+	/* Disable printing, simply clear the fault when ratelimited */
+	ratelimited = !__ratelimit(&rs);
 
 	raw_spin_lock_irqsave(&iommu->register_lock, flag);
 	fault_status = readl(iommu->reg + DMAR_FSTS_REG);
-	if (fault_status)
+	if (fault_status && !ratelimited)
 		pr_err("DRHD: handling fault status reg %x\n", fault_status);
 
 	/* TBD: ignore advanced fault log currently */
@@ -1627,24 +1634,28 @@ irqreturn_t dmar_fault(int irq, void *dev_id)
 		if (!(data & DMA_FRCD_F))
 			break;
 
-		fault_reason = dma_frcd_fault_reason(data);
-		type = dma_frcd_type(data);
+		if (!ratelimited) {
+			fault_reason = dma_frcd_fault_reason(data);
+			type = dma_frcd_type(data);
 
-		data = readl(iommu->reg + reg +
-				fault_index * PRIMARY_FAULT_REG_LEN + 8);
-		source_id = dma_frcd_source_id(data);
+			data = readl(iommu->reg + reg +
+				     fault_index * PRIMARY_FAULT_REG_LEN + 8);
+			source_id = dma_frcd_source_id(data);
+
+			guest_addr = dmar_readq(iommu->reg + reg +
+					fault_index * PRIMARY_FAULT_REG_LEN);
+			guest_addr = dma_frcd_page_addr(guest_addr);
+		}
 
-		guest_addr = dmar_readq(iommu->reg + reg +
-				fault_index * PRIMARY_FAULT_REG_LEN);
-		guest_addr = dma_frcd_page_addr(guest_addr);
 		/* clear the fault */
 		writel(DMA_FRCD_F, iommu->reg + reg +
 			fault_index * PRIMARY_FAULT_REG_LEN + 12);
 
 		raw_spin_unlock_irqrestore(&iommu->register_lock, flag);
 
-		dmar_fault_do_one(iommu, type, fault_reason,
-				source_id, guest_addr);
+		if (!ratelimited)
+			dmar_fault_do_one(iommu, type, fault_reason,
+					  source_id, guest_addr);
 
 		fault_index++;
 		if (fault_index >= cap_num_fault_regs(iommu->cap))

[toc] | [prev] | [next] | [standalone]

#1360179 — Re: [PATCH v2 1/2] iommu/vt-d: Ratelimit fault handler

On Thu, 2016-03-17 at 14:12 -0600, Alex Williamson wrote:
> Fault rates can easily overwhelm the console and make the system
> unresponsive.  Ratelimit to allow an opportunity for maintenance.
[]
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
[]
> @@ -1602,10 +1602,17 @@ irqreturn_t dmar_fault(int irq, void *dev_id)
>  	int reg, fault_index;
>  	u32 fault_status;
>  	unsigned long flag;
> +	bool ratelimited;
> +	static DEFINE_RATELIMIT_STATE(rs,
> +				      DEFAULT_RATELIMIT_INTERVAL,
> +				      DEFAULT_RATELIMIT_BURST);

Are these the appropriate limits for dmar?

include/linux/ratelimit.h:#define DEFAULT_RATELIMIT_INTERVAL    (5 * HZ)
include/linux/ratelimit.h:#define DEFAULT_RATELIMIT_BURST       10

[toc] | [prev] | [next] | [standalone]

#1360187 — Re: [PATCH v2 1/2] iommu/vt-d: Ratelimit fault handler

On Thu, 17 Mar 2016 13:33:30 -0700
Joe Perches <joe@perches.com> wrote:

> On Thu, 2016-03-17 at 14:12 -0600, Alex Williamson wrote:
> > Fault rates can easily overwhelm the console and make the system
> > unresponsive.  Ratelimit to allow an opportunity for maintenance.  
> []
> > diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c  
> []
> > @@ -1602,10 +1602,17 @@ irqreturn_t dmar_fault(int irq, void *dev_id)
> >  	int reg, fault_index;
> >  	u32 fault_status;
> >  	unsigned long flag;
> > +	bool ratelimited;
> > +	static DEFINE_RATELIMIT_STATE(rs,
> > +				      DEFAULT_RATELIMIT_INTERVAL,
> > +				      DEFAULT_RATELIMIT_BURST);  
> 
> Are these the appropriate limits for dmar?
> 
> include/linux/ratelimit.h:#define DEFAULT_RATELIMIT_INTERVAL    (5 * HZ)
> include/linux/ratelimit.h:#define DEFAULT_RATELIMIT_BURST       10

They seem OK to me, I've got a test running that continuously generates
DMA read faults and I get 20 lines of log every 5 seconds.  That seems
like enough to know there's an issue, it's ongoing, and maybe see some
patterns in the fault addresses.  I expect we could turn up the burst
value but generally when I'm looking at the logs I'm only looking for
things like is it a single target address, is it a sequential address,
or what's the general address space to know if it should or should not
be a valid fault address.  Thanks,

Alex

[toc] | [prev] | [next] | [standalone]

#1360170

On Thu, 2016-03-17 at 14:12 -0600, Alex Williamson wrote:
> Ratelimit and improve formatting.

Makes sense, thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web