diff mbox

[dpdk-dev,v2] vhost: add support for interrupt mode

Message ID 1522334234-98309-1-git-send-email-junjie.j.chen@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers show

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

junjie.j.chen@intel.com March 29, 2018, 2:37 p.m. UTC
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 <junjie.j.chen@intel.com>
---
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(+)

Comments

Jianfeng Tan March 31, 2018, 4 a.m. UTC | #1
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 <junjie.j.chen@intel.com>
> ---
> 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 = 0;
> +
> +	vq = dev->data->rx_queues[qid];
> +	if (!vq) {
> +		RTE_LOG(ERR, PMD, "rxq%d is not setup yet\n", qid);
> +		return -1;
> +	}
> +
> +	ret = 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 &= (~VRING_USED_F_NO_NOTIFY);

Instead, you might want to use the API, rte_vhost_enable_guest_notification.

> +	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 = 0;
> +
> +	vq = dev->data->rx_queues[qid];
> +	if (!vq) {
> +		RTE_LOG(ERR, PMD, "rxq%d is not setup yet\n", qid);
> +		return -1;
> +	}
> +
> +	ret = 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 |= VRING_USED_F_NO_NOTIFY;
> +	rte_wmb();

Ditto.

> +
> +	return 0;
> +}
> +
> +static void
> +eth_vhost_uninstall_intr(struct rte_eth_dev *dev)
> +{
> +	struct rte_intr_handle *intr_handle = dev->intr_handle;
> +
> +	if (intr_handle) {
> +		if (intr_handle->intr_vec)
> +			free(intr_handle->intr_vec);
> +		free(intr_handle);
> +	}
> +
> +	dev->intr_handle = NULL;
> +}
> +
> +static int
> +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;
> +
> +	/* uninstall firstly if we are reconnecting */
> +	if (dev->intr_handle)
> +		eth_vhost_uninstall_intr(dev);
> +
> +	dev->intr_handle = 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 = sizeof(uint64_t);

I did not read-clean this for virtio-user. But we'd better keep 
consistent, to avoid confusion. If we need read-clean this, we could fix 
virtio-user.

> +
> +	dev->intr_handle->intr_vec =
> +		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");

For completeness, we need to uninstall before return.

> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0; i < nb_rxq; i++) {
> +		vq = dev->data->rx_queues[i];
> +		if (!vq)
> +			continue;

Put such check at the entry of this function?

> +		ret = rte_vhost_get_vhost_vring(vq->vid, (i << 1) + 1, &vring);
> +		if (ret < 0)

Does that mean we shall take care of this failure?

> +			continue;
> +		RTE_LOG(INFO, PMD, "Install intr vec for rxq%d\n", i);
> +		dev->intr_handle->intr_vec[i] = RTE_INTR_VEC_RXTX_OFFSET + i;
> +		dev->intr_handle->efds[i] = vring.kickfd;

Before use of the kickfd, we need to check the validation?

> +		count++;
> +	}
> +
> +	dev->intr_handle->nb_efd = count;
> +	dev->intr_handle->max_intr = count + 1;
> +	dev->intr_handle->type = 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 = &eth_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 = {
>   	.xstats_reset = vhost_dev_xstats_reset,
>   	.xstats_get = vhost_dev_xstats_get,
>   	.xstats_get_names = vhost_dev_xstats_get_names,
> +	.rx_queue_intr_enable = eth_rxq_intr_enable,
> +	.rx_queue_intr_disable = eth_rxq_intr_disable,
>   };
>   
>   static struct rte_vdev_driver pmd_vhost_drv;
junjie.j.chen@intel.com April 2, 2018, 8:51 a.m. UTC | #2
> 
> 
> 
> 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 <junjie.j.chen@intel.com>
> > ---
> > 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 = 0;
> > +
> > +	vq = dev->data->rx_queues[qid];
> > +	if (!vq) {
> > +		RTE_LOG(ERR, PMD, "rxq%d is not setup yet\n", qid);
> > +		return -1;
> > +	}
> > +
> > +	ret = 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 &= (~VRING_USED_F_NO_NOTIFY);
> 
> Instead, you might want to use the API, rte_vhost_enable_guest_notification.
Will do.
> 
> > +	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 = 0;
> > +
> > +	vq = dev->data->rx_queues[qid];
> > +	if (!vq) {
> > +		RTE_LOG(ERR, PMD, "rxq%d is not setup yet\n", qid);
> > +		return -1;
> > +	}
> > +
> > +	ret = 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 |= VRING_USED_F_NO_NOTIFY;
> > +	rte_wmb();
> 
> Ditto.
Will do.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void
> > +eth_vhost_uninstall_intr(struct rte_eth_dev *dev) {
> > +	struct rte_intr_handle *intr_handle = dev->intr_handle;
> > +
> > +	if (intr_handle) {
> > +		if (intr_handle->intr_vec)
> > +			free(intr_handle->intr_vec);
> > +		free(intr_handle);
> > +	}
> > +
> > +	dev->intr_handle = NULL;
> > +}
> > +
> > +static int
> > +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;
> > +
> > +	/* uninstall firstly if we are reconnecting */
> > +	if (dev->intr_handle)
> > +		eth_vhost_uninstall_intr(dev);
> > +
> > +	dev->intr_handle = 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 = sizeof(uint64_t);
> 
> I did not read-clean this for virtio-user. But we'd better keep consistent, 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?
> 
> > +
> > +	dev->intr_handle->intr_vec =
> > +		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");
> 
> For completeness, we need to uninstall before return.
Will do.
> 
> > +		return -ENOMEM;
> > +	}
> > +
> > +	for (i = 0; i < nb_rxq; i++) {
> > +		vq = dev->data->rx_queues[i];
> > +		if (!vq)
> > +			continue;
> 
> Put such check at the entry of this function?
> 
> > +		ret = rte_vhost_get_vhost_vring(vq->vid, (i << 1) + 1, &vring);
> > +		if (ret < 0)
> 
> 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: https://dpdk.org/dev/patchwork/patch/36766/ recently.
So I would like to put install_intr function into queue_setup helper function 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? 

> 
> > +			continue;
> > +		RTE_LOG(INFO, PMD, "Install intr vec for rxq%d\n", i);
> > +		dev->intr_handle->intr_vec[i] = RTE_INTR_VEC_RXTX_OFFSET + i;
> > +		dev->intr_handle->efds[i] = vring.kickfd;
> 
> Before use of the kickfd, we need to check the validation?
Will do.
> 
> > +		count++;
> > +	}
> > +
> > +	dev->intr_handle->nb_efd = count;
> > +	dev->intr_handle->max_intr = count + 1;
> > +	dev->intr_handle->type = 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 = &eth_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 = {
> >   	.xstats_reset = vhost_dev_xstats_reset,
> >   	.xstats_get = vhost_dev_xstats_get,
> >   	.xstats_get_names = vhost_dev_xstats_get_names,
> > +	.rx_queue_intr_enable = eth_rxq_intr_enable,
> > +	.rx_queue_intr_disable = eth_rxq_intr_disable,
> >   };
> >
> >   static struct rte_vdev_driver pmd_vhost_drv;
diff mbox

Patch

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 = 0;
+
+	vq = dev->data->rx_queues[qid];
+	if (!vq) {
+		RTE_LOG(ERR, PMD, "rxq%d is not setup yet\n", qid);
+		return -1;
+	}
+
+	ret = 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 &= (~VRING_USED_F_NO_NOTIFY);
+	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 = 0;
+
+	vq = dev->data->rx_queues[qid];
+	if (!vq) {
+		RTE_LOG(ERR, PMD, "rxq%d is not setup yet\n", qid);
+		return -1;
+	}
+
+	ret = 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 |= VRING_USED_F_NO_NOTIFY;
+	rte_wmb();
+
+	return 0;
+}
+
+static void
+eth_vhost_uninstall_intr(struct rte_eth_dev *dev)
+{
+	struct rte_intr_handle *intr_handle = dev->intr_handle;
+
+	if (intr_handle) {
+		if (intr_handle->intr_vec)
+			free(intr_handle->intr_vec);
+		free(intr_handle);
+	}
+
+	dev->intr_handle = NULL;
+}
+
+static int
+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;
+
+	/* uninstall firstly if we are reconnecting */
+	if (dev->intr_handle)
+		eth_vhost_uninstall_intr(dev);
+
+	dev->intr_handle = 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 = sizeof(uint64_t);
+
+	dev->intr_handle->intr_vec =
+		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");
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < nb_rxq; i++) {
+		vq = dev->data->rx_queues[i];
+		if (!vq)
+			continue;
+		ret = rte_vhost_get_vhost_vring(vq->vid, (i << 1) + 1, &vring);
+		if (ret < 0)
+			continue;
+		RTE_LOG(INFO, PMD, "Install intr vec for rxq%d\n", i);
+		dev->intr_handle->intr_vec[i] = RTE_INTR_VEC_RXTX_OFFSET + i;
+		dev->intr_handle->efds[i] = vring.kickfd;
+		count++;
+	}
+
+	dev->intr_handle->nb_efd = count;
+	dev->intr_handle->max_intr = count + 1;
+	dev->intr_handle->type = 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 = &eth_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 = {
 	.xstats_reset = vhost_dev_xstats_reset,
 	.xstats_get = vhost_dev_xstats_get,
 	.xstats_get_names = vhost_dev_xstats_get_names,
+	.rx_queue_intr_enable = eth_rxq_intr_enable,
+	.rx_queue_intr_disable = eth_rxq_intr_disable,
 };
 
 static struct rte_vdev_driver pmd_vhost_drv;