From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <huawei.xie@intel.com>
Received: from mga03.intel.com (mga03.intel.com [134.134.136.65])
by dpdk.org (Postfix) with ESMTP id 4DD562C65
for <dev@dpdk.org>; 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" <huawei.xie@intel.com>
To: Yuanhan Liu <yuanhan.liu@linux.intel.com>, "dev@dpdk.org" <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: <C37D651A908B024F974696C65296B57B4C6384E3@SHSMSX101.ccr.corp.intel.com>
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 <vkaplans@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com>
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 <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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=