From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 3FEC71BA96 for ; Wed, 11 Apr 2018 10:35:15 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Apr 2018 01:35:14 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,435,1517904000"; d="scan'208";a="45998974" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga001.fm.intel.com with ESMTP; 11 Apr 2018 01:35:14 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 11 Apr 2018 01:35:13 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.115]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.149]) with mapi id 14.03.0319.002; Wed, 11 Apr 2018 16:35:11 +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: AQHT0UNA8CSFxmcL0kSwcpFQqPV89aP6sLUAgACIOBA= Date: Wed, 11 Apr 2018 08:35:11 +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> In-Reply-To: <0618790b-5ba6-e830-f5d2-35e25d091a36@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: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTdhNDE1MWMtMDJkOC00Mzk1LTkyNmItNTQ5NGU2ZTA2Yjk3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJpWEk4a2FVeW5hQUVyU2tLYWhyZ3FJbjVyOVZOTjJFcm5xNGcxTkNtaWNLQnorQ0ZpeXBtXC9kbXM0bTcrU0h6diJ9 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 08:35:16 -0000 >=20 >=20 > 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). >=20 > I see two issues from your description: >=20 > - 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 issu= e, 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 > > > > 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"); >=20 > 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 t= hen execute=20 "port stop", the queue status is not exist yet, right? >=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);