From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 915E5292D for ; Thu, 3 Nov 2016 22:11:48 +0100 (CET) Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B472961BB8; Thu, 3 Nov 2016 21:11:47 +0000 (UTC) Received: from [10.36.6.90] (vpn1-6-90.ams2.redhat.com [10.36.6.90]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uA3LBhhM011077 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 3 Nov 2016 17:11:45 -0400 To: Yuanhan Liu , dev@dpdk.org References: <1478189400-14606-1-git-send-email-yuanhan.liu@linux.intel.com> <1478189400-14606-5-git-send-email-yuanhan.liu@linux.intel.com> From: Maxime Coquelin Message-ID: <3106a280-db21-604d-7f3f-dd2575efa1e8@redhat.com> Date: Thu, 3 Nov 2016 22:11:43 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1478189400-14606-5-git-send-email-yuanhan.liu@linux.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Thu, 03 Nov 2016 21:11:47 +0000 (UTC) Cc: Ilya Maximets Subject: Re: [dpdk-dev] [PATCH 4/8] net/virtio: allocate queue at init stage X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 03 Nov 2016 21:11:49 -0000 On 11/03/2016 05:09 PM, Yuanhan Liu wrote: > Queue allocation should be done once, since the queue related info (such > as vring addreess) will only be informed to the vhost-user backend once > without virtio device reset. > > That means, if you allocate queues again after the vhost-user negotiation, > the vhost-user backend will not be informed any more. Leading to a state > that the vring info mismatches between virtio PMD driver and vhost-backend: > the driver switches to the new address has just been allocated, while the > vhost-backend still sticks to the old address has been assigned in the init > stage. > > Unfortunately, that is exactly how the virtio driver is coded so far: queue > allocation is done at queue_setup stage (when rte_eth_tx/rx_queue_setup is > invoked). This is wrong, because queue_setup can be invoked several times. > For example, > > $ start_testpmd.sh ... --txq=1 --rxq=1 ... > > port stop 0 > > port config all txq 1 # just trigger the queue_setup callback again > > port config all rxq 1 > > port start 0 > > The right way to do is allocate the queues in the init stage, so that the > vring info could be persistent with the vhost-user backend. > > Besides that, we should allocate max_queue pairs the device supports, but > not nr queue pairs firstly configured, to make following case work. I understand, but how much memory overhead does that represent? Have you considered performing a device reset when queue number is changed? > > $ start_testpmd.sh ... --txq=1 --rxq=1 ... > > port stop 0 > > port config all txq 2 > > port config all rxq 2 > > port start 0 > > Signed-off-by: Yuanhan Liu > --- > drivers/net/virtio/virtio_ethdev.c | 105 +++++++++++++++++++++++-------------- > drivers/net/virtio/virtio_ethdev.h | 8 --- > drivers/net/virtio/virtio_pci.h | 2 + > drivers/net/virtio/virtio_rxtx.c | 38 +++++++------- > 4 files changed, 85 insertions(+), 68 deletions(-) > > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c > index 5a2c14b..253bcb5 100644 > --- a/drivers/net/virtio/virtio_ethdev.c > +++ b/drivers/net/virtio/virtio_ethdev.c > @@ -295,13 +295,19 @@ virtio_dev_queue_release(struct virtqueue *vq) > } > } > > -int virtio_dev_queue_setup(struct rte_eth_dev *dev, > - int queue_type, > - uint16_t queue_idx, > - uint16_t vtpci_queue_idx, > - uint16_t nb_desc, > - unsigned int socket_id, > - void **pvq) > +static int > +virtio_get_queue_type(struct virtio_hw *hw, uint16_t vtpci_queue_idx) > +{ > + if (vtpci_queue_idx == hw->max_queue_pairs * 2) > + return VTNET_CQ; > + else if (vtpci_queue_idx % 2 == 0) > + return VTNET_RQ; > + else > + return VTNET_TQ; > +} > + > +static int > +virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx) > { > char vq_name[VIRTQUEUE_MAX_NAME_SZ]; > char vq_hdr_name[VIRTQUEUE_MAX_NAME_SZ]; > @@ -314,6 +320,7 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev, > struct virtqueue *vq; > size_t sz_hdr_mz = 0; > void *sw_ring = NULL; > + int queue_type = virtio_get_queue_type(hw, vtpci_queue_idx); > int ret; > > PMD_INIT_LOG(DEBUG, "setting up queue: %u", vtpci_queue_idx); > @@ -351,18 +358,18 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev, > sz_hdr_mz = PAGE_SIZE; > } > > - vq = rte_zmalloc_socket(vq_name, size, RTE_CACHE_LINE_SIZE, socket_id); > + vq = rte_zmalloc_socket(vq_name, size, RTE_CACHE_LINE_SIZE, > + SOCKET_ID_ANY); > if (vq == NULL) { > PMD_INIT_LOG(ERR, "can not allocate vq"); > return -ENOMEM; > } > + hw->vqs[vtpci_queue_idx] = vq; > + > vq->hw = hw; > vq->vq_queue_index = vtpci_queue_idx; > vq->vq_nentries = vq_size; > - > - if (nb_desc == 0 || nb_desc > vq_size) > - nb_desc = vq_size; > - vq->vq_free_cnt = nb_desc; > + vq->vq_free_cnt = vq_size; > > /* > * Reserve a memzone for vring elements > @@ -372,7 +379,8 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev, > PMD_INIT_LOG(DEBUG, "vring_size: %d, rounded_vring_size: %d", > size, vq->vq_ring_size); > > - mz = rte_memzone_reserve_aligned(vq_name, vq->vq_ring_size, socket_id, > + mz = rte_memzone_reserve_aligned(vq_name, vq->vq_ring_size, > + SOCKET_ID_ANY, > 0, VIRTIO_PCI_VRING_ALIGN); > if (mz == NULL) { > if (rte_errno == EEXIST) > @@ -396,7 +404,7 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev, > snprintf(vq_hdr_name, sizeof(vq_hdr_name), "port%d_vq%d_hdr", > dev->data->port_id, vtpci_queue_idx); > hdr_mz = rte_memzone_reserve_aligned(vq_hdr_name, sz_hdr_mz, > - socket_id, 0, > + SOCKET_ID_ANY, 0, > RTE_CACHE_LINE_SIZE); > if (hdr_mz == NULL) { > if (rte_errno == EEXIST) > @@ -413,7 +421,7 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev, > sizeof(vq->sw_ring[0]); > > sw_ring = rte_zmalloc_socket("sw_ring", sz_sw, > - RTE_CACHE_LINE_SIZE, socket_id); > + RTE_CACHE_LINE_SIZE, SOCKET_ID_ANY); > if (!sw_ring) { > PMD_INIT_LOG(ERR, "can not allocate RX soft ring"); > ret = -ENOMEM; > @@ -424,19 +432,14 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev, > rxvq = &vq->rxq; > rxvq->vq = vq; > rxvq->port_id = dev->data->port_id; > - rxvq->queue_id = queue_idx; > rxvq->mz = mz; > - *pvq = rxvq; > } else if (queue_type == VTNET_TQ) { > txvq = &vq->txq; > txvq->vq = vq; > txvq->port_id = dev->data->port_id; > - txvq->queue_id = queue_idx; > txvq->mz = mz; > txvq->virtio_net_hdr_mz = hdr_mz; > txvq->virtio_net_hdr_mem = hdr_mz->phys_addr; > - > - *pvq = txvq; > } else if (queue_type == VTNET_CQ) { > cvq = &vq->cq; > cvq->vq = vq; > @@ -444,7 +447,8 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev, > cvq->virtio_net_hdr_mz = hdr_mz; > cvq->virtio_net_hdr_mem = hdr_mz->phys_addr; > memset(cvq->virtio_net_hdr_mz->addr, 0, PAGE_SIZE); > - *pvq = cvq; > + > + hw->cvq = cvq; > } > > /* For virtio_user case (that is when dev->pci_dev is NULL), we use > @@ -502,23 +506,45 @@ fail_q_alloc: > } > > static int > -virtio_dev_cq_queue_setup(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx, > - uint32_t socket_id) > +virtio_alloc_queues(struct rte_eth_dev *dev) > { > - struct virtnet_ctl *cvq; > - int ret; > struct virtio_hw *hw = dev->data->dev_private; > + uint16_t nr_vq = hw->max_queue_pairs * 2; > + uint16_t i; > + int ret; > > - PMD_INIT_FUNC_TRACE(); > - ret = virtio_dev_queue_setup(dev, VTNET_CQ, VTNET_SQ_CQ_QUEUE_IDX, > - vtpci_queue_idx, 0, socket_id, (void **)&cvq); > - if (ret < 0) { > - PMD_INIT_LOG(ERR, "control vq initialization failed"); > - return ret; > + if (vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_VQ)) > + nr_vq += 1; > + > + hw->vqs = rte_zmalloc(NULL, sizeof(struct virtqueue *) * nr_vq, 0); > + if (!hw->vqs) { > + PMD_INIT_LOG(ERR, "failed to allocate vqs"); > + return -ENOMEM; > + } > + > + for (i = 0; i < nr_vq; i++) { > + ret = virtio_init_queue(dev, i); > + if (ret < 0) > + goto cleanup; > } > > - hw->cvq = cvq; > return 0; > + > +cleanup: > + /* > + * ctrl queue is the last queue; if we go here, it means the ctrl > + * queue is not allocated, that we can do no cleanup for it here. > + */ > + while (i > 0) { > + i--; > + if (i % 2 == 0) > + virtio_dev_rx_queue_release(&hw->vqs[i]->rxq); > + else > + virtio_dev_tx_queue_release(&hw->vqs[i]->txq); > + } > + rte_free(hw->vqs); > + > + return ret; > } > > static void > @@ -1141,6 +1167,7 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features) > struct virtio_net_config *config; > struct virtio_net_config local_config; > struct rte_pci_device *pci_dev = eth_dev->pci_dev; > + int ret; > > /* Reset the device although not necessary at startup */ > vtpci_reset(hw); > @@ -1222,6 +1249,10 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features) > hw->max_queue_pairs = 1; > } > > + ret = virtio_alloc_queues(eth_dev); > + if (ret < 0) > + return ret; > + > if (pci_dev) > PMD_INIT_LOG(DEBUG, "port %d vendorID=0x%x deviceID=0x%x", > eth_dev->data->port_id, pci_dev->id.vendor_id, > @@ -1390,15 +1421,9 @@ virtio_dev_configure(struct rte_eth_dev *dev) > return -ENOTSUP; > } > > - /* Setup and start control queue */ > - if (vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_VQ)) { > - ret = virtio_dev_cq_queue_setup(dev, > - hw->max_queue_pairs * 2, > - SOCKET_ID_ANY); > - if (ret < 0) > - return ret; > + /* start control queue */ > + if (vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_VQ)) > virtio_dev_cq_start(dev); > - } > > hw->vlan_strip = rxmode->hw_vlan_strip; > > diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h > index de33b32..5db8632 100644 > --- a/drivers/net/virtio/virtio_ethdev.h > +++ b/drivers/net/virtio/virtio_ethdev.h > @@ -80,14 +80,6 @@ void virtio_dev_cq_start(struct rte_eth_dev *dev); > */ > void virtio_dev_rxtx_start(struct rte_eth_dev *dev); > > -int virtio_dev_queue_setup(struct rte_eth_dev *dev, > - int queue_type, > - uint16_t queue_idx, > - uint16_t vtpci_queue_idx, > - uint16_t nb_desc, > - unsigned int socket_id, > - void **pvq); > - > void virtio_dev_queue_release(struct virtqueue *vq); > > int virtio_dev_rx_queue_setup(struct rte_eth_dev *dev, uint16_t rx_queue_id, > diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h > index 0c5ed31..f63f76c 100644 > --- a/drivers/net/virtio/virtio_pci.h > +++ b/drivers/net/virtio/virtio_pci.h > @@ -264,6 +264,8 @@ struct virtio_hw { > struct virtio_net_config *dev_cfg; > const struct virtio_pci_ops *vtpci_ops; > void *virtio_user_dev; > + > + struct virtqueue **vqs; > }; > > /* > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c > index b4c4aa4..fb703d2 100644 > --- a/drivers/net/virtio/virtio_rxtx.c > +++ b/drivers/net/virtio/virtio_rxtx.c > @@ -530,24 +530,24 @@ int > virtio_dev_rx_queue_setup(struct rte_eth_dev *dev, > uint16_t queue_idx, > uint16_t nb_desc, > - unsigned int socket_id, > + unsigned int socket_id __rte_unused, > __rte_unused const struct rte_eth_rxconf *rx_conf, > struct rte_mempool *mp) > { > uint16_t vtpci_queue_idx = 2 * queue_idx + VTNET_SQ_RQ_QUEUE_IDX; > + struct virtio_hw *hw = dev->data->dev_private; > + struct virtqueue *vq = hw->vqs[vtpci_queue_idx]; > struct virtnet_rx *rxvq; > - int ret; > > PMD_INIT_FUNC_TRACE(); > - ret = virtio_dev_queue_setup(dev, VTNET_RQ, queue_idx, vtpci_queue_idx, > - nb_desc, socket_id, (void **)&rxvq); > - if (ret < 0) { > - PMD_INIT_LOG(ERR, "rvq initialization failed"); > - return ret; > - } > > - /* Create mempool for rx mbuf allocation */ > + if (nb_desc == 0 || nb_desc > vq->vq_nentries) > + nb_desc = vq->vq_nentries; > + vq->vq_free_cnt = RTE_MIN(vq->vq_free_cnt, nb_desc); > + > + rxvq = &vq->rxq; > rxvq->mpool = mp; > + rxvq->queue_id = queue_idx; > > dev->data->rx_queues[queue_idx] = rxvq; > > @@ -613,27 +613,25 @@ int > virtio_dev_tx_queue_setup(struct rte_eth_dev *dev, > uint16_t queue_idx, > uint16_t nb_desc, > - unsigned int socket_id, > + unsigned int socket_id __rte_unused, > const struct rte_eth_txconf *tx_conf) > { > uint8_t vtpci_queue_idx = 2 * queue_idx + VTNET_SQ_TQ_QUEUE_IDX; > + struct virtio_hw *hw = dev->data->dev_private; > + struct virtqueue *vq = hw->vqs[vtpci_queue_idx]; > struct virtnet_tx *txvq; > - struct virtqueue *vq; > uint16_t tx_free_thresh; > - int ret; > > PMD_INIT_FUNC_TRACE(); > > - > virtio_update_rxtx_handler(dev, tx_conf); > > - ret = virtio_dev_queue_setup(dev, VTNET_TQ, queue_idx, vtpci_queue_idx, > - nb_desc, socket_id, (void **)&txvq); > - if (ret < 0) { > - PMD_INIT_LOG(ERR, "tvq initialization failed"); > - return ret; > - } > - vq = txvq->vq; > + if (nb_desc == 0 || nb_desc > vq->vq_nentries) > + nb_desc = vq->vq_nentries; > + vq->vq_free_cnt = RTE_MIN(vq->vq_free_cnt, nb_desc); > + > + txvq = &vq->txq; > + txvq->queue_id = queue_idx; > > tx_free_thresh = tx_conf->tx_free_thresh; > if (tx_free_thresh == 0) >