Re: [PATCH v6 bpf-next 4/9] veth: Handle xdp_frames in xdp napi ring

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

Context needed from: [PATCH v6 bpf-next 2/9] veth: Add driver XDP

On Mon, 30 Jul 2018 19:43:44 +0900
Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:

> +static struct sk_buff *veth_build_skb(void *head, int headroom, int len,
> +				      int buflen)
> +{
> +	struct sk_buff *skb;
> +
> +	if (!buflen) {
> +		buflen = SKB_DATA_ALIGN(headroom + len) +
> +			 SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> +	}
> +	skb = build_skb(head, buflen);
> +	if (!skb)
> +		return NULL;
> +
> +	skb_reserve(skb, headroom);
> +	skb_put(skb, len);
> +
> +	return skb;
> +}


On Mon, 30 Jul 2018 19:43:46 +0900
Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:

> +static struct sk_buff *veth_xdp_rcv_one(struct veth_priv *priv,
> +					struct xdp_frame *frame)
> +{
> +	int len = frame->len, delta = 0;
> +	struct bpf_prog *xdp_prog;
> +	unsigned int headroom;
> +	struct sk_buff *skb;
> +
> +	rcu_read_lock();
> +	xdp_prog = rcu_dereference(priv->xdp_prog);
> +	if (likely(xdp_prog)) {
> +		struct xdp_buff xdp;
> +		u32 act;
> +
> +		xdp.data_hard_start = frame->data - frame->headroom;
> +		xdp.data = frame->data;
> +		xdp.data_end = frame->data + frame->len;
> +		xdp.data_meta = frame->data - frame->metasize;
> +		xdp.rxq = &priv->xdp_rxq;
> +
> +		act = bpf_prog_run_xdp(xdp_prog, &xdp);
> +
> +		switch (act) {
> +		case XDP_PASS:
> +			delta = frame->data - xdp.data;
> +			len = xdp.data_end - xdp.data;
> +			break;
> +		default:
> +			bpf_warn_invalid_xdp_action(act);
> +		case XDP_ABORTED:
> +			trace_xdp_exception(priv->dev, xdp_prog, act);
> +		case XDP_DROP:
> +			goto err_xdp;
> +		}
> +	}
> +	rcu_read_unlock();
> +
> +	headroom = frame->data - delta - (void *)frame;
> +	skb = veth_build_skb(frame, headroom, len, 0);

Here you are adding an assumption that struct xdp_frame is always
located in-the-top of the packet-data area.  I tried hard not to add
such a dependency!  You can calculate the beginning of the frame from
the xdp_frame->data pointer.

Why not add such a dependency?  Because for AF_XDP zero-copy, we cannot
make such an assumption.  

Currently, when an RX-queue is in AF-XDP-ZC mode (MEM_TYPE_ZERO_COPY)
the packet will get dropped when calling convert_to_xdp_frame(), but as
the TODO comment indicated in convert_to_xdp_frame() this is not the
end-goal. 

The comment in convert_to_xdp_frame(), indicate we need a full
alloc+copy, but that is actually not necessary, if we can just use
another memory area for struct xdp_frame, and a pointer to data.  Thus,
allowing devmap-redir to work-ZC and allow cpumap-redir to do the copy
on the remote CPU.


> +	if (!skb) {
> +		xdp_return_frame(frame);
> +		goto err;
> +	}
> +
> +	memset(frame, 0, sizeof(*frame));
> +	skb->protocol = eth_type_trans(skb, priv->dev);
> +err:
> +	return skb;
> +err_xdp:
> +	rcu_read_unlock();
> +	xdp_return_frame(frame);
> +
> +	return NULL;
> +}


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