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 BF7422BE3 for ; Mon, 7 Mar 2016 10:27:50 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP; 07 Mar 2016 01:27:50 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,550,1449561600"; d="scan'208";a="903677593" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga001.jf.intel.com with ESMTP; 07 Mar 2016 01:27:49 -0800 Received: from fmsmsx117.amr.corp.intel.com (10.18.116.17) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 7 Mar 2016 01:27:49 -0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx117.amr.corp.intel.com (10.18.116.17) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 7 Mar 2016 01:27:48 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.136]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.132]) with mapi id 14.03.0248.002; Mon, 7 Mar 2016 17:27:47 +0800 From: "Xie, Huawei" To: Yuanhan Liu Thread-Topic: [PATCH v2 3/7] vhost: refactor virtio_dev_merge_rx Thread-Index: AdF4Rke8Er+Wg2beQUS0XzDMVdLF8A== Date: Mon, 7 Mar 2016 09:27:46 +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> <20160307083845.GF14300@yliu-dev.sh.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: "Michael S. Tsirkin" , "dev@dpdk.org" , Victor Kaplansky 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 09:27:51 -0000 On 3/7/2016 4:36 PM, Yuanhan Liu wrote:=0A= > On Mon, Mar 07, 2016 at 07:52:22AM +0000, Xie, Huawei wrote:=0A= >> 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_virtq= ueue *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= >> Is this inherited from old code?=0A= > Yes.=0A= >=0A= >> Let us remove the unnecessary check.=0A= >> Caller ensures it is not NULL.=0A= > ...=0A= >>> + desc_avail =3D vq->buf_vec[vec_idx].buf_len - vq->vhost_hlen;=0A= >>> + desc_offset =3D vq->vhost_hlen;=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= > Please, it's a refactor patch series. You have mentioned quite many=0A= > trivial issues here and there, which I don't care too much and I don't=0A= > think they would matter somehow. In addition, they are actually from=0A= > the old code.=0A= =0A= For normal code, it would be better using vq->vhost_hlen for example for=0A= future compatibility. For DPDK, we don't waste cycles whenever possible,=0A= especially vhost is the centralized bottleneck.=0A= For the check of m =3D=3D NULL, it should be removed, which not only=0A= occupies unnecessary branch predication resource but also causes=0A= confusion for return nr_used from copy_mbuf_to_desc_mergeable.=0A= It is OK if you don't want to fix this in this patchset.=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= >> In which case couldn't we get nr_used from start and end?=0A= > When pkts[pkt_idx] is NULL, though you suggest to remove it, the check=0A= > is here.=0A= >=0A= > --yliu=0A= >=0A= =0A=