Messages in this thread | | | From | Andy Lutomirski <> | Date | Mon, 14 Jan 2019 19:05:01 -0800 | Subject | Re: [PATCH v3 0/6] Static calls |
| |
On Mon, Jan 14, 2019 at 2:55 PM H. Peter Anvin <hpa@zytor.com> 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.
| |