From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4AAA6A052B; Wed, 29 Jul 2020 15:20:02 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2E11C10A3; Wed, 29 Jul 2020 15:20:02 +0200 (CEST) Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by dpdk.org (Postfix) with ESMTP id 682841023 for ; Wed, 29 Jul 2020 15:20:01 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1596028800; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:autocrypt:autocrypt; bh=KB/m5Ut2Jj3MNW12nH7RZXB2XlxPxa81Uotg8inf1XU=; b=XtdLjsAGbjAVX0XtYY51FFXgcJAHiMxlyOB4fSymjHKbQEp7XC6y9bIGHESC/gBLDlJCNU 0lmWfQQlCugRcMsrU4g8lmYE9FEVGH/prmNxesPx0V4k5cBLRME0xug9TjXaVCuveEZyoK H/DvQGwcOyNKEORlBIyNicMX/Gwk/dM= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-220-krZDSELXNf6MqAXDd83m-Q-1; Wed, 29 Jul 2020 09:19:57 -0400 X-MC-Unique: krZDSELXNf6MqAXDd83m-Q-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AA48318C63CE; Wed, 29 Jul 2020 13:19:55 +0000 (UTC) Received: from [10.36.110.21] (unknown [10.36.110.21]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A8BC960CD1; Wed, 29 Jul 2020 13:19:44 +0000 (UTC) To: David Marchand Cc: dev , Matan Azrad , "Xia, Chenbo" , Marvin Liu , "Wang, Yinan" , Thomas Monjalon , "Yigit, Ferruh" References: <20200729092000.233036-1-maxime.coquelin@redhat.com> <20200729092000.233036-4-maxime.coquelin@redhat.com> From: Maxime Coquelin Autocrypt: addr=maxime.coquelin@redhat.com; keydata= mQINBFOEQQIBEADjNLYZZqghYuWv1nlLisptPJp+TSxE/KuP7x47e1Gr5/oMDJ1OKNG8rlNg kLgBQUki3voWhUbMb69ybqdMUHOl21DGCj0BTU3lXwapYXOAnsh8q6RRM+deUpasyT+Jvf3a gU35dgZcomRh5HPmKMU4KfeA38cVUebsFec1HuJAWzOb/UdtQkYyZR4rbzw8SbsOemtMtwOx YdXodneQD7KuRU9IhJKiEfipwqk2pufm2VSGl570l5ANyWMA/XADNhcEXhpkZ1Iwj3TWO7XR uH4xfvPl8nBsLo/EbEI7fbuUULcAnHfowQslPUm6/yaGv6cT5160SPXT1t8U9QDO6aTSo59N jH519JS8oeKZB1n1eLDslCfBpIpWkW8ZElGkOGWAN0vmpLfdyiqBNNyS3eGAfMkJ6b1A24un /TKc6j2QxM0QK4yZGfAxDxtvDv9LFXec8ENJYsbiR6WHRHq7wXl/n8guyh5AuBNQ3LIK44x0 KjGXP1FJkUhUuruGyZsMrDLBRHYi+hhDAgRjqHgoXi5XGETA1PAiNBNnQwMf5aubt+mE2Q5r qLNTgwSo2dpTU3+mJ3y3KlsIfoaxYI7XNsPRXGnZi4hbxmeb2NSXgdCXhX3nELUNYm4ArKBP LugOIT/zRwk0H0+RVwL2zHdMO1Tht1UOFGfOZpvuBF60jhMzbQARAQABtCxNYXhpbWUgQ29x dWVsaW4gPG1heGltZS5jb3F1ZWxpbkByZWRoYXQuY29tPokCOAQTAQIAIgUCV3u/5QIbAwYL CQgHAwIGFQgCCQoLBBYCAwECHgECF4AACgkQyjiNKEaHD4ma2g/+P+Hg9WkONPaY1J4AR7Uf kBneosS4NO3CRy0x4WYmUSLYMLx1I3VH6SVjqZ6uBoYy6Fs6TbF6SHNc7QbB6Qjo3neqnQR1 71Ua1MFvIob8vUEl3jAR/+oaE1UJKrxjWztpppQTukIk4oJOmXbL0nj3d8dA2QgHdTyttZ1H xzZJWWz6vqxCrUqHU7RSH9iWg9R2iuTzii4/vk1oi4Qz7y/q8ONOq6ffOy/t5xSZOMtZCspu Mll2Szzpc/trFO0pLH4LZZfz/nXh2uuUbk8qRIJBIjZH3ZQfACffgfNefLe2PxMqJZ8mFJXc RQO0ONZvwoOoHL6CcnFZp2i0P5ddduzwPdGsPq1bnIXnZqJSl3dUfh3xG5ArkliZ/++zGF1O wvpGvpIuOgLqjyCNNRoR7cP7y8F24gWE/HqJBXs1qzdj/5Hr68NVPV1Tu/l2D1KMOcL5sOrz 2jLXauqDWn1Okk9hkXAP7+0Cmi6QwAPuBT3i6t2e8UdtMtCE4sLesWS/XohnSFFscZR6Vaf3 gKdWiJ/fW64L6b9gjkWtHd4jAJBAIAx1JM6xcA1xMbAFsD8gA2oDBWogHGYcScY/4riDNKXi lw92d6IEHnSf6y7KJCKq8F+Jrj2BwRJiFKTJ6ChbOpyyR6nGTckzsLgday2KxBIyuh4w+hMq TGDSp2rmWGJjASq5Ag0EVPSbkwEQAMkaNc084Qvql+XW+wcUIY+Dn9A2D1gMr2BVwdSfVDN7 0ZYxo9PvSkzh6eQmnZNQtl8WSHl3VG3IEDQzsMQ2ftZn2sxjcCadexrQQv3Lu60Tgj7YVYRM H+fLYt9W5YuWduJ+FPLbjIKynBf6JCRMWr75QAOhhhaI0tsie3eDsKQBA0w7WCuPiZiheJaL 4MDe9hcH4rM3ybnRW7K2dLszWNhHVoYSFlZGYh+MGpuODeQKDS035+4H2rEWgg+iaOwqD7bg CQXwTZ1kSrm8NxIRVD3MBtzp9SZdUHLfmBl/tLVwDSZvHZhhvJHC6Lj6VL4jPXF5K2+Nn/Su CQmEBisOmwnXZhhu8ulAZ7S2tcl94DCo60ReheDoPBU8PR2TLg8rS5f9w6mLYarvQWL7cDtT d2eX3Z6TggfNINr/RTFrrAd7NHl5h3OnlXj7PQ1f0kfufduOeCQddJN4gsQfxo/qvWVB7PaE 1WTIggPmWS+Xxijk7xG6x9McTdmGhYaPZBpAxewK8ypl5+yubVsE9yOOhKMVo9DoVCjh5To5 aph7CQWfQsV7cd9PfSJjI2lXI0dhEXhQ7lRCFpf3V3mD6CyrhpcJpV6XVGjxJvGUale7+IOp sQIbPKUHpB2F+ZUPWds9yyVxGwDxD8WLqKKy0WLIjkkSsOb9UBNzgRyzrEC9lgQ/ABEBAAGJ Ah8EGAECAAkFAlT0m5MCGwwACgkQyjiNKEaHD4nU8hAAtt0xFJAy0sOWqSmyxTc7FUcX+pbD KVyPlpl6urKKMk1XtVMUPuae/+UwvIt0urk1mXi6DnrAN50TmQqvdjcPTQ6uoZ8zjgGeASZg jj0/bJGhgUr9U7oG7Hh2F8vzpOqZrdd65MRkxmc7bWj1k81tOU2woR/Gy8xLzi0k0KUa8ueB iYOcZcIGTcs9CssVwQjYaXRoeT65LJnTxYZif2pfNxfINFzCGw42s3EtZFteczClKcVSJ1+L +QUY/J24x0/ocQX/M1PwtZbB4c/2Pg/t5FS+s6UB1Ce08xsJDcwyOPIH6O3tccZuriHgvqKP yKz/Ble76+NFlTK1mpUlfM7PVhD5XzrDUEHWRTeTJSvJ8TIPL4uyfzhjHhlkCU0mw7Pscyxn DE8G0UYMEaNgaZap8dcGMYH/96EfE5s/nTX0M6MXV0yots7U2BDb4soLCxLOJz4tAFDtNFtA wLBhXRSvWhdBJZiig/9CG3dXmKfi2H+wdUCSvEFHRpgo7GK8/Kh3vGhgKmnnxhl8ACBaGy9n fxjSxjSO6rj4/MeenmlJw1yebzkX8ZmaSi8BHe+n6jTGEFNrbiOdWpJgc5yHIZZnwXaW54QT UhhSjDL1rV2B4F28w30jYmlRmm2RdN7iCZfbyP3dvFQTzQ4ySquuPkIGcOOHrvZzxbRjzMx1 Mwqu3GQ= Message-ID: <0d057cd7-47a2-542e-7dd5-1348c41ba48a@redhat.com> Date: Wed, 29 Jul 2020 15:19:42 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3 3/3] net/vhost: fix interrupt mode 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 7/29/20 1:27 PM, David Marchand wrote: > On Wed, Jul 29, 2020 at 11:20 AM Maxime Coquelin > wrote: >> >> At .new_device() time, only the first vring pair is >> now ready, other vrings are consfigured later. > > configured* > >> >> Problem is that when application will setup and enable >> interrupts, only the first queue pair Rx interrupt will >> be enabled. >> >> This patches fixes the issue by setting the number of >> max interrupts to the number of Rx queues that will be >> later initialized. Then, as soon as a Rx vring is ready >> and interrupt enabled by the application, it removes the >> corresponding uninitialized epoll event, and install a > > installs* > >> new one with the valid FD. >> >> Fixes: 604052ae5395 ("net/vhost: support queue update") >> >> Signed-off-by: Maxime Coquelin > > It seems a bit of a hack, but I _think_ the patch is good wrt races on > epoll configuration. > > We are only touching the vhost pmd, in interrupt mode. > The interrupt mode is not that frequently used (I found no usage in > opensource projects). > The vhost pmd is not used in OVS as it lags behind the vhost library > and has limitations. > > So my opinion is that the risk of taking this patch rather than > reverting the changes (which is not trivial iiuc) in the vhost library > is acceptable. > > > One comment below: > >> --- >> drivers/net/vhost/rte_eth_vhost.c | 75 +++++++++++++++++++++++++++---- >> 1 file changed, 66 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c >> index 951929c663..237785dd66 100644 >> --- a/drivers/net/vhost/rte_eth_vhost.c >> +++ b/drivers/net/vhost/rte_eth_vhost.c >> @@ -5,6 +5,7 @@ >> #include >> #include >> #include >> +#include >> >> #include >> #include >> @@ -95,6 +96,8 @@ struct vhost_queue { >> uint16_t port; >> uint16_t virtqueue_id; >> struct vhost_stats stats; >> + int intr_enable; >> + rte_spinlock_t intr_lock; >> }; >> >> struct pmd_internal { >> @@ -524,6 +527,45 @@ find_internal_resource(char *ifname) >> return list; >> } >> >> +static int >> +eth_vhost_update_intr(struct rte_eth_dev *eth_dev, uint16_t rxq_idx) >> +{ >> + struct rte_intr_handle *handle = eth_dev->intr_handle; >> + struct rte_epoll_event rev; >> + int epfd, ret; >> + >> + if (handle->efds[rxq_idx] == handle->elist[rxq_idx].fd) >> + return 0; > > Feel free to ignore if this situation can not happen. > > We are expecting only -1 -> valid fd transitions. > Maybe add an error log if we are in another situation? > This would indicate something quite broken. That's a very good idea, I will add such warning in v4. Thanks, Maxime > > >> + >> + VHOST_LOG(INFO, "kickfd for rxq-%d was changed, updating handler.\n", >> + rxq_idx); >> + >> + /* >> + * First remove invalid epoll event, and then isntall >> + * the new one. May be solved with a proper API in the >> + * future. >> + */ >> + epfd = handle->elist[rxq_idx].epfd; >> + rev = handle->elist[rxq_idx]; >> + ret = rte_epoll_ctl(epfd, EPOLL_CTL_DEL, rev.fd, >> + &handle->elist[rxq_idx]); >> + if (ret) { >> + VHOST_LOG(ERR, "Delete epoll event failed.\n"); >> + return ret; >> + } >> + >> + rev.fd = handle->efds[rxq_idx]; >> + handle->elist[rxq_idx] = rev; >> + ret = rte_epoll_ctl(epfd, EPOLL_CTL_ADD, rev.fd, >> + &handle->elist[rxq_idx]); >> + if (ret) { >> + VHOST_LOG(ERR, "Add epoll event failed.\n"); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> static int >> eth_rxq_intr_enable(struct rte_eth_dev *dev, uint16_t qid) >> { >> @@ -537,6 +579,11 @@ eth_rxq_intr_enable(struct rte_eth_dev *dev, uint16_t qid) >> return -1; >> } >> >> + rte_spinlock_lock(&vq->intr_lock); >> + vq->intr_enable = 1; >> + ret = eth_vhost_update_intr(dev, qid); >> + rte_spinlock_unlock(&vq->intr_lock); >> + >> ret = rte_vhost_get_vhost_vring(vq->vid, (qid << 1) + 1, &vring); >> if (ret < 0) { >> VHOST_LOG(ERR, "Failed to get rxq%d's vring\n", qid); >> @@ -571,6 +618,8 @@ eth_rxq_intr_disable(struct rte_eth_dev *dev, uint16_t qid) >> rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 0); >> rte_wmb(); >> >> + vq->intr_enable = 0; >> + >> return 0; >> } >> >> @@ -593,7 +642,6 @@ eth_vhost_install_intr(struct rte_eth_dev *dev) >> { >> struct rte_vhost_vring vring; >> struct vhost_queue *vq; >> - int count = 0; >> int nb_rxq = dev->data->nb_rx_queues; >> int i; >> int ret; >> @@ -623,6 +671,8 @@ eth_vhost_install_intr(struct rte_eth_dev *dev) >> >> VHOST_LOG(INFO, "Prepare intr vec\n"); >> for (i = 0; i < nb_rxq; i++) { >> + dev->intr_handle->intr_vec[i] = RTE_INTR_VEC_RXTX_OFFSET + i; >> + dev->intr_handle->efds[i] = -1; >> vq = dev->data->rx_queues[i]; >> if (!vq) { >> VHOST_LOG(INFO, "rxq-%d not setup yet, skip!\n", i); >> @@ -641,14 +691,12 @@ eth_vhost_install_intr(struct rte_eth_dev *dev) >> "rxq-%d's kickfd is invalid, skip!\n", i); >> continue; >> } >> - dev->intr_handle->intr_vec[i] = RTE_INTR_VEC_RXTX_OFFSET + i; >> dev->intr_handle->efds[i] = vring.kickfd; >> - count++; >> VHOST_LOG(INFO, "Installed intr vec for rxq-%d\n", i); >> } >> >> - dev->intr_handle->nb_efd = count; >> - dev->intr_handle->max_intr = count + 1; >> + dev->intr_handle->nb_efd = nb_rxq; >> + dev->intr_handle->max_intr = nb_rxq + 1; >> dev->intr_handle->type = RTE_INTR_HANDLE_VDEV; >> >> return 0; >> @@ -835,6 +883,7 @@ vring_conf_update(int vid, struct rte_eth_dev *eth_dev, uint16_t vring_id) >> { >> struct rte_eth_conf *dev_conf = ð_dev->data->dev_conf; >> struct pmd_internal *internal = eth_dev->data->dev_private; >> + struct vhost_queue *vq; >> struct rte_vhost_vring vring; >> int rx_idx = vring_id % 2 ? (vring_id - 1) >> 1 : -1; >> int ret = 0; >> @@ -853,12 +902,18 @@ vring_conf_update(int vid, struct rte_eth_dev *eth_dev, uint16_t vring_id) >> vring_id); >> return ret; >> } >> + eth_dev->intr_handle->efds[rx_idx] = vring.kickfd; >> >> - if (vring.kickfd != eth_dev->intr_handle->efds[rx_idx]) { >> - VHOST_LOG(INFO, "kickfd for rxq-%d was changed.\n", >> - rx_idx); >> - eth_dev->intr_handle->efds[rx_idx] = vring.kickfd; >> + vq = eth_dev->data->rx_queues[rx_idx]; >> + if (!vq) { >> + VHOST_LOG(ERR, "rxq%d is not setup yet\n", rx_idx); >> + return -1; >> } >> + >> + rte_spinlock_lock(&vq->intr_lock); >> + if (vq->intr_enable) >> + ret = eth_vhost_update_intr(eth_dev, rx_idx); >> + rte_spinlock_unlock(&vq->intr_lock); >> } >> >> return ret; >> @@ -1152,6 +1207,7 @@ eth_rx_queue_setup(struct rte_eth_dev *dev, uint16_t rx_queue_id, >> >> vq->mb_pool = mb_pool; >> vq->virtqueue_id = rx_queue_id * VIRTIO_QNUM + VIRTIO_TXQ; >> + rte_spinlock_init(&vq->intr_lock); >> dev->data->rx_queues[rx_queue_id] = vq; >> >> return 0; >> @@ -1173,6 +1229,7 @@ eth_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id, >> } >> >> vq->virtqueue_id = tx_queue_id * VIRTIO_QNUM + VIRTIO_RXQ; >> + rte_spinlock_init(&vq->intr_lock); >> dev->data->tx_queues[tx_queue_id] = vq; >> >> return 0; >> -- >> 2.26.2 >> > > > > -- > David Marchand >