Re: [PATCH v3 0/6] Static calls

["H. Peter Anvin" <hpa@zytor.com>]

On 1/11/19 11:39 AM, Jiri Kosina wrote:
> On Fri, 11 Jan 2019, hpa@zytor.com wrote:
> 
>> I still don't see why can't simply spin in the #BP handler until the 
>> patch is complete.
> 
> I think this brings us to the already discussed possible deadlock, when 
> one CPU#0 is in the middle of text_poke_bp(), CPU#1 is spinning inside 
> spin_lock_irq*(&lock) and CPU#2 hits the breakpont while holding that very 
> 'lock'.
> 
> Then we're stuck forever, because CPU#1 will never handle the pending 
> sync_core() IPI (it's not NMI).
> 
> Or have I misunderstood what you meant?
> 

OK, I was thinking about this quite a while ago, and even started hacking on
it, but apparently I managed to forget some key details.

Specifically, you do *not* want to use the acknowledgment of the IPI as the
blocking condition, so don't use a waiting IPI.

Instead, you want a CPU bitmap (or percpu variable) that the IPI handler
clears.  When you are spinning in the IPI handler, you *also* need to clear
that bit.  Doing so is safe even in the case of batched updates, because you
are guaranteed to execute an IRET before you get to patched code.

So the synchronization part of the patching routine becomes:

static cpumask_t text_poke_cpumask;

static void text_poke_sync(void)
{
	smp_wmb();
	text_poke_cpumask = cpu_online_mask;
	smp_wmb();	/* Optional on x86 */
	cpumask_clear_cpu(&text_poke_cpumask, smp_processor_id());
	on_each_cpu_mask(&text_poke_cpumask, text_poke_sync_cpu, NULL, false);
	while (!cpumask_empty(&text_poke_cpumask)) {
		cpu_relax();
		smp_rmb();
	}
}

static void text_poke_sync_cpu(void *dummy)
{
	(void)dummy;

	smp_rmb();
	cpumask_clear_cpu(&poke_bitmask, smp_processor_id());
	/*
	 * We are guaranteed to return with an IRET, either from the
	 * IPI or the #BP handler; this provides serialization.
	 */
}

The spin routine then needs add a call to do something like this. By
(optionally) not comparing to a specific breakpoint address we allow for
batching, but we may end up spinning on a breakpoint that is not actually a
patching breakpoint until the patching is done.

int poke_int3_handler(struct pt_regs *regs)
{
	/* In the current handler, but shouldn't be needed... */
	smp_rmb();

	if (likely(!atomic_read(bp_patching_in_progress)))
		return 0;

	if (user_mode(regs) unlikely(!user_mode(regs) &&
	    atomic_read(&bp_patching_in_progress)) {
		text_poke_sync();
		regs->ip--;
		return 1;	/* May end up retaking the trap */
	} else {
		return 0;
	}
}

Unless I'm totally mistaken, the worst thing that will happen with this code
is that it may end up taking a harmless spurious IPI at a later time.

	-hpa