Changeset 145 for trunk/src/os2ahci/ahci.c

Timestamp:

Jan 5, 2013, 4:19:48 PM (13 years ago)

Author:

rousseau

Message:

Possibly fixed VirtualBox trap #8/reset problem (TRAC ticket #15)

Depending on kernel (W4/SMP) and ACPI (enabled or not), massive copying of
files produced a trap #8 or reset when using OS2AHCI in VirtualBox.

Causes & Fixes

o Interrupts were not disabled before doing DevHelp_EOI().

Since the AHCI controller in VirtualBox is a software implementation,
it cannot process requests as fast as a true hardware controller.
This caused stacked interrupts to a level that exhausted the interrupt
stack, resulting in a trap #8 on W4 or a reset on SMP with ACPI.
Interrupts are now disabled before doing the EOI.
The "Physycal Device Driver Reference" mentions this in:
?:\IBMDDK\DOCS\PDDREF.INF->Device Helper (DevHlp) Services)->EOI
and cross-referencing with DANIS506 shows she does the same in:
s506m.c (state machine) around line 244.

To investigate

o SMP safety

The disable() function in lib.c (around line 793) mentions that SMP systems
should use spinlocks. Possibly because a CLI is only executed on the
current CPU and a new interrupt could come from another CPU where
interrupts are still enabled. However, doing the EOI before unlocking did
not solve the VBox issue, possibly because spin_unlock() enables
interrupts again. So, at least in the case of VBox, the interrupt handler
has to return with interrupts disabled.
This will only be an issue in VBox if it realy can receive interrupts from
multiple CPU's in it's software implementation.

o Real hardware

As far as I can tell this patch has no influence on performance when
using real hardware. Also, since real hardware can handle requests in a
much shorter timespan, the likeliness of stacked interrupts occuring is
much less.

Performance measurements in VirtualBox

VirtualBox is a virtual machine and therefore subjected to any system load
on the host OS. Even comparing DANIS506 with OS2AHCI makes no real sense
since with DANI the SATA-contoller will run in ATA-compatibility and with
OS2AHCI it will run in AHCI-mode. Also, I have experienced that when my CPU
get's hot, the duty-cycle throttling goes active, reducing performance and
thus impacting any benchmarks in VBox. (or real hardware)
The only way to compare DANIS506 with OS2AHCI is on real hardware, on the
same controller, with the same disks and doing the same tests.
And most importantly, with the same CPU core-temperature.

Note

Diff's may show changed lines that are actually the same.
That's because my editor is configured to strip trailing white space.

File:

• trunk/src/os2ahci/ahci.c (modified) (1 diff)

trunk/src/os2ahci/ahci.c

@@ -1305 +1305 @@
      * writes), we may be stacking interrupts on top of each other. If we
      * detect this, we'll pass this on to the engine context hook.
+     *
+     * Rousseau:
+     * The "Physycal Device Driver Reference" states that it's a good idea
+     * to disable interrupts before doing EOI so that it can proceed for this
+     * level without being interrupted, which could cause stacked interrupts,
+     * possibly exhausting the interrupt stack.
+     * (?:\IBMDDK\DOCS\PDDREF.INF->Device Helper (DevHlp) Services)->EOI)
+     *
+     * This is what seemed to happen when running in VirtualBox.
+     * Since in VBox the AHCI-controller is a software implementation, it is
+     * just not fast enough to handle a large bulk of requests, like when JFS
+     * flushes it's caches.
+     *
+     * Cross referencing with DANIS506 shows she does the same in the
+     * state-machine code in s506sm.c around line 244; disable interrupts
+     * before doing the EOI.
+     *
+     * Comments on the disable() function state that SMP systems should use
+     * a spinlock, but putting the EOI before spin_unlock() did not solve the
+     * VBox ussue. This is probably because spin_unlock() enables interrupts,
+     * which implies we need to return from this handler with interrupts
+     * disabled.
      */
     if ((u16) (u32) (void _far *) &irq_stat < 0xf000) {
       ddprintf("IRQ stack running low; arming engine context hook\n");
+      /* Rousseau:
+       * A context hook cannot be re-armed before it has completed.
+       * (?:\IBMDDK\DOCS\PDDREF.INF->Device Helper (DevHlp) Services)->ArmCtxHook)
+       * Also, it is executed at task-time, thus in the context of some
+       * application thread. Stacked interrupts with a stack below the
+       * threshold specified above, (0xf000), will repeatly try to arm the
+       * context hook, but since we are in an interrupted interrupt handler,
+       * it's highly unlikely the hook has completed.
+       * So, possibly only the first arming is succesful and subsequent armings
+       * will fail because no task-time thread has run between the stacked
+       * interrupts. One hint would be that if the dispatching truely worked,
+       * excessive stacked interrupts in VBox would not be a problem.
+       * This needs some more investigation.
+       */
       DevHelp_ArmCtxHook(0, engine_ctxhook_h);
-
+//      DevHelp_EOI(irq);
     } else {
       spin_lock(drv_lock);
       trigger_engine();
+//      DevHelp_EOI(irq);
       spin_unlock(drv_lock);
     }
-
+    /* disable interrupts to prevent stacking. (See comments above) */
+    disable();
     /* complete the interrupt */
     DevHelp_EOI(irq);