Re: [PATCH] arm64: insn: Simulate nop and push instruction for better uprobe performance

[Andrii Nakryiko]

On Mon, Sep 9, 2024 at 3:18 AM Mark Rutland <mark.rutland@xxxxxxx> wrote:
>
> On Fri, Sep 06, 2024 at 10:46:00AM -0700, Andrii Nakryiko wrote:
> > On Fri, Sep 6, 2024 at 2:39 AM Mark Rutland <mark.rutland@xxxxxxx> wrote:
> > >
> > > On Tue, Aug 27, 2024 at 07:33:55PM +0800, Liao, Chang wrote:
> > > > Hi, Mark
> > > >
> > > > Would you like to discuss this patch further, or do you still believe emulating
> > > > STP to push FP/LR into the stack in kernel is not a good idea?
> > >
> > > I'm happy with the NOP emulation in principle, so please send a new
> > > version with *just* the NOP emulation, and I can review that.
> >
> > Let's definitely start with that, this is important for faster USDT tracing.
> >
> > > Regarding STP emulation, I stand by my earlier comments, and in addition
> > > to those comments, AFAICT it's currently unsafe to use any uaccess
> > > routine in the uprobe BRK handler anyway, so that's moot. The uprobe BRK
> > > handler runs with preemption disabled and IRQs (and all other maskable
> > > exceptions) masked, and faults cannot be handled. IIUC
> > > CONFIG_DEBUG_ATOMIC_SLEEP should scream about that.
> >
> > This part I don't really get, and this might be some very
> > ARM64-specific issue, so I'm sorry ahead of time.
> >
> > But in general, at the lowest level uprobes work in two logical steps.
> > First, there is a breakpoint that user space hits, kernel gets
> > control, and if VMA which hit breakpoint might contain uprobe, kernel
> > sets TIF_UPROBE thread flag and exits. This is the only part that's in
> > hard IRQ context. See uprobe_notify_resume() and comments around it.
> >
> > Then uprobe infrastructure gets called in user context on the way back
> > to user space. This is where we confirm that this is uprobe/uretprobe
> > hit, and, if supported, perform instruction emulation.
> >
> > So I'm wondering if your above comment refers to instruction emulation
> > within the first part of uprobe handling? If yes, then, no, that's not
> > where emulation will happen.
>
> You're right -- I had misunderstood that the emulation happened during
> handling of the breakpoint, rather than on the return-to-userspace path.
> Looking at the arm64 entry code, the way uprobe_notify_resume() is
> plumbed in is safe as it happens after we've re-enabled preemption and
> unmasked other exceptions.
>
> Sorry about that.
>
> For the moment I'd still prefer to get the NOP case out of the way
> first, so I'll review the NOP-only patch shortly.
>

Yep, one step at a time makes sense, thanks! Regardless, I'm glad we
clarified the confusion.

> Mark.