On Wed, Jan 12, 2022 at 11:47 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Wed, Jan 12, 2022 at 11:21 AM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Wed, Jan 12, 2022 at 11:17 AM Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> wrote:
> > >
> > > On Wed, Jan 12, 2022 at 10:24 AM Andrii Nakryiko
> > > <andrii.nakryiko@xxxxxxxxx> wrote:
> > > >
> > > > On Wed, Jan 12, 2022 at 10:18 AM Lorenzo Bianconi <lorenzo@xxxxxxxxxx> wrote:
> > > > >
> > > > > > On Sun, Jan 9, 2022 at 7:05 AM Lorenzo Bianconi <lorenzo@xxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > 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
> > > >
> > > > > 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),
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Acked-by: Toke Hoiland-Jorgensen <toke@xxxxxxxxxx>
> > > > > > > Acked-by: John Fastabend <john.fastabend@xxxxxxxxx>
> > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> > > > > > > ---
> > > > > > > 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.
--Zvi
