From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Xie, Huawei" Subject: Re: [PATCH 1/5] virtio: clean up space checks on xmit 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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" To: Stephen Hemminger 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) Content-Language: en-US List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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=