Re: [PATCHv3 perf/core 08/22] uprobes/x86: Add mapping for optimized uprobe trampolines

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Jun 27, 2025 at 03:01:45PM +0900, Masami Hiramatsu wrote:

SNIP

> > > 
> > > > +			return tramp;
> > > > +	}
> > > > +
> > > > +	tramp = create_uprobe_trampoline(vaddr);
> > > > +	if (!tramp)
> > > > +		return NULL;
> > > > +
> > > > +	*new = true;
> > > > +	hlist_add_head(&tramp->node, &state->head_tramps);
> > > > +	return tramp;
> > > > +}
> > > > +
> > > > +static void destroy_uprobe_trampoline(struct uprobe_trampoline *tramp)
> > > > +{
> > > > +	hlist_del(&tramp->node);
> > > > +	kfree(tramp);
> > > 
> > > Don't we need to unmap the tramp->vaddr?
> > 
> > that's tricky because we have no way to make sure the application is
> > no longer executing the trampoline, it's described in the changelog
> > of following patch:
> > 
> >     uprobes/x86: Add support to optimize uprobes
> > 
> >     ...
> > 
> >     We do not unmap and release uprobe trampoline when it's no longer needed,
> >     because there's no easy way to make sure none of the threads is still
> >     inside the trampoline. But we do not waste memory, because there's just
> >     single page for all the uprobe trampoline mappings.
> > 
> 
> I think we should put this as a code comment.

ok

> 
> >     We do waste frame on page mapping for every 4GB by keeping the uprobe
> >     trampoline page mapped, but that seems ok.
> 
> Hmm, this is not right with the current find_nearest_page(), because
> it always finds a page from the farthest +2GB range until it is full.
> Thus, in the worst case, if we hits uprobes with the order of
> uprobe0 -> 1 -> 2 which is put as below;
> 
> 0x0abc0004  [uprobe2]
> ...
> 0x0abc2004  [uprobe1]
> ...
> 0x0abc4004  [uprobe0]
> 
> Then the trampoline pages can be allocated as below.
> 
> 0x8abc0000  [uprobe_tramp2]
> [gap]
> 0x8abc2000  [uprobe_tramp1]
> [gap]
> 0x8abc4000  [uprobe_tramp0]
> 
> Using true "find_nearest_page()", this will be mitigated. But not
> allocated for "every 4GB". So I think we should drop that part
> from the comment :)

I think you're right, it's better to start with nearest page,
will change it in new version

SNIP

> > > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> > > > index 5080619560d4..b40d33aae016 100644
> > > > --- a/include/linux/uprobes.h
> > > > +++ b/include/linux/uprobes.h
> > > > @@ -17,6 +17,7 @@
> > > >  #include <linux/wait.h>
> > > >  #include <linux/timer.h>
> > > >  #include <linux/seqlock.h>
> > > > +#include <linux/mutex.h>
> > > >  
> > > >  struct uprobe;
> > > >  struct vm_area_struct;
> > > > @@ -185,6 +186,9 @@ struct xol_area;
> > > >  
> > > >  struct uprobes_state {
> > > >  	struct xol_area		*xol_area;
> > > > +#ifdef CONFIG_X86_64
> > > 
> > > Maybe we can introduce struct arch_uprobe_state{} here?
> > 
> > ok, on top of that Andrii also asked for [1]:
> >   - alloc 'struct uprobes_state' for mm_struct only when needed
> > 
> > this could be part of that follow up? I'd rather not complicate this
> > patchset any further
> > 
> > [1] https://lore.kernel.org/bpf/CAEf4BzY2zKPM9JHgn_wa8yCr8q5KntE5w8g=AoT2MnrD2Dx6gA@mail.gmail.com/
> 
> Hmm, OK. But if you need to avoid #ifdef CONFIG_<arch>,
> you can use include/asm-generic to override macros.
> 
> struct uprobes_state {
>  struct xol_area *xol_area;
>  uprobe_arch_specific_data
> };
> 
> 
>  --- include/asm-generic/uprobes.h
> 
> #define uprobe_arch_specific_data
> 
>  --- arch/x86/include/asm/uprobes.h
> 
> #undef uprobe_arch_specific_data
> #define uprobe_arch_specific_data \
> 	struct hlist_head	head_tramps;

ok

SNIP

> > > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > > index 1ee8eb11f38b..7108ca558518 100644
> > > > --- a/kernel/fork.c
> > > > +++ b/kernel/fork.c
> > > > @@ -1010,6 +1010,7 @@ static void mm_init_uprobes_state(struct mm_struct *mm)
> > > >  {
> > > >  #ifdef CONFIG_UPROBES
> > > >  	mm->uprobes_state.xol_area = NULL;
> > > > +	arch_uprobe_init_state(mm);
> > > >  #endif
> > > 
> > > Can't we make this uprobe_init_state(mm)?
> > 
> > hum, there are other mm_init_* functions around, I guess we should keep
> > the same pattern?
> > 
> > unless you mean s/arch_uprobe_init_state/uprobe_init_state/ but that's
> > arch code.. so probably not sure what you mean ;-)
> 
> Ah, I misunderstood. Yeah, this part is good to me.

ok, thanks

jirka