Re: [PATCH RFC 1/6] perf/x86: Add perf text poke event

[Peter Zijlstra]

On Fri, Nov 08, 2019 at 11:05:30PM +0800, Leo Yan wrote:

> I will update some status for prototype (the code has been uploaded into
> git server [1]) and found some issues for text poke perf event on arm64.
> These issues are mainly related with arch64 architecture.
> 
> - The first issue is for the nosync instruction emulation.  On arm64,
>   some instructions need to be single stepped and some instructions
>   is emulated.  Especially, after I read the code for kprobe
>   implementation on Arm64.  So the main idea for prototyping is to use
>   the almos same method with kprobe for nosync instruction.

This makes no sense to me what so ever. What actual instructions are
patched with _nosync() ? ftrace/jump_label only use 'NOP/JMP/CALL'

For NOP you can advance regs->ip, for JMP you can adjust regs->ip, for
CALL you adjust regs->ip and regs->r14 (IIUC you do something like:
regs->r14 = regs->ip+4; regs->ip = func;)

(FWIW emulating CALL on x86 is fun because we get to PUSH something on
the stack, let me know if you want to see the patches that were required
to make that happen :-)

> - The second issue is race condition between the CPU alters
>   instructions and other CPUs hit the altered instructions (and
>   breakpointed).
> 
>   Peter's suggestion uses global variables 'nosync_insn' and
>   'nosync_addr' to record the altered instruction.  But from the
>   testing I found for single static key, usually it will change for
>   multiple address at once.
> 
>   So this might cause the issue is: CPU(a) will loop to alter
>   instructions for different address (sometimes the opcode also is
>   different for different address), at the meantime, CPU(b) hits an
>   altered instruction and handle exception for the breakpoint, if
>   CPU(a) is continuing to alter instruction for the next address, thne
>   CPU(a) might wrongly to use the value from 'nosync_insn' and
>   'nosync_addr'.
> 
>   Simply to say, we cannot only support single nosync instruction but
>   need to support multiple nosync instructions in the loop.

On x86 all actual text poking is serialized by text_mutex.

> - The third issue is might be nested issue.  E.g. for the injected
>   break instruction, we have no method to pass perf event for this
>   instruction; and if we connect with the first issue, if the
>   instruction is single stepped with slot (the slot is allocated with
>   get_insn_slot()), we cannot to allow the perf user space to know
>   the instructions which are executed in the slots.
> 
>   I am not sure if I think too complex here.  But seems to me, we
>   inject more instructions for poke text event, and if these injected
>   instructions are executed but the userspace decoder has no way to
>   pass the related info.

That's a misunderstanding, the text_poke event is a side-band event and
as such delivered to all events that expressed interest in it. You don't
need any exception to event mapping yourself.

> Just clarify, I am fine for merging this patch set, but we might
> consider more what's the best way on Arm64.  Welcome any public
> comments and suggestions; I will sync internally for how to follow up
> this functionality.

Thanks!