Re: [PATCH v2 17/18] vhost: don't bother copying iovecs in handle_rx(), kill memcpy_toiovecend()
On Tue, Feb 03, 2015 at 04:21:54PM +0100, Michael S. Tsirkin wrote: > > Hmm having second thoughts here. > > Will this modify the iov in vq->iov? > > If yes, how will recvmsg fill it? > > OK that was just me misunderstanding what the > function does. As it doesn't modify the iovec itself, > I think there's no issue, my ack stands. That, BTW, was the point of switching ->sendmsg() and ->recvmsg() to iov_iter primitives - not only do memcpy_toiovec() et.al. change the iovec, the way it's done was protocol-dependent, up to and including the things like "have sent 60 caller-supplied bytes, iovec modified with only 6 bytes consumed". What's worse, on TCP sendmsg we usually left iovec unchanged, but sometimes it got drained. Granted, that happened only after setsockopt() playing caller with TCP_REPAIR (i.e. checkpoint/restart stuff), but... -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 17/18] vhost: don't bother copying iovecs in handle_rx(), kill memcpy_toiovecend()
> Hmm having second thoughts here. > Will this modify the iov in vq->iov? > If yes, how will recvmsg fill it? OK that was just me misunderstanding what the function does. As it doesn't modify the iovec itself, I think there's no issue, my ack stands. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 17/18] vhost: don't bother copying iovecs in handle_rx(), kill memcpy_toiovecend()
On Mon, Feb 02, 2015 at 07:59:36AM +, Al Viro wrote:
> From: Al Viro
>
> Cc: Michael S. Tsirkin
> Cc: kvm@vger.kernel.org
> Signed-off-by: Al Viro
> ---
> drivers/vhost/net.c | 79
> ++---
> include/linux/uio.h | 3 --
> lib/iovec.c | 26 --
> 3 files changed, 20 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index d86cc9b..73c0ebf 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -84,10 +84,6 @@ struct vhost_net_ubuf_ref {
>
> struct vhost_net_virtqueue {
> struct vhost_virtqueue vq;
> - /* hdr is used to store the virtio header.
> - * Since each iovec has >= 1 byte length, we never need more than
> - * header length entries to store the header. */
> - struct iovec hdr[sizeof(struct virtio_net_hdr_mrg_rxbuf)];
> size_t vhost_hlen;
> size_t sock_hlen;
> /* vhost zerocopy support fields below: */
> @@ -235,44 +231,6 @@ static bool vhost_sock_zcopy(struct socket *sock)
> sock_flag(sock->sk, SOCK_ZEROCOPY);
> }
>
> -/* Pop first len bytes from iovec. Return number of segments used. */
> -static int move_iovec_hdr(struct iovec *from, struct iovec *to,
> - size_t len, int iov_count)
> -{
> - int seg = 0;
> - size_t size;
> -
> - while (len && seg < iov_count) {
> - size = min(from->iov_len, len);
> - to->iov_base = from->iov_base;
> - to->iov_len = size;
> - from->iov_len -= size;
> - from->iov_base += size;
> - len -= size;
> - ++from;
> - ++to;
> - ++seg;
> - }
> - return seg;
> -}
> -/* Copy iovec entries for len bytes from iovec. */
> -static void copy_iovec_hdr(const struct iovec *from, struct iovec *to,
> -size_t len, int iovcount)
> -{
> - int seg = 0;
> - size_t size;
> -
> - while (len && seg < iovcount) {
> - size = min(from->iov_len, len);
> - to->iov_base = from->iov_base;
> - to->iov_len = size;
> - len -= size;
> - ++from;
> - ++to;
> - ++seg;
> - }
> -}
> -
> /* In case of DMA done not in order in lower device driver for some reason.
> * upend_idx is used to track end of used idx, done_idx is used to track head
> * of used idx. Once lower device DMA done contiguously, we will signal KVM
> @@ -570,9 +528,9 @@ static void handle_rx(struct vhost_net *net)
> .msg_controllen = 0,
> .msg_flags = MSG_DONTWAIT,
> };
> - struct virtio_net_hdr_mrg_rxbuf hdr = {
> - .hdr.flags = 0,
> - .hdr.gso_type = VIRTIO_NET_HDR_GSO_NONE
> + struct virtio_net_hdr hdr = {
> + .flags = 0,
> + .gso_type = VIRTIO_NET_HDR_GSO_NONE
> };
> size_t total_len = 0;
> int err, mergeable;
> @@ -580,6 +538,7 @@ static void handle_rx(struct vhost_net *net)
> size_t vhost_hlen, sock_hlen;
> size_t vhost_len, sock_len;
> struct socket *sock;
> + struct iov_iter fixup;
>
> mutex_lock(&vq->mutex);
> sock = vq->private_data;
> @@ -624,14 +583,17 @@ static void handle_rx(struct vhost_net *net)
> break;
> }
> /* We don't need to be notified again. */
> - if (unlikely((vhost_hlen)))
> - /* Skip header. TODO: support TSO. */
> - move_iovec_hdr(vq->iov, nvq->hdr, vhost_hlen, in);
> - else
> - /* Copy the header for use in VIRTIO_NET_F_MRG_RXBUF:
> - * needed because recvmsg can modify msg_iov. */
> - copy_iovec_hdr(vq->iov, nvq->hdr, sock_hlen, in);
> - iov_iter_init(&msg.msg_iter, READ, vq->iov, in, sock_len);
> + iov_iter_init(&msg.msg_iter, READ, vq->iov, in, vhost_len);
> + fixup = msg.msg_iter;
> + if (unlikely((vhost_hlen))) {
> + /* We will supply the header ourselves
> + * TODO: support TSO. */
> + iov_iter_advance(&msg.msg_iter, vhost_hlen);
> + } else {
> + /* It'll come from socket; we'll need to patch
> + * ->num_buffers over if VIRTIO_NET_F_MRG_RXBUF */
> + iov_iter_advance(&fixup, sizeof(hdr));
> + }
> err = sock->ops->recvmsg(NULL, sock, &msg,
>sock_len, MSG_DONTWAIT | MSG_TRUNC);
> /* Userspace might have consumed the packet meanwhile:
Hmm having second thoughts here.
Will this modify the iov in vq->iov?
If yes, how will recvmsg fill it?
> @@ -643,18 +605,17 @@ static void handle_rx(struct vhost_net *net)
> vhost_discard_vq_desc(vq, headcount);
>
Re: [PATCH v2 17/18] vhost: don't bother copying iovecs in handle_rx(), kill memcpy_toiovecend()
On Mon, Feb 02, 2015 at 07:59:36AM +, Al Viro wrote:
> From: Al Viro
>
> Cc: Michael S. Tsirkin
> Cc: kvm@vger.kernel.org
> Signed-off-by: Al Viro
> ---
So this made me notice a bug in vhost introduced in 3.19.
I sent a patch for that, this one will have to be
rebased on top. Otherwise:
Acked-by: Michael S. Tsirkin
But, can you pls copy virtualizat...@lists.linux-foundation.org ?
I think some guys working on virtio might only hang out there.
> drivers/vhost/net.c | 79
> ++---
> include/linux/uio.h | 3 --
> lib/iovec.c | 26 --
> 3 files changed, 20 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index d86cc9b..73c0ebf 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -84,10 +84,6 @@ struct vhost_net_ubuf_ref {
>
> struct vhost_net_virtqueue {
> struct vhost_virtqueue vq;
> - /* hdr is used to store the virtio header.
> - * Since each iovec has >= 1 byte length, we never need more than
> - * header length entries to store the header. */
> - struct iovec hdr[sizeof(struct virtio_net_hdr_mrg_rxbuf)];
> size_t vhost_hlen;
> size_t sock_hlen;
> /* vhost zerocopy support fields below: */
> @@ -235,44 +231,6 @@ static bool vhost_sock_zcopy(struct socket *sock)
> sock_flag(sock->sk, SOCK_ZEROCOPY);
> }
>
> -/* Pop first len bytes from iovec. Return number of segments used. */
> -static int move_iovec_hdr(struct iovec *from, struct iovec *to,
> - size_t len, int iov_count)
> -{
> - int seg = 0;
> - size_t size;
> -
> - while (len && seg < iov_count) {
> - size = min(from->iov_len, len);
> - to->iov_base = from->iov_base;
> - to->iov_len = size;
> - from->iov_len -= size;
> - from->iov_base += size;
> - len -= size;
> - ++from;
> - ++to;
> - ++seg;
> - }
> - return seg;
> -}
> -/* Copy iovec entries for len bytes from iovec. */
> -static void copy_iovec_hdr(const struct iovec *from, struct iovec *to,
> -size_t len, int iovcount)
> -{
> - int seg = 0;
> - size_t size;
> -
> - while (len && seg < iovcount) {
> - size = min(from->iov_len, len);
> - to->iov_base = from->iov_base;
> - to->iov_len = size;
> - len -= size;
> - ++from;
> - ++to;
> - ++seg;
> - }
> -}
> -
> /* In case of DMA done not in order in lower device driver for some reason.
> * upend_idx is used to track end of used idx, done_idx is used to track head
> * of used idx. Once lower device DMA done contiguously, we will signal KVM
> @@ -570,9 +528,9 @@ static void handle_rx(struct vhost_net *net)
> .msg_controllen = 0,
> .msg_flags = MSG_DONTWAIT,
> };
> - struct virtio_net_hdr_mrg_rxbuf hdr = {
> - .hdr.flags = 0,
> - .hdr.gso_type = VIRTIO_NET_HDR_GSO_NONE
> + struct virtio_net_hdr hdr = {
> + .flags = 0,
> + .gso_type = VIRTIO_NET_HDR_GSO_NONE
> };
> size_t total_len = 0;
> int err, mergeable;
> @@ -580,6 +538,7 @@ static void handle_rx(struct vhost_net *net)
> size_t vhost_hlen, sock_hlen;
> size_t vhost_len, sock_len;
> struct socket *sock;
> + struct iov_iter fixup;
>
> mutex_lock(&vq->mutex);
> sock = vq->private_data;
> @@ -624,14 +583,17 @@ static void handle_rx(struct vhost_net *net)
> break;
> }
> /* We don't need to be notified again. */
> - if (unlikely((vhost_hlen)))
> - /* Skip header. TODO: support TSO. */
> - move_iovec_hdr(vq->iov, nvq->hdr, vhost_hlen, in);
> - else
> - /* Copy the header for use in VIRTIO_NET_F_MRG_RXBUF:
> - * needed because recvmsg can modify msg_iov. */
> - copy_iovec_hdr(vq->iov, nvq->hdr, sock_hlen, in);
> - iov_iter_init(&msg.msg_iter, READ, vq->iov, in, sock_len);
> + iov_iter_init(&msg.msg_iter, READ, vq->iov, in, vhost_len);
> + fixup = msg.msg_iter;
> + if (unlikely((vhost_hlen))) {
> + /* We will supply the header ourselves
> + * TODO: support TSO. */
> + iov_iter_advance(&msg.msg_iter, vhost_hlen);
> + } else {
> + /* It'll come from socket; we'll need to patch
> + * ->num_buffers over if VIRTIO_NET_F_MRG_RXBUF */
> + iov_iter_advance(&fixup, sizeof(hdr));
> + }
> err = sock->ops->recvmsg(NULL, sock, &msg,
>sock_len, MSG_DONTWAIT | MSG_TRUNC);
> /* U
