On Tue, Jan 18, 2022 at 2:33 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Tue, Jan 18, 2022 at 2:31 PM Lorenzo Bianconi [off-list ref] wrote:

quoted

quoted

On Mon, Jan 17, 2022 at 06:28:30PM +0100, Lorenzo Bianconi wrote:

quoted

Introduce support for the following SEC entries for XDP multi-frags
property:
- SEC("xdp.frags")
- SEC("xdp.frags/devmap")
- SEC("xdp.frags/cpumap")

Acked-by: Toke Hoiland-Jorgensen <redacted>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 tools/lib/bpf/libbpf.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index fdb3536afa7d..611e81357fb6 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -6562,6 +6562,9 @@ static int libbpf_preload_prog(struct bpf_program *prog,
    if (def & SEC_SLEEPABLE)
            opts->prog_flags |= BPF_F_SLEEPABLE;

+   if (prog->type == BPF_PROG_TYPE_XDP && strstr(prog->sec_name, ".frags"))
+           opts->prog_flags |= BPF_F_XDP_HAS_FRAGS;

That's a bit sloppy.
Could you handle it similar to SEC_SLEEPABLE?

quoted

+
    if ((prog->type == BPF_PROG_TYPE_TRACING ||
         prog->type == BPF_PROG_TYPE_LSM ||
         prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) {

@@ -8600,8 +8603,11 @@ static const struct bpf_sec_def section_defs[] = {
    SEC_DEF("lsm.s/",               LSM, BPF_LSM_MAC, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_lsm),
    SEC_DEF("iter/",                TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF, attach_iter),
    SEC_DEF("syscall",              SYSCALL, 0, SEC_SLEEPABLE),
+   SEC_DEF("xdp.frags/devmap",     XDP, BPF_XDP_DEVMAP, SEC_NONE),
    SEC_DEF("xdp_devmap/",          XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+   SEC_DEF("xdp.frags/cpumap",     XDP, BPF_XDP_CPUMAP, SEC_NONE),
    SEC_DEF("xdp_cpumap/",          XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+   SEC_DEF("xdp.frags",            XDP, BPF_XDP, SEC_NONE),

It would be SEC_FRAGS here instead of SEC_NONE.

ack, I dropped SEC_FRAGS (SEC_XDP_MB before) from sec_def_flags because Andrii asked to remove
it but I am fine to add it back. Agree?

Andrii,
what was the motivation?
imo that's cleaner than strstr.

Given it was XDP-specific (as opposed to sleepable flag that applies
more widely), it felt cleaner ([0]) to handle that as a special case
in libbpf_preload_prog. But I didn't feel that strongly about that
back then and still don't, so if you think SEC_FRAGS is better, I'm
fine with it. I'd make it SEC_XDP_FRAGS to make it more obvious it's
still XDP-specific (there are no plans to extend it to non-XDP,
right?).

I do not think so and anyway it is an internal flag so we can change it in the
future if necessary, right?

Regards,
Lorenzo

But whichever way, it's internal implementation detail and pretty
small one at that, so I don't care much.

  [0] https://lore.kernel.org/bpf/CAEf4BzbfDvH5CYNsWg9Dx7JcFEp4jNmNRR6H-6sJEUxDSy1zZw@mail.gmail.com/ (local)