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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

David Ahern <dsahern@kernel.org> writes:

> 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/include/linux/netdevice.h b/include/linux/netdevice.h
> index 39e1b42c042f..d75e31ac2751 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -3715,6 +3715,9 @@ static inline void dev_consume_skb_any(struct sk_buff *skb)
>  void generic_xdp_tx(struct sk_buff *skb, struct bpf_prog *xdp_prog);
>  int do_xdp_generic_rx(struct bpf_prog *xdp_prog, struct sk_buff *skb);
>  u32 do_xdp_egress_skb(struct net_device *dev, struct sk_buff *skb);
> +unsigned int do_xdp_egress_frame(struct net_device *dev,
> +				 struct xdp_frame **frames,
> +				 unsigned int *pcount);
>  int netif_rx(struct sk_buff *skb);
>  int netif_rx_ni(struct sk_buff *skb);
>  int netif_receive_skb(struct sk_buff *skb);
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index 58bdca5d978a..bedecd07d898 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -322,24 +322,33 @@ static int bq_xmit_all(struct xdp_dev_bulk_queue *bq, u32 flags)
>  {
>  	struct net_device *dev = bq->dev;
>  	int sent = 0, drops = 0, err = 0;
> +	unsigned int count = bq->count;
>  	int i;
>  
> -	if (unlikely(!bq->count))
> +	if (unlikely(!count))
>  		return 0;
>  
> -	for (i = 0; i < bq->count; i++) {
> +	for (i = 0; i < count; i++) {
>  		struct xdp_frame *xdpf = bq->q[i];
>  
>  		prefetch(xdpf);
>  	}
>  
> -	sent = dev->netdev_ops->ndo_xdp_xmit(dev, bq->count, bq->q, flags);
> +	if (static_branch_unlikely(&xdp_egress_needed_key)) {
> +		count = do_xdp_egress_frame(dev, bq->q, &count);

nit: seems a bit odd to pass the point to count, then reassign it with
the return value?

> +		drops += bq->count - count;
> +		/* all frames consumed by the xdp program? */
> +		if (!count)
> +			goto out;
> +	}
> +
> +	sent = dev->netdev_ops->ndo_xdp_xmit(dev, count, bq->q, flags);
>  	if (sent < 0) {
>  		err = sent;
>  		sent = 0;
>  		goto error;
>  	}
> -	drops = bq->count - sent;
> +	drops += count - sent;
>  out:
>  	bq->count = 0;
>  
> @@ -351,7 +360,7 @@ static int bq_xmit_all(struct xdp_dev_bulk_queue *bq, u32 flags)
>  	/* If ndo_xdp_xmit fails with an errno, no frames have been
>  	 * xmit'ed and it's our responsibility to them free all.
>  	 */
> -	for (i = 0; i < bq->count; i++) {
> +	for (i = 0; i < count; i++) {
>  		struct xdp_frame *xdpf = bq->q[i];
>  
>  		xdp_return_frame_rx_napi(xdpf);
> 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;
> +	xdp.data = xdp_frame->data;
> +	xdp.data_end = xdp.data + xdp_frame->len;
> +	xdp_set_data_meta_invalid(&xdp);

Why invalidate the metadata? On the contrary we'd want metadata from the
RX side to survive, wouldn't we?

> +	xdp.txq = txq;
> +
> +	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;
> +		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 */
> +	}
> +
> +	return act;
> +}
> +
> +unsigned int do_xdp_egress_frame(struct net_device *dev,
> +				 struct xdp_frame **frames,
> +				 unsigned int *pcount)
> +{
> +	struct bpf_prog *xdp_prog;
> +	unsigned int count = *pcount;
> +
> +	xdp_prog = rcu_dereference(dev->xdp_egress_prog);
> +	if (xdp_prog) {
> +		struct xdp_txq_info txq = { .dev = dev };

Do you have any thoughts on how to populate this for the redirect case?
I guess using Magnus' HWQ abstraction when that lands? Or did you have
something different in mind?

> +		unsigned int i, j;
> +		u32 act;
> +
> +		for (i = 0, j = 0; i < count; i++) {
> +			struct xdp_frame *frame = frames[i];
> +
> +			act = __xdp_egress_frame(dev, xdp_prog, frame, &txq);
> +			if (act == XDP_DROP) {
> +				xdp_return_frame_rx_napi(frame);
> +				continue;
> +			}
> +
> +			frames[j] = frame;
> +			j++;
> +		}
> +		count = j;
> +	}
> +
> +	return count;
> +}
> +
>  static int netif_rx_internal(struct sk_buff *skb)
>  {
>  	int ret;
> -- 
> 2.21.1 (Apple Git-122.3)