Messages in this thread | | | Date | Wed, 21 May 2014 12:29:34 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH 1/6] x86, nmi: Implement delayed irq_work mechanism to handle lost NMIs |
| |
On Thu, May 15, 2014 at 03:25:44PM -0400, Don Zickus wrote: > +DEFINE_PER_CPU(bool, nmi_delayed_work_pending); > + > +static void nmi_delayed_work_func(struct irq_work *irq_work) > +{ > + DECLARE_BITMAP(nmi_mask, NR_CPUS);
That's _far_ too big for on-stack, 4k cpus would make that 512 bytes.
> + cpumask_t *mask; > + > + preempt_disable();
That's superfluous, irq_work's are guaranteed to be called with IRQs disabled.
> + > + /* > + * Can't use send_IPI_self here because it will > + * send an NMI in IRQ context which is not what > + * we want. Create a cpumask for local cpu and > + * force an IPI the normal way (not the shortcut). > + */ > + bitmap_zero(nmi_mask, NR_CPUS); > + mask = to_cpumask(nmi_mask); > + cpu_set(smp_processor_id(), *mask); > + > + __this_cpu_xchg(nmi_delayed_work_pending, true);
Why is this xchg and not __this_cpu_write() ?
> + apic->send_IPI_mask(to_cpumask(nmi_mask), NMI_VECTOR);
What's wrong with apic->send_IPI_self(NMI_VECTOR); ?
> + > + preempt_enable(); > +} > + > +struct irq_work nmi_delayed_work = > +{ > + .func = nmi_delayed_work_func, > + .flags = IRQ_WORK_LAZY, > +};
OK, so I don't particularly like the LAZY stuff and was hoping to remove it before more users could show up... apparently I'm too late :-(
Frederic, I suppose this means dual lists.
> +static bool nmi_queue_work_clear(void) > +{ > + bool set = __this_cpu_read(nmi_delayed_work_pending); > + > + __this_cpu_write(nmi_delayed_work_pending, false); > + > + return set; > +}
That's a test-and-clear, the name doesn't reflect this. And here you do _not_ use xchg where you actually could have.
That said, try and avoid using xchg() its unconditionally serialized.
> + > +static int nmi_queue_work(void) > +{ > + bool queued = irq_work_queue(&nmi_delayed_work); > + > + if (queued) { > + /* > + * If the delayed NMI actually finds a 'dropped' NMI, the > + * work pending bit will never be cleared. A new delayed > + * work NMI is supposed to be sent in that case. But there > + * is no guarantee that the same cpu will be used. So > + * pro-actively clear the flag here (the new self-IPI will > + * re-set it. > + * > + * However, there is a small chance that a real NMI and the > + * simulated one occur at the same time. What happens is the > + * simulated IPI NMI sets the work_pending flag and then sends > + * the IPI. At this point the irq_work allows a new work > + * event. So when the simulated IPI is handled by a real NMI > + * handler it comes in here to queue more work. Because > + * irq_work returns success, the work_pending bit is cleared. > + * The second part of the back-to-back NMI is kicked off, the > + * work_pending bit is not set and an unknown NMI is generated. > + * Therefore check the BUSY bit before clearing. The theory is > + * if the BUSY bit is set, then there should be an NMI for this > + * cpu latched somewhere and will be cleared when it runs. > + */ > + if (!(nmi_delayed_work.flags & IRQ_WORK_BUSY)) > + nmi_queue_work_clear();
So I'm utterly and completely failing to parse that. It just doesn't make sense.
> + } > + > + return 0; > +}
Why does this function have a return value if all it can return is 0 and everybody ignores it?
> + > static int __kprobes nmi_handle(unsigned int type, struct pt_regs *regs, bool b2b) > { > struct nmi_desc *desc = nmi_to_desc(type); > @@ -341,6 +441,9 @@ static __kprobes void default_do_nmi(struct pt_regs *regs) > */ > if (handled > 1) > __this_cpu_write(swallow_nmi, true); > + > + /* kick off delayed work in case we swallowed external NMI */
That's inaccurate, there's no guarantee we actually swallowed one afaict, this is where we have to assume we lost one because there's really no other place.
> + nmi_queue_work(); > return; > } > > @@ -362,10 +465,16 @@ static __kprobes void default_do_nmi(struct pt_regs *regs) > #endif > __this_cpu_add(nmi_stats.external, 1); > raw_spin_unlock(&nmi_reason_lock); > + /* kick off delayed work in case we swallowed external NMI */ > + nmi_queue_work();
Again, inaccurate, there's no guarantee we did swallow an external NMI, but the thing is, there's no guarantee we didn't either, which is why we need to do this.
> return; > } > raw_spin_unlock(&nmi_reason_lock); > > + /* expected delayed queued NMI? Don't flag as unknown */ > + if (nmi_queue_work_clear()) > + return; > +
Right, so here we effectively swallow the extra nmi and avoid the unknown_nmi_error() bits.
| |