From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 4DD562C65 for ; Mon, 7 Mar 2016 08:52:31 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga103.jf.intel.com with ESMTP; 06 Mar 2016 23:52:30 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,550,1449561600"; d="scan'208";a="665045533" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by FMSMGA003.fm.intel.com with ESMTP; 06 Mar 2016 23:52:30 -0800 Received: from fmsmsx120.amr.corp.intel.com (10.18.124.208) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 6 Mar 2016 23:52:29 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx120.amr.corp.intel.com (10.18.124.208) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 6 Mar 2016 23:52:29 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.136]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.42]) with mapi id 14.03.0248.002; Mon, 7 Mar 2016 15:52:23 +0800 From: "Xie, Huawei" To: Yuanhan Liu , "dev@dpdk.org" Thread-Topic: [PATCH v2 3/7] vhost: refactor virtio_dev_merge_rx Thread-Index: AdF4Rke8Er+Wg2beQUS0XzDMVdLF8A== Date: Mon, 7 Mar 2016 07:52:22 +0000 Message-ID: References: <1449122773-25510-1-git-send-email-yuanhan.liu@linux.intel.com> <1455803352-5518-1-git-send-email-yuanhan.liu@linux.intel.com> <1455803352-5518-4-git-send-email-yuanhan.liu@linux.intel.com> 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: Victor Kaplansky , "Michael S. Tsirkin" Subject: Re: [dpdk-dev] [PATCH v2 3/7] vhost: refactor virtio_dev_merge_rx 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, 07 Mar 2016 07:52:31 -0000 On 2/18/2016 9:48 PM, Yuanhan Liu wrote:=0A= > Current virtio_dev_merge_rx() implementation just looks like the=0A= > old rte_vhost_dequeue_burst(), full of twisted logic, that you=0A= > can see same code block in quite many different places.=0A= >=0A= > However, the logic of virtio_dev_merge_rx() is quite similar to=0A= > virtio_dev_rx(). The big difference is that the mergeable one=0A= > could allocate more than one available entries to hold the data.=0A= > Fetching all available entries to vec_buf at once makes the=0A= [...]=0A= > - }=0A= > +static inline uint32_t __attribute__((always_inline))=0A= > +copy_mbuf_to_desc_mergeable(struct virtio_net *dev, struct vhost_virtque= ue *vq,=0A= > + uint16_t res_start_idx, uint16_t res_end_idx,=0A= > + struct rte_mbuf *m)=0A= > +{=0A= > + struct virtio_net_hdr_mrg_rxbuf virtio_hdr =3D {{0, 0, 0, 0, 0, 0}, 0};= =0A= > + uint32_t vec_idx =3D 0;=0A= > + uint16_t cur_idx =3D res_start_idx;=0A= > + uint64_t desc_addr;=0A= > + uint32_t mbuf_offset, mbuf_avail;=0A= > + uint32_t desc_offset, desc_avail;=0A= > + uint32_t cpy_len;=0A= > + uint16_t desc_idx, used_idx;=0A= > + uint32_t nr_used =3D 0;=0A= > =0A= > - cpy_len =3D RTE_MIN(vb_avail, seg_avail);=0A= > + if (m =3D=3D NULL)=0A= > + return 0;=0A= =0A= Is this inherited from old code? Let us remove the unnecessary check.=0A= Caller ensures it is not NULL.=0A= =0A= > =0A= > - while (cpy_len > 0) {=0A= > - /* Copy mbuf data to vring buffer */=0A= > - rte_memcpy((void *)(uintptr_t)(vb_addr + vb_offset),=0A= > - rte_pktmbuf_mtod_offset(pkt, const void *, seg_offset),=0A= > - cpy_len);=0A= > + LOG_DEBUG(VHOST_DATA,=0A= > + "(%"PRIu64") Current Index %d| End Index %d\n",=0A= > + dev->device_fh, cur_idx, res_end_idx);=0A= > =0A= > - PRINT_PACKET(dev,=0A= > - (uintptr_t)(vb_addr + vb_offset),=0A= > - cpy_len, 0);=0A= > + desc_addr =3D gpa_to_vva(dev, vq->buf_vec[vec_idx].buf_addr);=0A= > + rte_prefetch0((void *)(uintptr_t)desc_addr);=0A= > +=0A= > + virtio_hdr.num_buffers =3D res_end_idx - res_start_idx;=0A= > + LOG_DEBUG(VHOST_DATA, "(%"PRIu64") RX: Num merge buffers %d\n",=0A= > + dev->device_fh, virtio_hdr.num_buffers);=0A= > =0A= > - seg_offset +=3D cpy_len;=0A= > - vb_offset +=3D cpy_len;=0A= > - seg_avail -=3D cpy_len;=0A= > - vb_avail -=3D cpy_len;=0A= > - entry_len +=3D cpy_len;=0A= > -=0A= > - if (seg_avail !=3D 0) {=0A= > - /*=0A= > - * The virtio buffer in this vring=0A= > - * entry reach to its end.=0A= > - * But the segment doesn't complete.=0A= > - */=0A= > - if ((vq->desc[vq->buf_vec[vec_idx].desc_idx].flags &=0A= > - VRING_DESC_F_NEXT) =3D=3D 0) {=0A= > + virtio_enqueue_offload(m, &virtio_hdr.hdr);=0A= > + rte_memcpy((void *)(uintptr_t)desc_addr,=0A= > + (const void *)&virtio_hdr, vq->vhost_hlen);=0A= > + PRINT_PACKET(dev, (uintptr_t)desc_addr, vq->vhost_hlen, 0);=0A= > +=0A= > + desc_avail =3D vq->buf_vec[vec_idx].buf_len - vq->vhost_hlen;=0A= > + desc_offset =3D vq->vhost_hlen;=0A= =0A= As we know we are in merge-able path, use sizeof(virtio_net_hdr) to save=0A= one load for the header len.=0A= =0A= > +=0A= > + mbuf_avail =3D rte_pktmbuf_data_len(m);=0A= > + mbuf_offset =3D 0;=0A= > + while (1) {=0A= > + /* done with current desc buf, get the next one */=0A= > +=0A= [...]=0A= > + if (reserve_avail_buf_mergeable(vq, pkt_len, &start, &end) < 0)=0A= > + break;=0A= > =0A= > + nr_used =3D copy_mbuf_to_desc_mergeable(dev, vq, start, end,=0A= > + pkts[pkt_idx]);=0A= =0A= In which case couldn't we get nr_used from start and end?=0A= =0A= > rte_compiler_barrier();=0A= > =0A= > /*=0A= > * Wait until it's our turn to add our buffer=0A= > * to the used ring.=0A= > */=0A= > - while (unlikely(vq->last_used_idx !=3D res_base_idx))=0A= > + while (unlikely(vq->last_used_idx !=3D start))=0A= > rte_pause();=0A= > =0A= > - *(volatile uint16_t *)&vq->used->idx +=3D entry_success;=0A= > - vq->last_used_idx =3D res_cur_idx;=0A= > + *(volatile uint16_t *)&vq->used->idx +=3D nr_used;=0A= > + vq->last_used_idx =3D end;=0A= > }=0A= > =0A= > -merge_rx_exit:=0A= > if (likely(pkt_idx)) {=0A= > /* flush used->idx update before we read avail->flags. */=0A= > rte_mb();=0A= =0A=