From: Daniel Borkmann <daniel@iogearbox.net>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: ast@kernel.org, andrii@kernel.org, martin.lau@linux.dev,
razor@blackwall.org, sdf@google.com, john.fastabend@gmail.com,
kuba@kernel.org, dxu@dxuuu.xyz, joe@cilium.io, toke@kernel.org,
davem@davemloft.net, bpf@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH bpf-next v4 3/8] libbpf: Add opts-based attach/detach/query API for tcx
Date: Tue, 11 Jul 2023 16:03:37 +0200 [thread overview]
Message-ID: <dd7aaf1e-9abf-0b9c-bfba-ee3bc4cfa852@iogearbox.net> (raw)
In-Reply-To: <CAEf4BzaGbVe3ip_cDxV0u8bBUEVExdqHXOFBorHWZ0tpDBLLnw@mail.gmail.com>
On 7/11/23 6:00 AM, Andrii Nakryiko wrote:
> On Mon, Jul 10, 2023 at 1:12 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> Extend libbpf attach opts and add a new detach opts API so this can be used
>> to add/remove fd-based tcx BPF programs. The old-style bpf_prog_detach() and
>> bpf_prog_detach2() APIs are refactored to reuse the new bpf_prog_detach_opts()
>> internally.
>>
>> The bpf_prog_query_opts() API got extended to be able to handle the new
>> link_ids, link_attach_flags and revision fields.
>>
>> For concrete usage examples, see the extensive selftests that have been
>> developed as part of this series.
>>
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> ---
>> tools/lib/bpf/bpf.c | 105 +++++++++++++++++++++++++--------------
>> tools/lib/bpf/bpf.h | 92 ++++++++++++++++++++++++++++------
>> tools/lib/bpf/libbpf.c | 12 +++--
>> tools/lib/bpf/libbpf.map | 1 +
>> 4 files changed, 157 insertions(+), 53 deletions(-)
>>
>
> Thanks for doc comments! Looks good, left a few nits with suggestions
> for simplifying code, but it's minor.
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
>> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
>> index 3b0da19715e1..3dfc43b477c3 100644
>> --- a/tools/lib/bpf/bpf.c
>> +++ b/tools/lib/bpf/bpf.c
>> @@ -629,55 +629,87 @@ int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type,
>> return bpf_prog_attach_opts(prog_fd, target_fd, type, &opts);
>> }
>>
>> -int bpf_prog_attach_opts(int prog_fd, int target_fd,
>> - enum bpf_attach_type type,
>> - const struct bpf_prog_attach_opts *opts)
>> +int bpf_prog_attach_opts(int prog_fd, int target,
>> + enum bpf_attach_type type,
>> + const struct bpf_prog_attach_opts *opts)
>> {
>> - const size_t attr_sz = offsetofend(union bpf_attr, replace_bpf_fd);
>> + const size_t attr_sz = offsetofend(union bpf_attr, expected_revision);
>> + __u32 relative_id, flags;
>> union bpf_attr attr;
>> - int ret;
>> + int ret, relative;
>>
>> if (!OPTS_VALID(opts, bpf_prog_attach_opts))
>> return libbpf_err(-EINVAL);
>>
>> + relative_id = OPTS_GET(opts, relative_id, 0);
>> + relative = OPTS_GET(opts, relative_fd, 0);
>> + flags = OPTS_GET(opts, flags, 0);
>> +
>> + /* validate we don't have unexpected combinations of non-zero fields */
>> + if (relative > 0 && relative_id)
>> + return libbpf_err(-EINVAL);
>
> I left a comment in the next patch about this, I think it should be
> simple `if (relative_fd && relative_id) { /* bad */ }`. But see the
> next patch for why.
>
>> + if (relative_id) {
>> + relative = relative_id;
>> + flags |= BPF_F_ID;
>> + }
>
> it's a bit hard to follow as written (to me at least). How about a
> slight variation that has less in-place state update
>
>
> int relative_fd, relative_id;
>
> relative_fd = OPTS_GET(opts, relative_fd, 0);
> relative_id = OPTS_GET(opts, relative_id, 0);
>
> /* only one of fd or id can be specified */
> if (relative_fd && relative_id > 0)
> return libbpf_err(-EINVAL);
>
> ... then see further below
>
>> +
>> memset(&attr, 0, attr_sz);
>> - attr.target_fd = target_fd;
>> - attr.attach_bpf_fd = prog_fd;
>> - attr.attach_type = type;
>> - attr.attach_flags = OPTS_GET(opts, flags, 0);
>> - attr.replace_bpf_fd = OPTS_GET(opts, replace_prog_fd, 0);
>> + attr.target_fd = target;
>> + attr.attach_bpf_fd = prog_fd;
>> + attr.attach_type = type;
>> + attr.attach_flags = flags;
>> + attr.relative_fd = relative;
>
> instead of two lines above, have simple if/else
>
> if (relative_if) {
> attr.relative_id = relative_id;
> attr.attach_flags = flags | BPF_F_ID;
> } else {
> attr.relative_fd = relative_fd;
> attr.attach_flags = flags;
> }
>
> This combined with the piece above seems very straightforward in terms
> of what is checked and what's passed into attr. WDYT?
All sgtm, I've implemented the suggestions locally for v5.
>> + attr.replace_bpf_fd = OPTS_GET(opts, replace_fd, 0);
>> + attr.expected_revision = OPTS_GET(opts, expected_revision, 0);
>>
>> ret = sys_bpf(BPF_PROG_ATTACH, &attr, attr_sz);
>> return libbpf_err_errno(ret);
>> }
>>
>> -int bpf_prog_detach(int target_fd, enum bpf_attach_type type)
>> +int bpf_prog_detach_opts(int prog_fd, int target,
>> + enum bpf_attach_type type,
>> + const struct bpf_prog_detach_opts *opts)
>> {
>> - const size_t attr_sz = offsetofend(union bpf_attr, replace_bpf_fd);
>> + const size_t attr_sz = offsetofend(union bpf_attr, expected_revision);
>> + __u32 relative_id, flags;
>> union bpf_attr attr;
>> - int ret;
>> + int ret, relative;
>> +
>> + if (!OPTS_VALID(opts, bpf_prog_detach_opts))
>> + return libbpf_err(-EINVAL);
>> +
>> + relative_id = OPTS_GET(opts, relative_id, 0);
>> + relative = OPTS_GET(opts, relative_fd, 0);
>> + flags = OPTS_GET(opts, flags, 0);
>> +
>> + /* validate we don't have unexpected combinations of non-zero fields */
>> + if (relative > 0 && relative_id)
>> + return libbpf_err(-EINVAL);
>> + if (relative_id) {
>> + relative = relative_id;
>> + flags |= BPF_F_ID;
>> + }
>
> see above, I think the same data flow simplification can be done
>
>>
>> memset(&attr, 0, attr_sz);
>> - attr.target_fd = target_fd;
>> - attr.attach_type = type;
>> + attr.target_fd = target;
>> + attr.attach_bpf_fd = prog_fd;
>> + attr.attach_type = type;
>> + attr.attach_flags = flags;
>> + attr.relative_fd = relative;
>> + attr.expected_revision = OPTS_GET(opts, expected_revision, 0);
>>
>> ret = sys_bpf(BPF_PROG_DETACH, &attr, attr_sz);
>> return libbpf_err_errno(ret);
>> }
>>
>
> [...]
>
>> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
>> index d9ec4407befa..a95d39bbef90 100644
>> --- a/tools/lib/bpf/libbpf.map
>> +++ b/tools/lib/bpf/libbpf.map
>> @@ -396,4 +396,5 @@ LIBBPF_1.3.0 {
>> global:
>> bpf_obj_pin_opts;
>> bpf_program__attach_netfilter;
>> + bpf_prog_detach_opts;
>
> I think it sorts before bpf_program__attach_netfilter?
Yeap, also fixed.
next prev parent reply other threads:[~2023-07-11 14:03 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-10 20:12 [PATCH bpf-next v4 0/8] BPF link support for tc BPF programs Daniel Borkmann
2023-07-10 20:12 ` [PATCH bpf-next v4 1/8] bpf: Add generic attach/detach/query API for multi-progs Daniel Borkmann
2023-07-11 0:23 ` Alexei Starovoitov
2023-07-11 18:51 ` Andrii Nakryiko
2023-07-14 16:06 ` Daniel Borkmann
2023-07-11 18:48 ` Andrii Nakryiko
2023-07-14 16:00 ` Daniel Borkmann
2023-07-10 20:12 ` [PATCH bpf-next v4 2/8] bpf: Add fd-based tcx multi-prog infra with link support Daniel Borkmann
2023-07-10 20:12 ` [PATCH bpf-next v4 3/8] libbpf: Add opts-based attach/detach/query API for tcx Daniel Borkmann
2023-07-11 4:00 ` Andrii Nakryiko
2023-07-11 14:03 ` Daniel Borkmann [this message]
2023-07-10 20:12 ` [PATCH bpf-next v4 4/8] libbpf: Add link-based " Daniel Borkmann
2023-07-11 4:00 ` Andrii Nakryiko
2023-07-11 14:08 ` Daniel Borkmann
2023-07-10 20:12 ` [PATCH bpf-next v4 5/8] libbpf: Add helper macro to clear opts structs Daniel Borkmann
2023-07-11 4:02 ` Andrii Nakryiko
2023-07-11 9:42 ` Daniel Borkmann
2023-07-10 20:12 ` [PATCH bpf-next v4 6/8] bpftool: Extend net dump with tcx progs Daniel Borkmann
2023-07-11 14:19 ` Quentin Monnet
2023-07-11 16:46 ` Daniel Borkmann
2023-07-10 20:12 ` [PATCH bpf-next v4 7/8] selftests/bpf: Add mprog API tests for BPF tcx opts Daniel Borkmann
2023-07-10 20:12 ` [PATCH bpf-next v4 8/8] selftests/bpf: Add mprog API tests for BPF tcx links Daniel Borkmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dd7aaf1e-9abf-0b9c-bfba-ee3bc4cfa852@iogearbox.net \
--to=daniel@iogearbox.net \
--cc=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=davem@davemloft.net \
--cc=dxu@dxuuu.xyz \
--cc=joe@cilium.io \
--cc=john.fastabend@gmail.com \
--cc=kuba@kernel.org \
--cc=martin.lau@linux.dev \
--cc=netdev@vger.kernel.org \
--cc=razor@blackwall.org \
--cc=sdf@google.com \
--cc=toke@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).