On Fri, 9 Mar 2018 16:03:28 +0800
Jason Wang <jasow...@redhat.com> wrote:
> On 2018年03月08日 21:08, Jesper Dangaard Brouer wrote:
> > The virtio_net driver assumes XDP frames are always released based on
> > page refcnt (via put_page). Thus, is only queues the XDP data pointer
> > address and uses virt_to_head_page() to retrieve struct page.
> >
> > Use the XDP return API to get away from such assumptions. Instead
> > queue an xdp_frame, which allow us to use the xdp_return_frame API,
> > when releasing the frame.
> >
> > Signed-off-by: Jesper Dangaard Brouer <bro...@redhat.com>
> > ---
> > drivers/net/virtio_net.c | 31 +++++++++++++++++++++----------
> > 1 file changed, 21 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 23374603e4d9..6c4220450506 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -419,30 +419,41 @@ static bool __virtnet_xdp_xmit(struct virtnet_info
> > *vi,
> > struct xdp_buff *xdp)
> > {
> > struct virtio_net_hdr_mrg_rxbuf *hdr;
> > - unsigned int len;
> > + struct xdp_frame *xdpf, *xdpf_sent;
> > struct send_queue *sq;
> > + unsigned int len;
> > unsigned int qp;
> > - void *xdp_sent;
> > int err;
> >
> > qp = vi->curr_queue_pairs - vi->xdp_queue_pairs + smp_processor_id();
> > sq = &vi->sq[qp];
> >
> > /* Free up any pending old buffers before queueing new ones. */
> > - while ((xdp_sent = virtqueue_get_buf(sq->vq, &len)) != NULL) {
> > - struct page *sent_page = virt_to_head_page(xdp_sent);
> > + while ((xdpf_sent = virtqueue_get_buf(sq->vq, &len)) != NULL)
> > + xdp_return_frame(xdpf_sent->data, &xdpf_sent->mem);
> >
> > - put_page(sent_page);
> > - }
> > + xdpf = convert_to_xdp_frame(xdp);
> > + if (unlikely(!xdpf))
> > + return -EOVERFLOW;
> > +
> > + /* virtqueue want to use data area in-front of packet */
> > + if (unlikely(xdpf->metasize > 0))
> > + return -EOPNOTSUPP;
> > +
> > + if (unlikely(xdpf->headroom < vi->hdr_len))
> > + return -EOVERFLOW;
>
> I think this need another independent patch. For now we can simply drop
> the packet, but we probably need to think more to address it completely,
> allocate header part either dynamically or statically.
Okay, so we can followup later if we want to handle this case better
than drop.
> >
> > - xdp->data -= vi->hdr_len;
> > + /* Make room for virtqueue hdr (also change xdpf->headroom?) */
> > + xdpf->data -= vi->hdr_len;
> > /* Zero header and leave csum up to XDP layers */
> > - hdr = xdp->data;
> > + hdr = xdpf->data;
> > memset(hdr, 0, vi->hdr_len);
> > + hdr->hdr.hdr_len = xdpf->len; /* Q: is this needed? */
>
> Maybe another patch but not a must, hdr_len is hint for the linear part
> of skb used in host. Backend implementation may simply ignore this value.
So, I should leave it out for now?
Or it is okay to keep it?
> > + xdpf->len += vi->hdr_len;
> >
> > - sg_init_one(sq->sg, xdp->data, xdp->data_end - xdp->data);
> > + sg_init_one(sq->sg, xdpf->data, xdpf->len);
When _later_ introducing bulking, can we use something else than
sg_init_one() to send/queue multiple raw XDP frames for sending?
(I'm asking as I don't know this "sg_*" API usage)
> > - err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdp->data,
> > GFP_ATOMIC);
> > + err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdpf,
> > GFP_ATOMIC); if (unlikely(err))
> > return false; /* Caller handle free/refcnt */
> >
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
