Re: [PATCHv5 perf/core 10/22] uprobes/x86: Add support to optimize uprobes

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Jul 14, 2025 at 07:13:04PM +0900, Masami Hiramatsu wrote:

SNIP

> > diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> > index 678fb546f0a7..1ee2e5115955 100644
> > --- a/arch/x86/include/asm/uprobes.h
> > +++ b/arch/x86/include/asm/uprobes.h
> > @@ -20,6 +20,11 @@ typedef u8 uprobe_opcode_t;
> >  #define UPROBE_SWBP_INSN		0xcc
> >  #define UPROBE_SWBP_INSN_SIZE		   1
> >  
> > +enum {
> > +	ARCH_UPROBE_FLAG_CAN_OPTIMIZE   = 0,
> > +	ARCH_UPROBE_FLAG_OPTIMIZE_FAIL  = 1,
> > +};
> > +
> >  struct uprobe_xol_ops;
> >  
> >  struct arch_uprobe {
> > @@ -45,6 +50,8 @@ struct arch_uprobe {
> >  			u8	ilen;
> >  		}			push;
> >  	};
> > +
> > +	unsigned long flags;
> >  };
> >  
> >  struct arch_uprobe_task {
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > index 5eecab712376..b80942768f77 100644
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> > @@ -18,6 +18,7 @@
> >  #include <asm/processor.h>
> >  #include <asm/insn.h>
> >  #include <asm/mmu_context.h>
> > +#include <asm/nops.h>
> >  
> >  /* Post-execution fixups. */
> >  
> > @@ -702,7 +703,6 @@ static struct uprobe_trampoline *create_uprobe_trampoline(unsigned long vaddr)
> >  	return tramp;
> >  }
> >  
> > -__maybe_unused
> >  static struct uprobe_trampoline *get_uprobe_trampoline(unsigned long vaddr, bool *new)
> >  {
> >  	struct uprobes_state *state = &current->mm->uprobes_state;
> > @@ -874,6 +874,285 @@ static int __init arch_uprobes_init(void)
> >  
> >  late_initcall(arch_uprobes_init);
> >  
> > +enum {
> > +	OPT_PART,
> > +	OPT_INSN,
> > +	UNOPT_INT3,
> > +	UNOPT_PART,
> > +};
> > +
> > +struct write_opcode_ctx {
> > +	unsigned long base;
> > +	int update;
> > +};
> > +
> > +static int is_call_insn(uprobe_opcode_t *insn)
> > +{
> > +	return *insn == CALL_INSN_OPCODE;
> > +}
> > +
> 
> nit: Maybe we need a comment how to verify it as below, or just say
>  "See swbp_optimize/unoptimize() for how it works"
> 
> /*
>  * verify the old opcode starts from swbp or call before update to new opcode.
>  * When optimizing from swbp -> call, write 4 byte oprand (OPT_PART), and write
>  * the first opcode (OPT_INSN). Also, in unoptimizing, write the first opcode
>  * (UNOPT_INT3) and write the rest bytes (OPT_PART).
>  * Thus, the *old* `opcode` byte (not @vaddr[0], but ctx->base[0]) must be
>  * INT3 (OPT_PART, OPT_INSN, and UNOPT_PART) or CALL(UNOPT_INT3).
>  */

will add the comment in here

> 
> > +static int verify_insn(struct page *page, unsigned long vaddr, uprobe_opcode_t *new_opcode,
> > +		       int nbytes, void *data)
> > +{
> > +	struct write_opcode_ctx *ctx = data;
> > +	uprobe_opcode_t old_opcode[5];
> > +
> > +	uprobe_copy_from_page(page, ctx->base, (uprobe_opcode_t *) &old_opcode, 5);
> > +
> > +	switch (ctx->update) {
> > +	case OPT_PART:
> > +	case OPT_INSN:
> > +		if (is_swbp_insn(&old_opcode[0]))
> > +			return 1;
> > +		break;
> > +	case UNOPT_INT3:
> > +		if (is_call_insn(&old_opcode[0]))
> > +			return 1;
> > +		break;
> 
> > +	case UNOPT_PART:
> > +		if (is_swbp_insn(&old_opcode[0]))
> > +			return 1;
> > +		break;
> 
> nit: Can we fold this case to the OPT_PART & OPT_INSN case?
> It seems the same.

sure, thanks

jirka