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 5D1175958 for ; Mon, 21 Sep 2015 08:34:57 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP; 20 Sep 2015 23:34:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,566,1437462000"; d="scan'208";a="793931354" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.60]) by fmsmga001.fm.intel.com with ESMTP; 20 Sep 2015 23:34:55 -0700 Date: Mon, 21 Sep 2015 14:36:47 +0800 From: Yuanhan Liu To: "Michael S. Tsirkin" Message-ID: <20150921063647.GQ2339@yliu-dev.sh.intel.com> References: <1442589061-19225-1-git-send-email-yuanhan.liu@linux.intel.com> <1442589061-19225-8-git-send-email-yuanhan.liu@linux.intel.com> <20150920121748-mutt-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150920121748-mutt-send-email-mst@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v5 resend 07/12] virtio: resolve for control queue 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, 21 Sep 2015 06:34:57 -0000 On Sun, Sep 20, 2015 at 12:21:14PM +0300, Michael S. Tsirkin wrote: > On Fri, Sep 18, 2015 at 11:10:56PM +0800, Yuanhan Liu wrote: > > From: Changchun Ouyang > > > > Fix the max virtio queue pair read issue. > > > > Control queue can't work for vhost-user mulitple queue mode, > > so introduce a counter to void the dead loop when polling > > the control queue. > > > > Signed-off-by: Changchun Ouyang > > Signed-off-by: Yuanhan Liu > > Per virtio spec, the multiqueue feature depends on control queue - > what do you mean when you say it can't work? > > > --- > > drivers/net/virtio/virtio_ethdev.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c > > index 465d3cd..b2f4120 100644 > > --- a/drivers/net/virtio/virtio_ethdev.c > > +++ b/drivers/net/virtio/virtio_ethdev.c > > @@ -1162,7 +1162,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) > > struct virtio_hw *hw = eth_dev->data->dev_private; > > struct virtio_net_config *config; > > struct virtio_net_config local_config; > > - uint32_t offset_conf = sizeof(config->mac); > > struct rte_pci_device *pci_dev; > > > > RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr)); > > @@ -1222,7 +1221,9 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) > > config = &local_config; > > > > if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS)) { > > - offset_conf += sizeof(config->status); > > + vtpci_read_dev_config(hw, > > + offsetof(struct virtio_net_config, status), > > + &config->status, sizeof(config->status)); > > } else { > > PMD_INIT_LOG(DEBUG, > > "VIRTIO_NET_F_STATUS is not supported"); > > @@ -1230,15 +1231,16 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) > > } > > > > if (vtpci_with_feature(hw, VIRTIO_NET_F_MQ)) { > > - offset_conf += sizeof(config->max_virtqueue_pairs); > > + vtpci_read_dev_config(hw, > > + offsetof(struct virtio_net_config, max_virtqueue_pairs), > > + &config->max_virtqueue_pairs, > > + sizeof(config->max_virtqueue_pairs)); > > } else { > > PMD_INIT_LOG(DEBUG, > > "VIRTIO_NET_F_MQ is not supported"); > > config->max_virtqueue_pairs = 1; > > } > > > > - vtpci_read_dev_config(hw, 0, (uint8_t *)config, offset_conf); > > - > > hw->max_rx_queues = > > (VIRTIO_MAX_RX_QUEUES < config->max_virtqueue_pairs) ? > > VIRTIO_MAX_RX_QUEUES : config->max_virtqueue_pairs; > > > Does the patch actually do what the commit log says? Sorry, the commit log is wrong as you said. It was actually a bug in our code, which happens to be revealed when MQ is enabled. The old code adjusts the config bytes we want to read depending on what kind of features we have, but we later cast the entire buf we read with "struct virtio_net_config", which is obviously wrong. The right way to go is to read related config bytes when corresponding feature is set, which is exactly what this patch does. > It seems tobe about reading the device confing, > not breaking out of a loop ... It's just a (bad) side effect of getting the vritio_net_config wrongly: the wrong config causes a dead loop in our code. And sorry for the buggy commit log, will fix it next version. Thanks. --yliu