From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 56D7E1BC32 for ; Thu, 12 Apr 2018 09:34:26 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Apr 2018 00:34:25 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,440,1517904000"; d="scan'208";a="190888568" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga004.jf.intel.com with ESMTP; 12 Apr 2018 00:34:24 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 12 Apr 2018 00:34:24 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.115]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.239]) with mapi id 14.03.0319.002; Thu, 12 Apr 2018 15:34:22 +0800 From: "Chen, Junjie J" To: Maxime Coquelin , "Tan, Jianfeng" , "mtetsuyah@gmail.com" CC: "dev@dpdk.org" Thread-Topic: [PATCH v3] net/vhost: fix vhost invalid state Thread-Index: AQHT0XbcVrMsvSAaHkGVYwS99VBy5aP8NK0AgAACYACAAIZmsA== Date: Thu, 12 Apr 2018 07:34:22 +0000 Message-ID: References: <1523443993-176139-1-git-send-email-junjie.j.chen@intel.com> <1523466152-181190-1-git-send-email-junjie.j.chen@intel.com> <75c33a22-9b4c-f293-07ee-daad99f3cad0@intel.com> <6288ba60-eebb-a743-aa1d-a61c32a07c58@redhat.com> In-Reply-To: <6288ba60-eebb-a743-aa1d-a61c32a07c58@redhat.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: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOTcyYmQwZjItNjcxNi00YTliLTk5YTEtNWExYmE5YTJkODljIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiIzWmNkMWdyOWFaYWh2Z3FkZzlyTE11ZVFMdVFZbGFibDdoMWh4TWwyWWlEa2wyMjNGQzBcL1pXMmtRc2YxeDJZeiJ9 x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3] 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: Thu, 12 Apr 2018 07:34:26 -0000 >=20 >=20 >=20 > On 04/12/2018 09:21 AM, Tan, Jianfeng wrote: > > > > > > On 4/12/2018 1:02 AM, 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). > >> > >> Fixes: 30a701a53737 ("net/vhost: fix crash when creating vdev > >> dynamically") > >> > >> Signed-off-by: Junjie Chen > >> Tested-by: Jens Freimann > > > > Overall, looks great to me except a nit below. > > > > Reviewed-by: Jianfeng Tan >=20 > Thanks Jianfeng, I can handle the small change while applying. >=20 > Can you confirm that it is implied that the queue are already allocated, = else we > wouldn't find the internal resource and quit earlier (in case of eth_dev_= close > called twice for example)? That is required, otherwise it generate segfault if we close device before = queue setup. For example we execute following steps in testpmd: 1. port attach 2. ctrl+D >=20 > Thanks, > Maxime >=20 > > > >> --- > >> Changes in v3: > >> - remove useless log in queue status showing Changes in v2: > >> - use started to determine vhost queues readiness > >> - revert setting started to zero in destroy_device > >> =A0 drivers/net/vhost/rte_eth_vhost.c | 59 > >> +++++++++++++++++++-------------------- > >> =A0 1 file changed, 29 insertions(+), 30 deletions(-) > >> > >> diff --git a/drivers/net/vhost/rte_eth_vhost.c > >> b/drivers/net/vhost/rte_eth_vhost.c > >> index 11b6076..e392d71 100644 > >> --- a/drivers/net/vhost/rte_eth_vhost.c > >> +++ b/drivers/net/vhost/rte_eth_vhost.c > >> @@ -528,10 +528,11 @@ update_queuing_status(struct rte_eth_dev > *dev) > >> =A0=A0=A0=A0=A0 unsigned int i; > >> =A0=A0=A0=A0=A0 int allow_queuing =3D 1; > >> -=A0=A0=A0 if (rte_atomic32_read(&internal->dev_attached) =3D=3D 0) > >> +=A0=A0=A0 if (!dev->data->rx_queues || !dev->data->tx_queues) > >> =A0=A0=A0=A0=A0=A0=A0=A0=A0 return; > >> -=A0=A0=A0 if (rte_atomic32_read(&internal->started) =3D=3D 0) > >> +=A0=A0=A0 if (rte_atomic32_read(&internal->started) =3D=3D 0 || > >> +=A0=A0=A0=A0=A0=A0=A0 rte_atomic32_read(&internal->dev_attached) =3D= =3D 0) > >> =A0=A0=A0=A0=A0=A0=A0=A0=A0 allow_queuing =3D 0; > >> =A0=A0=A0=A0=A0 /* Wait until rx/tx_pkt_burst stops accessing vhost de= vice */ > >> @@ -607,13 +608,10 @@ new_device(int vid) > >> =A0 #endif > >> =A0=A0=A0=A0=A0 internal->vid =3D vid; > >> -=A0=A0=A0 if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) { > >> +=A0=A0=A0 if (rte_atomic32_read(&internal->started) =3D=3D 1) > >> =A0=A0=A0=A0=A0=A0=A0=A0=A0 queue_setup(eth_dev, internal); > >> -=A0=A0=A0=A0=A0=A0=A0 rte_atomic32_set(&internal->dev_attached, 1); > >> -=A0=A0=A0 } else { > >> -=A0=A0=A0=A0=A0=A0=A0 RTE_LOG(INFO, PMD, "RX/TX queues have not setup= yet\n"); > >> -=A0=A0=A0=A0=A0=A0=A0 rte_atomic32_set(&internal->dev_attached, 0); > >> -=A0=A0=A0 } > >> +=A0=A0=A0 else > >> +=A0=A0=A0=A0=A0=A0=A0 RTE_LOG(INFO, PMD, "RX/TX queues not exist yet\= n"); > >> =A0=A0=A0=A0=A0 for (i =3D 0; i < rte_vhost_get_vring_num(vid); i++) > >> =A0=A0=A0=A0=A0=A0=A0=A0=A0 rte_vhost_enable_guest_notification(vid, i= , 0); @@ -622,6 > >> +620,7 @@ new_device(int vid) > >> =A0=A0=A0=A0=A0 eth_dev->data->dev_link.link_status =3D ETH_LINK_UP; > >> +=A0=A0=A0 rte_atomic32_set(&internal->dev_attached, 1); > >> =A0=A0=A0=A0=A0 update_queuing_status(eth_dev); > >> =A0=A0=A0=A0=A0 RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid); = @@ > >> -651,23 +650,24 @@ destroy_device(int vid) > >> =A0=A0=A0=A0=A0 eth_dev =3D list->eth_dev; > >> =A0=A0=A0=A0=A0 internal =3D eth_dev->data->dev_private; > >> -=A0=A0=A0 rte_atomic32_set(&internal->started, 0); > >> -=A0=A0=A0 update_queuing_status(eth_dev); > >> =A0=A0=A0=A0=A0 rte_atomic32_set(&internal->dev_attached, 0); > >> +=A0=A0=A0 update_queuing_status(eth_dev); > >> =A0=A0=A0=A0=A0 eth_dev->data->dev_link.link_status =3D ETH_LINK_DOWN; > >> -=A0=A0=A0 for (i =3D 0; i < eth_dev->data->nb_rx_queues; i++) { > >> -=A0=A0=A0=A0=A0=A0=A0 vq =3D eth_dev->data->rx_queues[i]; > >> -=A0=A0=A0=A0=A0=A0=A0 if (vq =3D=3D NULL) > >> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 continue; > >> -=A0=A0=A0=A0=A0=A0=A0 vq->vid =3D -1; > >> -=A0=A0=A0 } > >> -=A0=A0=A0 for (i =3D 0; i < eth_dev->data->nb_tx_queues; i++) { > >> -=A0=A0=A0=A0=A0=A0=A0 vq =3D eth_dev->data->tx_queues[i]; > >> -=A0=A0=A0=A0=A0=A0=A0 if (vq =3D=3D NULL) > >> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 continue; > >> -=A0=A0=A0=A0=A0=A0=A0 vq->vid =3D -1; > >> +=A0=A0=A0 if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) { > >> +=A0=A0=A0=A0=A0=A0=A0 for (i =3D 0; i < eth_dev->data->nb_rx_queues; = i++) { > >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 vq =3D eth_dev->data->rx_queues[i]; > >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (!vq) > >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 continue; > >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 vq->vid =3D -1; > >> +=A0=A0=A0=A0=A0=A0=A0 } > >> +=A0=A0=A0=A0=A0=A0=A0 for (i =3D 0; i < eth_dev->data->nb_tx_queues; = i++) { > >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 vq =3D eth_dev->data->tx_queues[i]; > >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (!vq) > >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 continue; > >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 vq->vid =3D -1; > >> +=A0=A0=A0=A0=A0=A0=A0 } > >> =A0=A0=A0=A0=A0 } > >> =A0=A0=A0=A0=A0 state =3D vring_states[eth_dev->data->port_id]; > >> @@ -792,11 +792,7 @@ eth_dev_start(struct rte_eth_dev *eth_dev) > >> =A0 { > >> =A0=A0=A0=A0=A0 struct pmd_internal *internal =3D eth_dev->data->dev_p= rivate; > >> -=A0=A0=A0 if (unlikely(rte_atomic32_read(&internal->dev_attached) =3D= =3D 0)) { > >> -=A0=A0=A0=A0=A0=A0=A0 queue_setup(eth_dev, internal); > >> -=A0=A0=A0=A0=A0=A0=A0 rte_atomic32_set(&internal->dev_attached, 1); > >> -=A0=A0=A0 } > >> - > >> +=A0=A0=A0 queue_setup(eth_dev, internal); > >> =A0=A0=A0=A0=A0 rte_atomic32_set(&internal->started, 1); > >> =A0=A0=A0=A0=A0 update_queuing_status(eth_dev); @@ -836,10 +832,13 @@ > >> eth_dev_close(struct rte_eth_dev *dev) > >> =A0=A0=A0=A0=A0 pthread_mutex_unlock(&internal_list_lock); > >> =A0=A0=A0=A0=A0 rte_free(list); > >> -=A0=A0=A0 for (i =3D 0; i < dev->data->nb_rx_queues; i++) > >> -=A0=A0=A0=A0=A0=A0=A0 rte_free(dev->data->rx_queues[i]); > >> -=A0=A0=A0 for (i =3D 0; i < dev->data->nb_tx_queues; i++) > >> -=A0=A0=A0=A0=A0=A0=A0 rte_free(dev->data->tx_queues[i]); > >> +=A0=A0=A0 if (dev->data->rx_queues) > > > > This is implied that rx_queues is already allocated. So I don't think > > we need this. > > > >> +=A0=A0=A0=A0=A0=A0=A0 for (i =3D 0; i < dev->data->nb_rx_queues; i++) > >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 rte_free(dev->data->rx_queues[i]); > >> + > >> +=A0=A0=A0 if (dev->data->tx_queues) > >> +=A0=A0=A0=A0=A0=A0=A0 for (i =3D 0; i < dev->data->nb_tx_queues; i++) > >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 rte_free(dev->data->tx_queues[i]); > >> =A0=A0=A0=A0=A0 rte_free(dev->data->mac_addrs); > >> =A0=A0=A0=A0=A0 free(internal->dev_name); > >