Re: [PATCH net-next 2/8] virtio_net: mergeable xdp: introduce mergeable_xdp_prepare

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 23 Mar 2023 12:45:41 +0800, Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
> On 2023/3/23 9:45, Xuan Zhuo wrote:
> > On Wed, 22 Mar 2023 19:52:48 +0800, Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
> >> On 2023/3/22 11:03, Xuan Zhuo wrote:
> >>> Separating the logic of preparation for xdp from receive_mergeable.
> >>>
> >>> The purpose of this is to simplify the logic of execution of XDP.
> >>>
> >>> The main logic here is that when headroom is insufficient, we need to
> >>> allocate a new page and calculate offset. It should be noted that if
> >>> there is new page, the variable page will refer to the new page.
> >>>
> >>> Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> >>> ---
> >>>  drivers/net/virtio_net.c | 135 ++++++++++++++++++++++-----------------
> >>>  1 file changed, 77 insertions(+), 58 deletions(-)
> >>>
> >>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >>> index 4d2bf1ce0730..bb426958cdd4 100644
> >>> --- a/drivers/net/virtio_net.c
> >>> +++ b/drivers/net/virtio_net.c
> >>> @@ -1162,6 +1162,79 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev,
> >>>  	return 0;
> >>>  }
> >>>
> >>> +static void *mergeable_xdp_prepare(struct virtnet_info *vi,
> >>> +				   struct receive_queue *rq,
> >>> +				   struct bpf_prog *xdp_prog,
> >>> +				   void *ctx,
> >>> +				   unsigned int *frame_sz,
> >>> +				   int *num_buf,
> >>> +				   struct page **page,
> >>> +				   int offset,
> >>> +				   unsigned int *len,
> >>> +				   struct virtio_net_hdr_mrg_rxbuf *hdr)
> >>
> >> The naming convention seems to be xdp_prepare_mergeable().
> >
> > What convention?
> >
> >
> >>
> >>> +{
> >>> +	unsigned int truesize = mergeable_ctx_to_truesize(ctx);
> >>> +	unsigned int headroom = mergeable_ctx_to_headroom(ctx);
> >>> +	struct page *xdp_page;
> >>> +	unsigned int xdp_room;
> >>> +
> >>> +	/* Transient failure which in theory could occur if
> >>> +	 * in-flight packets from before XDP was enabled reach
> >>> +	 * the receive path after XDP is loaded.
> >>> +	 */
> >>> +	if (unlikely(hdr->hdr.gso_type))
> >>> +		return NULL;
> >>> +
> >>> +	/* Now XDP core assumes frag size is PAGE_SIZE, but buffers
> >>> +	 * with headroom may add hole in truesize, which
> >>> +	 * make their length exceed PAGE_SIZE. So we disabled the
> >>> +	 * hole mechanism for xdp. See add_recvbuf_mergeable().
> >>> +	 */
> >>> +	*frame_sz = truesize;
> >>> +
> >>> +	/* This happens when headroom is not enough because
> >>> +	 * of the buffer was prefilled before XDP is set.
> >>> +	 * This should only happen for the first several packets.
> >>> +	 * In fact, vq reset can be used here to help us clean up
> >>> +	 * the prefilled buffers, but many existing devices do not
> >>> +	 * support it, and we don't want to bother users who are
> >>> +	 * using xdp normally.
> >>> +	 */
> >>> +	if (!xdp_prog->aux->xdp_has_frags &&
> >>> +	    (*num_buf > 1 || headroom < virtnet_get_headroom(vi))) {
> >>> +		/* linearize data for XDP */
> >>> +		xdp_page = xdp_linearize_page(rq, num_buf,
> >>> +					      *page, offset,
> >>> +					      VIRTIO_XDP_HEADROOM,
> >>> +					      len);
> >>> +
> >>> +		if (!xdp_page)
> >>> +			return NULL;
> >>> +	} else if (unlikely(headroom < virtnet_get_headroom(vi))) {
> >>> +		xdp_room = SKB_DATA_ALIGN(VIRTIO_XDP_HEADROOM +
> >>> +					  sizeof(struct skb_shared_info));
> >>> +		if (*len + xdp_room > PAGE_SIZE)
> >>> +			return NULL;
> >>> +
> >>> +		xdp_page = alloc_page(GFP_ATOMIC);
> >>> +		if (!xdp_page)
> >>> +			return NULL;
> >>> +
> >>> +		memcpy(page_address(xdp_page) + VIRTIO_XDP_HEADROOM,
> >>> +		       page_address(*page) + offset, *len);
> >>
> >> It seems the above 'else if' was not really tested even before this patch,
> >> as there is no "--*num_buf" if xdp_linearize_page() is not called, which
> >> may causes virtnet_build_xdp_buff_mrg() to comsume one more buffer than
> >> expected?
> >
> > Why do you think so?
>
>
> In first 'if' block, there is a "--*num_buf" before gotoing 'err_xdp'
> for virtqueue_get_buf() failure in xdp_linearize_page().
>
> But here there is no "--*num_buf" before gotoing 'err_xdp' for
> alloc_page() failure.

Inside err_xdp, we will get all bufs and free them util num_buf is 0.

Thanks.

>
> So one of them has to be wrong, right?
>
> >
> >>
> >> Also, it seems better to split the xdp_linearize_page() to two functions
> >> as pskb_expand_head() and __skb_linearize() do, one to expand the headroom,
> >> the other one to do the linearizing.
> >
> > No skb here.
>
> I means following the semantics of pskb_expand_head() and __skb_linearize(),
> not to combine the headroom expanding and linearizing into one function as
> xdp_linearize_page() does now if we want a better refoctor result.
>
> >
> >
> >>
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization



[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux