From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id A25AEC3BC for ; Mon, 20 Jul 2015 08:21:44 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga103.fm.intel.com with ESMTP; 19 Jul 2015 23:21:43 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,507,1432623600"; d="scan'208";a="765764844" Received: from pgsmsx106.gar.corp.intel.com ([10.221.44.98]) by fmsmga002.fm.intel.com with ESMTP; 19 Jul 2015 23:21:42 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by PGSMSX106.gar.corp.intel.com (10.221.44.98) with Microsoft SMTP Server (TLS) id 14.3.224.2; Mon, 20 Jul 2015 14:19:23 +0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.165]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.129]) with mapi id 14.03.0224.002; Mon, 20 Jul 2015 14:18:53 +0800 From: "Ouyang, Changchun" To: "Xu, Qian Q" , Stephen Hemminger , 'Thomas Monjalon' Thread-Topic: [dpdk-dev] [PATCH] virtio: fix the vq size issue Thread-Index: AQHQs9JlQEOaxBUIeEyxKL7zRjwCJJ3fbVEAgAPgnICAAIjiMA== Date: Mon, 20 Jul 2015 06:18:53 +0000 Message-ID: References: <1435736930-26737-1-git-send-email-changchun.ouyang@intel.com> <20150717092756.16d7265e@urahara> <82F45D86ADE5454A95A89742C8D1410E01D7DBAF@shsmsx102.ccr.corp.intel.com> In-Reply-To: <82F45D86ADE5454A95A89742C8D1410E01D7DBAF@shsmsx102.ccr.corp.intel.com> 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 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue 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, 20 Jul 2015 06:21:45 -0000 Hi Thomas, I think we have 3 options for this issue. 1) applying this patch; 2) reverting Stephen's original patch; 3) new patch to make both QEMU and GCE work. 1) and 2) will make the test case recover quickly from fail. As for 3) I don't know whether Stephen has such a patch which can work on b= oth or not. I don't have GCE environment on hand and I am not an expert on that yet, my= current focus is virtio on QEMU, So at present I have no chance to make a new one to make sure both can work= , But I can help on reviewing if Stephen has a new patch to do that.=20 Another thing burst into my thought. Can we think more about how to setup a mechanism to block those patches whi= ch causes critical regression issue? e.g. this case we are talking about.=20 Commit d78deadae4dca240e85054bf2d604a801676becc breaks basic functionality = of virtio PMD on QEMU, It means DPDK sample like vhost, vxlan can't rx any packet and accordingly = it can't forward any packet with virtio PMD. Neither does ovs. I did review that patch before, but fail to realize it will break the basic= function of virtio PMD, it is my bad.=20 (Can I send the nack to that patch even after it has been merged into dpdk.= org?) After that, we find that in our testing cycle, we spend time in investigati= ng that and root the cause, and sent out the fixing patch on July 1. Keeping virtio basic functionality broken more= than 20 days is bad thing for me.=20 If we can run a regression automation test with every patch set sent out to= dpdk.org, and put those patches breaking any test cases Into failing-list and notify author, reviewer and maintainer, all those thi= ngs should be done before theirs being merged, then it will prevent from merging the erroneous patch into mainline, and thus reduce mos= t reverting patch. Hi Stephen, and guys in Brocade Since you nack my patch, then would you pls send out a new patch to fix the= issue which your previous patch broke ASAP? I am not sure you validate your patches on GCE or not, but I strongly sugge= st you validate each of them on QEMU before you send out a formal one to dpdk.org. Hi Qian, Thanks very much for raising this critical issue in virtio! thanks, Changchun > -----Original Message----- > From: Xu, Qian Q > Sent: Monday, July 20, 2015 11:41 AM > To: Stephen Hemminger; Ouyang, Changchun; 'Thomas Monjalon' > Cc: dev@dpdk.org; Xu, Qian Q > Subject: RE: [dpdk-dev] [PATCH] virtio: fix the vq size issue >=20 > Hi, Thomas and all > I saw in the latest rc1 package, the patch is not merged, and it's a crit= ical issue > from validation view. I'm responsible for testing the dpdk vhost/virtio > features, and I found using the latest code, dpdk-vhost/dpdk-virtio can't > RX/TX package, then my 50% tests are failed while in DPDK2.0 they can pas= s. > As you know, it's the basic functions for dpdk virtio to RX/TX, if it's n= ot fixed, I > think we can't release the R2.1 package. Please help merge the patch, thx= . >=20 >=20 >=20 > Thanks > Qian >=20 >=20 > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen > Hemminger > Sent: Saturday, July 18, 2015 12:28 AM > To: Ouyang, Changchun > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] virtio: fix the vq size issue >=20 > On Wed, 1 Jul 2015 15:48:50 +0800 > Ouyang Changchun wrote: >=20 > > This commit breaks virtio basic packets rx functionality: > > d78deadae4dca240e85054bf2d604a801676becc > > > > The QEMU use 256 as default vring size, also use this default value to > > calculate the virtio avail ring base address and used ring base > > address, and vhost in the backend use the ring base address to do packe= t > IO. > > > > Virtio spec also says the queue size in PCI configuration is > > read-only, so virtio front end can't change it. just need use the > > read-only value to allocate space for vring and calculate the avail > > and used ring base address. Otherwise, the avail and used ring base > address will be different between host and guest, accordingly, packet IO > can't work normally. > > > > Signed-off-by: Changchun Ouyang > > --- > > drivers/net/virtio/virtio_ethdev.c | 14 +++----------- > > 1 file changed, 3 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/net/virtio/virtio_ethdev.c > > b/drivers/net/virtio/virtio_ethdev.c > > index fe5f9a1..d84de13 100644 > > --- a/drivers/net/virtio/virtio_ethdev.c > > +++ b/drivers/net/virtio/virtio_ethdev.c > > @@ -263,8 +263,6 @@ int virtio_dev_queue_setup(struct rte_eth_dev > *dev, > > */ > > vq_size =3D VIRTIO_READ_REG_2(hw, VIRTIO_PCI_QUEUE_NUM); > > PMD_INIT_LOG(DEBUG, "vq_size: %d nb_desc:%d", vq_size, > nb_desc); > > - if (nb_desc =3D=3D 0) > > - nb_desc =3D vq_size; >=20 > command queue is setup with nb_desc =3D 0 >=20 > > if (vq_size =3D=3D 0) { > > PMD_INIT_LOG(ERR, "%s: virtqueue does not exist", > __func__); > > return -EINVAL; > > @@ -275,15 +273,9 @@ int virtio_dev_queue_setup(struct rte_eth_dev > *dev, > > return -EINVAL; > > } > > > > - if (nb_desc < vq_size) { > > - if (!rte_is_power_of_2(nb_desc)) { > > - PMD_INIT_LOG(ERR, > > - "nb_desc(%u) size is not powerof 2", > > - nb_desc); > > - return -EINVAL; > > - } > > - vq_size =3D nb_desc; > > - } > > + if (nb_desc !=3D vq_size) > > + PMD_INIT_LOG(ERR, "Warning: nb_desc(%d) is not equal to > vq size (%d), fall to vq size", > > + nb_desc, vq_size); >=20 > Nack. This breaks onn Google Compute Engine the vring size is 16K. >=20 > An application that wants to work on both QEMU and GCE will want to pass = a > reasonable size and have the negotiation resolve to best value. >=20 > For example, vRouter passes 512 as Rx ring size. > On QEMU this gets rounded down to 256 and on GCE only 512 elements are > used. >=20 > This is what the Linux kernel virtio does.