From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 3CC1EADA2 for ; Mon, 18 May 2015 15:23:18 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga101.fm.intel.com with ESMTP; 18 May 2015 06:23:17 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,453,1427785200"; d="scan'208";a="731132783" Received: from pgsmsx103.gar.corp.intel.com ([10.221.44.82]) by orsmga002.jf.intel.com with ESMTP; 18 May 2015 06:23:16 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by PGSMSX103.gar.corp.intel.com (10.221.44.82) with Microsoft SMTP Server (TLS) id 14.3.224.2; Mon, 18 May 2015 21:23:14 +0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.109]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.94]) with mapi id 14.03.0224.002; Mon, 18 May 2015 21:23:12 +0800 From: "Ouyang, Changchun" To: "Xie, Huawei" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] virtio: Fix enqueue/dequeue can't handle chained vring descriptors. Thread-Index: AQHQkU6E6g8ZTsZtDkq/rQICG5BeLZ2BtW8Q Date: Mon, 18 May 2015 13:23:12 +0000 Message-ID: References: <1430720780-27525-1-git-send-email-changchun.ouyang@intel.com> In-Reply-To: Accept-Language: zh-CN, 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 Subject: Re: [dpdk-dev] [PATCH] virtio: Fix enqueue/dequeue can't handle chained vring descriptors. 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, 18 May 2015 13:23:19 -0000 Hi Huawei, > -----Original Message----- > From: Xie, Huawei > Sent: Monday, May 18, 2015 5:39 PM > To: Ouyang, Changchun; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] virtio: Fix enqueue/dequeue can't handle > chained vring descriptors. >=20 > On 5/4/2015 2:27 PM, Ouyang Changchun wrote: > > Vring enqueue need consider the 2 cases: > > 1. Vring descriptors chained together, the first one is for virtio > > header, the rest are for real data; 2. Only one descriptor, virtio > > header and real data share one single descriptor; > > > > So does vring dequeue. > > > > Signed-off-by: Changchun Ouyang > > --- > > lib/librte_vhost/vhost_rxtx.c | 60 > > +++++++++++++++++++++++++++++++------------ > > 1 file changed, 44 insertions(+), 16 deletions(-) > > > > diff --git a/lib/librte_vhost/vhost_rxtx.c > > b/lib/librte_vhost/vhost_rxtx.c index 510ffe8..3135883 100644 > > --- a/lib/librte_vhost/vhost_rxtx.c > > +++ b/lib/librte_vhost/vhost_rxtx.c > > @@ -59,7 +59,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t > queue_id, > > struct virtio_net_hdr_mrg_rxbuf virtio_hdr =3D {{0, 0, 0, 0, 0, 0}, 0= }; > > uint64_t buff_addr =3D 0; > > uint64_t buff_hdr_addr =3D 0; > > - uint32_t head[MAX_PKT_BURST], packet_len =3D 0; > > + uint32_t head[MAX_PKT_BURST]; > > uint32_t head_idx, packet_success =3D 0; > > uint16_t avail_idx, res_cur_idx; > > uint16_t res_base_idx, res_end_idx; > > @@ -113,6 +113,10 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t > queue_id, > > rte_prefetch0(&vq->desc[head[packet_success]]); > > > > while (res_cur_idx !=3D res_end_idx) { > > + uint32_t offset =3D 0; > > + uint32_t data_len, len_to_cpy; > > + uint8_t plus_hdr =3D 0; > > + > > /* Get descriptor from available ring */ > > desc =3D &vq->desc[head[packet_success]]; > > > > @@ -125,7 +129,6 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t > > queue_id, > > > > /* Copy virtio_hdr to packet and increment buffer address */ > > buff_hdr_addr =3D buff_addr; > > - packet_len =3D rte_pktmbuf_data_len(buff) + vq->vhost_hlen; > > > > /* > > * If the descriptors are chained the header and data are @@ > > -136,24 +139,44 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t > queue_id, > > desc =3D &vq->desc[desc->next]; > > /* Buffer address translation. */ > > buff_addr =3D gpa_to_vva(dev, desc->addr); > > - desc->len =3D rte_pktmbuf_data_len(buff); > > } else { > > buff_addr +=3D vq->vhost_hlen; > > - desc->len =3D packet_len; > > + plus_hdr =3D 1; > > } > > > > + data_len =3D rte_pktmbuf_data_len(buff); > > + len_to_cpy =3D RTE_MIN(data_len, desc->len); > > + do { > > + if (len_to_cpy > 0) { > > + /* Copy mbuf data to buffer */ > > + rte_memcpy((void *)(uintptr_t)buff_addr, > > + (const void > *)(rte_pktmbuf_mtod(buff, const char *) + offset), > > + len_to_cpy); > > + PRINT_PACKET(dev, (uintptr_t)buff_addr, > > + len_to_cpy, 0); > > + > > + desc->len =3D len_to_cpy + (plus_hdr ? vq- > >vhost_hlen : 0); >=20 > Do we really need to rewrite the desc->len again and again? At least we = only > have the possibility to change the value of desc->len of the last descrip= tor. Well, I think we need change each descriptor's len in the chain here, If aggregate all len to the last descriptor's len, it is possibly the lengt= h will exceed its original len, e.g. use 8 descriptor(each has len of 1024) chained to recv a 8K packet, th= en last descriptor's len will be 8K, and all other descriptor is 0, I don't think this situation mak= e sense. =20 >=20 > > + offset +=3D len_to_cpy; > > + if (desc->flags & VRING_DESC_F_NEXT) { > > + desc =3D &vq->desc[desc->next]; > > + buff_addr =3D gpa_to_vva(dev, desc- > >addr); > > + len_to_cpy =3D RTE_MIN(data_len - > offset, desc->len); > > + } else > > + break; >=20 > Still there are two issues here. > a) If the data couldn't be fully copied to chain of guest buffers, we sho= uldn't > do any copy. Why don't copy any data is better than the current implementation? > b) scatter mbuf isn't considered. If we also consider scatter mbuf here, then this function will have exactly= same logic with mergeable_rx, Do you want to totally remove this function, just keep the mergeable rx fun= ction for all cases? Changchun