From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id C8DC0234 for ; Mon, 19 Oct 2015 18:27:12 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP; 19 Oct 2015 09:27:11 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,702,1437462000"; d="scan'208";a="830455601" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga002.fm.intel.com with ESMTP; 19 Oct 2015 09:27:11 -0700 Received: from fmsmsx122.amr.corp.intel.com (10.18.125.37) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 19 Oct 2015 09:27:11 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by fmsmsx122.amr.corp.intel.com (10.18.125.37) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 19 Oct 2015 09:27:10 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.96]) by shsmsx102.ccr.corp.intel.com ([169.254.2.253]) with mapi id 14.03.0248.002; Tue, 20 Oct 2015 00:27:08 +0800 From: "Xie, Huawei" To: Stephen Hemminger Thread-Topic: [PATCH 1/5] virtio: clean up space checks on xmit Thread-Index: AdEKRHKfeZZSBpchSXK4S2m0tifEhg== Date: Mon, 19 Oct 2015 16:27:08 +0000 Message-ID: References: <1445231772-17467-1-git-send-email-stephen@networkplumber.org> <1445231772-17467-2-git-send-email-stephen@networkplumber.org> <20151019084835.7bbeb2c3@xeon-e3> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" 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 16:27:13 -0000 On 10/19/2015 11:48 PM, Stephen Hemminger wrote:=0A= > On Mon, 19 Oct 2015 08:02:07 +0000=0A= > "Xie, Huawei" wrote:=0A= >=0A= >> On 10/19/2015 1:16 PM, Stephen Hemminger wrote:=0A= >>> The space check for transmit ring only needs a single conditional.=0A= >>> I.e only need to recheck for space if there was no space in first check= .=0A= >> I see you reorganized the code. It is more clear and readable with your= =0A= >> change, but no check is removed.=0A= >> We could remove the check after virtio_xmit_cleanup. In current=0A= >> implementation, we assume the virtio_xmit_cleanup and vq_ring_free_chain= =0A= >> always succeed.=0A= > The only case that matters is if virtio_xmit_cleanup can not=0A= > free up any slots.=0A= Is there the case in current implementation virtio_xmit_cleanup could=0A= free up any descriptors, i.e, dxp->ndesc is zero?=0A= vq->free_cnt +=3D dxp->ndesc=0A= >=0A= >>> This can help performance and simplifies loop.=0A= >>>=0A= >>> Signed-off-by: Stephen Hemminger =0A= >>> ---=0A= >>> drivers/net/virtio/virtio_rxtx.c | 66 ++++++++++++++++----------------= --------=0A= >>> 1 file changed, 27 insertions(+), 39 deletions(-)=0A= >>>=0A= >>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virt= io_rxtx.c=0A= >>> index c5b53bb..5b50ed0 100644=0A= >>> --- a/drivers/net/virtio/virtio_rxtx.c=0A= >>> +++ b/drivers/net/virtio/virtio_rxtx.c=0A= >>> @@ -745,7 +745,6 @@ uint16_t=0A= >>> virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t n= b_pkts)=0A= >>> {=0A= >>> struct virtqueue *txvq =3D tx_queue;=0A= >>> - struct rte_mbuf *txm;=0A= >>> uint16_t nb_used, nb_tx;=0A= >>> int error;=0A= >>> =0A= >>> @@ -759,57 +758,46 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf = **tx_pkts, uint16_t nb_pkts)=0A= >>> if (likely(nb_used > txvq->vq_nentries - txvq->vq_free_thresh))=0A= >>> virtio_xmit_cleanup(txvq, nb_used);=0A= >>> =0A= >>> - nb_tx =3D 0;=0A= >>> + for (nb_tx =3D 0; nb_tx < nb_pkts; nb_tx++) {=0A= >>> + struct rte_mbuf *txm =3D tx_pkts[nb_tx];=0A= >>> + int need =3D txm->nb_segs - txvq->vq_free_cnt + 1;=0A= >>> =0A= >>> - while (nb_tx < nb_pkts) {=0A= >>> - /* Need one more descriptor for virtio header. */=0A= >>> - int need =3D tx_pkts[nb_tx]->nb_segs - txvq->vq_free_cnt + 1;=0A= >>> -=0A= >>> - /*Positive value indicates it need free vring descriptors */=0A= >>> + /* Positive value indicates it need free vring descriptors */=0A= >> As you fix the comment, if it is correct, could you do s/need/needs/ as= =0A= >> well, :)?=0A= >>> if (unlikely(need > 0)) {=0A= >>> nb_used =3D VIRTQUEUE_NUSED(txvq);=0A= >>> virtio_rmb();=0A= >>> need =3D RTE_MIN(need, (int)nb_used);=0A= >>> =0A= >>> virtio_xmit_cleanup(txvq, need);=0A= >>> - need =3D (int)tx_pkts[nb_tx]->nb_segs -=0A= >>> - txvq->vq_free_cnt + 1;=0A= >>> - }=0A= >>> -=0A= >>> - /*=0A= >>> - * Zero or negative value indicates it has enough free=0A= >>> - * descriptors to use for transmitting.=0A= >>> - */=0A= >>> - if (likely(need <=3D 0)) {=0A= >>> - txm =3D tx_pkts[nb_tx];=0A= >>> -=0A= >>> - /* Do VLAN tag insertion */=0A= >>> - if (unlikely(txm->ol_flags & PKT_TX_VLAN_PKT)) {=0A= >>> - error =3D rte_vlan_insert(&txm);=0A= >>> - if (unlikely(error)) {=0A= >>> - rte_pktmbuf_free(txm);=0A= >>> - ++nb_tx;=0A= >>> - continue;=0A= >>> - }=0A= >> Still need is rechecked here. It could be removed if virtio_xmit_cleanup= =0A= >> ensures need would be <=3D 0 after the call.=0A= >>> + need =3D txm->nb_segs - txvq->vq_free_cnt + 1;=0A= >>> + if (unlikely(need > 0)) {=0A= >>> + PMD_TX_LOG(ERR,=0A= >>> + "No free tx descriptors to transmit");=0A= >>> + break;=0A= >>> }=0A= >>> + }=0A= >>> =0A= >>> - /* Enqueue Packet buffers */=0A= >>> - error =3D virtqueue_enqueue_xmit(txvq, txm);=0A= >>> + /* Do VLAN tag insertion */=0A= >>> + if (unlikely(txm->ol_flags & PKT_TX_VLAN_PKT)) {=0A= >>> + error =3D rte_vlan_insert(&txm);=0A= >>> if (unlikely(error)) {=0A= >>> - if (error =3D=3D ENOSPC)=0A= >>> - PMD_TX_LOG(ERR, "virtqueue_enqueue Free count =3D 0");=0A= >>> - else if (error =3D=3D EMSGSIZE)=0A= >>> - PMD_TX_LOG(ERR, "virtqueue_enqueue Free count < 1");=0A= >>> - else=0A= >>> - PMD_TX_LOG(ERR, "virtqueue_enqueue error: %d", error);=0A= >>> - break;=0A= >>> + rte_pktmbuf_free(txm);=0A= >>> + continue;=0A= >>> }=0A= >>> - nb_tx++;=0A= >>> - txvq->bytes +=3D txm->pkt_len;=0A= >>> - } else {=0A= >>> - PMD_TX_LOG(ERR, "No free tx descriptors to transmit");=0A= >>> + }=0A= >>> +=0A= >>> + /* Enqueue Packet buffers */=0A= >>> + error =3D virtqueue_enqueue_xmit(txvq, txm);=0A= >>> + if (unlikely(error)) {=0A= >>> + if (error =3D=3D ENOSPC)=0A= >>> + PMD_TX_LOG(ERR, "virtqueue_enqueue Free count =3D 0");=0A= >>> + else if (error =3D=3D EMSGSIZE)=0A= >>> + PMD_TX_LOG(ERR, "virtqueue_enqueue Free count < 1");=0A= >>> + else=0A= >>> + PMD_TX_LOG(ERR, "virtqueue_enqueue error: %d", error);=0A= >>> break;=0A= >>> }=0A= >>> + txvq->bytes +=3D txm->pkt_len;=0A= >>> }=0A= >>> =0A= >>> txvq->packets +=3D nb_tx;=0A= >=0A= =0A=