Re: [PATCH v4 01/39] task_work: Fix TWA_NMI_CURRENT error handling
/me is resurrecting the sframe work On Wed, 22 Jan 2025 13:28:21 +0100 Peter Zijlstra wrote: > > The error has to be checked before the write to task->task_works. Also > > the try_cmpxchg() loop isn't needed in NMI context. The TWA_NMI_CURRENT > > case really is special, keep things simple by keeping its code all > > together in one place. > > NMIs can nest, consider #DB (which is NMI like) doing task_work_add() > and getting interrupted with NMI doing the same. > I was looking at this patch and was thinking that the trycmpxchg() is still for the race against other CPUs. Another CPU can add a task_work for this task, right? If so, then even though the NMI adding the task work can't be interrupted (considering there's no #DB and such), it still can clobber an update done for this tasks task_work from another CPU. Is this patch still even needed? -- Steve
Re: [PATCH v4 01/39] task_work: Fix TWA_NMI_CURRENT error handling
On Thu, Jan 23, 2025 at 09:15:09AM -0800, Josh Poimboeuf wrote: > On Thu, Jan 23, 2025 at 09:14:03AM +0100, Peter Zijlstra wrote: > > On Wed, Jan 22, 2025 at 12:47:20PM -0800, Josh Poimboeuf wrote: > > > What exactly do you mean by "NMI like"? Is it because a #DB might be > > > basically running in NMI context, if the NMI hit a breakpoint? > > > > No, #DB, #BP and such are considered NMI (and will have in_nmi() true) > > because they can trigger anywhere, including sections where IRQs are > > disabled. > > So: > > - while exceptions are technically not NMI, they're "NMI" because they > can occur in NMI or IRQ-disabled regions > > - such "NMI" exceptions can be preempted by NMIs and "NMIs" > > - NMIs can be preempted by "NMIs" but not NMIs (except in entry code!) > > ... did I get all that right? Not subtle at all! Yeah, sounds about right :-)
Re: [PATCH v4 01/39] task_work: Fix TWA_NMI_CURRENT error handling
On Thu, Jan 23, 2025 at 09:14:03AM +0100, Peter Zijlstra wrote: > On Wed, Jan 22, 2025 at 12:47:20PM -0800, Josh Poimboeuf wrote: > > What exactly do you mean by "NMI like"? Is it because a #DB might be > > basically running in NMI context, if the NMI hit a breakpoint? > > No, #DB, #BP and such are considered NMI (and will have in_nmi() true) > because they can trigger anywhere, including sections where IRQs are > disabled. So: - while exceptions are technically not NMI, they're "NMI" because they can occur in NMI or IRQ-disabled regions - such "NMI" exceptions can be preempted by NMIs and "NMIs" - NMIs can be preempted by "NMIs" but not NMIs (except in entry code!) ... did I get all that right? Not subtle at all! I feel like in_nmi() needs a comment explaining all that nonobviousness. -- Josh
Re: [PATCH v4 01/39] task_work: Fix TWA_NMI_CURRENT error handling
On Wed, Jan 22, 2025 at 12:47:20PM -0800, Josh Poimboeuf wrote: > On Wed, Jan 22, 2025 at 01:28:21PM +0100, Peter Zijlstra wrote: > > On Tue, Jan 21, 2025 at 06:30:53PM -0800, Josh Poimboeuf wrote: > > > It's possible for irq_work_queue() to fail if the work has already been > > > claimed. That can happen if a TWA_NMI_CURRENT task work is requested > > > before a previous TWA_NMI_CURRENT IRQ work on the same CPU has gotten a > > > chance to run. > > > > I'm confused, if it fails then it's already pending, and we'll get the > > notification already. You can still add the work. > > Yeah, I suppose that makes sense. If the pending irq_work is already > going to set TIF_NOTIFY_RESUME anyway, there's no need to do that again. > > > > The error has to be checked before the write to task->task_works. Also > > > the try_cmpxchg() loop isn't needed in NMI context. The TWA_NMI_CURRENT > > > case really is special, keep things simple by keeping its code all > > > together in one place. > > > > NMIs can nest, > > Just for my understanding: for nested NMIs, the entry code basically > queues up the next NMI, so the C handler (exc_nmi) can't nest. Right? > > > consider #DB (which is NMI like) > > What exactly do you mean by "NMI like"? Is it because a #DB might be > basically running in NMI context, if the NMI hit a breakpoint? No, #DB, #BP and such are considered NMI (and will have in_nmi() true) because they can trigger anywhere, including sections where IRQs are disabled. > > doing task_work_add() and getting interrupted with NMI doing the same. > > How exactly would that work? At least with my patch the #DB wouldn't be > able to use TWA_NMI_CURRENT unless in_nmi() were true It is, see exc_debug_kernel() doing irqentry_nmi_enter().
Re: [PATCH v4 01/39] task_work: Fix TWA_NMI_CURRENT error handling
On Wed, Jan 22, 2025 at 01:28:21PM +0100, Peter Zijlstra wrote: > On Tue, Jan 21, 2025 at 06:30:53PM -0800, Josh Poimboeuf wrote: > > It's possible for irq_work_queue() to fail if the work has already been > > claimed. That can happen if a TWA_NMI_CURRENT task work is requested > > before a previous TWA_NMI_CURRENT IRQ work on the same CPU has gotten a > > chance to run. > > I'm confused, if it fails then it's already pending, and we'll get the > notification already. You can still add the work. Yeah, I suppose that makes sense. If the pending irq_work is already going to set TIF_NOTIFY_RESUME anyway, there's no need to do that again. > > The error has to be checked before the write to task->task_works. Also > > the try_cmpxchg() loop isn't needed in NMI context. The TWA_NMI_CURRENT > > case really is special, keep things simple by keeping its code all > > together in one place. > > NMIs can nest, Just for my understanding: for nested NMIs, the entry code basically queues up the next NMI, so the C handler (exc_nmi) can't nest. Right? > consider #DB (which is NMI like) What exactly do you mean by "NMI like"? Is it because a #DB might be basically running in NMI context, if the NMI hit a breakpoint? > doing task_work_add() and getting interrupted with NMI doing the same. How exactly would that work? At least with my patch the #DB wouldn't be able to use TWA_NMI_CURRENT unless in_nmi() were true due to NMI hitting a breakpoint. In which case a nested NMI wouldn't actually nest, it would get "queued" by the entry code. But yeah, I do see how the reverse can be true: somebody sets a breakpoint in task_work, right where it's fiddling with the list head. NMI calls task_work_add(TWA_NMI_CURRENT), triggering the #DB, which also calls task_work_add(). -- Josh
Re: [PATCH v4 01/39] task_work: Fix TWA_NMI_CURRENT error handling
On Tue, Jan 21, 2025 at 06:30:53PM -0800, Josh Poimboeuf wrote: > It's possible for irq_work_queue() to fail if the work has already been > claimed. That can happen if a TWA_NMI_CURRENT task work is requested > before a previous TWA_NMI_CURRENT IRQ work on the same CPU has gotten a > chance to run. I'm confused, if it fails then it's already pending, and we'll get the notification already. You can still add the work. > The error has to be checked before the write to task->task_works. Also > the try_cmpxchg() loop isn't needed in NMI context. The TWA_NMI_CURRENT > case really is special, keep things simple by keeping its code all > together in one place. NMIs can nest, consider #DB (which is NMI like) doing task_work_add() and getting interrupted with NMI doing the same. Might all this be fallout from trying to fix that schedule() bug from the next patch, because as is, I don't see it.
