From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 635D91BAFE for ; Wed, 11 Apr 2018 11:00:57 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Apr 2018 02:00:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,435,1517904000"; d="scan'208";a="45275688" Received: from tanjianf-mobl.ccr.corp.intel.com (HELO [10.67.64.73]) ([10.67.64.73]) by fmsmga004.fm.intel.com with ESMTP; 11 Apr 2018 02:00:55 -0700 To: "Chen, Junjie J" , "maxime.coquelin@redhat.com" , "mtetsuyah@gmail.com" References: <1523369889-73457-1-git-send-email-junjie.j.chen@intel.com> <1523443993-176139-1-git-send-email-junjie.j.chen@intel.com> <0618790b-5ba6-e830-f5d2-35e25d091a36@intel.com> Cc: "dev@dpdk.org" From: "Tan, Jianfeng" Message-ID: <7946ae32-8728-fca3-f5da-61bda57f2289@intel.com> Date: Wed, 11 Apr 2018 17:00:54 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2] net/vhost: fix vhost invalid state X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 11 Apr 2018 09:00:57 -0000 On 4/11/2018 4:35 PM, Chen, Junjie J wrote: >> >> On 4/11/2018 6:53 PM, Junjie Chen wrote: >>> dev_start sets *dev_attached* after setup queues, this sets device to >>> invalid state since no frontend is attached. Also destroy_device set >>> *started* to zero which makes *allow_queuing* always zero until >>> dev_start get called again. Actually, we should not determine queues >>> existence by >>> *dev_attached* but by queues pointers or other separated variable(s). >> I see two issues from your description: >> >> - We shall fix the wrong use of dev_attached by commit 30a701a53737. >> - The allow_queuing is not set correctly. But I doubt if it's a real issue, as we do >> update the queue status in dev_start(), dev_stop(), and new_device(). Can you >> explain more? > The started is set to 0 in destroy_device in 30a701a53737 commit, so allow_queuing is always > set to 0 once update_queuing_status get called. We have to use "port start" to reset to 1. OK, that means it is also due to the wrong use of dev_attached? >>> Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev >>> dynamically") >>> >>> Signed-off-by: Junjie Chen >>> --- >>> Changes in v2: >>> - use started to determine vhost queues readiness >>> - revert setting started to zero in destroy_device >>> drivers/net/vhost/rte_eth_vhost.c | 61 >> ++++++++++++++++++++------------------- >>> 1 file changed, 31 insertions(+), 30 deletions(-) >>> >>> diff --git a/drivers/net/vhost/rte_eth_vhost.c >> b/drivers/net/vhost/rte_eth_vhost.c >>> index 11b6076..ff462b3 100644 >>> --- a/drivers/net/vhost/rte_eth_vhost.c >>> +++ b/drivers/net/vhost/rte_eth_vhost.c >>> @@ -528,10 +528,13 @@ update_queuing_status(struct rte_eth_dev *dev) >>> unsigned int i; >>> int allow_queuing = 1; >>> >>> - if (rte_atomic32_read(&internal->dev_attached) == 0) >>> + if (!dev->data->rx_queues || !dev->data->tx_queues) { >>> + RTE_LOG(ERR, PMD, "RX/TX queues not exist yet\n"); >> Even it's not introduced in this patch, but I think we shall not report >> error log here. > if you attach one port with "port attach" (no vhost queue setup yet), and then execute > "port stop", the queue status is not exist yet, right? My point is that we allow a dev is not started but attached, so it's not an error, and we shall not print error log. >>> return; >>> + } >>> >>> - if (rte_atomic32_read(&internal->started) == 0) >>> + if (rte_atomic32_read(&internal->started) == 0 || >>> + rte_atomic32_read(&internal->dev_attached) == 0) >>> allow_queuing = 0; >>> >>> /* Wait until rx/tx_pkt_burst stops accessing vhost device */ >>> @@ -607,13 +610,10 @@ new_device(int vid) >>> #endif >>> >>> internal->vid = vid; >>> - if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) { >>> + if (rte_atomic32_read(&internal->started) == 1) >>> queue_setup(eth_dev, internal); >>> - rte_atomic32_set(&internal->dev_attached, 1); >>> - } else { >>> - RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n"); >>> - rte_atomic32_set(&internal->dev_attached, 0); >>> - } >>> + else >>> + RTE_LOG(INFO, PMD, "RX/TX queues not exist yet\n"); >>> >>> for (i = 0; i < rte_vhost_get_vring_num(vid); i++) >>> rte_vhost_enable_guest_notification(vid, i, 0); >>> @@ -622,6 +622,7 @@ new_device(int vid) >>> >>> eth_dev->data->dev_link.link_status = ETH_LINK_UP; >>> >>> + rte_atomic32_set(&internal->dev_attached, 1); >>> update_queuing_status(eth_dev); >>> >>> RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid); >>> @@ -651,23 +652,24 @@ destroy_device(int vid) >>> eth_dev = list->eth_dev; >>> internal = eth_dev->data->dev_private; >>> >>> - rte_atomic32_set(&internal->started, 0); >>> - update_queuing_status(eth_dev); >>> rte_atomic32_set(&internal->dev_attached, 0); >>> + update_queuing_status(eth_dev); >>> >>> eth_dev->data->dev_link.link_status = ETH_LINK_DOWN; >>> >>> - for (i = 0; i < eth_dev->data->nb_rx_queues; i++) { >>> - vq = eth_dev->data->rx_queues[i]; >>> - if (vq == NULL) >>> - continue; >>> - vq->vid = -1; >>> - } >>> - for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { >>> - vq = eth_dev->data->tx_queues[i]; >>> - if (vq == NULL) >>> - continue; >>> - vq->vid = -1; >>> + if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) { >>> + for (i = 0; i < eth_dev->data->nb_rx_queues; i++) { >>> + vq = eth_dev->data->rx_queues[i]; >>> + if (!vq) >>> + continue; >>> + vq->vid = -1; >>> + } >>> + for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { >>> + vq = eth_dev->data->tx_queues[i]; >>> + if (!vq) >>> + continue; >>> + vq->vid = -1; >>> + } >>> } >>> >>> state = vring_states[eth_dev->data->port_id]; >>> @@ -792,11 +794,7 @@ eth_dev_start(struct rte_eth_dev *eth_dev) >>> { >>> struct pmd_internal *internal = eth_dev->data->dev_private; >>> >>> - if (unlikely(rte_atomic32_read(&internal->dev_attached) == 0)) { >>> - queue_setup(eth_dev, internal); >>> - rte_atomic32_set(&internal->dev_attached, 1); >>> - } >>> - >>> + queue_setup(eth_dev, internal); >>> rte_atomic32_set(&internal->started, 1); >>> update_queuing_status(eth_dev); >>> >>> @@ -836,10 +834,13 @@ eth_dev_close(struct rte_eth_dev *dev) >>> pthread_mutex_unlock(&internal_list_lock); >>> rte_free(list); >>> >>> - for (i = 0; i < dev->data->nb_rx_queues; i++) >>> - rte_free(dev->data->rx_queues[i]); >>> - for (i = 0; i < dev->data->nb_tx_queues; i++) >>> - rte_free(dev->data->tx_queues[i]); >>> + if (dev->data->rx_queues) >>> + for (i = 0; i < dev->data->nb_rx_queues; i++) >>> + rte_free(dev->data->rx_queues[i]); >>> + >>> + if (dev->data->tx_queues) >>> + for (i = 0; i < dev->data->nb_tx_queues; i++) >>> + rte_free(dev->data->tx_queues[i]); >>> >>> rte_free(dev->data->mac_addrs); >>> free(internal->dev_name);