| Message ID | 20211202000232.380824-6-toke@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Add support for transmitting packets using XDP in bpf_prog_run() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 3278 this patch: 3278 |
| netdev/cc_maintainers | success | CCed 13 of 13 maintainers |
| netdev/build_clang | success | Errors and warnings before: 437 this patch: 437 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 3416 this patch: 3416 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 63 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next | success | VM_Test |
Toke Høiland-Jørgensen wrote: > Add an xdp_do_redirect_frame() variant which supports pre-computed > xdp_frame structures. This will be used in bpf_prog_run() to avoid having > to write to the xdp_frame structure when the XDP program doesn't modify the > frame boundaries. > > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- > include/linux/filter.h | 4 ++++ > net/core/filter.c | 28 +++++++++++++++++++++------- > 2 files changed, 25 insertions(+), 7 deletions(-) > > diff --git a/include/linux/filter.h b/include/linux/filter.h > index b6a216eb217a..845452c83e0f 100644 > --- a/include/linux/filter.h > +++ b/include/linux/filter.h > @@ -1022,6 +1022,10 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb, > int xdp_do_redirect(struct net_device *dev, > struct xdp_buff *xdp, > struct bpf_prog *prog); > +int xdp_do_redirect_frame(struct net_device *dev, > + struct xdp_buff *xdp, > + struct xdp_frame *xdpf, > + struct bpf_prog *prog); I don't really like that we are passing both the xdp_buff ptr and xdp_frame *xdpf around when one is always null it looks like? > void xdp_do_flush(void); > > /* The xdp_do_flush_map() helper has been renamed to drop the _map suffix, as > diff --git a/net/core/filter.c b/net/core/filter.c > index 1e86130a913a..d8fe74cc8b66 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -3957,14 +3957,13 @@ u32 xdp_master_redirect(struct xdp_buff *xdp) > } > EXPORT_SYMBOL_GPL(xdp_master_redirect); > > -int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp, > - struct bpf_prog *xdp_prog) > +static int __xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp, > + struct xdp_frame *xdpf, struct bpf_prog *xdp_prog) > { > struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info); > enum bpf_map_type map_type = ri->map_type; > void *fwd = ri->tgt_value; > u32 map_id = ri->map_id; > - struct xdp_frame *xdpf; > struct bpf_map *map; > int err; > > @@ -3976,10 +3975,12 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp, > goto out; > } > > - xdpf = xdp_convert_buff_to_frame(xdp); > - if (unlikely(!xdpf)) { > - err = -EOVERFLOW; > - goto err; > + if (!xdpf) { > + xdpf = xdp_convert_buff_to_frame(xdp); > + if (unlikely(!xdpf)) { > + err = -EOVERFLOW; > + goto err; > + } This is a bit ugly imo. Can we just decide what gets handed to the function rather than having this mid function conversion? If we can't get consistency at least a xdpf_do_redirect() and then make a xdp_do_redirect( return xdpf_do_redirect(xdp_convert_buff_to_frame(xdp))) that just does the conversion and passes it through. Or did I miss something? > } > > switch (map_type) { > @@ -4024,8 +4025,21 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp, > _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map_type, map_id, ri->tgt_index, err); > return err; > } > + > +int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp, > + struct bpf_prog *xdp_prog) > +{ > + return __xdp_do_redirect(dev, xdp, NULL, xdp_prog); same here. Just do the conversion and call, __xdpf_do_redirect(dev, xdpf, xdp_prog) skipping the null pointr? > +} > EXPORT_SYMBOL_GPL(xdp_do_redirect); > > +int xdp_do_redirect_frame(struct net_device *dev, struct xdp_buff *xdp, > + struct xdp_frame *xdpf, struct bpf_prog *xdp_prog) > +{ > + return __xdp_do_redirect(dev, xdp, xdpf, xdp_prog); > +} > +EXPORT_SYMBOL_GPL(xdp_do_redirect_frame); > + > static int xdp_do_generic_redirect_map(struct net_device *dev, > struct sk_buff *skb, > struct xdp_buff *xdp, > -- > 2.34.0 > Thanks, John
John Fastabend <john.fastabend@gmail.com> writes: > Toke Høiland-Jørgensen wrote: >> Add an xdp_do_redirect_frame() variant which supports pre-computed >> xdp_frame structures. This will be used in bpf_prog_run() to avoid having >> to write to the xdp_frame structure when the XDP program doesn't modify the >> frame boundaries. >> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> >> --- >> include/linux/filter.h | 4 ++++ >> net/core/filter.c | 28 +++++++++++++++++++++------- >> 2 files changed, 25 insertions(+), 7 deletions(-) >> >> diff --git a/include/linux/filter.h b/include/linux/filter.h >> index b6a216eb217a..845452c83e0f 100644 >> --- a/include/linux/filter.h >> +++ b/include/linux/filter.h >> @@ -1022,6 +1022,10 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb, >> int xdp_do_redirect(struct net_device *dev, >> struct xdp_buff *xdp, >> struct bpf_prog *prog); >> +int xdp_do_redirect_frame(struct net_device *dev, >> + struct xdp_buff *xdp, >> + struct xdp_frame *xdpf, >> + struct bpf_prog *prog); > > I don't really like that we are passing both the xdp_buff ptr and > xdp_frame *xdpf around when one is always null it looks like? Yeah, the problem is basically that AF_XDP uses xdp_buff all the way through, so we can't pass xdp_frame to that. I do agree that it's a bit ugly, though; maybe we can just do the XSK disambiguation in the caller; will take another look at this - thanks! -Toke
diff --git a/include/linux/filter.h b/include/linux/filter.h index b6a216eb217a..845452c83e0f 100644 --- a/include/linux/filter.h +++ b/include/linux/filter.h @@ -1022,6 +1022,10 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb, int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp, struct bpf_prog *prog); +int xdp_do_redirect_frame(struct net_device *dev, + struct xdp_buff *xdp, + struct xdp_frame *xdpf, + struct bpf_prog *prog); void xdp_do_flush(void); /* The xdp_do_flush_map() helper has been renamed to drop the _map suffix, as diff --git a/net/core/filter.c b/net/core/filter.c index 1e86130a913a..d8fe74cc8b66 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -3957,14 +3957,13 @@ u32 xdp_master_redirect(struct xdp_buff *xdp) } EXPORT_SYMBOL_GPL(xdp_master_redirect); -int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp, - struct bpf_prog *xdp_prog) +static int __xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp, + struct xdp_frame *xdpf, struct bpf_prog *xdp_prog) { struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info); enum bpf_map_type map_type = ri->map_type; void *fwd = ri->tgt_value; u32 map_id = ri->map_id; - struct xdp_frame *xdpf; struct bpf_map *map; int err; @@ -3976,10 +3975,12 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp, goto out; } - xdpf = xdp_convert_buff_to_frame(xdp); - if (unlikely(!xdpf)) { - err = -EOVERFLOW; - goto err; + if (!xdpf) { + xdpf = xdp_convert_buff_to_frame(xdp); + if (unlikely(!xdpf)) { + err = -EOVERFLOW; + goto err; + } } switch (map_type) { @@ -4024,8 +4025,21 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp, _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map_type, map_id, ri->tgt_index, err); return err; } + +int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp, + struct bpf_prog *xdp_prog) +{ + return __xdp_do_redirect(dev, xdp, NULL, xdp_prog); +} EXPORT_SYMBOL_GPL(xdp_do_redirect); +int xdp_do_redirect_frame(struct net_device *dev, struct xdp_buff *xdp, + struct xdp_frame *xdpf, struct bpf_prog *xdp_prog) +{ + return __xdp_do_redirect(dev, xdp, xdpf, xdp_prog); +} +EXPORT_SYMBOL_GPL(xdp_do_redirect_frame); + static int xdp_do_generic_redirect_map(struct net_device *dev, struct sk_buff *skb, struct xdp_buff *xdp,
Add an xdp_do_redirect_frame() variant which supports pre-computed xdp_frame structures. This will be used in bpf_prog_run() to avoid having to write to the xdp_frame structure when the XDP program doesn't modify the frame boundaries. Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> --- include/linux/filter.h | 4 ++++ net/core/filter.c | 28 +++++++++++++++++++++------- 2 files changed, 25 insertions(+), 7 deletions(-)