On 26.08.20 16:27, Thomas Gleixner wrote:
On Wed, Aug 26 2020 at 13:53, Alexander Graf wrote:
Commit 633260fa143 ("x86/irq: Convey vector as argument and not in ptregs") changed the handover logic of the vector identifier from ~vector in orig_ax to purely register based. Unfortunately, this field has another consumer in the APIC code which the commit did not touch. The net result was that IRQ balancing did not work and instead resulted in interrupt storms, slowing down the system.
The net result is an observationof the symptom but that does not explain what the underlying technical issue is.
This patch restores the original semantics that orig_ax contains the vector. When we receive an interrupt now, the actual vector number stays stored in the orig_ax field which then gets consumed by the APIC code.
To ensure that nobody else trips over this in the future, the patch also adds comments at strategic places to warn anyone who would refactor the code that there is another consumer of the field.
With this patch in place, IRQ balancing works as expected and performance levels are restored to previous levels.
There's a lot of 'This patch and we' in that changelog. Care to grep for 'This patch' in Documentation/process/ ?
diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S index df8c017..22e829c 100644 --- a/arch/x86/entry/entry_32.S +++ b/arch/x86/entry/entry_32.S @@ -727,7 +727,7 @@ SYM_CODE_START_LOCAL(asm_\cfunc) ENCODE_FRAME_POINTER movl %esp, %eax movl PT_ORIG_EAX(%esp), %edx /* get the vector from stack */
movl $-1, PT_ORIG_EAX(%esp) /* no syscall to restart */
/* keep vector on stack for APIC's irq_complete_move() */Yes that's fixing your observed wreckage, but it introduces a worse one.
user space -> interrupt push vector into orig_ax (values are in the ranges of 0-127 and -128 - 255 except for the system vectors which do not go through this code) handle() ... exit_to_user_mode_loop() arch_do_signal() /* Did we come from a system call? */ if (syscall_get_nr(current, regs) >= 0) {
---> BOOM for any vector 0-127 because syscall_get_nr() resolves to regs->orig_axGoing to be fun to debug.
Hah, that's the code flow I was looking for to understand why the value was negative in the first place. Thanks a lot for pointing it out!
The below nasty hack cures it, but I hate it with a passion. I'll look deeper for a sane variant.
An alternative (that doesn't make the code easier to read, but would fix the issue at hand) would be touse a pushq imm16 with vector | 0x8000 instead to always make the value negative, no?
Alex
Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879