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 33B9E1B8D4 for ; Wed, 11 Apr 2018 11:17:30 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Apr 2018 02:17:28 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,435,1517904000"; d="scan'208";a="32813253" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga008.jf.intel.com with ESMTP; 11 Apr 2018 02:17:28 -0700 Received: from fmsmsx117.amr.corp.intel.com (10.18.116.17) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 11 Apr 2018 02:17:28 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx117.amr.corp.intel.com (10.18.116.17) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 11 Apr 2018 02:17:27 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.115]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.197]) with mapi id 14.03.0319.002; Wed, 11 Apr 2018 17:17:26 +0800 From: "Chen, Junjie J" To: "Tan, Jianfeng" , "maxime.coquelin@redhat.com" , "mtetsuyah@gmail.com" CC: "dev@dpdk.org" Thread-Topic: [PATCH v2] net/vhost: fix vhost invalid state Thread-Index: AQHT0UNA8CSFxmcL0kSwcpFQqPV89aP6sLUAgACIOBD//4WhAIAAho6g Date: Wed, 11 Apr 2018 09:17:25 +0000 Message-ID: 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> <7946ae32-8728-fca3-f5da-61bda57f2289@intel.com> In-Reply-To: <7946ae32-8728-fca3-f5da-61bda57f2289@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZjMyN2I2YTMtMjYzNC00MmEyLWEzMzYtMTVjMjAwMGEzMThhIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJkcnVIRUExbGZuZ0loTW93UU9jaWZkR1pBNkMxVFB2aml0TUZNbjg0RTRuVWhDd0FzRXZcL2wwWlZraEN4Qm9rUiJ9 x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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:17:30 -0000 Hi Jianfeng. > 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. >=20 > OK, that means it is also due to the wrong use of dev_attached? Yes, a mixed wrong. That is why I tried to use new variable to track queue.= One variable for one state. >=20 > >>> 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 =3D 1; > >>> > >>> - if (rte_atomic32_read(&internal->dev_attached) =3D=3D 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? >=20 > 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. OK, will remove it in V3.=20 >=20 > >>> return; > >>> + } > >>> > >>> - if (rte_atomic32_read(&internal->started) =3D=3D 0) > >>> + if (rte_atomic32_read(&internal->started) =3D=3D 0 || > >>> + rte_atomic32_read(&internal->dev_attached) =3D=3D 0) > >>> allow_queuing =3D 0; > >>> > >>> /* Wait until rx/tx_pkt_burst stops accessing vhost device */ @@ > >>> -607,13 +610,10 @@ new_device(int vid) > >>> #endif > >>> > >>> internal->vid =3D vid; > >>> - if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) { > >>> + if (rte_atomic32_read(&internal->started) =3D=3D 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 =3D 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 =3D 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 =3D list->eth_dev; > >>> internal =3D 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 =3D ETH_LINK_DOWN; > >>> > >>> - for (i =3D 0; i < eth_dev->data->nb_rx_queues; i++) { > >>> - vq =3D eth_dev->data->rx_queues[i]; > >>> - if (vq =3D=3D NULL) > >>> - continue; > >>> - vq->vid =3D -1; > >>> - } > >>> - for (i =3D 0; i < eth_dev->data->nb_tx_queues; i++) { > >>> - vq =3D eth_dev->data->tx_queues[i]; > >>> - if (vq =3D=3D NULL) > >>> - continue; > >>> - vq->vid =3D -1; > >>> + if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) { > >>> + for (i =3D 0; i < eth_dev->data->nb_rx_queues; i++) { > >>> + vq =3D eth_dev->data->rx_queues[i]; > >>> + if (!vq) > >>> + continue; > >>> + vq->vid =3D -1; > >>> + } > >>> + for (i =3D 0; i < eth_dev->data->nb_tx_queues; i++) { > >>> + vq =3D eth_dev->data->tx_queues[i]; > >>> + if (!vq) > >>> + continue; > >>> + vq->vid =3D -1; > >>> + } > >>> } > >>> > >>> state =3D vring_states[eth_dev->data->port_id]; > >>> @@ -792,11 +794,7 @@ eth_dev_start(struct rte_eth_dev *eth_dev) > >>> { > >>> struct pmd_internal *internal =3D eth_dev->data->dev_private; > >>> > >>> - if (unlikely(rte_atomic32_read(&internal->dev_attached) =3D=3D 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 =3D 0; i < dev->data->nb_rx_queues; i++) > >>> - rte_free(dev->data->rx_queues[i]); > >>> - for (i =3D 0; i < dev->data->nb_tx_queues; i++) > >>> - rte_free(dev->data->tx_queues[i]); > >>> + if (dev->data->rx_queues) > >>> + for (i =3D 0; i < dev->data->nb_rx_queues; i++) > >>> + rte_free(dev->data->rx_queues[i]); > >>> + > >>> + if (dev->data->tx_queues) > >>> + for (i =3D 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);