On Fri, Jun 23, 2023 at 10:11 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Fri, Jun 23, 2023 at 9:39 AM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Fri, Jun 23, 2023 at 9:24 AM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> 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();
>
>
> 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.
