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@xxxxxxxxxx>
> ---
> 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
