From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout3.w1.samsung.com (mailout3.w1.samsung.com [210.118.77.13]) by dpdk.org (Postfix) with ESMTP id 0C82B5960 for ; Thu, 8 Oct 2015 17:32:10 +0200 (CEST) Received: from eucpsbgm1.samsung.com (unknown [203.254.199.244]) by mailout3.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0NVW00KWCR5KUOB0@mailout3.w1.samsung.com> for dev@dpdk.org; Thu, 08 Oct 2015 16:32:08 +0100 (BST) X-AuditID: cbfec7f4-f79c56d0000012ee-78-56168c77c2d8 Received: from eusync3.samsung.com ( [203.254.199.213]) by eucpsbgm1.samsung.com (EUCPMTA) with SMTP id 96.C7.04846.77C86165; Thu, 8 Oct 2015 16:32:07 +0100 (BST) Received: from localhost ([106.109.131.58]) by eusync3.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTPA id <0NVW00M1BR5JSI30@eusync3.samsung.com>; Thu, 08 Oct 2015 16:32:07 +0100 (BST) Date: Thu, 08 Oct 2015 18:32:05 +0300 From: Nikita Kalyazin To: Yuanhan Liu Message-id: <20151008153205.GB21145@kalyazin.rnd.samsung.ru> 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> <20150921063647.GQ2339@yliu-dev.sh.intel.com> MIME-version: 1.0 Content-type: text/plain; charset=utf-8 Content-disposition: inline In-reply-to: <20150921063647.GQ2339@yliu-dev.sh.intel.com> X-Priority: 3 User-Agent: Mutt/1.5.23 (2014-03-12) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrOLMWRmVeSWpSXmKPExsVy+t/xq7rlPWJhBu9mGVu8+7SdyeL/r1es FtcnXGB1YPb4tWApq8e8k4Ee7/ddZQtgjuKySUnNySxLLdK3S+DKOH31GWvBasWKaZtuMzYw 9kl1MXJySAiYSLS/nMIOYYtJXLi3nq2LkYtDSGApo8S+42uZIZyvjBJPF75lAqliEVCVaDv9 lLWLkYODTUBP4vTpYJCwiICuxNM568DCzALmEn/+VYOEhQWCJeZ+2gLWyStgI7Fx0zNGiJHv GSW+tUyCSghK/Jh8jwWiV11iypRckDCzgLTEo78zwG7jFLCS+LPyISvEnbwSM9qfsoDYogIq ElcmvGWfwCg4C8mkWQiTZiGZtICReRWjaGppckFxUnquoV5xYm5xaV66XnJ+7iZGSPB+2cG4 +JjVIUYBDkYlHt4FdmJhQqyJZcWVuYcYJTiYlUR46yuBQrwpiZVVqUX58UWlOanFhxilOViU xHnn7nofIiSQnliSmp2aWpBaBJNl4uCUamBccO55+XLTN5fX66hEr8pp9yudtbzjiSbLZZ1z P3mXbbayCvM7fOi2ZMzTh7v76irv5b3fteJa4WOfk03cl2Xyan9k751s38otOsU/ZlkoT/SW qSey/rNkhrmcOOHh5HfCd5bAC8nlzIqtOxarsDAm+d3mtCkoY3VJC6m9tqkkNbH2ibeg1m4l luKMREMt5qLiRABAGg8WWgIAAA== Cc: dev@dpdk.org, "Michael S. Tsirkin" 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: Thu, 08 Oct 2015 15:32:10 -0000 Hi Yuanhan, As I understand, the dead loop happened here (virtio_send_command): while (vq->vq_used_cons_idx == vq->vq_ring.used->idx) { rte_rmb(); usleep(100); } Could you explain why wrong config reading caused that and how correct reading helps to avoid? -- Best regards, Nikita Kalyazin, n.kalyazin@samsung.com Software Engineer Virtualization Group Samsung R&D Institute Russia Tel: +7 (495) 797-25-00 #3816 Tel: +7 (495) 797-25-03 Office #1501, 12-1, Dvintsev str., Moscow, 127018, Russia On Mon, Sep 21, 2015 at 02:36:47PM +0800, Yuanhan Liu wrote: > 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