From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pd0-f180.google.com (mail-pd0-f180.google.com [209.85.192.180]) by dpdk.org (Postfix) with ESMTP id 5207B2E8F for ; Mon, 20 Jul 2015 17:47:08 +0200 (CEST) Received: by pdjr16 with SMTP id r16so105399481pdj.3 for ; Mon, 20 Jul 2015 08:47:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-type:content-transfer-encoding; bh=i2PWsp06AaljJajxmKA8RdcSOAg94oNnOHYX2nYs+vA=; b=BEO7NhB44DRfXMrbpKvhbNeizLnAhAcKImt5IUyat/o2DuOKxTb16OyjrV87vAYI2K D3bmZwY8zBYsWJ4aRsk9chjVl1OFlwJKtYJpogSERZL1U8M6YBaNWbO3nErtujpn0wNL JRXgat+qPGMEgRVWuFG8HrWaHGXrhmp1oyrfB8jainpg0eh0hF8oJ7RquP+2wY80bsmM cAuktHTqe8Rf+VHKkL+kKSnkGH4GyddbF31x+YrC4gLX6O/wpPa4y6vh+ca1ny1KsxN1 FHJIKyOEG50puCtrqB1Ta1eTgr6Wk5hrMKoMXslCbfJN5CKv/k61i+/p3V77imjAfugz LFMQ== X-Gm-Message-State: ALoCoQmsqVvq+u0f18xF+zRrDjLC8VptIC0ZoSDRRGatxodJArSg7r4XS2YpfUuJzl2/kcM4Gctk X-Received: by 10.70.130.199 with SMTP id og7mr10967111pdb.161.1437407227120; Mon, 20 Jul 2015 08:47:07 -0700 (PDT) Received: from urahara (static-50-53-82-155.bvtn.or.frontiernet.net. [50.53.82.155]) by smtp.gmail.com with ESMTPSA id l1sm22487825pdp.71.2015.07.20.08.47.06 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 20 Jul 2015 08:47:06 -0700 (PDT) Date: Mon, 20 Jul 2015 08:47:17 -0700 From: Stephen Hemminger To: "Ouyang, Changchun" Message-ID: <20150720084717.7c02ce8f@urahara> In-Reply-To: References: <1435736930-26737-1-git-send-email-changchun.ouyang@intel.com> <20150717092756.16d7265e@urahara> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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 15:47:08 -0000 On Sat, 18 Jul 2015 12:11:11 +0000 "Ouyang, Changchun" wrote: > Hi Stephen, > > > -----Original Message----- > > From: Stephen Hemminger [mailto:stephen@networkplumber.org] > > 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 > > > > On Wed, 1 Jul 2015 15:48:50 +0800 > > Ouyang Changchun wrote: > > > > > 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 packet > > 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 = 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 == 0) > > > - nb_desc = vq_size; > > > > command queue is setup with nb_desc = 0 > > nb_desc is not used in the rest of the function, then why we need such an assignment here? > Why command queues is setup whit nb_desc = 0? > Even if it is the case, what the code change break? > > > > > > if (vq_size == 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 = nb_desc; > > > - } > > > + if (nb_desc != 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); > > > > Nack. This breaks onn Google Compute Engine the vring size is 16K. > > > As I mentioned before, the commit d78deadae4dca240e85054bf2d604a801676becc break the basic functionality of virtio pmd, > I don't think keeping it broken is good way for us. > We have to revert it firstly to recover its functionality on qemu! > Why we need break current functionality to just meet a new thing's requirement? > > > > > 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. > > Do you have already a patch to revert the mistaken and support both qemu and gce? > If you have, then pls send out it, and let's review. > > > > > 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. > > > > This is what the Linux kernel virtio does. The part in dev_queue_setup is correct, but there is a different problem if the user has requested smaller number of descriptors. What happens is that the receive start process runs the mbuf pool out of space getting more packets than the application expected. Imagine application expects 512 packets in rx ring but full 16K are allocated. Working on a fix to the rx initialization logic to take that into account.