| Message ID | 269c70c6c529a09eb6d6b489eb9bf5e5513c943a.1635196496.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: introduce bpf_map_get_xdp_prog utility routine | expand |
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. 2. bpf_map_get_xdp_prog is a confusing name. what 'map' doing in there? 3. it's placed in core.c while it's really cpumap/devmap only. I suggest leaving the code as-is.
> 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.
On Tue, Nov 2, 2021 at 5:14 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > > 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. None of that information was in the original commit log. Please make sure to provide such details in the future and make it part of the series. That patch alone is unnecessary.
> On Tue, Nov 2, 2021 at 5:14 PM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > > > > 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. > > None of that information was in the original commit log. > Please make sure to provide such details in the future and make it > part of the series. > That patch alone is unnecessary. Yes, right. Sorry for the noise. Regarding this patch, do you want me to repost with a proper commit log (maybe included in the xdp multi-buff series) or do you prefer to just drop it? Regards, Lorenzo
On Wed, Nov 3, 2021 at 9:44 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > That patch alone is unnecessary. > > Yes, right. Sorry for the noise. > Regarding this patch, do you want me to repost with a proper commit log (maybe > included in the xdp multi-buff series) or do you prefer to just drop it? I think this patch alone is not necessary. When you'll get to the full series it could be meaningful. It's hard to tell without seeing the rest of the patches.
> On Wed, Nov 3, 2021 at 9:44 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > That patch alone is unnecessary. > > > > Yes, right. Sorry for the noise. > > Regarding this patch, do you want me to repost with a proper commit log (maybe > > included in the xdp multi-buff series) or do you prefer to just drop it? > > I think this patch alone is not necessary. > When you'll get to the full series it could be meaningful. > It's hard to tell without seeing the rest of the patches. ack, fine to me. I will drop it for the moment and then we will re-evaluate. Thanks. Regards, Lorenzo
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; +} + static void bpf_free_used_maps(struct bpf_prog_aux *aux) { __bpf_free_used_maps(aux, aux->used_maps, aux->used_map_cnt); diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c index 585b2b77ccc4..0b3e561e0c2a 100644 --- a/kernel/bpf/cpumap.c +++ b/kernel/bpf/cpumap.c @@ -397,19 +397,15 @@ static int cpu_map_kthread_run(void *data) return 0; } -static int __cpu_map_load_bpf_program(struct bpf_cpu_map_entry *rcpu, int fd) +static int __cpu_map_load_bpf_program(struct bpf_cpu_map_entry *rcpu, + struct bpf_map *map, int fd) { struct bpf_prog *prog; - prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_XDP); + prog = bpf_map_get_xdp_prog(map, fd, BPF_XDP_CPUMAP); if (IS_ERR(prog)) return PTR_ERR(prog); - if (prog->expected_attach_type != BPF_XDP_CPUMAP) { - bpf_prog_put(prog); - return -EINVAL; - } - rcpu->value.bpf_prog.id = prog->aux->id; rcpu->prog = prog; @@ -457,7 +453,7 @@ __cpu_map_entry_alloc(struct bpf_map *map, struct bpf_cpumap_val *value, rcpu->map_id = map->id; rcpu->value.qsize = value->qsize; - if (fd > 0 && __cpu_map_load_bpf_program(rcpu, fd)) + if (fd > 0 && __cpu_map_load_bpf_program(rcpu, map, fd)) goto free_ptr_ring; /* Setup kthread */ diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c index f02d04540c0c..59df0745f72d 100644 --- a/kernel/bpf/devmap.c +++ b/kernel/bpf/devmap.c @@ -864,12 +864,12 @@ static struct bpf_dtab_netdev *__dev_map_alloc_node(struct net *net, goto err_out; if (val->bpf_prog.fd > 0) { - prog = bpf_prog_get_type_dev(val->bpf_prog.fd, - BPF_PROG_TYPE_XDP, false); - if (IS_ERR(prog)) - goto err_put_dev; - if (prog->expected_attach_type != BPF_XDP_DEVMAP) - goto err_put_prog; + prog = bpf_map_get_xdp_prog(&dtab->map, val->bpf_prog.fd, + BPF_XDP_DEVMAP); + if (IS_ERR(prog)) { + dev_put(dev->dev); + goto err_out; + } } dev->idx = idx; @@ -884,10 +884,6 @@ static struct bpf_dtab_netdev *__dev_map_alloc_node(struct net *net, dev->val.ifindex = val->ifindex; return dev; -err_put_prog: - bpf_prog_put(prog); -err_put_dev: - dev_put(dev->dev); err_out: kfree(dev); return ERR_PTR(-EINVAL);
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(-)