Re: [PATCH bpf-next] bpf: introduce bpf_map_get_xdp_prog utility routine

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2959 bytes --]

> On Mon, Oct 25, 2021 at 2:18 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >
> > Introduce bpf_map_get_xdp_prog to load an eBPF program on
> > CPUMAP/DEVMAP entries since both of them share the same code.
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  include/linux/bpf.h |  2 ++
> >  kernel/bpf/core.c   | 17 +++++++++++++++++
> >  kernel/bpf/cpumap.c | 12 ++++--------
> >  kernel/bpf/devmap.c | 16 ++++++----------
> >  4 files changed, 29 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 26bf8c865103..891936b54b55 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1910,6 +1910,8 @@ static inline struct bpf_prog *bpf_prog_get_type(u32 ufd,
> >         return bpf_prog_get_type_dev(ufd, type, false);
> >  }
> >
> > +struct bpf_prog *bpf_map_get_xdp_prog(struct bpf_map *map, int fd,
> > +                                     enum bpf_attach_type attach_type);
> >  void __bpf_free_used_maps(struct bpf_prog_aux *aux,
> >                           struct bpf_map **used_maps, u32 len);
> >
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index dee91a2eea7b..7e72c21b6589 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -2228,6 +2228,23 @@ void __bpf_free_used_maps(struct bpf_prog_aux *aux,
> >         }
> >  }
> >
> > +struct bpf_prog *bpf_map_get_xdp_prog(struct bpf_map *map, int fd,
> > +                                     enum bpf_attach_type attach_type)
> > +{
> > +       struct bpf_prog *prog;
> > +
> > +       prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_XDP);
> > +       if (IS_ERR(prog))
> > +               return prog;
> > +
> > +       if (prog->expected_attach_type != attach_type) {
> > +               bpf_prog_put(prog);
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       return prog;
> > +}
> 
> It is supposed to be a cleanup... but...
> 
> 1. it's tweaking __cpu_map_load_bpf_program()
> to pass extra 'map' argument further into this helper,
> but the 'map' is unused.

For xdp multi-buff we will need to extend Toke's bpf_prog_map_compatible fix
running bpf_prog_map_compatible routine for cpumaps and devmaps in
order to avoid mixing xdp mb and xdp legacy programs in a cpumaps or devmaps.
For this reason I guess we will need to pass map pointer to
__cpu_map_load_bpf_program anyway.
I do not have a strong opinion on it, but the main idea here is just to have a
common code and avoid adding the same changes to cpumap and devmap.
Anyway if you prefer to do it separately for cpumap  and devmap I am fine
with it.

> 
> 2. bpf_map_get_xdp_prog is a confusing name. what 'map' doing in there?

maybe bpf_xdp_map_load_prog? (naming is always hard :))

Regards,
Lorenzo

> 
> 3. it's placed in core.c while it's really cpumap/devmap only.
> 
> I suggest leaving the code as-is.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]