[RFC,net-next,04/12] vhost_net: split out datacopy logic
diff mbox series

Message ID 1526893473-20128-5-git-send-email-jasowang@redhat.com
State New, archived
Headers show
Series
  • XDP batching for TUN/vhost_net
Related show

Commit Message

Jason Wang May 21, 2018, 9:04 a.m. UTC
Instead of mixing zerocopy and datacopy logics, this patch tries to
split datacopy logic out. This results for a more compact code and
specific optimization could be done on top more easily.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vhost/net.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 102 insertions(+), 9 deletions(-)

Comments

Jesse Brandeburg May 21, 2018, 4:46 p.m. UTC | #1
On Mon, 21 May 2018 17:04:25 +0800 Jason wrote:
> Instead of mixing zerocopy and datacopy logics, this patch tries to
> split datacopy logic out. This results for a more compact code and
> specific optimization could be done on top more easily.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/vhost/net.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 102 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 4ebac76..4682fcc 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -492,9 +492,95 @@ static bool vhost_has_more_pkts(struct vhost_net *net,
>  	       likely(!vhost_exceeds_maxpend(net));
>  }
>  
> +static void handle_tx_copy(struct vhost_net *net)
> +{
> +	struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
> +	struct vhost_virtqueue *vq = &nvq->vq;
> +	unsigned out, in;

move "out" and "in" down to inside the for loop as well.

> +	int head;

move this "head" declaration inside for-loop below.

> +	struct msghdr msg = {
> +		.msg_name = NULL,
> +		.msg_namelen = 0,
> +		.msg_control = NULL,
> +		.msg_controllen = 0,
> +		.msg_flags = MSG_DONTWAIT,
> +	};
> +	size_t len, total_len = 0;
> +	int err;
> +	size_t hdr_size;
> +	struct socket *sock;
> +	struct vhost_net_ubuf_ref *uninitialized_var(ubufs);
> +	int sent_pkts = 0;

why do we need so many locals?

> +
> +	mutex_lock(&vq->mutex);
> +	sock = vq->private_data;
> +	if (!sock)
> +		goto out;
> +
> +	if (!vq_iotlb_prefetch(vq))
> +		goto out;
> +
> +	vhost_disable_notify(&net->dev, vq);
> +	vhost_net_disable_vq(net, vq);
> +
> +	hdr_size = nvq->vhost_hlen;
> +
> +	for (;;) {
> +		head = vhost_net_tx_get_vq_desc(net, vq, vq->iov,
> +						ARRAY_SIZE(vq->iov),
> +						&out, &in);
> +		/* On error, stop handling until the next kick. */
> +		if (unlikely(head < 0))
> +			break;
> +		/* Nothing new?  Wait for eventfd to tell us they refilled. */
> +		if (head == vq->num) {
> +			if (unlikely(vhost_enable_notify(&net->dev, vq))) {
> +				vhost_disable_notify(&net->dev, vq);
> +				continue;
> +			}
> +			break;
> +		}
> +		if (in) {
> +			vq_err(vq, "Unexpected descriptor format for TX: "
> +			       "out %d, int %d\n", out, in);

don't break strings, keep all the bits between " " together, even if it
is longer than 80 chars.

> +			break;
> +		}
> +
> +		len = init_iov_iter(vq, &msg.msg_iter, hdr_size, out);
> +		if (len < 0)
> +			break;

same comment as previous patch, len is a size_t which is unsigned.

> +
> +		total_len += len;
> +		if (total_len < VHOST_NET_WEIGHT &&
> +		    vhost_has_more_pkts(net, vq)) {
> +			msg.msg_flags |= MSG_MORE;
> +		} else {
> +			msg.msg_flags &= ~MSG_MORE;
> +		}

don't need { } here.

> +
> +		/* TODO: Check specific error and bomb out unless ENOBUFS? */
> +		err = sock->ops->sendmsg(sock, &msg, len);
> +		if (unlikely(err < 0)) {
> +			vhost_discard_vq_desc(vq, 1);
> +			vhost_net_enable_vq(net, vq);
> +			break;
> +		}
> +		if (err != len)
> +			pr_debug("Truncated TX packet: "
> +				 " len %d != %zd\n", err, len);

single line " " string please
Jason Wang May 22, 2018, 12:39 p.m. UTC | #2
On 2018年05月22日 00:46, Jesse Brandeburg wrote:
> On Mon, 21 May 2018 17:04:25 +0800 Jason wrote:
>> Instead of mixing zerocopy and datacopy logics, this patch tries to
>> split datacopy logic out. This results for a more compact code and
>> specific optimization could be done on top more easily.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   drivers/vhost/net.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 102 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>> index 4ebac76..4682fcc 100644
>> --- a/drivers/vhost/net.c
>> +++ b/drivers/vhost/net.c
>> @@ -492,9 +492,95 @@ static bool vhost_has_more_pkts(struct vhost_net *net,
>>   	       likely(!vhost_exceeds_maxpend(net));
>>   }
>>   
>> +static void handle_tx_copy(struct vhost_net *net)
>> +{
>> +	struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
>> +	struct vhost_virtqueue *vq = &nvq->vq;
>> +	unsigned out, in;
> move "out" and "in" down to inside the for loop as well.
>
>> +	int head;
> move this "head" declaration inside for-loop below.

Will do these in next version, basically this function were just copied 
from handle_tx_zerocopy and remove unnecessary parts. So I'm thinking an 
independent patch from the beginning to avoid changes in two places.
>
>> +	struct msghdr msg = {
>> +		.msg_name = NULL,
>> +		.msg_namelen = 0,
>> +		.msg_control = NULL,
>> +		.msg_controllen = 0,
>> +		.msg_flags = MSG_DONTWAIT,
>> +	};
>> +	size_t len, total_len = 0;
>> +	int err;
>> +	size_t hdr_size;
>> +	struct socket *sock;
>> +	struct vhost_net_ubuf_ref *uninitialized_var(ubufs);
>> +	int sent_pkts = 0;
> why do we need so many locals?

So it looks to me ubufs is unnecessary but all other is really needed.

>
>> +
>> +	mutex_lock(&vq->mutex);
>> +	sock = vq->private_data;
>> +	if (!sock)
>> +		goto out;
>> +
>> +	if (!vq_iotlb_prefetch(vq))
>> +		goto out;
>> +
>> +	vhost_disable_notify(&net->dev, vq);
>> +	vhost_net_disable_vq(net, vq);
>> +
>> +	hdr_size = nvq->vhost_hlen;
>> +
>> +	for (;;) {
>> +		head = vhost_net_tx_get_vq_desc(net, vq, vq->iov,
>> +						ARRAY_SIZE(vq->iov),
>> +						&out, &in);
>> +		/* On error, stop handling until the next kick. */
>> +		if (unlikely(head < 0))
>> +			break;
>> +		/* Nothing new?  Wait for eventfd to tell us they refilled. */
>> +		if (head == vq->num) {
>> +			if (unlikely(vhost_enable_notify(&net->dev, vq))) {
>> +				vhost_disable_notify(&net->dev, vq);
>> +				continue;
>> +			}
>> +			break;
>> +		}
>> +		if (in) {
>> +			vq_err(vq, "Unexpected descriptor format for TX: "
>> +			       "out %d, int %d\n", out, in);
> don't break strings, keep all the bits between " " together, even if it
> is longer than 80 chars.

Ok.

>
>> +			break;
>> +		}
>> +
>> +		len = init_iov_iter(vq, &msg.msg_iter, hdr_size, out);
>> +		if (len < 0)
>> +			break;
> same comment as previous patch, len is a size_t which is unsigned.

Yes.

>
>> +
>> +		total_len += len;
>> +		if (total_len < VHOST_NET_WEIGHT &&
>> +		    vhost_has_more_pkts(net, vq)) {
>> +			msg.msg_flags |= MSG_MORE;
>> +		} else {
>> +			msg.msg_flags &= ~MSG_MORE;
>> +		}
> don't need { } here.

Right.

>> +
>> +		/* TODO: Check specific error and bomb out unless ENOBUFS? */
>> +		err = sock->ops->sendmsg(sock, &msg, len);
>> +		if (unlikely(err < 0)) {
>> +			vhost_discard_vq_desc(vq, 1);
>> +			vhost_net_enable_vq(net, vq);
>> +			break;
>> +		}
>> +		if (err != len)
>> +			pr_debug("Truncated TX packet: "
>> +				 " len %d != %zd\n", err, len);
> single line " " string please
>

Will fix.

Thanks

Patch
diff mbox series

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 4ebac76..4682fcc 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -492,9 +492,95 @@  static bool vhost_has_more_pkts(struct vhost_net *net,
 	       likely(!vhost_exceeds_maxpend(net));
 }
 
+static void handle_tx_copy(struct vhost_net *net)
+{
+	struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
+	struct vhost_virtqueue *vq = &nvq->vq;
+	unsigned out, in;
+	int head;
+	struct msghdr msg = {
+		.msg_name = NULL,
+		.msg_namelen = 0,
+		.msg_control = NULL,
+		.msg_controllen = 0,
+		.msg_flags = MSG_DONTWAIT,
+	};
+	size_t len, total_len = 0;
+	int err;
+	size_t hdr_size;
+	struct socket *sock;
+	struct vhost_net_ubuf_ref *uninitialized_var(ubufs);
+	int sent_pkts = 0;
+
+	mutex_lock(&vq->mutex);
+	sock = vq->private_data;
+	if (!sock)
+		goto out;
+
+	if (!vq_iotlb_prefetch(vq))
+		goto out;
+
+	vhost_disable_notify(&net->dev, vq);
+	vhost_net_disable_vq(net, vq);
+
+	hdr_size = nvq->vhost_hlen;
+
+	for (;;) {
+		head = vhost_net_tx_get_vq_desc(net, vq, vq->iov,
+						ARRAY_SIZE(vq->iov),
+						&out, &in);
+		/* On error, stop handling until the next kick. */
+		if (unlikely(head < 0))
+			break;
+		/* Nothing new?  Wait for eventfd to tell us they refilled. */
+		if (head == vq->num) {
+			if (unlikely(vhost_enable_notify(&net->dev, vq))) {
+				vhost_disable_notify(&net->dev, vq);
+				continue;
+			}
+			break;
+		}
+		if (in) {
+			vq_err(vq, "Unexpected descriptor format for TX: "
+			       "out %d, int %d\n", out, in);
+			break;
+		}
+
+		len = init_iov_iter(vq, &msg.msg_iter, hdr_size, out);
+		if (len < 0)
+			break;
+
+		total_len += len;
+		if (total_len < VHOST_NET_WEIGHT &&
+		    vhost_has_more_pkts(net, vq)) {
+			msg.msg_flags |= MSG_MORE;
+		} else {
+			msg.msg_flags &= ~MSG_MORE;
+		}
+
+		/* TODO: Check specific error and bomb out unless ENOBUFS? */
+		err = sock->ops->sendmsg(sock, &msg, len);
+		if (unlikely(err < 0)) {
+			vhost_discard_vq_desc(vq, 1);
+			vhost_net_enable_vq(net, vq);
+			break;
+		}
+		if (err != len)
+			pr_debug("Truncated TX packet: "
+				 " len %d != %zd\n", err, len);
+		vhost_add_used_and_signal(&net->dev, vq, head, 0);
+		if (vhost_exceeds_weight(++sent_pkts, total_len)) {
+			vhost_poll_queue(&vq->poll);
+			break;
+		}
+	}
+out:
+	mutex_unlock(&vq->mutex);
+}
+
 /* Expects to be always run from workqueue - which acts as
  * read-size critical section for our kind of RCU. */
-static void handle_tx(struct vhost_net *net)
+static void handle_tx_zerocopy(struct vhost_net *net)
 {
 	struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
 	struct vhost_virtqueue *vq = &nvq->vq;
@@ -512,7 +598,7 @@  static void handle_tx(struct vhost_net *net)
 	size_t hdr_size;
 	struct socket *sock;
 	struct vhost_net_ubuf_ref *uninitialized_var(ubufs);
-	bool zcopy, zcopy_used;
+	bool zcopy_used;
 	int sent_pkts = 0;
 
 	mutex_lock(&vq->mutex);
@@ -527,13 +613,10 @@  static void handle_tx(struct vhost_net *net)
 	vhost_net_disable_vq(net, vq);
 
 	hdr_size = nvq->vhost_hlen;
-	zcopy = nvq->ubufs;
 
 	for (;;) {
 		/* Release DMAs done buffers first */
-		if (zcopy)
-			vhost_zerocopy_signal_used(net, vq);
-
+		vhost_zerocopy_signal_used(net, vq);
 
 		head = vhost_net_tx_get_vq_desc(net, vq, vq->iov,
 						ARRAY_SIZE(vq->iov),
@@ -559,9 +642,9 @@  static void handle_tx(struct vhost_net *net)
 		if (len < 0)
 			break;
 
-		zcopy_used = zcopy && len >= VHOST_GOODCOPY_LEN
-				   && !vhost_exceeds_maxpend(net)
-				   && vhost_net_tx_select_zcopy(net);
+		zcopy_used = len >= VHOST_GOODCOPY_LEN
+			     && !vhost_exceeds_maxpend(net)
+			     && vhost_net_tx_select_zcopy(net);
 
 		/* use msg_control to pass vhost zerocopy ubuf info to skb */
 		if (zcopy_used) {
@@ -620,6 +703,16 @@  static void handle_tx(struct vhost_net *net)
 	mutex_unlock(&vq->mutex);
 }
 
+static void handle_tx(struct vhost_net *net)
+{
+	struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
+
+	if (nvq->ubufs)
+		handle_tx_zerocopy(net);
+	else
+		handle_tx_copy(net);
+}
+
 static int peek_head_len(struct vhost_net_virtqueue *rvq, struct sock *sk)
 {
 	struct sk_buff *head;