Re: [PATCH v3 0/6] Static calls

[Andy Lutomirski]

On Mon, Jan 14, 2019 at 2:55 PM H. Peter Anvin <hpa@xxxxxxxxx> wrote:
>
> I think this sequence ought to work (keep in mind we are already under a
> mutex, so the global data is safe even if we are preempted):

I'm trying to wrap my head around this.  The states are:

0: normal operation
1: writing 0xcc, can be canceled
2: writing final instruction.  The 0xcc was definitely synced to all CPUs.
3: patch is definitely installed but maybe not sync_cored.

>
>         set up page table entries
>         invlpg
>         set up bp patching global data
>
>         cpu = get_cpu()
>
So we're assuming that the state is

>         bp_old_value = atomic_read(bp_write_addr)
>
>         do {

So we're assuming that the state is 0 here.  A WARN_ON_ONCE to check
that would be nice.

>                 atomic_write(&bp_poke_state, 1)
>
>                 atomic_write(bp_write_addr, 0xcc)
>
>                 mask <- online_cpu_mask - self
>                 send IPIs
>                 wait for mask = 0
>
>         } while (cmpxchg(&bp_poke_state, 1, 2) != 1);
>
>         patch sites, remove breakpoints after patching each one

Not sure what you mean by patch *sites*.  As written, this only
supports one patch site at a time, since there's only one
bp_write_addr, and fixing that may be complicated.  Not fixing it
might also be a scalability problem.

>
>         atomic_write(&bp_poke_state, 3);
>
>         mask <- online_cpu_mask - self
>         send IPIs
>         wait for mask = 0
>
>         atomic_write(&bp_poke_state, 0);
>
>         tear down patching global data
>         tear down page table entries
>
>
>
> The #BP handler would then look like:
>
>         state = cmpxchg(&bp_poke_state, 1, 4);
>         switch (state) {
>                 case 1:
>                 case 4:

What is state 4?

>                         invlpg
>                         cmpxchg(bp_write_addr, 0xcc, bp_old_value)
>                         break;
>                 case 2:
>                         invlpg
>                         complete patch sequence
>                         remove breakpoint
>                         break;

ISTM you might as well change state to 3 here, but it's arguably unnecessary.

>                 case 3:
>                         /* If we are here, the #BP will go away on its own */
>                         break;
>                 case 0:
>                         /* No patching in progress!!! */
>                         return 0;
>         }
>
>         clear bit in mask
>         return 1;
>
> The IPI handler:
>
>         clear bit in mask
>         sync_core       /* Needed if multiple IPI events are chained */

I really like that this doesn't require fixups -- text_poke_bp() just
works.  But I'm nervous about livelocks or maybe just extreme slowness
under nasty loads.  Suppose some perf NMI code does a static call or
uses a static call.  Now there's a situation where, under high
frequency perf sampling, the patch process might almost always hit the
breakpoint while in state 1.  It'll get reversed and done again, and
we get stuck.  It would be neat if we could get the same "no
deadlocks" property while significantly reducing the chance of a
rollback.

This is why I proposed something where we try to guarantee forward
progress by making sure that any NMI code that might spin and wait for
other CPUs is guaranteed to eventually sync_core(), clear its bit, and
possibly finish a patch.  But this is a bit gross.