From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Junjie J" Subject: Re: [PATCH v2] vhost: add support for interrupt mode Date: Mon, 2 Apr 2018 08:51:30 +0000 Message-ID: References: <1519293583-54933-1-git-send-email-junjie.j.chen@intel.com> <1522334234-98309-1-git-send-email-junjie.j.chen@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" To: "Tan, Jianfeng" , "maxime.coquelin@redhat.com" , "mtetsuyah@gmail.com" Return-path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 0EF581E35 for ; Mon, 2 Apr 2018 10:51:34 +0200 (CEST) In-Reply-To: Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" >=20 >=20 >=20 > On 3/29/2018 10:37 PM, Junjie Chen wrote: > > In some cases we want vhost dequeue work in interrupt mode to release > > cpus to others when no data to transmit. So we install interrupt > > handler of vhost device and interrupt vectors for each rx queue when > > creating new backend according to vhost intrerupt configuration. Thus, > > applications could register a epoll event fd to associate rx queues > > with interrupt vectors. > > > > Signed-off-by: Junjie Chen > > --- > > Changes in v2: > > - update rx queue index > > - fill efd_counter_size for intr handler > > - update log > > > > drivers/net/vhost/rte_eth_vhost.c | 130 > ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 130 insertions(+) > > > > diff --git a/drivers/net/vhost/rte_eth_vhost.c > > b/drivers/net/vhost/rte_eth_vhost.c > > index 3aae01c..b2d925e 100644 > > --- a/drivers/net/vhost/rte_eth_vhost.c > > +++ b/drivers/net/vhost/rte_eth_vhost.c > > @@ -552,12 +552,129 @@ update_queuing_status(struct rte_eth_dev > *dev) > > } > > > > static int > > +eth_rxq_intr_enable(struct rte_eth_dev *dev, uint16_t qid) { > > + struct rte_vhost_vring vring; > > + struct vhost_queue *vq; > > + int ret =3D 0; > > + > > + vq =3D dev->data->rx_queues[qid]; > > + if (!vq) { > > + RTE_LOG(ERR, PMD, "rxq%d is not setup yet\n", qid); > > + return -1; > > + } > > + > > + ret =3D rte_vhost_get_vhost_vring(vq->vid, (qid << 1) + 1, &vring); > > + if (ret < 0) { > > + RTE_LOG(ERR, PMD, "Failed to get rxq%d's vring\n", qid); > > + return ret; > > + } > > + RTE_LOG(INFO, PMD, "Enable interrupt for rxq%d\n", qid); > > + vring.used->flags &=3D (~VRING_USED_F_NO_NOTIFY); >=20 > Instead, you might want to use the API, rte_vhost_enable_guest_notificati= on. Will do. >=20 > > + rte_wmb(); > > + > > + return ret; > > +} > > + > > +static int > > +eth_rxq_intr_disable(struct rte_eth_dev *dev, uint16_t qid) { > > + struct rte_vhost_vring vring; > > + struct vhost_queue *vq; > > + int ret =3D 0; > > + > > + vq =3D dev->data->rx_queues[qid]; > > + if (!vq) { > > + RTE_LOG(ERR, PMD, "rxq%d is not setup yet\n", qid); > > + return -1; > > + } > > + > > + ret =3D rte_vhost_get_vhost_vring(vq->vid, (qid << 1) + 1, &vring); > > + if (ret < 0) { > > + RTE_LOG(ERR, PMD, "Failed to get rxq%d's vring", qid); > > + return ret; > > + } > > + RTE_LOG(INFO, PMD, "Disable interrupt for rxq%d\n", qid); > > + vring.used->flags |=3D VRING_USED_F_NO_NOTIFY; > > + rte_wmb(); >=20 > Ditto. Will do. >=20 > > + > > + return 0; > > +} > > + > > +static void > > +eth_vhost_uninstall_intr(struct rte_eth_dev *dev) { > > + struct rte_intr_handle *intr_handle =3D dev->intr_handle; > > + > > + if (intr_handle) { > > + if (intr_handle->intr_vec) > > + free(intr_handle->intr_vec); > > + free(intr_handle); > > + } > > + > > + dev->intr_handle =3D NULL; > > +} > > + > > +static int > > +eth_vhost_install_intr(struct rte_eth_dev *dev) { > > + struct rte_vhost_vring vring; > > + struct vhost_queue *vq; > > + int count =3D 0; > > + int nb_rxq =3D dev->data->nb_rx_queues; > > + int i; > > + int ret; > > + > > + /* uninstall firstly if we are reconnecting */ > > + if (dev->intr_handle) > > + eth_vhost_uninstall_intr(dev); > > + > > + dev->intr_handle =3D malloc(sizeof(*dev->intr_handle)); > > + if (!dev->intr_handle) { > > + RTE_LOG(ERR, PMD, "Fail to allocate intr_handle\n"); > > + return -ENOMEM; > > + } > > + memset(dev->intr_handle, 0, sizeof(*dev->intr_handle)); > > + > > + dev->intr_handle->efd_counter_size =3D sizeof(uint64_t); >=20 > I did not read-clean this for virtio-user. But we'd better keep consisten= t, to > avoid confusion. If we need read-clean this, we could fix virtio-user. So, do you want to change this in this patch? Or in other separated one? >=20 > > + > > + dev->intr_handle->intr_vec =3D > > + malloc(nb_rxq * sizeof(dev->intr_handle->intr_vec[0])); > > + > > + if (!dev->intr_handle->intr_vec) { > > + RTE_LOG(ERR, PMD, > > + "Failed to allocate memory for interrupt vector\n"); >=20 > For completeness, we need to uninstall before return. Will do. >=20 > > + return -ENOMEM; > > + } > > + > > + for (i =3D 0; i < nb_rxq; i++) { > > + vq =3D dev->data->rx_queues[i]; > > + if (!vq) > > + continue; >=20 > Put such check at the entry of this function? >=20 > > + ret =3D rte_vhost_get_vhost_vring(vq->vid, (i << 1) + 1, &vring); > > + if (ret < 0) >=20 > Does that mean we shall take care of this failure? For above two checks, I had a fix to check overall queue setup in patch: ht= tps://dpdk.org/dev/patchwork/patch/36766/ recently. So I would like to put install_intr function into queue_setup helper functi= on in patch 36766, and for internal vq, vring, and kickfd check of install_= intr, I'd like to skip setup if check fail. How do you think?=20 >=20 > > + continue; > > + RTE_LOG(INFO, PMD, "Install intr vec for rxq%d\n", i); > > + dev->intr_handle->intr_vec[i] =3D RTE_INTR_VEC_RXTX_OFFSET + i; > > + dev->intr_handle->efds[i] =3D vring.kickfd; >=20 > Before use of the kickfd, we need to check the validation? Will do. >=20 > > + count++; > > + } > > + > > + dev->intr_handle->nb_efd =3D count; > > + dev->intr_handle->max_intr =3D count + 1; > > + dev->intr_handle->type =3D RTE_INTR_HANDLE_VDEV; > > + > > + return 0; > > +} > > + > > +static int > > new_device(int vid) > > { > > struct rte_eth_dev *eth_dev; > > struct internal_list *list; > > struct pmd_internal *internal; > > struct vhost_queue *vq; > > + struct rte_eth_conf *dev_conf; > > unsigned i; > > char ifname[PATH_MAX]; > > #ifdef RTE_LIBRTE_VHOST_NUMA > > @@ -609,6 +726,15 @@ new_device(int vid) > > > > RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid); > > > > + dev_conf =3D ð_dev->data->dev_conf; > > + > > + if (dev_conf->intr_conf.rxq) { > > + if (eth_vhost_install_intr(eth_dev) < 0) { > > + RTE_LOG(INFO, PMD, "Failed to prepare intr handler."); > > + return -1; > > + } > > + } > > + > > _rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_INTR_LSC, > > NULL); > > > > return 0; > > @@ -663,6 +789,8 @@ destroy_device(int vid) > > > > RTE_LOG(INFO, PMD, "Vhost device %d destroyed\n", vid); > > > > + eth_vhost_uninstall_intr(eth_dev); > > + > > _rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_INTR_LSC, > NULL); > > } > > > > @@ -1007,6 +1135,8 @@ static const struct eth_dev_ops ops =3D { > > .xstats_reset =3D vhost_dev_xstats_reset, > > .xstats_get =3D vhost_dev_xstats_get, > > .xstats_get_names =3D vhost_dev_xstats_get_names, > > + .rx_queue_intr_enable =3D eth_rxq_intr_enable, > > + .rx_queue_intr_disable =3D eth_rxq_intr_disable, > > }; > > > > static struct rte_vdev_driver pmd_vhost_drv;