Re: [PATCH bpf-next v3 1/5] bpf: Add bpf_link support for sk_msg and sk_skb progs

[John Fastabend <john.fastabend@gmail.com>]

Yonghong Song wrote:
> 
> On 4/3/24 10:47 AM, John Fastabend wrote:
> > Andrii Nakryiko wrote:
> >> On Tue, Apr 2, 2024 at 6:08 PM Yonghong Song <yonghong.song@linux.dev> wrote:
> >>>
> >>> On 4/2/24 10:45 AM, Andrii Nakryiko wrote:
> >>>> On Mon, Mar 25, 2024 at 7:22 PM Yonghong Song <yonghong.song@linux.dev> wrote:
> >>>>> Add bpf_link support for sk_msg and sk_skb programs. We have an
> >>>>> internal request to support bpf_link for sk_msg programs so user
> >>>>> space can have a uniform handling with bpf_link based libbpf
> >>>>> APIs. Using bpf_link based libbpf API also has a benefit which
> >>>>> makes system robust by decoupling prog life cycle and
> >>>>> attachment life cycle.
> >>>>>
> > Thanks again for working on it.
> >
> >>>>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> >>>>> ---
> >>>>>    include/linux/bpf.h            |   6 +
> >>>>>    include/linux/skmsg.h          |   4 +
> >>>>>    include/uapi/linux/bpf.h       |   5 +
> >>>>>    kernel/bpf/syscall.c           |   4 +
> >>>>>    net/core/sock_map.c            | 263 ++++++++++++++++++++++++++++++++-
> >>>>>    tools/include/uapi/linux/bpf.h |   5 +
> >>>>>    6 files changed, 279 insertions(+), 8 deletions(-)
> >>>>>
> >> [...]
> >>
> >>>>>           psock_set_prog(pprog, prog);
> >>>>> -       return 0;
> >>>>> +       if (link)
> >>>>> +               *plink = link;
> >>>>> +
> >>>>> +out:
> >>>>> +       mutex_unlock(&sockmap_prog_update_mutex);
> >>>> why this mutex is not per-sockmap?
> >>> My thinking is the system probably won't have lots of sockmaps and
> >>> sockmap attach/detach/update_prog should not be that frequent. But
> >>> I could be wrong.
> >>>
> > For my use case at least we have a map per protocol we want to inspect.
> > So its rather small set <10 I would say. Also they are created once
> > when the agent starts and when config changes from operator (user decides
> > to remove/add a parser). Config changing is rather rare. I don't think
> > this would be paticularly painful in practice now to have a global
> > lock.
> >
> >> That seems like an even more of an argument to keep mutex per sockmap.
> >> It won't add a lot of memory, but it is conceptually cleaner, as each
> >> sockmap instance (and corresponding links) are completely independent,
> >> even from a locking perspective.
> >>
> >> But I can't say I feel very strongly about this.
> >>
> >>>>> +       return ret;
> >>>>>    }
> >>>>>
> >> [...]
> >>
> >>>>> +
> >>>>> +static void sock_map_link_release(struct bpf_link *link)
> >>>>> +{
> >>>>> +       struct sockmap_link *sockmap_link = get_sockmap_link(link);
> >>>>> +
> >>>>> +       mutex_lock(&sockmap_link_mutex);
> >>>> similar to the above, why is this mutex not sockmap-specific? And I'd
> >>>> just combine sockmap_link_mutex and sockmap_prog_update_mutex in this
> >>>> case to keep it simple.
> >>> This is to protect sockmap_link->map. They could share the same lock.
> >>> Let me double check...
> >> If you keep that global sockmap_prog_update_mutex then I'd probably
> >> reuse that one here for simplicity (and named it a bit more
> >> generically, "sockmap_mutex" or something like that, just like we have
> >> global "cgroup_mutex").
> > I was leaning to a per map lock, but because a global lock simplifies this
> > part a bunch I would agree just use a single sockmap_mutex throughout.
> >
> > If someone has a use case where they want to add/remove maps dynamically
> > maybe they can let us know what that is. For us, on my todo list, I want
> > to just remove the map notion and bind progs to socks directly. The
> > original map idea was for a L7 load balancer, but other than quick hacks
> > I've never built such a thing nor ran it in production. Maybe someday
> > I'll find the time.
> 
> I am using a single global lock.
>    https://lore.kernel.org/bpf/20240404025305.2210999-1-yonghong.song@linux.dev/
> Let us whether it makes sense or not with code.
> 
> John, it would be great if you can review the patch set. I am afraid
> that I could miss something...

Yep I will. Hopefully tonight because I intended to do it today but worse
case top of list tomorrow. I can also drop it into our test harness which
runs some longer running stress stuff. Thanks!