From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id 99CA629CB for ; Thu, 29 Mar 2018 15:16:05 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B1743BD9E; Thu, 29 Mar 2018 13:16:03 +0000 (UTC) Received: from [10.36.112.54] (ovpn-112-54.ams2.redhat.com [10.36.112.54]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8FC1F215CDC5; Thu, 29 Mar 2018 13:16:02 +0000 (UTC) To: Junjie Chen , jianfeng.tan@intel.com, mtetsuyah@gmail.com Cc: dev@dpdk.org References: <1522166726-42025-1-git-send-email-junjie.j.chen@intel.com> <20180329153544.270488-1-junjie.j.chen@intel.com> From: Maxime Coquelin Message-ID: Date: Thu, 29 Mar 2018 15:16:01 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180329153544.270488-1-junjie.j.chen@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Thu, 29 Mar 2018 13:16:03 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Thu, 29 Mar 2018 13:16:03 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'maxime.coquelin@redhat.com' RCPT:'' Subject: Re: [dpdk-dev] [PATCH v2] net/vhost: fix segfault when creating vdev dynamically 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, 29 Mar 2018 13:16:05 -0000 Hi Junjie, Thanks for the patch, it looks like the right thing to do. Please find my comments below: On 03/29/2018 05:35 PM, Junjie Chen wrote: > when creating vdev dynamically, vhost pmd driver start directly without s/when/When/ s/start/starts/ > checking TX/RX queues ready or not, and thus cause segmentation fault when s/ready/are ready/ s/cause/causes/ > vhost library accessing queues. This patch add flag to check whether queues s/accessing/accesses/ s/add flag/adds a flag/ > setup or not, and add driver start call into dev_start to allow user start s/setup/are setup/ s/add/adds/ s/start/ to start/ > it after setting up queue. s/queue/queues/ > > Signed-off-by: Junjie Chen > --- > Changes in v2: > - check queue status in new_device, create queue in dev_start if not setup yet > drivers/net/vhost/rte_eth_vhost.c | 73 ++++++++++++++++++++++++++++----------- > 1 file changed, 53 insertions(+), 20 deletions(-) > > diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c > index 3aae01c39..41410fa5a 100644 > --- a/drivers/net/vhost/rte_eth_vhost.c > +++ b/drivers/net/vhost/rte_eth_vhost.c > @@ -117,7 +117,9 @@ struct pmd_internal { > char *dev_name; > char *iface_name; > uint16_t max_queues; > + uint16_t vid; > rte_atomic32_t started; > + rte_atomic32_t once; The name is a bit vague. Also, I wonder if we need a new variable. Couldn't we rely on dev_attached? In new_device(), only set dev_attached=1 if queues are allocated. And in eth_dev_start(), attach queues if !dev_attached and then set dev_attached to 1. > }; > > struct internal_list { > @@ -580,21 +582,27 @@ new_device(int vid) > eth_dev->data->numa_node = newnode; > #endif > > - for (i = 0; i < eth_dev->data->nb_rx_queues; i++) { > - vq = eth_dev->data->rx_queues[i]; > - if (vq == NULL) > - continue; > - vq->vid = vid; > - vq->internal = internal; > - vq->port = eth_dev->data->port_id; > - } > - for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { > - vq = eth_dev->data->tx_queues[i]; > - if (vq == NULL) > - continue; > - vq->vid = vid; > - vq->internal = internal; > - vq->port = eth_dev->data->port_id; > + 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 = vid; > + vq->internal = internal; > + vq->port = eth_dev->data->port_id; > + } > + for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { > + vq = eth_dev->data->tx_queues[i]; > + if (!vq) > + continue; > + vq->vid = vid; > + vq->internal = internal; > + vq->port = eth_dev->data->port_id; > + } > + } else { > + RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n"); > + internal->vid = vid; > + rte_atomic32_set(&internal->once, 0); > } > > for (i = 0; i < rte_vhost_get_vring_num(vid); i++) > @@ -605,7 +613,8 @@ 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); > + if (likely(rte_atomic32_read(&internal->once) == 1)) > + update_queuing_status(eth_dev); > > RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid); > > @@ -770,12 +779,34 @@ rte_eth_vhost_get_vid_from_port_id(uint16_t port_id) > } > > static int > -eth_dev_start(struct rte_eth_dev *dev) > +eth_dev_start(struct rte_eth_dev *eth_dev) > { > - struct pmd_internal *internal = dev->data->dev_private; > + struct pmd_internal *internal = eth_dev->data->dev_private; > + struct vhost_queue *vq; > + int i; > + > + if (unlikely(rte_atomic32_read(&internal->once) == 0)) { > + for (i = 0; i < eth_dev->data->nb_rx_queues; i++) { > + vq = eth_dev->data->rx_queues[i]; > + if (!vq) > + continue; > + vq->vid = internal->vid; > + vq->internal = internal; > + vq->port = eth_dev->data->port_id; > + } > + for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { > + vq = eth_dev->data->tx_queues[i]; > + if (!vq) > + continue; > + vq->vid = internal->vid; > + vq->internal = internal; > + vq->port = eth_dev->data->port_id; > + } I think you could avoid the code duplication by creating a new function to setup queues: void queue_setup(struct rte_eth_dev *eth_dev, struct pmd_internal *internal) { int i; for (i = 0; i < eth_dev->data->nb_rx_queues; i++) { vq = eth_dev->data->rx_queues[i]; if (!vq) continue; vq->vid = internal->vid; vq->internal = internal; vq->port = eth_dev->data->port_id; } for (i = 0; i < eth_dev->data->nb_tx_queues; i++) { vq = eth_dev->data->tx_queues[i]; if (!vq) continue; vq->vid = internal->vid; vq->internal = internal; vq->port = eth_dev->data->port_id; } } > + rte_atomic32_set(&internal->once, 1); Indentation looks wrong here. > + } > > rte_atomic32_set(&internal->started, 1); > - update_queuing_status(dev); > + update_queuing_status(eth_dev); > > return 0; > } > @@ -786,7 +817,9 @@ eth_dev_stop(struct rte_eth_dev *dev) > struct pmd_internal *internal = dev->data->dev_private; > > rte_atomic32_set(&internal->started, 0); > - update_queuing_status(dev); > + > + if (likely(rte_atomic32_read(&internal->once) == 1)) > + update_queuing_status(dev); > } > > static void >