Re: [PATCH bpf-next v4 1/8] bpf: Add generic attach/detach/query API for multi-progs

[Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>]

On Mon, Jul 10, 2023 at 10:12:11PM +0200, Daniel Borkmann wrote:
> + *
> + *   struct bpf_mprog_entry *entry, *peer;
> + *   int ret;
> + *
> + *   // bpf_mprog user-side lock
> + *   // fetch active @entry from attach location
> + *   [...]
> + *   ret = bpf_mprog_attach(entry, [...]);
> + *   if (ret >= 0) {
> + *       peer = bpf_mprog_peer(entry);
> + *       if (bpf_mprog_swap_entries(ret))
> + *           // swap @entry to @peer at attach location
> + *       bpf_mprog_commit(entry);
> + *       ret = 0;
> + *   } else {
> + *       // error path, bail out, propagate @ret
> + *   }
> + *   // bpf_mprog user-side unlock
> + *
> + *  Detach case:
> + *
> + *   struct bpf_mprog_entry *entry, *peer;
> + *   bool release;
> + *   int ret;
> + *
> + *   // bpf_mprog user-side lock
> + *   // fetch active @entry from attach location
> + *   [...]
> + *   ret = bpf_mprog_detach(entry, [...]);
> + *   if (ret >= 0) {
> + *       release = ret == BPF_MPROG_FREE;
> + *       peer = release ? NULL : bpf_mprog_peer(entry);
> + *       if (bpf_mprog_swap_entries(ret))
> + *           // swap @entry to @peer at attach location
> + *       bpf_mprog_commit(entry);
> + *       if (release)
> + *           // free bpf_mprog_bundle
> + *       ret = 0;
> + *   } else {
> + *       // error path, bail out, propagate @ret
> + *   }
> + *   // bpf_mprog user-side unlock

Thanks for the doc. It helped a lot.
And when it's contained like this it's easier to discuss api.
It seems bpf_mprog_swap_entries() is trying to abstract the error code
away, but BPF_MPROG_FREE leaks out and tcx_entry_needs_release()
captures it with extra miniq_active twist, which I don't understand yet.
bpf_mprog_peer() is also leaking a bit of implementation detail.
Can we abstract it further, like:

ret = bpf_mprog_detach(entry, [...], &new_entry);
if (ret >= 0) {
   if (entry != new_entry)
     // swap @entry to @new_entry at attach location
   bpf_mprog_commit(entry);
   if (!new_entry)
     // free bpf_mprog_bundle
}
and make bpf_mprog_peer internal to mprog. It will also allow removing
BPF_MPROG_FREE vs SWAP distinction. peer is hidden.
   if (entry != new_entry)
      // update
also will be easier to read inside tcx code without looking into mprog details.