Re: [PATCHv2 bpf-next 01/24] bpf: Add multi uprobe link

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Jun 23, 2023 at 10:20:26AM -0700, Alexei Starovoitov wrote:
> On Fri, Jun 23, 2023 at 10:11 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Fri, Jun 23, 2023 at 9:39 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Fri, Jun 23, 2023 at 9:24 AM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > > > > +
> > > > > > > +static int uprobe_prog_run(struct bpf_uprobe *uprobe,
> > > > > > > +                          unsigned long entry_ip,
> > > > > > > +                          struct pt_regs *regs)
> > > > > > > +{
> > > > > > > +       struct bpf_uprobe_multi_link *link = uprobe->link;
> > > > > > > +       struct bpf_uprobe_multi_run_ctx run_ctx = {
> > > > > > > +               .entry_ip = entry_ip,
> > > > > > > +       };
> > > > > > > +       struct bpf_prog *prog = link->link.prog;
> > > > > > > +       struct bpf_run_ctx *old_run_ctx;
> > > > > > > +       int err = 0;
> > > > > > > +
> > > > > > > +       might_fault();
> > > > > > > +
> > > > > > > +       rcu_read_lock_trace();
> > > > > >
> > > > > > we don't need this if uprobe is not sleepable, right? why unconditional then?
> > > > >
> > > > > I won't pretend I understand what rcu_read_lock_trace does ;-)
> > > > >
> > > > > I tried to follow bpf_prog_run_array_sleepable where it's called
> > > > > unconditionally for both sleepable and non-sleepable progs
> > > > >
> > > > > there are conditional rcu_read_un/lock calls later on
> > > > >
> > > > > I will check
> > > >
> > > > hm... Alexei can chime in here, but given here we actually are trying
> > > > to run one BPF program (not entire array of them), we do know whether
> > > > it's going to be sleepable or not. So we can avoid unnecessary
> > > > rcu_read_{lock,unlock}_trace() calls. rcu_read_lock_trace() is used
> > > > when there is going to be sleepable BPF program executed to protect
> > > > BPF maps and other resources from being freed too soon. But if we know
> > > > that we don't need sleepable, we can avoid that.
> > >
> > > We can add more checks and bool flags to avoid rcu_read_{lock,unlock}_trace(),
> > > but it will likely be slower. These calls are very fast.
> >
> > that's ok then. But seeing how we do
> >
> > rcu_read_lock_trace();
> > if (!sleepable)
> >     rcu_read_lock();
> >
> > it felt like we might as well just do
> >
> > if (sleepable)
> >     rcu_read_lock_trace();
> > else
> >     rcu_read_lock();

ok

> >
> >
> > As I mentioned, in this case we have a single bpf_prog, not a
> > bpf_prog_array, so that changes things a bit.
> 
> Ahh. It's only one prog. I missed that. Above makes sense then.
> But why is it not an array? We can attach multiple uprobes to the same
> location. Anyway that can be dealt with later.

I think we could add support for this later if it's needed

jirka