Patchwork [V2,4/6] vhost_net: determine whether or not to use zerocopy at one time

login
register
mail settings
Submitter Jason Wang
Date Aug. 30, 2013, 4:29 a.m.
Message ID <1377836962-49780-5-git-send-email-jasowang@redhat.com>
Download mbox | patch
Permalink /patch/271100/
State Superseded
Delegated to: David Miller
Headers show

Comments

Jason Wang - Aug. 30, 2013, 4:29 a.m.
Currently, even if the packet length is smaller than VHOST_GOODCOPY_LEN, if
upend_idx != done_idx we still set zcopy_used to true and rollback this choice
later. This could be avoided by determine zerocopy once by checking all
conditions at one time before.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vhost/net.c |   46 +++++++++++++++++++---------------------------
 1 files changed, 19 insertions(+), 27 deletions(-)
Sergei Shtylyov - Aug. 30, 2013, 6:35 p.m.
Hello.

On 08/30/2013 08:29 AM, Jason Wang wrote:

> Currently, even if the packet length is smaller than VHOST_GOODCOPY_LEN, if
> upend_idx != done_idx we still set zcopy_used to true and rollback this choice
> later. This could be avoided by determine zerocopy once by checking all
> conditions at one time before.

> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>   drivers/vhost/net.c |   46 +++++++++++++++++++---------------------------
>   1 files changed, 19 insertions(+), 27 deletions(-)

> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 8a6dd0d..ff60c2a 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -404,43 +404,35 @@ static void handle_tx(struct vhost_net *net)
>   			       iov_length(nvq->hdr, s), hdr_size);
>   			break;
>   		}
> -		zcopy_used = zcopy && (len >= VHOST_GOODCOPY_LEN ||
> -				       nvq->upend_idx != nvq->done_idx);
> +
> +		zcopy_used = zcopy && len >= VHOST_GOODCOPY_LEN
> +			&& (nvq->upend_idx + 1) % UIO_MAXIOV != nvq->done_idx
> +			&& vhost_net_tx_select_zcopy(net);

    Could you leave && on a first of two lines, matching the previous style?

>
>   		/* use msg_control to pass vhost zerocopy ubuf info to skb */
>   		if (zcopy_used) {
> +			struct ubuf_info *ubuf;
> +			ubuf = nvq->ubuf_info + nvq->upend_idx;
> +
>   			vq->heads[nvq->upend_idx].id = head;
[...]
> +			vq->heads[nvq->upend_idx].len = VHOST_DMA_IN_PROGRESS;
> +			ubuf->callback = vhost_zerocopy_callback;
> +			ubuf->ctx = nvq->ubufs;
> +			ubuf->desc = nvq->upend_idx;
> +			msg.msg_control = ubuf;
> +			msg.msg_controllen = sizeof(ubuf);

    'sizeof(ubuf)' where 'ubuf' is a pointer? Are you sure it shouldn't be 
'sizeof(*ubuf)'?

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Wang - Sept. 2, 2013, 3:15 a.m.
On 08/31/2013 02:35 AM, Sergei Shtylyov wrote:
> Hello.
>
> On 08/30/2013 08:29 AM, Jason Wang wrote:
>
>> Currently, even if the packet length is smaller than
>> VHOST_GOODCOPY_LEN, if
>> upend_idx != done_idx we still set zcopy_used to true and rollback
>> this choice
>> later. This could be avoided by determine zerocopy once by checking all
>> conditions at one time before.
>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   drivers/vhost/net.c |   46
>> +++++++++++++++++++---------------------------
>>   1 files changed, 19 insertions(+), 27 deletions(-)
>
>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>> index 8a6dd0d..ff60c2a 100644
>> --- a/drivers/vhost/net.c
>> +++ b/drivers/vhost/net.c
>> @@ -404,43 +404,35 @@ static void handle_tx(struct vhost_net *net)
>>                      iov_length(nvq->hdr, s), hdr_size);
>>               break;
>>           }
>> -        zcopy_used = zcopy && (len >= VHOST_GOODCOPY_LEN ||
>> -                       nvq->upend_idx != nvq->done_idx);
>> +
>> +        zcopy_used = zcopy && len >= VHOST_GOODCOPY_LEN
>> +            && (nvq->upend_idx + 1) % UIO_MAXIOV != nvq->done_idx
>> +            && vhost_net_tx_select_zcopy(net);
>
>    Could you leave && on a first of two lines, matching the previous
> style?
>

ok.
>>
>>           /* use msg_control to pass vhost zerocopy ubuf info to skb */
>>           if (zcopy_used) {
>> +            struct ubuf_info *ubuf;
>> +            ubuf = nvq->ubuf_info + nvq->upend_idx;
>> +
>>               vq->heads[nvq->upend_idx].id = head;
> [...]
>> +            vq->heads[nvq->upend_idx].len = VHOST_DMA_IN_PROGRESS;
>> +            ubuf->callback = vhost_zerocopy_callback;
>> +            ubuf->ctx = nvq->ubufs;
>> +            ubuf->desc = nvq->upend_idx;
>> +            msg.msg_control = ubuf;
>> +            msg.msg_controllen = sizeof(ubuf);
>
>    'sizeof(ubuf)' where 'ubuf' is a pointer? Are you sure it shouldn't
> be 'sizeof(*ubuf)'?

Yes, pointer is sufficiet. Vhost allocate an arrays of ubuf and
tun/macvtap just need a reference of it.
>
> WBR, Sergei
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 8a6dd0d..ff60c2a 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -404,43 +404,35 @@  static void handle_tx(struct vhost_net *net)
 			       iov_length(nvq->hdr, s), hdr_size);
 			break;
 		}
-		zcopy_used = zcopy && (len >= VHOST_GOODCOPY_LEN ||
-				       nvq->upend_idx != nvq->done_idx);
+
+		zcopy_used = zcopy && len >= VHOST_GOODCOPY_LEN
+			&& (nvq->upend_idx + 1) % UIO_MAXIOV != nvq->done_idx
+			&& vhost_net_tx_select_zcopy(net);
 
 		/* use msg_control to pass vhost zerocopy ubuf info to skb */
 		if (zcopy_used) {
+			struct ubuf_info *ubuf;
+			ubuf = nvq->ubuf_info + nvq->upend_idx;
+
 			vq->heads[nvq->upend_idx].id = head;
-			if (!vhost_net_tx_select_zcopy(net) ||
-			    len < VHOST_GOODCOPY_LEN) {
-				/* copy don't need to wait for DMA done */
-				vq->heads[nvq->upend_idx].len =
-							VHOST_DMA_DONE_LEN;
-				msg.msg_control = NULL;
-				msg.msg_controllen = 0;
-				ubufs = NULL;
-			} else {
-				struct ubuf_info *ubuf;
-				ubuf = nvq->ubuf_info + nvq->upend_idx;
-
-				vq->heads[nvq->upend_idx].len =
-					VHOST_DMA_IN_PROGRESS;
-				ubuf->callback = vhost_zerocopy_callback;
-				ubuf->ctx = nvq->ubufs;
-				ubuf->desc = nvq->upend_idx;
-				msg.msg_control = ubuf;
-				msg.msg_controllen = sizeof(ubuf);
-				ubufs = nvq->ubufs;
-				kref_get(&ubufs->kref);
-			}
+			vq->heads[nvq->upend_idx].len = VHOST_DMA_IN_PROGRESS;
+			ubuf->callback = vhost_zerocopy_callback;
+			ubuf->ctx = nvq->ubufs;
+			ubuf->desc = nvq->upend_idx;
+			msg.msg_control = ubuf;
+			msg.msg_controllen = sizeof(ubuf);
+			ubufs = nvq->ubufs;
+			kref_get(&ubufs->kref);
 			nvq->upend_idx = (nvq->upend_idx + 1) % UIO_MAXIOV;
-		} else
+		} else {
 			msg.msg_control = NULL;
+			ubufs = NULL;
+		}
 		/* TODO: Check specific error and bomb out unless ENOBUFS? */
 		err = sock->ops->sendmsg(NULL, sock, &msg, len);
 		if (unlikely(err < 0)) {
 			if (zcopy_used) {
-				if (ubufs)
-					vhost_net_ubuf_put(ubufs);
+				vhost_net_ubuf_put(ubufs);
 				nvq->upend_idx = ((unsigned)nvq->upend_idx - 1)
 					% UIO_MAXIOV;
 			}