From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752004AbdI1Hu1 (ORCPT ); Thu, 28 Sep 2017 03:50:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47502 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751592AbdI1HuZ (ORCPT ); Thu, 28 Sep 2017 03:50:25 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 2E5BCA058A Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=jasowang@redhat.com Subject: Re: [PATCH net-next RFC 5/5] vhost_net: basic tx virtqueue batched processing To: Willem de Bruijn Cc: "Michael S. Tsirkin" , virtualization@lists.linux-foundation.org, Network Development , LKML , kvm@vger.kernel.org References: <1506067355-5771-1-git-send-email-jasowang@redhat.com> <1506067355-5771-6-git-send-email-jasowang@redhat.com> From: Jason Wang Message-ID: Date: Thu, 28 Sep 2017 15:50:14 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Thu, 28 Sep 2017 07:50:25 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2017年09月28日 08:55, Willem de Bruijn wrote: >> @@ -461,6 +460,7 @@ static void handle_tx(struct vhost_net *net) >> struct socket *sock; >> struct vhost_net_ubuf_ref *uninitialized_var(ubufs); >> bool zcopy, zcopy_used; >> + int i, batched = VHOST_NET_BATCH; >> >> mutex_lock(&vq->mutex); >> sock = vq->private_data; >> @@ -475,6 +475,12 @@ static void handle_tx(struct vhost_net *net) >> hdr_size = nvq->vhost_hlen; >> zcopy = nvq->ubufs; >> >> + /* Disable zerocopy batched fetching for simplicity */ > This special case can perhaps be avoided if we no longer block > on vhost_exceeds_maxpend, but revert to copying. Yes, I think so. For simplicity, I do it for data copy first. If the idea is convinced, I will try to do zerocopy on top. > >> + if (zcopy) { >> + heads = &used; > Can this special case of batchsize 1 not use vq->heads? It doesn't in fact? > >> + batched = 1; >> + } >> + >> for (;;) { >> /* Release DMAs done buffers first */ >> if (zcopy) >> @@ -486,95 +492,114 @@ static void handle_tx(struct vhost_net *net) >> if (unlikely(vhost_exceeds_maxpend(net))) >> break; >> + /* TODO: Check specific error and bomb out >> + * unless ENOBUFS? >> + */ >> + err = sock->ops->sendmsg(sock, &msg, len); >> + if (unlikely(err < 0)) { >> + if (zcopy_used) { >> + vhost_net_ubuf_put(ubufs); >> + nvq->upend_idx = >> + ((unsigned)nvq->upend_idx - 1) % UIO_MAXIOV; >> + } >> + vhost_discard_vq_desc(vq, 1); >> + goto out; >> + } >> + if (err != len) >> + pr_debug("Truncated TX packet: " >> + " len %d != %zd\n", err, len); >> + if (!zcopy) { >> + vhost_add_used_idx(vq, 1); >> + vhost_signal(&net->dev, vq); >> + } else if (!zcopy_used) { >> + vhost_add_used_and_signal(&net->dev, >> + vq, head, 0); > While batching, perhaps can also move this producer index update > out of the loop and using vhost_add_used_and_signal_n. Yes. > >> + } else >> + vhost_zerocopy_signal_used(net, vq); >> + vhost_net_tx_packet(net); >> + if (unlikely(total_len >= VHOST_NET_WEIGHT)) { >> + vhost_poll_queue(&vq->poll); >> + goto out; >> } >> - vhost_discard_vq_desc(vq, 1); >> - break; >> - } >> - if (err != len) >> - pr_debug("Truncated TX packet: " >> - " len %d != %zd\n", err, len); >> - if (!zcopy_used) >> - vhost_add_used_and_signal(&net->dev, vq, head, 0); >> - else >> - vhost_zerocopy_signal_used(net, vq); >> - vhost_net_tx_packet(net); >> - if (unlikely(total_len >= VHOST_NET_WEIGHT)) { >> - vhost_poll_queue(&vq->poll); >> - break; > This patch touches many lines just for indentation. If having to touch > these lines anyway (dirtying git blame), it may be a good time to move > the processing of a single descriptor code into a separate helper function. > And while breaking up, perhaps another helper for setting up ubuf_info. > If you agree, preferably in a separate noop refactor patch that precedes > the functional changes. Right and it looks better, will try to do this. Thanks