On Thu, Jun 11, 2020 at 03:29:09PM -0700, Alexei Starovoitov wrote:
> On Thu, Jun 11, 2020 at 3:23 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > /* dummy _ops. The verifier will operate on target program's ops. */
> > const struct bpf_verifier_ops bpf_extension_verifier_ops = {
> > @@ -205,14 +206,12 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)
> > tprogs[BPF_TRAMP_MODIFY_RETURN].nr_progs)
> > flags = BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_SKIP_FRAME;
> >
> > - /* Though the second half of trampoline page is unused a task could be
> > - * preempted in the middle of the first half of trampoline and two
> > - * updates to trampoline would change the code from underneath the
> > - * preempted task. Hence wait for tasks to voluntarily schedule or go
> > - * to userspace.
> > + /* the same trampoline can hold both sleepable and non-sleepable progs.
> > + * synchronize_rcu_tasks_trace() is needed to make sure all sleepable
> > + * programs finish executing. It also ensures that the rest of
> > + * generated tramopline assembly finishes before updating trampoline.
> > */
> > -
> > - synchronize_rcu_tasks();
> > + synchronize_rcu_tasks_trace();
>
> Hi Paul,
>
> I've been looking at rcu_trace implementation and I think above change
> is correct.
> Could you please double check my understanding?
>From an RCU Tasks Trace perspective, it looks good to me!
You have rcu_read_lock_trace() and rcu_read_unlock_trace() protecting
the readers and synchronize_rcu_trace() waiting for them.
One question given my lack of understanding of BPF: Are there still
tramoplines for non-sleepable BPF programs? If so, they might still
need to use synchronize_rcu_tasks() or some such.
The general principle is "never mix one type of RCU reader with another
type of RCU updater".
But in this case, one approach is to use synchronize_rcu_mult():
synchronize_rcu_mult(call_rcu_tasks, call_rcu_tasks_trace);
That would wait for both types of readers, and do so concurrently.
And if there is also a need to wait on rcu_read_lock() and friends,
you could do this:
synchronize_rcu_mult(call_rcu, call_rcu_tasks, call_rcu_tasks_trace);
> Also see benchmarking numbers in the cover letter :)
Now -that- is what I am talking about!!! Very nice! ;-)
Thanx, Paul
> > err = arch_prepare_bpf_trampoline(new_image, new_image + PAGE_SIZE / 2,
> > &tr->func.model, flags, tprogs,
> > @@ -344,7 +343,14 @@ void bpf_trampoline_put(struct bpf_trampoline *tr)
> > if (WARN_ON_ONCE(!hlist_empty(&tr->progs_hlist[BPF_TRAMP_FEXIT])))
> > goto out;
> > bpf_image_ksym_del(&tr->ksym);
> > - /* wait for tasks to get out of trampoline before freeing it */
> > + /* This code will be executed when all bpf progs (both sleepable and
> > + * non-sleepable) went through
> > + * bpf_prog_put()->call_rcu[_tasks_trace]()->bpf_prog_free_deferred().
> > + * Hence no need for another synchronize_rcu_tasks_trace() here,
> > + * but synchronize_rcu_tasks() is still needed, since trampoline
> > + * may not have had any sleepable programs and we need to wait
> > + * for tasks to get out of trampoline code before freeing it.
> > + */
> > synchronize_rcu_tasks();
> > bpf_jit_free_exec(tr->image);
> > hlist_del(&tr->hlist);
> > @@ -394,6 +400,21 @@ void notrace __bpf_prog_exit(struct bpf_prog *prog, u64 start)
> > rcu_read_unlock();
> > }
> >
> > +/* when rcu_read_lock_trace is held it means that some sleepable bpf program is
> > + * running. Those programs can use bpf arrays and preallocated hash maps. These
> > + * map types are waiting on programs to complete via
> > + * synchronize_rcu_tasks_trace();
> > + */
> > +void notrace __bpf_prog_enter_sleepable(void)
> > +{
> > + rcu_read_lock_trace();
> > +}
> > +
> > +void notrace __bpf_prog_exit_sleepable(void)
> > +{
> > + rcu_read_unlock_trace();
> > +}
> > +
