Re: [RFC PATCH bpf-next v2 1/3] libbpf: Auto-upgrade uprobes to multi-uprobes when supported

[Varun R Mallya <varunrmallya@gmail.com>]

On Mon, Mar 30, 2026 at 04:52:27PM +0200, Jiri Olsa wrote:
> On Mon, Mar 30, 2026 at 04:30:17PM +0530, Varun R Mallya wrote:
> > +			const char *sec_name = prog->sec_name;
> > +			/* Here, we filter out for u[ret]probe or "u[ret]probe/"
> > +			 * but we leave out anything with an '@'
> > +			 * in it as uprobe_multi does not support versioned
> > +			 * symbols yet, so we don't upgrade.
> > +			 */
> 
> nice, I missed that uprobe.multi does not support versioned symbols,
> I guess we should fix that

Thanks! I intend to fix that after I am done with this patch.

> > +			if (((strncmp(sec_name, "uprobe", 6) == 0 &&
> 
> str_has_pfx ?
> 
Implementing on v3. This looks much cleaner.

> > @@ -9909,9 +9926,11 @@ static const struct bpf_sec_def section_defs[] = {
> >  	SEC_DEF("kprobe+",		KPROBE,	0, SEC_NONE, attach_kprobe),
> >  	SEC_DEF("uprobe+",		KPROBE,	0, SEC_NONE, attach_uprobe),
> >  	SEC_DEF("uprobe.s+",		KPROBE,	0, SEC_SLEEPABLE, attach_uprobe),
> > +	SEC_DEF("uprobe.single+",	KPROBE,	0, SEC_NONE, attach_uprobe),
> 
> should we add sleepable counterparts?
> 

> >  	SEC_DEF("kretprobe+",		KPROBE, 0, SEC_NONE, attach_kprobe),
> >  	SEC_DEF("uretprobe+",		KPROBE, 0, SEC_NONE, attach_uprobe),
> >  	SEC_DEF("uretprobe.s+",		KPROBE, 0, SEC_SLEEPABLE, attach_uprobe),
> > +	SEC_DEF("uretprobe.single+",	KPROBE,	0, SEC_NONE, attach_uprobe),
> 
> just an idea for discussion.. I wonder if it'd be better to add new uprobe
> section that will upgrade itself to uprobe.multi if it's present, instead
> of changing the existing (expected) type
> 
> but I guess we want existing uprobe programs to benefit from that and
> there's not really a reason anyone would want perf based uprobe when
> uprobe_multi is supported
> 
> ok I talked myself out of it ;-)
Yeah, that does seem like it's redundant. I think integrating this into
uprobe and kprobe is the best we can do. I have tried my best to ensure
that it does not really break any current functionality though.

> > +		LIBBPF_OPTS(bpf_uprobe_multi_opts, multi_opts);
> > +		unsigned long offsets[1] = {func_offset};
> > +		__u64 bpf_cookie;
> > +
> > +		multi_opts.retprobe = OPTS_GET(opts, retprobe, false);
> > +		if (offsets[0] || func_name) {
> > +			multi_opts.offsets = offsets;
> 
> could we do the same as for ref_ctr_off case and drop the offsets array?
> 
>      multi_opts.offsets = &func_offset;
> 
An artifact from a previous version. Fixing this.
> > +			multi_opts.cnt = 1;
> > +		}
> > +		if (ref_ctr_off) {
> > +			multi_opts.ref_ctr_offsets = &ref_ctr_off;
> > +			multi_opts.cnt = 1;
> > +		}
> > +		bpf_cookie = OPTS_GET(opts, bpf_cookie, 0);
> > +		if (bpf_cookie) {
> > +			multi_opts.cookies = &bpf_cookie;
> > +			multi_opts.cnt = 1;
> 
> I think it's better just to set multi_opts.cnt = 1 once outside those if conditions
Fixed this as well. I think it solves a part of the AI review as well.
> > +		}
> > +
> > +		return bpf_program__attach_uprobe_multi(prog, pid, binary_path,
> > +							NULL, &multi_opts);
> > +	}
> >  	legacy = determine_uprobe_perf_type() < 0;
> >  	switch (attach_mode) {
> >  	case PROBE_ATTACH_MODE_LEGACY:
> > @@ -12830,6 +12875,7 @@ static int attach_uprobe(const struct bpf_program *prog, long cookie, struct bpf
> >  	char *probe_type = NULL, *binary_path = NULL, *func_name = NULL, *func_off;
> >  	int n, c, ret = -EINVAL;
> >  	long offset = 0;
> > +	bool is_retprobe;
> >  
> >  	*link = NULL;
> >  
> > @@ -12856,13 +12902,14 @@ static int attach_uprobe(const struct bpf_program *prog, long cookie, struct bpf
> >  			else
> >  				offset = 0;
> >  		}
> > -		opts.retprobe = strcmp(probe_type, "uretprobe") == 0 ||
> > -				strcmp(probe_type, "uretprobe.s") == 0;
> > -		if (opts.retprobe && offset != 0) {
> > +		is_retprobe = strcmp(probe_type, "uretprobe") == 0 ||
> > +			      strcmp(probe_type, "uretprobe.s") == 0;
> > +		if (is_retprobe && offset != 0) {
> >  			pr_warn("prog '%s': uretprobes do not support offset specification\n",
> >  				prog->name);
> >  			break;
> >  		}
> > +		opts.retprobe = is_retprobe;
> 
> is there any functional change above? looks like just opts.retprobe
> is replaced with is_retprobe ?
> 
> jirka
Again, sorry about that. It was an artifact from a previous version.
Fixing it in v3.