On 12/01/2022 23.04, Toke Høiland-Jørgensen wrote:

Lorenzo Bianconi [off-list ref] writes:

quoted

quoted

On Wed, Jan 12, 2022 at 11:47 AM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Wed, Jan 12, 2022 at 11:21 AM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Wed, Jan 12, 2022 at 11:17 AM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Wed, Jan 12, 2022 at 10:24 AM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Wed, Jan 12, 2022 at 10:18 AM Lorenzo Bianconi [off-list ref] wrote:

quoted

quoted

On Sun, Jan 9, 2022 at 7:05 AM Lorenzo Bianconi [off-list ref] wrote:

quoted

Introduce support for the following SEC entries for XDP multi-buff
property:
- SEC("xdp_mb/")
- SEC("xdp_devmap_mb/")
- SEC("xdp_cpumap_mb/")

Libbpf seemed to went with .<suffix> rule (e.g., fentry.s for
sleepable, seems like we'll have kprobe.multi or  something along
those lines as well), so let's stay consistent and call this "xdp_mb",
"xdp_devmap.mb", "xdp_cpumap.mb" (btw, is "mb" really all that
recognizable? would ".multibuf" be too verbose?). Also, why the "/"
part? Also it shouldn't be "sloppy" either. Neither expected attach
type should be optional.  Also not sure SEC_ATTACHABLE is needed. So
at most it should be SEC_XDP_MB, probably.

ack, I fine with it. Something like:

         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_devmap.multibuf",  XDP, BPF_XDP_DEVMAP, 0),
         SEC_DEF("xdp_devmap/",          XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp_cpumap.multibuf",  XDP, BPF_XDP_CPUMAP, 0),
         SEC_DEF("xdp_cpumap/",          XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
+       SEC_DEF("xdp.multibuf",         XDP, BPF_XDP, 0),

yep, but please use SEC_NONE instead of zero

quoted

         SEC_DEF("xdp",                  XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
         SEC_DEF("perf_event",           PERF_EVENT, 0, SEC_NONE | SEC_SLOPPY_PFX),
         SEC_DEF("lwt_in",               LWT_IN, 0, SEC_NONE | SEC_SLOPPY_PFX),

quoted

quoted

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 | 8 ++++++++
  1 file changed, 8 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 7f10dd501a52..c93f6afef96c 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -235,6 +235,8 @@ enum sec_def_flags {
         SEC_SLEEPABLE = 8,
         /* allow non-strict prefix matching */
         SEC_SLOPPY_PFX = 16,
+       /* BPF program support XDP multi-buff */
+       SEC_XDP_MB = 32,
  };

  struct bpf_sec_def {

@@ -6562,6 +6564,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 && (def & SEC_XDP_MB))
+               opts->prog_flags |= BPF_F_XDP_MB;

I'd say you don't even need SEC_XDP_MB flag at all, you can just check
that prog->sec_name is one of "xdp.mb", "xdp_devmap.mb" or
"xdp_cpumap.mb" and add the flag. SEC_XDP_MB doesn't seem generic
enough to warrant a flag.

ack, something like:

+       if (prog->type == BPF_PROG_TYPE_XDP &&
+           (!strcmp(prog->sec_name, "xdp_devmap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp_cpumap.multibuf") ||
+            !strcmp(prog->sec_name, "xdp.multibuf")))
+               opts->prog_flags |= BPF_F_XDP_MB;

yep, can also simplify it a bit with strstr(prog->sec_name,
".multibuf") instead of three strcmp

Maybe ".mb" ?
".multibuf" is too verbose.
We're fine with ".s" for sleepable :)

I had reservations about "mb" because the first and strong association
is "megabyte", not "multibuf". And it's not like anyone would have
tens of those programs in a single file so that ".multibuf" becomes
way too verbose. But I don't feel too strongly about this, if the
consensus is on ".mb".

The rest of the patches are using _mb everywhere.
I would keep libbpf consistent.

Should the rest of the patches maybe use "multibuf" instead of "mb"? I've been
following this patch series closely and excitedly, and I keep having to remind
myself that "mb" is "multibuff" and not "megabyte". If I'm having to correct
myself while following the patch series, I'm wondering if future confusion is
inevitable?

But, is it enough confusion to be worth updating many other patches? I'm not
sure.

I agree consistency is more important than the specific term we're consistent
on.

I would prefer to keep the "_mb" postfix, but naming is hard and I am
polarized :)

I would lean towards keeping _mb as well, but if it does have to be
changed why not _mbuf? At least that's not quite as verbose :)

I dislike the "mb" abbreviation as I forget it stands for multi-buffer.
I like the "mbuf" suggestion, even-though it conflicts with (Free)BSD 
mbufs (which is their SKB).

I prefer/support the .<suffix> idea from Andrii.
Which would then be ".mbuf" for my taste.

--Jesper
p.s. I like the bikeshed red, meaning I don't feel too strongly about this.