From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yuanhan Liu Subject: Re: [PATCH 1/5] vhost: refactor rte_vhost_dequeue_burst Date: Mon, 14 Dec 2015 09:55:37 +0800 Message-ID: <20151214015537.GZ29571@yliu-dev.sh.intel.com> References: <1449122773-25510-1-git-send-email-yuanhan.liu@linux.intel.com> <1449122773-25510-2-git-send-email-yuanhan.liu@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Cc: dev@dpdk.org, Victor Kaplansky , "Michael S. Tsirkin" To: Rich Lane Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 075478D39 for ; Mon, 14 Dec 2015 02:55:01 +0100 (CET) Content-Disposition: inline In-Reply-To: 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 Fri, Dec 11, 2015 at 10:55:48PM -0800, Rich Lane wrote: > On Wed, Dec 2, 2015 at 10:06 PM, Yuanhan Liu > wrote: >=20 > +static inline struct rte_mbuf * > +copy_desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *= vq, > +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0uint16_t desc_idx, struct rte_m= empool *mbuf_pool) > +{ >=20 > ...=A0 >=20 > + > +=A0 =A0 =A0 =A0desc =3D &vq->desc[desc_idx]; > +=A0 =A0 =A0 =A0desc_addr =3D gpa_to_vva(dev, desc->addr); > +=A0 =A0 =A0 =A0rte_prefetch0((void *)(uintptr_t)desc_addr); > + > +=A0 =A0 =A0 =A0/* Discard virtio header */ > +=A0 =A0 =A0 =A0desc_avail=A0 =3D desc->len - vq->vhost_hlen; >=20 >=20 > If desc->len is zero (happens all the time when restarting DPDK apps in= the > guest) then desc_avail will be huge. I'm aware of it; I have already noted that in the cover letter. This patch set is just a code refactor. Things like that will do a in a latter patch set. (The thing is that Huawei is very cagy about making any changes to vhost rxtx code, as it's the hot data path. So, I will not make any future changes base on this refactor, unless it's applied). > =A0 >=20 > +=A0 =A0 =A0 =A0desc_offset =3D vq->vhost_hlen; > + > +=A0 =A0 =A0 =A0mbuf_avail=A0 =3D 0; > +=A0 =A0 =A0 =A0mbuf_offset =3D 0; > +=A0 =A0 =A0 =A0while (desc_avail || (desc->flags & VRING_DESC_F_NE= XT) !=3D 0) {=A0 >=20 > +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* This desc reachs to its end, get= the next one */ > +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (desc_avail =3D=3D 0) { > +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0desc =3D &vq->desc[= desc->next]; >=20 >=20 > Need to check desc->next against vq->size. Thanks for the remind. >=20 > There should be a limit on the number of descriptors in a chain to prev= ent an > infinite loop. >=20 >=20 > =A0uint16_t > =A0rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_i= d, > =A0 =A0 =A0 =A0 struct rte_mempool *mbuf_pool, struct rte_mbuf **pk= ts, uint16_t > count) > =A0{ > ... > =A0 =A0 =A0 =A0 avail_idx =3D=A0 *((volatile uint16_t *)&vq->avail-= >idx); > - > -=A0 =A0 =A0 =A0/* If there are no available buffers then return. *= / > -=A0 =A0 =A0 =A0if (vq->last_used_idx =3D=3D avail_idx) > +=A0 =A0 =A0 =A0free_entries =3D avail_idx - vq->last_used_idx; > +=A0 =A0 =A0 =A0if (free_entries =3D=3D 0) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0; >=20 >=20 > A common problem is that avail_idx goes backwards when the guest zeroes= its > virtqueues. This function could check for free_entries > vq->size and r= eset > vq->last_used_idx. Thanks, but ditto, will consider it in another patch set. >=20 >=20 > +=A0 =A0 =A0 =A0LOG_DEBUG(VHOST_DATA, "(%"PRIu64") about to dequene= %u buffers\n", > +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev->device_fh, cou= nt); >=20 >=20 > Typo at "dequene". Oops; thanks. --yliu