From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f43.google.com (mail-pa0-f43.google.com [209.85.220.43]) by dpdk.org (Postfix) with ESMTP id BA0438E69 for ; Mon, 19 Oct 2015 17:48:25 +0200 (CEST) Received: by padhk11 with SMTP id hk11so33346952pad.1 for ; Mon, 19 Oct 2015 08:48:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-type:content-transfer-encoding; bh=WBXikemBC7RBREbkjsCNd2cTCDbBk7hTPXNP28X6D6s=; b=Qc+G4TVXJ4y+UDRaQDPvHPEclYBXAW0eArS6Mk+ABQ+XzIE64/a8LAWFW19AjaFWyM xw3UXPqJRvKgkwa9dntR+vVcAcflqcmgfsnQ1G48+lvdb2ZyjAEip+Y5ErYQ6I/oKZ6S bi5cV79DZQfXLCi1ZjfyJwMGIcWLBg8upgwLkPvwbaybrZ/ZoNvqJSZy6Br1kh47R6AW 2P0LJiQN4DoTiqXkE2PmN28UTB9mg+0KWx+lSmzIjkHwJY6Yav0cfeZum5EqzOzCr4UB bbMgjdcoKrSOMxUGRsYFrlDzTSRCpagBj48Rl+yAvoowN3nOuplGMfIPdSxgisKs+WJF Pspw== X-Gm-Message-State: ALoCoQlbpML8TljhRcJyg/vRCCkSD0nxyHvltGALZValjaaF5FDGcNpfQlgugy/+T6/oryh18fke X-Received: by 10.68.213.4 with SMTP id no4mr15920195pbc.85.1445269705179; Mon, 19 Oct 2015 08:48:25 -0700 (PDT) Received: from xeon-e3 (static-50-53-82-155.bvtn.or.frontiernet.net. [50.53.82.155]) by smtp.gmail.com with ESMTPSA id rz9sm37011426pbb.61.2015.10.19.08.48.24 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 19 Oct 2015 08:48:25 -0700 (PDT) Date: Mon, 19 Oct 2015 08:48:35 -0700 From: Stephen Hemminger To: "Xie, Huawei" Message-ID: <20151019084835.7bbeb2c3@xeon-e3> In-Reply-To: References: <1445231772-17467-1-git-send-email-stephen@networkplumber.org> <1445231772-17467-2-git-send-email-stephen@networkplumber.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: "dev@dpdk.org" , "changchun.ouyang@intel.com" Subject: Re: [dpdk-dev] [PATCH 1/5] virtio: clean up space checks on xmit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Oct 2015 15:48:26 -0000 On Mon, 19 Oct 2015 08:02:07 +0000 "Xie, Huawei" wrote: > On 10/19/2015 1:16 PM, Stephen Hemminger wrote: > > The space check for transmit ring only needs a single conditional. > > I.e only need to recheck for space if there was no space in first check. > I see you reorganized the code. It is more clear and readable with your > change, but no check is removed. > We could remove the check after virtio_xmit_cleanup. In current > implementation, we assume the virtio_xmit_cleanup and vq_ring_free_chain > always succeed. The only case that matters is if virtio_xmit_cleanup can not free up any slots. > > This can help performance and simplifies loop. > > > > Signed-off-by: Stephen Hemminger > > --- > > drivers/net/virtio/virtio_rxtx.c | 66 ++++++++++++++++------------------------ > > 1 file changed, 27 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c > > index c5b53bb..5b50ed0 100644 > > --- a/drivers/net/virtio/virtio_rxtx.c > > +++ b/drivers/net/virtio/virtio_rxtx.c > > @@ -745,7 +745,6 @@ uint16_t > > virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts) > > { > > struct virtqueue *txvq = tx_queue; > > - struct rte_mbuf *txm; > > uint16_t nb_used, nb_tx; > > int error; > > > > @@ -759,57 +758,46 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts) > > if (likely(nb_used > txvq->vq_nentries - txvq->vq_free_thresh)) > > virtio_xmit_cleanup(txvq, nb_used); > > > > - nb_tx = 0; > > + for (nb_tx = 0; nb_tx < nb_pkts; nb_tx++) { > > + struct rte_mbuf *txm = tx_pkts[nb_tx]; > > + int need = txm->nb_segs - txvq->vq_free_cnt + 1; > > > > - while (nb_tx < nb_pkts) { > > - /* Need one more descriptor for virtio header. */ > > - int need = tx_pkts[nb_tx]->nb_segs - txvq->vq_free_cnt + 1; > > - > > - /*Positive value indicates it need free vring descriptors */ > > + /* Positive value indicates it need free vring descriptors */ > As you fix the comment, if it is correct, could you do s/need/needs/ as > well, :)? > > if (unlikely(need > 0)) { > > nb_used = VIRTQUEUE_NUSED(txvq); > > virtio_rmb(); > > need = RTE_MIN(need, (int)nb_used); > > > > virtio_xmit_cleanup(txvq, need); > > - need = (int)tx_pkts[nb_tx]->nb_segs - > > - txvq->vq_free_cnt + 1; > > - } > > - > > - /* > > - * Zero or negative value indicates it has enough free > > - * descriptors to use for transmitting. > > - */ > > - if (likely(need <= 0)) { > > - txm = tx_pkts[nb_tx]; > > - > > - /* Do VLAN tag insertion */ > > - if (unlikely(txm->ol_flags & PKT_TX_VLAN_PKT)) { > > - error = rte_vlan_insert(&txm); > > - if (unlikely(error)) { > > - rte_pktmbuf_free(txm); > > - ++nb_tx; > > - continue; > > - } > Still need is rechecked here. It could be removed if virtio_xmit_cleanup > ensures need would be <= 0 after the call. > > + need = txm->nb_segs - txvq->vq_free_cnt + 1; > > + if (unlikely(need > 0)) { > > + PMD_TX_LOG(ERR, > > + "No free tx descriptors to transmit"); > > + break; > > } > > + } > > > > - /* Enqueue Packet buffers */ > > - error = virtqueue_enqueue_xmit(txvq, txm); > > + /* Do VLAN tag insertion */ > > + if (unlikely(txm->ol_flags & PKT_TX_VLAN_PKT)) { > > + error = rte_vlan_insert(&txm); > > if (unlikely(error)) { > > - if (error == ENOSPC) > > - PMD_TX_LOG(ERR, "virtqueue_enqueue Free count = 0"); > > - else if (error == EMSGSIZE) > > - PMD_TX_LOG(ERR, "virtqueue_enqueue Free count < 1"); > > - else > > - PMD_TX_LOG(ERR, "virtqueue_enqueue error: %d", error); > > - break; > > + rte_pktmbuf_free(txm); > > + continue; > > } > > - nb_tx++; > > - txvq->bytes += txm->pkt_len; > > - } else { > > - PMD_TX_LOG(ERR, "No free tx descriptors to transmit"); > > + } > > + > > + /* Enqueue Packet buffers */ > > + error = virtqueue_enqueue_xmit(txvq, txm); > > + if (unlikely(error)) { > > + if (error == ENOSPC) > > + PMD_TX_LOG(ERR, "virtqueue_enqueue Free count = 0"); > > + else if (error == EMSGSIZE) > > + PMD_TX_LOG(ERR, "virtqueue_enqueue Free count < 1"); > > + else > > + PMD_TX_LOG(ERR, "virtqueue_enqueue error: %d", error); > > break; > > } > > + txvq->bytes += txm->pkt_len; > > } > > > > txvq->packets += nb_tx; >