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.
But ultimately, the context switch required for uprobe dwarfs overhead
of any of this, presumably, so it's a minor concern.
> Simpler and faster to do it unconditionally even when the array doesn't
> have sleepable progs.
> rcu_read_lock() we have to do conditionally, because it won't be ok
> if sleepable progs are in the array.
