Re: [PATCH RFC-v5 bpf-next 09/12] dev: Support xdp in the Tx path for xdp_frames

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Mon, 13 Apr 2020 11:17:58 -0600
David Ahern <dsahern@kernel.org> wrote:

> From: David Ahern <dahern@digitalocean.com>
> 
> Add support to run Tx path program on xdp_frames by adding a hook to
> bq_xmit_all before xdp_frames are passed to ndo_xdp_xmit for the device.
> 
> If an xdp_frame is dropped by the program, it is removed from the
> xdp_frames array with subsequent entries moved up.
> 
> Signed-off-by: David Ahern <dahern@digitalocean.com>
> ---
>  include/linux/netdevice.h |  3 ++
>  kernel/bpf/devmap.c       | 19 ++++++++---
>  net/core/dev.c            | 70 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 87 insertions(+), 5 deletions(-)
> 
[...]
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1bbaeb8842ed..f23dc6043329 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4720,6 +4720,76 @@ u32 do_xdp_egress_skb(struct net_device *dev, struct sk_buff *skb)
>  }
>  EXPORT_SYMBOL_GPL(do_xdp_egress_skb);
>  
> +static u32 __xdp_egress_frame(struct net_device *dev,
> +			      struct bpf_prog *xdp_prog,
> +			      struct xdp_frame *xdp_frame,
> +			      struct xdp_txq_info *txq)
> +{
> +	struct xdp_buff xdp;
> +	u32 act;
> +
> +	xdp.data_hard_start = xdp_frame->data - xdp_frame->headroom;

You also need: minus sizeof(*xdp_frame).

The BPF-helper xdp_adjust_head will not allow BPF-prog to access the
memory area that is used for xdp_frame, thus it still is safe.


> +	xdp.data = xdp_frame->data;
> +	xdp.data_end = xdp.data + xdp_frame->len;
> +	xdp_set_data_meta_invalid(&xdp);
> +	xdp.txq = txq;

I think this will be the 3rd place we convert xdp-frame to xdp_buff,
perhaps we should introduce a helper function call.

> +	act = bpf_prog_run_xdp(xdp_prog, &xdp);
> +	act = handle_xdp_egress_act(act, dev, xdp_prog);
> +
> +	/* if not dropping frame, readjust pointers in case
> +	 * program made changes to the buffer
> +	 */
> +	if (act != XDP_DROP) {
> +		int headroom = xdp.data - xdp.data_hard_start;
> +		int metasize = xdp.data - xdp.data_meta;
> +
> +		metasize = metasize > 0 ? metasize : 0;
> +		if (unlikely((headroom - metasize) < sizeof(*xdp_frame)))
> +			return XDP_DROP;
> +
> +		xdp_frame = xdp.data_hard_start;

Is this needed?

> +		xdp_frame->data = xdp.data;
> +		xdp_frame->len  = xdp.data_end - xdp.data;
> +		xdp_frame->headroom = headroom - sizeof(*xdp_frame);
> +		xdp_frame->metasize = metasize;
> +		/* xdp_frame->mem is unchanged */

This looks very similar to convert_to_xdp_frame.
Maybe we need an central update_xdp_frame(xdp_buff) call?


Untested code-up:

diff --git a/include/net/xdp.h b/include/net/xdp.h
index 40c6d3398458..180800c4e7d1 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -93,6 +93,24 @@ static inline void xdp_scrub_frame(struct xdp_frame *frame)
 
 struct xdp_frame *xdp_convert_zc_to_xdp_frame(struct xdp_buff *xdp);
 
+static inline
+bool update_xdp_frame(struct xdp_buff *xdp, struct xdp_frame *xdp_frame)
+{
+       /* Assure headroom is available for storing info */
+       headroom = xdp->data - xdp->data_hard_start;
+       metasize = xdp->data - xdp->data_meta;
+       metasize = metasize > 0 ? metasize : 0;
+       if (unlikely((headroom - metasize) < sizeof(*xdp_frame)))
+               return false;
+
+       xdp_frame->data = xdp->data;
+       xdp_frame->len  = xdp->data_end - xdp->data;
+       xdp_frame->headroom = headroom - sizeof(*xdp_frame);
+       xdp_frame->metasize = metasize;
+
+       return true;
+}
+
 /* Convert xdp_buff to xdp_frame */
 static inline
 struct xdp_frame *convert_to_xdp_frame(struct xdp_buff *xdp)
@@ -104,20 +122,11 @@ struct xdp_frame *convert_to_xdp_frame(struct xdp_buff *xdp)
        if (xdp->rxq->mem.type == MEM_TYPE_ZERO_COPY)
                return xdp_convert_zc_to_xdp_frame(xdp);
 
-       /* Assure headroom is available for storing info */
-       headroom = xdp->data - xdp->data_hard_start;
-       metasize = xdp->data - xdp->data_meta;
-       metasize = metasize > 0 ? metasize : 0;
-       if (unlikely((headroom - metasize) < sizeof(*xdp_frame)))
-               return NULL;
-
        /* Store info in top of packet */
        xdp_frame = xdp->data_hard_start;
 
-       xdp_frame->data = xdp->data;
-       xdp_frame->len  = xdp->data_end - xdp->data;
-       xdp_frame->headroom = headroom - sizeof(*xdp_frame);
-       xdp_frame->metasize = metasize;
+       if (unlikely(!update_xdp_frame(xdp, xdp_frame))
+           return NULL;
 
        /* rxq only valid until napi_schedule ends, convert to xdp_mem_info */
        xdp_frame->mem = xdp->rxq->mem;

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer