Message-ID: <dd57daf3-4762-07c3-e0be-2cbb605802e4@zytor.com>
Date:   Mon, 14 Jan 2019 21:01:38 -0800
From:   "H. Peter Anvin" <hpa@...or.com>
To:     Andy Lutomirski <luto@...nel.org>
Cc:     Jiri Kosina <jikos@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Nadav Amit <namit@...are.com>,
        Peter Zijlstra <peterz@...radead.org>,
        the arch/x86 maintainers <x86@...nel.org>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Jason Baron <jbaron@...mai.com>,
        David Laight <David.Laight@...lab.com>,
        Borislav Petkov <bp@...en8.de>,
        Julia Cartwright <julia@...com>, Jessica Yu <jeyu@...nel.org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Edward Cree <ecree@...arflare.com>,
        Daniel Bristot de Oliveira <bristot@...hat.com>
Subject: Re: [PATCH v3 0/6] Static calls

On 1/14/19 7:05 PM, Andy Lutomirski wrote:
> On Mon, Jan 14, 2019 at 2:55 PM H. Peter Anvin <hpa@...or.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.
> 

4: breakpoint has been canceled; need to redo patching.

>>
>>         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.

The state here can be 0 or 4.

>>                 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.

Fixing it isn't all that complicated; we just need to have a list of
patch locations (which we need anyway!) and walk (or search) it instead
of checking just one; I omitted that detail for simplicity.

>>         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)

I'm 85% sure that the cmpxchg here is actually unnecessary, an
atomic_write() is sufficient.

>>                         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.

If and only if you have only one patch location you could, but again,
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 could be as simple as spinning for a limited time waiting for
states 0 or 3 if we are not the patching CPU. It is also not necessary
to wait for the mask to become zero for the first sync if we find
ourselves suddenly in state 4.

This wouldn't reduce the livelock probability to zero, but it ought to
reduce it enough that if we really are under such heavy event load we
may end up getting stuck in any number of ways...

> 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.

Yes, this gets really grotty and who knows how many code paths it would
touch.

	-hpa

Powered by blists - more mailing lists