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.
>
> >
> >
> >>
