On Wed, Jan 26, 2022 at 8:11 AM Lorenzo Bianconi <lorenzo@xxxxxxxxxx> wrote:
>
> > On Fri, Jan 14, 2022 at 10:55 AM Zvi Effron <zeffron@xxxxxxxxxxxxx> wrote:
> > >
> > > On Fri, Jan 14, 2022 at 8:50 AM Jesper Dangaard Brouer
> > > <jbrouer@xxxxxxxxxx> wrote:
> > > >
> > > >
> > > >
> > > > On 14/01/2022 03.09, Alexei Starovoitov wrote:
> > > > > On Thu, Jan 13, 2022 at 3:58 PM Lorenzo Bianconi <lorenzo@xxxxxxxxxx> wrote:
> > > > >>>
> > > > >>> Btw "xdp_cpumap" should be cleaned up.
> > > > >>> xdp_cpumap is an attach type. It's not prog type.
> > > > >>> Probably it should be "xdp/cpumap" to align with "cgroup/bind[46]" ?
> > > > >>
> > > > >> so for xdp "mb" or xdp "frags" it will be xdp/cpumap.mb (xdp/devmap.mb) or
> > > > >> xdp/cpumap.frags (xdp/devmap.frags), right?
> > > > >
> > > > > xdp.frags/cpumap
> > > > > xdp.frags/devmap
> > > > >
> > > > > The current de-facto standard for SEC("") in libbpf:
> > > > > prog_type.prog_flags/attach_place
> > > >
> > > > Ups, did we make a mistake with SEC("xdp_devmap/")
> > > >
> > > > and can we correct without breaking existing programs?
> > > >
> > >
> > > We can (at the very least) add the correct sections, even if we leave the
> > > current incorrect ones as well. Ideally we'd mark the incorrect ones deprecated
> > > and either remove them before libbpf 1.0 or as part of 2.0?
> > >
> >
> > Correct, those would need to be new aliases. We can also deprecate old
> > ones, if we have consensus on that. We can teach libbpf to emit
> > warnings (through logs, of course) for such uses of to-be-removed
> > sections aliases. We still have probably a few months before the final
> > 1.0 release, should hopefully be plenty of time to people to adapt.
>
> If we all agree on old cpumap/devmap sec deprecation and replace them with
> xdp/cpumap and xdp/devmap, would it be ok something like the patch below or
> would be necessary something different?
>
> Regards,
> Lorenzo
>
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -6575,6 +6575,12 @@ static int libbpf_preload_prog(struct bpf_program *prog,
> if (prog->type == BPF_PROG_TYPE_XDP && (def & SEC_XDP_FRAGS))
> opts->prog_flags |= BPF_F_XDP_HAS_FRAGS;
>
> + if (strstr(prog->sec_name, "xdp_devmap") ||
> + strstr(prog->sec_name, "xdp_cpumap")) {
> + pr_warn("sec_name '%s' is deprecated, use xdp/devmap or xdp_cpumap instead\n",
> + prog->sec_name);
> + }
This time it's actually a clear use case for a flag, SEC_DEPRECATED.
:) We can look through other SEC_DEF()s and see if some of them have
to go.
> +
> if ((prog->type == BPF_PROG_TYPE_TRACING ||
> prog->type == BPF_PROG_TYPE_LSM ||
> prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) {
> @@ -8618,8 +8624,10 @@ static const struct bpf_sec_def section_defs[] = {
> SEC_DEF("iter.s/", TRACING, BPF_TRACE_ITER, SEC_ATTACH_BTF | SEC_SLEEPABLE, attach_iter),
> SEC_DEF("syscall", SYSCALL, 0, SEC_SLEEPABLE),
> SEC_DEF("xdp.frags/devmap", XDP, BPF_XDP_DEVMAP, SEC_XDP_FRAGS),
> + SEC_DEF("xdp/devmap", XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
> SEC_DEF("xdp_devmap/", XDP, BPF_XDP_DEVMAP, SEC_ATTACHABLE),
> SEC_DEF("xdp.frags/cpumap", XDP, BPF_XDP_CPUMAP, SEC_XDP_FRAGS),
> + SEC_DEF("xdp/cpumap", XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
> SEC_DEF("xdp_cpumap/", XDP, BPF_XDP_CPUMAP, SEC_ATTACHABLE),
> SEC_DEF("xdp.frags", XDP, BPF_XDP, SEC_XDP_FRAGS),
> SEC_DEF("xdp", XDP, BPF_XDP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
>
>
> >
> > > --Zvi
> > >
> > > > > "attach_place" is either function_name for fentry/, tp/, lsm/, etc.
> > > > > or attach_type/hook/target for cgroup/bind4, cgroup_skb/egress.
> > > > >
> > > > > lsm.s/socket_bind -> prog_type = LSM, flags = SLEEPABLE
> > > > > lsm/socket_bind -> prog_type = LSM, non sleepable.
> > > > >
> > > >
