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 1C4D0A052B; Wed, 29 Jul 2020 13:27:44 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id CE3FB4C8E; Wed, 29 Jul 2020 13:27:43 +0200 (CEST) Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [63.128.21.74]) by dpdk.org (Postfix) with ESMTP id EA04737B7 for ; Wed, 29 Jul 2020 13:27:42 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1596022062; 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: in-reply-to:in-reply-to:references:references; bh=CayZAc+JG43hWjsKPZp82UB/EmZRyux5SNp4gQNKcCM=; b=DETVYNkqZuFruG0rT2q3LI2GuKuYPQ0Px0MHpcS2wUpygB02H+qKRZ8vGlUjCYpat5OZ1D IvMgJUF/QU7epONjy1dsTNLBbPd4Cv2zr+qmtkzdu1kyeduBo9xoeoyl/KANMOOSNslrJV Hd0MIO6Kug1n+Mq8wmb2yAA8rug+h1k= Received: from mail-ua1-f71.google.com (mail-ua1-f71.google.com [209.85.222.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-378-ILsYv8wzPziQCqgLmm1-UQ-1; Wed, 29 Jul 2020 07:27:40 -0400 X-MC-Unique: ILsYv8wzPziQCqgLmm1-UQ-1 Received: by mail-ua1-f71.google.com with SMTP id r13so2832644uap.10 for ; Wed, 29 Jul 2020 04:27:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=CayZAc+JG43hWjsKPZp82UB/EmZRyux5SNp4gQNKcCM=; b=idOYsanZ/jmEDCNh6ugesusT3blf2iDrTsTZ0QQZVVPtMTPI7VRUE8flBLlSG/eyf7 2bKSa+TckwyeqeQxH6svkuWkcSHQ5EKG4+D4jexLaQljeGJeYJYkJuemiipRoI5x/Hpx cU8rTPo48NG7g+jEgRnQjgk2r6ZFPUGTIP3nR1a+Tef8gUt94Oa/jrtfN6xuHg2dQdId IV4jAHjOBN+Envetlj6Vhs7aKoOd0Yp0/zMxfAP6VlxMkBRNJCGLOrcPnAA0YeL0CWcY aiYGs/Zu7J+6HwqOObDVpcB4rbf0Xs4ILCVL4JbiiTj/EX2ki7XV4KMK1w7cgw7dhGF/ HRKA== X-Gm-Message-State: AOAM532JEbVx9GprzCvUhC+GjS9kYEvFSq6jKhmYFeOs/4VISGLa5XyV 6FMO6uDb5ehd2qHOIETd5dI4uyhkMEMtLUNh55CLbQIBHzu2cRa/izs+8u75Kp6creJ4EjfvH/V za+yck5pl/DKUtPTWcAY= X-Received: by 2002:a67:2d4f:: with SMTP id t76mr24371181vst.105.1596022060090; Wed, 29 Jul 2020 04:27:40 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz+oeMDt4e1zgSJTQA6VYuinR2+vetQULNa6PAgJdGWyJD8wDdJefhliudWM3GP2atU+ck8ah8+2GLf3m/q6PY= X-Received: by 2002:a67:2d4f:: with SMTP id t76mr24371161vst.105.1596022059767; Wed, 29 Jul 2020 04:27:39 -0700 (PDT) MIME-Version: 1.0 References: <20200729092000.233036-1-maxime.coquelin@redhat.com> <20200729092000.233036-4-maxime.coquelin@redhat.com> In-Reply-To: <20200729092000.233036-4-maxime.coquelin@redhat.com> From: David Marchand Date: Wed, 29 Jul 2020 13:27:28 +0200 Message-ID: To: Maxime Coquelin Cc: dev , Matan Azrad , "Xia, Chenbo" , Marvin Liu , "Wang, Yinan" , Thomas Monjalon , "Yigit, Ferruh" X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" 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 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. > + > + 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