Perhaps a side effect regarding NMI returns

I was looking at the return sequence of NMIs in x86_64 and I came across this in entry_64.S:

	jz paranoid_swapgs
	movq %rsp,%rdi			/* &pt_regs */
	call sync_regs
	movq %rax,%rsp			/* switch stack for scheduling */
	testl $_TIF_NEED_RESCHED,%ebx
	jnz paranoid_schedule
	movl %ebx,%edx			/* arg3: thread flags */
	TRACE_IRQS_ON
	ENABLE_INTERRUPTS(CLBR_NONE)
	xorl %esi,%esi 			/* arg2: oldset */
	movq %rsp,%rdi 			/* arg1: &pt_regs */
	call do_notify_resume
	DISABLE_INTERRUPTS(CLBR_NONE)
	TRACE_IRQS_OFF
	jmp paranoid_userspace

paranoid_schedule:
TRACE_IRQS_ON
ENABLE_INTERRUPTS(CLBR_ANY)
call schedule
DISABLE_INTERRUPTS(CLBR_ANY)
TRACE_IRQS_OFF
jmp paranoid_userspace
CFI_ENDPROC

Specifically the code after jnz paranoid_schedule.

Just before that jnz, we swap the stack back to the task's stack (no more NMI stack). If NEED_RESCHED is set, we jump to paranoid_schedule and enable interrupts and call schedule.

Is there a bit of a side effect here? What happens when you enable interrupts in NMI context? Can more NMIs come in? If not, we just went into schedule and went off and running, and NMIs will have to wait till the next interrupt comes in and calls iretq to re-enable NMIs. If we lock up here, don't expect NMI watchdog to help you out.

If enabling interrupts also enables NMIs, then there's no side effect.

This email is more of an FYI than anything else. Maybe there's an issue here, and maybe there isn't. But this is so subtle that I figured I would bring it to other people's attention. I'll let others do the hard work to figure out if we should worry about this or not ;-)

-- Steve

The only thing that enables NMI's again is 'iret', afaik. Of course, it can be *any* iret, so once you enable interrupts, you can get a timer interrupt, and the timer interrupt returning with iret will re-enable NMI's then too.

I would suggest that the actual NMI handler itself should probably never use that paranoid exit at all, and just always use a regular iret. Screw scheduling and TIF checks.

Probably only the exceptions that can happen *during* NMI (eg debug, stack exception, double-fault etc) should use the paranoid versions that try to avoid using iret.

Because I think you're right - we shouldn't call schedule() from within the NMI handler, even if we do enable interrupts and switch to the normal stack. Even if it probably does happen to work normally.

But I have to admit to not necessarily thinking it through a lot.

Linus

I double checked versus the manuals tonight after seeing this and I believe this correct -- an iret is required; some of the verbage is reasonably explicit to this fact.

At a high level I think we could apply the same logic from the first paragraph; namely that we will see a timer tick within a jiffy (where we will have an opportunity to process TIF flags) and it's better to push schedule() out than the NMI handler.

That said, if TIF_RESCHED has been set by a higher priority RT task this temporary inversion may not be appreciated. It's probably worth fixing properly.

- Paul

Linus Torvalds <[email protected]> writes:

Not sure what problem Steven thinks is there. Once you switched the stack any nesting is fine.

The reason I added them originally is to prevent the race of remote kernel events being delayed for a long time. With Frederic's nohz work this will be more important in the future. Today it would be eventually picked up by the regular timer interrupts.

-Andi

[email protected] -- Speaking for myself only

The problem is that NMI's are blocked!

We're potentially switching to another process, there is no guarantee there will be an "iret" *anywhere* in any patch for a long time. So any subsequent NMI's will not be able to come in.

You're just delaying another kind of event: the next NMI.

And NMI's really shouldn't be triggering any scheduling-related events afaik.

Linus

True. That's a bug that needs to be fixed.

As a simple fix your proposal of forcing IRET sounds good.

-Andi

We could of course use iret to return to the regular kernel stack, and do the schedule from there.

So instead of doing the manual stack switch, just build a fake iret stack on our exception stack. Subtle and somewhat complicated. I'd almost rather just do a blind iret, and leave the 'iret to regular stack' as a possible future option.

Linus

Note, the reason that I've been looking at this code, is because I'm looking at implementing your idea to handle irets in NMIs, caused by faults, exceptions, and the reason I really care about: debugging.

Your proposal is here:

https://lkml.org/lkml/2010/7/14/264

But to make this work, it would be really nice if the NMI routine wasn't convoluted with the paranoid_exit code.

For things like static_branch()/jump_label and modifying ftrace nops to calls and back, we currently use the big hammer approach stop_machine(). This keeps another CPU from executing code that is being modified. There's also tricks to handle NMIs that may be running on the stopped CPUs.

But people don't like the overhead that stop_machine() causes, and I have code that can make the modifications for ftrace with break points. By adding a break point, syncing, then modifying the code and break point to a new op will greatly reduce the overhead. At least the latency will be much less.

The problem is that ftrace affects code in NMIs. We tried to not trace NMIs, but there's so many functions that NMIs call, it ended up being a losing battle. But if we can fix the NMI enabled on iret, we can then use the break point scheme for both static_branch() and ftrace, and remove the overhead of stop_machine. I think there's a possibility to use kprobes in NMIs too, with this fix.

-- Steve

Ahh, good that you pointed to it, I'd completely forgotten about this one.

Yeah. Simplifying NMI and not mixing up the paranoid stuff sounds like a good idea, and then if we do the nice NMI counting thing and avoid the whole problem with NMI and iret, that would be a nice cleanup in itself.

So if that patch I posted works for you (with some NMI-heavy workload like non-PEBS tracing) I think it's the way to go.

Too late for 3.2 obviously, since I don't think anybody has actually reported the "delayed NMI's" as a real problem - so even if it's a bug, it's not a bug we should try to fix at this stage.

Linus

Oh no, I had no plans to get this into 3.2. I'm optimistic just to get this into 3.3.

Thanks!

-- Steve

But if there's still has to be some sort of 'syncing' after we add a break point, how much are we going to save? Or I guess your're using an IPI?

Thanks,

-Jason

Well, anything is better than stop machine, event synchronize_sched() ;)

But the code I have in ftrace does bulk changes. It adds a break point to all functions, then it does the sync, then it updates all the points to the new code.

Looking at my code, here's what I did after setting up the breakpoints:

static void do_sync_core(void *data)

{
        sync_core();
}

static void run_sync(void)

{
        int enable_irqs = irqs_disabled();

        /* We may be called with interrupts disbled. */
        if (enable_irqs)
                local_irq_enable();
        on_each_cpu(do_sync_core, NULL, 1);
        if (enable_irqs)
                local_irq_disable();
}

Note, it's fine to enable interrupts here, it's only used by ftrace.

-- Steve

Still ugly as hell though, why not push that pain to the caller?

Problem? I never said there was a problem. I just said there was a subtle side effect, that may or may not be a problem. But I never said there was an actually problem.

There may be a problem where you are profiling at a high frequency, and then hit this and suddenly, your high frequency NMIs are stopped till the next interrupt. If you are running at 100HZ, the next interrupt may not happen for 10ms, where we don't have an NMI triggered until then.

Perhaps for some reason NEED_RESCHED is set, but the task is no longer going to run. Not sure if that could even happen, but for kicks, lets assume it can. Then we schedule into the idle task that goes to sleep. With NO_HZ, we may not even get another interrupt for a very long time. But we keep NMIs disabled.

The difference here is that we call schedule without ever enabling NMIs. Now is that a problem? It may be.

I'm curious to what remote events can be set by an NMI that wont take affect in what the NMI interrupted. I would think that NMIs should be treated as if they didn't exist, because they should not be calling anything that sets NEED_RESCHED or grabbing locks and such.

If a process on another CPU sets NEED_RESCHED on the task for this CPU, it doesn't matter if it is in an NMI or not. The other process still needs to send an IPI or something (that will be blocked until the NMI finishes) in which case, the return from the NMI will trigger the scheduling.

I don't see the problem where we can't just ignore the flags for NMIs. What exactly is the race that causes issues when NMIs are not checking for scheduling and such?

-- Steve

It could be much worse than that with no-HZ and some CPU-intensive load with no timers. I don't think "sysret" enables NMI's either, so return-to-user-mode may well leave NMI blocked.

So I really think this might be a real issue. And the simplest approach seems to be to just remove the code. Something like the attached (TOTALLY UNTESTED!) patch.

Hmm?

Linus

 arch/x86/kernel/entry_64.S |   32 --------------------------------
 1 files changed, 0 insertions(+), 32 deletions(-)

index faf8d5e74b0b..3819ea907339 100644
@@ -1489,46 +1489,14 @@ ENTRY(nmi)
 	movq %rsp,%rdi
 	movq $-1,%rsi
 	call do_nmi
-#ifdef CONFIG_TRACE_IRQFLAGS
-	/* paranoidexit; without TRACE_IRQS_OFF */
-	/* ebx:	no swapgs flag */
-	DISABLE_INTERRUPTS(CLBR_NONE)
 	testl %ebx,%ebx				/* swapgs needed? */
 	jnz nmi_restore
-	testl $3,CS(%rsp)
-	jnz nmi_userspace
 nmi_swapgs:
 	SWAPGS_UNSAFE_STACK
 nmi_restore:
 	RESTORE_ALL 8
 	jmp irq_return
-nmi_userspace:
-	GET_THREAD_INFO(%rcx)
-	movl TI_flags(%rcx),%ebx
-	andl $_TIF_WORK_MASK,%ebx
-	jz nmi_swapgs
-	movq %rsp,%rdi			/* &pt_regs */
-	call sync_regs
-	movq %rax,%rsp			/* switch stack for scheduling */
-	testl $_TIF_NEED_RESCHED,%ebx
-	jnz nmi_schedule
-	movl %ebx,%edx			/* arg3: thread flags */
-	ENABLE_INTERRUPTS(CLBR_NONE)
-	xorl %esi,%esi 			/* arg2: oldset */
-	movq %rsp,%rdi 			/* arg1: &pt_regs */
-	call do_notify_resume
-	DISABLE_INTERRUPTS(CLBR_NONE)
-	jmp nmi_userspace
-nmi_schedule:
-	ENABLE_INTERRUPTS(CLBR_ANY)
-	call schedule
-	DISABLE_INTERRUPTS(CLBR_ANY)
-	jmp nmi_userspace
 	CFI_ENDPROC
-#else
-	jmp paranoid_exit
-	CFI_ENDPROC
-#endif
 END(nmi)
 
 ENTRY(ignore_sysret)

Linus,

I've tested this patch quite a bit with 'perf top -C 1000' and it holds up well. Could you give me your Signed-off-by so I can include it into my patch queue?

Thanks,

-- Steve

 arch/x86/kernel/entry_64.S |   32 --------------------------------
 1 files changed, 0 insertions(+), 32 deletions(-)

index faf8d5e74b0b..3819ea907339 100644
@@ -1489,46 +1489,14 @@ ENTRY(nmi)
 	movq %rsp,%rdi
 	movq $-1,%rsi
 	call do_nmi
-#ifdef CONFIG_TRACE_IRQFLAGS
-	/* paranoidexit; without TRACE_IRQS_OFF */
-	/* ebx:	no swapgs flag */
-	DISABLE_INTERRUPTS(CLBR_NONE)
 	testl %ebx,%ebx				/* swapgs needed? */
 	jnz nmi_restore
-	testl $3,CS(%rsp)
-	jnz nmi_userspace
 nmi_swapgs:
 	SWAPGS_UNSAFE_STACK
 nmi_restore:
 	RESTORE_ALL 8
 	jmp irq_return
-nmi_userspace:
-	GET_THREAD_INFO(%rcx)
-	movl TI_flags(%rcx),%ebx
-	andl $_TIF_WORK_MASK,%ebx
-	jz nmi_swapgs
-	movq %rsp,%rdi			/* &pt_regs */
-	call sync_regs
-	movq %rax,%rsp			/* switch stack for scheduling */
-	testl $_TIF_NEED_RESCHED,%ebx
-	jnz nmi_schedule
-	movl %ebx,%edx			/* arg3: thread flags */
-	ENABLE_INTERRUPTS(CLBR_NONE)
-	xorl %esi,%esi 			/* arg2: oldset */
-	movq %rsp,%rdi 			/* arg1: &pt_regs */
-	call do_notify_resume
-	DISABLE_INTERRUPTS(CLBR_NONE)
-	jmp nmi_userspace
-nmi_schedule:
-	ENABLE_INTERRUPTS(CLBR_ANY)
-	call schedule
-	DISABLE_INTERRUPTS(CLBR_ANY)
-	jmp nmi_userspace
 	CFI_ENDPROC
-#else
-	jmp paranoid_exit
-	CFI_ENDPROC
-#endif
 END(nmi)
 
 ENTRY(ignore_sysret)

Sure. If it's tested, I have no problem with signing off on it, so just add it. Do you have some kind of commit message that makes sense and explains the NMI shadow issue?

Linus

How about something like this:

The NMI handler uses the paranoid_exit routine that checks the NEED_RESCHED flag, and if it is set and the return is for userspace, then interrupts are enabled, the stack is swapped to the thread's stack, and schedule is called. The problem with this is that we are still in an NMI context until an iret is executed. This means that any new NMIs are now starved until an interrupt or exception occurs and does the iret.

As NMIs can not be masked and can interrupt any location, they are treated as a special case. NEED_RESCHED should not be set in an NMI handler. The interruption by the NMI should not disturb the work flow for scheduling. Any IPI sent to a processor after sending the NEED_RESCHED would have to wait for the NMI anyway, and after the IPI finishes the schedule would be called as required.

There is no reason to do anything special leaving an NMI. Remove the call to paranoid_exit and do a simple return. This not only fixes the bug of starved NMIs, but it also cleans up the code.

-- Steve

Ack, looks good to me,

Linus

-Andi

Commit-ID:  549c89b98c4530b278dde1a3f68ce5ebbb1e6304
Gitweb:     http://git.kernel.org/tip/549c89b98c4530b278dde1a3f68ce5ebbb1e6304
Author:     Linus Torvalds <[email protected]>
AuthorDate: Tue, 29 Nov 2011 12:44:55 -0800
Committer:  Steven Rostedt <[email protected]>
CommitDate: Wed, 21 Dec 2011 15:38:52 -0500

x86: Do not schedule while still in NMI context

The NMI handler uses the paranoid_exit routine that checks the NEED_RESCHED flag, and if it is set and the return is for userspace, then interrupts are enabled, the stack is swapped to the thread's stack, and schedule is called. The problem with this is that we are still in an NMI context until an iret is executed. This means that any new NMIs are now starved until an interrupt or exception occurs and does the iret.

As NMIs can not be masked and can interrupt any location, they are treated as a special case. NEED_RESCHED should not be set in an NMI handler. The interruption by the NMI should not disturb the work flow for scheduling. Any IPI sent to a processor after sending the NEED_RESCHED would have to wait for the NMI anyway, and after the IPI finishes the schedule would be called as required.

There is no reason to do anything special leaving an NMI. Remove the call to paranoid_exit and do a simple return. This not only fixes the bug of starved NMIs, but it also cleans up the code.

---
 arch/x86/kernel/entry_64.S |   32 --------------------------------
 1 files changed, 0 insertions(+), 32 deletions(-)

index faf8d5e..3819ea9 100644
@@ -1489,46 +1489,14 @@ ENTRY(nmi)
 	movq %rsp,%rdi
 	movq $-1,%rsi
 	call do_nmi
-#ifdef CONFIG_TRACE_IRQFLAGS
-	/* paranoidexit; without TRACE_IRQS_OFF */
-	/* ebx:	no swapgs flag */
-	DISABLE_INTERRUPTS(CLBR_NONE)
 	testl %ebx,%ebx				/* swapgs needed? */
 	jnz nmi_restore
-	testl $3,CS(%rsp)
-	jnz nmi_userspace
 nmi_swapgs:
 	SWAPGS_UNSAFE_STACK
 nmi_restore:
 	RESTORE_ALL 8
 	jmp irq_return
-nmi_userspace:
-	GET_THREAD_INFO(%rcx)
-	movl TI_flags(%rcx),%ebx
-	andl $_TIF_WORK_MASK,%ebx
-	jz nmi_swapgs
-	movq %rsp,%rdi			/* &pt_regs */
-	call sync_regs
-	movq %rax,%rsp			/* switch stack for scheduling */
-	testl $_TIF_NEED_RESCHED,%ebx
-	jnz nmi_schedule
-	movl %ebx,%edx			/* arg3: thread flags */
-	ENABLE_INTERRUPTS(CLBR_NONE)
-	xorl %esi,%esi 			/* arg2: oldset */
-	movq %rsp,%rdi 			/* arg1: &pt_regs */
-	call do_notify_resume
-	DISABLE_INTERRUPTS(CLBR_NONE)
-	jmp nmi_userspace
-nmi_schedule:
-	ENABLE_INTERRUPTS(CLBR_ANY)
-	call schedule
-	DISABLE_INTERRUPTS(CLBR_ANY)
-	jmp nmi_userspace
 	CFI_ENDPROC
-#else
-	jmp paranoid_exit
-	CFI_ENDPROC
-#endif
 END(nmi)
 
 ENTRY(ignore_sysret)

Hmm, i thought there were cases where we checked if it was in kernel mode instead of IPIng, and assume the check is done when returning.

But cannot come up with a concrete example right now. It may have been wrong. Or I forgot it :)

You're right anything with interrupts should be fine because it's just blocked.

-Andi