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 422908E76 for ; Mon, 19 Oct 2015 10:02:38 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga101.jf.intel.com with ESMTP; 19 Oct 2015 01:02:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,701,1437462000"; d="scan'208";a="583598217" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by FMSMGA003.fm.intel.com with ESMTP; 19 Oct 2015 01:02:16 -0700 Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 19 Oct 2015 01:02:15 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX154.amr.corp.intel.com (10.18.116.70) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 19 Oct 2015 01:02:15 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.96]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.204]) with mapi id 14.03.0248.002; Mon, 19 Oct 2015 16:02:07 +0800 From: "Xie, Huawei" To: Stephen Hemminger , "changchun.ouyang@intel.com" Thread-Topic: [PATCH 1/5] virtio: clean up space checks on xmit Thread-Index: AdEKRHKfeZZSBpchSXK4S2m0tifEhg== Date: Mon, 19 Oct 2015 08:02:07 +0000 Message-ID: References: <1445231772-17467-1-git-send-email-stephen@networkplumber.org> <1445231772-17467-2-git-send-email-stephen@networkplumber.org> 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 08:02:39 -0000 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= > 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/virtio= _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 nb_= 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=