iommu/vt-d: Ratelimit each dmar fault printing

Theoretically, on some machines faults might be generated faster than they're cleared by CPU.

Let's limit the cleaning-loop by number of hw
fault registers.

Cc Alex Williamson <[email protected]>

Cc David Woodhouse <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Lu Baolu <baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]>

Cc iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org

Signed-off-by Dmitry Safonov <[email protected]>

---
 drivers/iommu/dmar.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/iommu/dmar.c +2 -2

index 6c4ea32ee6a9..cf1105111209 100644
@@ -1615,7 +1615,7 @@ static int dmar_fault_do_one(struct intel_iommu *iommu, int type,
 irqreturn_t dmar_fault(int irq, void *dev_id)
 {
 	struct intel_iommu *iommu = dev_id;
-	int reg, fault_index;
+	int reg, fault_index, i;
 	u32 fault_status;
 	unsigned long flag;
 	static DEFINE_RATELIMIT_STATE(rs,

@@ -1633,7 +1633,7 @@ irqreturn_t dmar_fault(int irq, void *dev_id)
 
 	fault_index = dma_fsts_fault_record_index(fault_status);
 	reg = cap_fault_reg_offset(iommu->cap);
-	while (1) {
+	for (i = 0; i < cap_num_fault_regs(iommu->cap); i++) {
 		/* Disable printing, simply clear the fault when ratelimited */
 		bool ratelimited = !__ratelimit(&rs);
 		u8 fault_reason;

Hi,

Quoted 3 lines · Dmitry Safonov

Theoretically, on some machines faults might be generated faster than
they're cleared by CPU.

Is this a real case?

Quoted 3 lines

Let's limit the cleaning-loop by number of hw
fault registers.

Will this cause the fault recording registers full of faults, hence new faults will be dropped without logging?

And even worse, new faults will not generate interrupts?

Hi Lu,

Quoted 6 lines · Lu Baolu

Hi,

Quoted 3 lines · Dmitry Safonov

Theoretically, on some machines faults might be generated faster than
they're cleared by CPU.

Quoted 1 line

Is this a real case?

No. 1/2 is a real case and this one was discussed on v3:
lkml.kernel.org/r/<20180215191729.15777-1-dima-nzgTgzXrdUbQT0dZR+AlfA@public.gmane.org>

It's not possible on my hw as far as I tried, but the discussion result was to fix this theoretical issue too.

Quoted 5 lines

Quoted 3 lines

Let's limit the cleaning-loop by number of hw
fault registers.

Will this cause the fault recording registers full of faults, hence new faults will be dropped without logging?

If faults come faster then they're being cleared - some of them will be dropped without logging. Not sure if it's worth to report all faults in such theoretical(!) situation.
If amount of reported faults for such situation is not enough and it's worth to keep all the faults, then probably we should introduce a workqueue here (which I did in v1, but it was rejected by the reason that it will introduce some latency in fault reporting).

Quoted 1 line

And even worse, new faults will not generate interrupts?

They will, we clear page fault overflow outside of the loop, so any new fault will raise interrupt, iiuc.

Hi,

Quoted 16 lines · Dmitry Safonov

Hi Lu,

Quoted 9 lines · Lu Baolu

Hi,

Quoted 4 lines · Dmitry Safonov

Theoretically, on some machines faults might be generated faster
than
they're cleared by CPU.

Is this a real case?

No. 1/2 is a real case and this one was discussed on v3:
lkml.kernel.org/r/<20180215191729.15777-1-dima-nzgTgzXrdUbQT0dZR+AlfA@public.gmane.org>

Quoted 3 lines

It's not possible on my hw as far as I tried, but the discussion result
was to fix this theoretical issue too.

If faults are generated faster than CPU can clear them, the PCIe device should be in a very very bad state. How about disabling the PCIe device and ask the administrator to replace it? Anyway, I don't think that's goal of this patch series. :-)

Quoted 19 lines

Quoted 6 lines

Quoted 2 lines

Let's limit the cleaning-loop by number of hw
fault registers.

Will this cause the fault recording registers full of faults, hence
new faults will be dropped without logging?

If faults come faster then they're being cleared - some of them will be
dropped without logging. Not sure if it's worth to report all faults in
such theoretical(!) situation.
If amount of reported faults for such situation is not enough and it's
worth to keep all the faults, then probably we should introduce a workqueue here (which I did in v1, but it was rejected by the reason
that it will introduce some latency in fault reporting).

Quoted 6 lines

Quoted 1 line

And even worse, new faults will not generate interrupts?

They will, we clear page fault overflow outside of the loop, so any new
fault will raise interrupt, iiuc.

I am afraid that they might not generate interrupts any more.

Say, the fault registers are full of events that are not cleared, then a new fault comes. There is no room for this event and hence the hardware might drop it silently.

Quoted 17 lines · Lu Baolu

Hi,

Quoted 14 lines · Dmitry Safonov

Hi Lu,

Quoted 8 lines · Lu Baolu

Hi,

Quoted 4 lines · Dmitry Safonov

Theoretically, on some machines faults might be generated faster
than
they're cleared by CPU.

Is this a real case?

No. 1/2 is a real case and this one was discussed on v3:
lkml.kernel.org/r/<20180215191729.15777-1-dima-nzgTgzXrdUbQT0dZR+AlfA@public.gmane.org>

Quoted 7 lines

Quoted 3 lines

It's not possible on my hw as far as I tried, but the discussion result
was to fix this theoretical issue too.

If faults are generated faster than CPU can clear them, the PCIe device should be in a very very bad state. How about disabling the PCIe device and ask the administrator to replace it? Anyway, I don't think that's goal of this patch series. :-)

Uhm, yeah, my point is not about the number of faults, but about physical ability of iommu to generate faults faster than cpu processes them. I might be wrong that it's not possible (like low cpu freq?)

But the number of interrupts might be high. It's like you've many mappings on iommu and PCIe device went off. It could be just a link flap. I think it makes sense not lockup on such occasions.

Quoted 24 lines

Quoted 17 lines

Quoted 5 lines

Quoted 2 lines

Let's limit the cleaning-loop by number of hw
fault registers.

Will this cause the fault recording registers full of faults, hence
new faults will be dropped without logging?

If faults come faster then they're being cleared - some of them will be
dropped without logging. Not sure if it's worth to report all faults in
such theoretical(!) situation.
If amount of reported faults for such situation is not enough and it's
worth to keep all the faults, then probably we should introduce a workqueue here (which I did in v1, but it was rejected by the reason
that it will introduce some latency in fault reporting).

Quoted 5 lines

Quoted 1 line

And even worse, new faults will not generate interrupts?

They will, we clear page fault overflow outside of the loop, so any new
fault will raise interrupt, iiuc.

I am afraid that they might not generate interrupts any more.

Quoted 3 lines

Say, the fault registers are full of events that are not cleared, then a new fault comes. There is no room for this event and hence the hardware might drop it silently.

AFAICS, we're doing fault-clearing in a loop inside irq handler. That means that while we're clearing if a fault raises, it'll make an irq level triggered (or on edge) on lapic. So, whenever we return from the irq handler, irq will raise again.

Hi,

Quoted 6 lines · Dmitry Safonov

AFAICS, we're doing fault-clearing in a loop inside irq handler. That means that while we're clearing if a fault raises, it'll make an irq level triggered (or on edge) on lapic. So, whenever we return
from the irq handler, irq will raise again.

Uhm, double checked with the spec. Interrupts should be generated since we always clear the fault overflow bit.

Anyway, we can't clear faults in a limited loop, as the spec says in 7.3.1:

Software is expected to process the non-recoverable faults reported through the Fault Recording
Registers in a circular FIFO fashion starting from the Fault Recording Register referenced by the Fault
Recording Index (FRI) field, until it finds a Fault Recording Register with no faults (F field Clear).

Best regards,
Lu Baolu

Quoted 12 lines · Lu Baolu

Hi,

Quoted 6 lines · Dmitry Safonov

AFAICS, we're doing fault-clearing in a loop inside irq handler. That means that while we're clearing if a fault raises, it'll make an irq level triggered (or on edge) on lapic. So, whenever we return
from the irq handler, irq will raise again.

Uhm, double checked with the spec. Interrupts should be generated since we always clear the fault overflow bit.

Quoted 2 lines

Anyway, we can't clear faults in a limited loop, as the spec says in 7.3.1:

Mind to elaborate?
ITOW, I do not see a contradiction. We're still clearing faults in FIFO fashion. There is no limitation to do some spare work in between clearings (return from interrupt, then fault again and continue).

Hi,

Quoted 13 lines · Dmitry Safonov

Quoted 12 lines · Lu Baolu

Hi,

Quoted 6 lines · Dmitry Safonov

AFAICS, we're doing fault-clearing in a loop inside irq handler. That means that while we're clearing if a fault raises, it'll make an irq level triggered (or on edge) on lapic. So, whenever we return
from the irq handler, irq will raise again.

Uhm, double checked with the spec. Interrupts should be generated since we always clear the fault overflow bit.

Quoted 6 lines

Quoted 2 lines

Anyway, we can't clear faults in a limited loop, as the spec says in 7.3.1:

Mind to elaborate?
ITOW, I do not see a contradiction. We're still clearing faults in FIFO fashion. There is no limitation to do some spare work in between clearings (return from interrupt, then fault again and continue).

Hardware maintains an internal index to reference the fault recording register in which the next fault can be recorded. When a fault comes, hardware will check the Fault bit (bit 31 of the 4th 32-bit register recording register) referenced by the internal index. If this bit is set, hardware will not record the fault.

Since we now don't clear the F bit until a register entry which has the F bit cleared, we might exit the fault handling with some register entries still have the F bit set.

F

| 0 |  xxxxxxxxxxxxx|
| 0 |  xxxxxxxxxxxxx|
| 0 |  xxxxxxxxxxxxx|  <--- Fault record index in fault status register

| 0 |  xxxxxxxxxxxxx|
| 1 |  xxxxxxxxxxxxx|  <--- hardware maintained index
| 1 |  xxxxxxxxxxxxx|
| 1 |  xxxxxxxxxxxxx|
| 0 |  xxxxxxxxxxxxx|
| 0 |  xxxxxxxxxxxxx|
| 0 |  xxxxxxxxxxxxx|
| 0 |  xxxxxxxxxxxxx|

Take an example as above, hardware could only record 2 more faults with others all dropped.

Best regards,
Lu Baolu

Hi,

Quoted 20 lines · Dmitry Safonov

Quoted 12 lines · Lu Baolu

Hi,

Quoted 8 lines · Dmitry Safonov

AFAICS, we're doing fault-clearing in a loop inside irq handler.
That means that while we're clearing if a fault raises, it'll make
an irq level triggered (or on edge) on lapic. So, whenever we return
from the irq handler, irq will raise again.

Quoted 6 lines

Uhm, double checked with the spec. Interrupts should be generated since we always clear the fault overflow bit.

Anyway, we can't clear faults in a limited loop, as the spec says in
7.3.1:

Quoted 5 lines

Mind to elaborate?
ITOW, I do not see a contradiction. We're still clearing faults in FIFO
fashion. There is no limitation to do some spare work in between clearings (return from interrupt, then fault again and continue).

Hardware maintains an internal index to reference the fault recording register in which the next fault can be recorded. When a fault comes, hardware will check the Fault bit (bit 31 of the 4th 32-bit register recording
register) referenced by the internal index. If this bit is set, hardware will
not record the fault.

Since we now don't clear the F bit until a register entry which has the F bit
cleared, we might exit the fault handling with some register entries still
have the F bit set.

F

Quoted 4 lines

0 |  xxxxxxxxxxxxx|
0 |  xxxxxxxxxxxxx|
0 |  xxxxxxxxxxxxx|  <--- Fault record index in fault status

register

Quoted 8 lines

0 |  xxxxxxxxxxxxx|
1 |  xxxxxxxxxxxxx|  <--- hardware maintained index
1 |  xxxxxxxxxxxxx|
1 |  xxxxxxxxxxxxx|
0 |  xxxxxxxxxxxxx|
0 |  xxxxxxxxxxxxx|
0 |  xxxxxxxxxxxxx|
0 |  xxxxxxxxxxxxx|

Take an example as above, hardware could only record 2 more faults with
others all dropped.

Quoted 53 lines · Lu Baolu

Hi,

Quoted 15 lines · Dmitry Safonov

Quoted 14 lines · Lu Baolu

Hi,

Quoted 8 lines · Dmitry Safonov

AFAICS, we're doing fault-clearing in a loop inside irq handler.
That means that while we're clearing if a fault raises, it'll make
an irq level triggered (or on edge) on lapic. So, whenever we return
from the irq handler, irq will raise again.

Uhm, double checked with the spec. Interrupts should be generated since we always clear the fault overflow bit.

Quoted 8 lines

Quoted 3 lines

Anyway, we can't clear faults in a limited loop, as the spec says in
7.3.1:

Mind to elaborate?
ITOW, I do not see a contradiction. We're still clearing faults in FIFO
fashion. There is no limitation to do some spare work in between clearings (return from interrupt, then fault again and continue).

Hardware maintains an internal index to reference the fault recording register in which the next fault can be recorded. When a fault comes, hardware will check the Fault bit (bit 31 of the 4th 32-bit register recording
register) referenced by the internal index. If this bit is set, hardware will
not record the fault.

Since we now don't clear the F bit until a register entry which has the F bit
cleared, we might exit the fault handling with some register entries still
have the F bit set.

F

Quoted 4 lines

0 |  xxxxxxxxxxxxx|
0 |  xxxxxxxxxxxxx|
0 |  xxxxxxxxxxxxx|  <--- Fault record index in fault status

register

Quoted 8 lines

0 |  xxxxxxxxxxxxx|
1 |  xxxxxxxxxxxxx|  <--- hardware maintained index
1 |  xxxxxxxxxxxxx|
1 |  xxxxxxxxxxxxx|
0 |  xxxxxxxxxxxxx|
0 |  xxxxxxxxxxxxx|
0 |  xxxxxxxxxxxxx|
0 |  xxxxxxxxxxxxx|

Quoted 3 lines

Take an example as above, hardware could only record 2 more faults with
others all dropped.

Ugh, yeah, I got what you're saying.. Thanks for explanations. So, we shouldn't mark faults as cleared until we've actually processed them here:
: writel(DMA_FSTS_PFO | DMA_FSTS_PPF | DMA_FSTS_PRO,
: iommu->reg + DMAR_FSTS_REG);

As Joerg mentioned, we do care about latency here, so this fault work can't be moved entirely into workqueue.. but we might limit loop and check if we've hit the limit - to proceed servicing faults in a wq, as in that case we should care about being too long in irq-disabled section more than about latencies.
Does that makes any sense, what do you think?

I can possibly re-write 2/2 with idea above..

There is a ratelimit for printing, but it's incremented each time the cpu recives dmar fault interrupt. While one interrupt may signal about
*many* faults.
So, measuring the impact it turns out that reading/clearing one fault takes < 1 usec, and printing info about the fault takes ~170 msec.

Having in mind that maximum number of fault recording registers per remapping hardware unit is 256.. IRQ handler may run for (170*256) msec.
And as fault-serving loop runs without a time limit, during servicing new faults may occur..

Ratelimit each fault printing rather than each irq printing.

Fixes commit c43fce4eebae ("iommu/vt-d: Ratelimit fault handler")

BUG: spinlock lockup suspected on CPU#0, CliShell/9903
 lock: 0xffffffff81a47440, .magic: dead4ead, .owner:
kworker/u16:2/8915, .owner_cpu: 6
CPU: 0 PID: 9903 Comm: CliShell
Call Trace:$\n'
[..] dump_stack+0x65/0x83$\n'
[..] spin_dump+0x8f/0x94$\n'
[..] do_raw_spin_lock+0x123/0x170$\n'
[..] _raw_spin_lock_irqsave+0x32/0x3a$\n'
[..] uart_chars_in_buffer+0x20/0x4d$\n'
[..] tty_chars_in_buffer+0x18/0x1d$\n'
[..] n_tty_poll+0x1cb/0x1f2$\n'
[..] tty_poll+0x5e/0x76$\n'
[..] do_select+0x363/0x629$\n'
[..] compat_core_sys_select+0x19e/0x239$\n'
[..] compat_SyS_select+0x98/0xc0$\n'
[..] sysenter_dispatch+0x7/0x25$\n'
[..]
NMI backtrace for cpu 6
CPU: 6 PID: 8915 Comm: kworker/u16:2
Workqueue: dmar_fault dmar_fault_work
Call Trace:$\n'
[..] wait_for_xmitr+0x26/0x8f$\n'
[..] serial8250_console_putchar+0x1c/0x2c$\n'
[..] uart_console_write+0x40/0x4b$\n'
[..] serial8250_console_write+0xe6/0x13f$\n'
[..] call_console_drivers.constprop.13+0xce/0x103$\n'
[..] console_unlock+0x1f8/0x39b$\n'
[..] vprintk_emit+0x39e/0x3e6$\n'
[..] printk+0x4d/0x4f$\n'
[..] dmar_fault+0x1a8/0x1fc$\n'
[..] dmar_fault_work+0x15/0x17$\n'
[..] process_one_work+0x1e8/0x3a9$\n'
[..] worker_thread+0x25d/0x345$\n'
[..] kthread+0xea/0xf2$\n'
[..] ret_from_fork+0x58/0x90$\n'

Cc Alex Williamson <[email protected]>

Cc David Woodhouse <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc Lu Baolu <baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]>

Cc iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org

Signed-off-by Dmitry Safonov <[email protected]>

---
 drivers/iommu/dmar.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

drivers/iommu/dmar.c +1 -5

index accf58388bdb..6c4ea32ee6a9 100644
@@ -1618,17 +1618,13 @@ 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 && !ratelimited)
+	if (fault_status && __ratelimit(&rs))
 		pr_err("DRHD: handling fault status reg %x\n",
 fault_status);

drivers/iommu/dmar.c +1 -0

 	/* TBD: ignore advanced fault log currently */

@@ -1638,6 +1634,8 @@ irqreturn_t dmar_fault(int irq, void *dev_id)
 	fault_index = dma_fsts_fault_record_index(fault_status);
 	reg = cap_fault_reg_offset(iommu->cap);
 	while (1) {
+		/* Disable printing, simply clear the fault when

ratelimited */

drivers/iommu/dmar.c +1 -0

+		bool ratelimited = !__ratelimit(&rs);
 		u8 fault_reason;
 		u16 source_id;
 		u64 guest_addr;

Hi Joerg,

is there anything I may do about those two patches? In 2/2 I've limited loop cnt as discussed in v3. This one solves softlockup for us, might be useful.

Quoted 5 lines · Dmitry Safonov

Hi Joerg,

is there anything I may do about those two patches? In 2/2 I've limited loop cnt as discussed in v3. This one solves softlockup for us, might be useful.

Applied the first patch, thanks. Please re-work the second one according
to the comments.