DPDK patches and discussions
 help / color / mirror / Atom feed
From: Maxime Coquelin <maxime.coquelin@redhat.com>
To: "Xia, Chenbo" <chenbo.xia@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	"matan@mellanox.com" <matan@mellanox.com>,
	"Liu, Yong" <yong.liu@intel.com>,
	"Wang, Yinan" <yinan.wang@intel.com>
Cc: "thomas@monjalon.net" <thomas@monjalon.net>,
	"Yigit, Ferruh" <ferruh.yigit@intel.com>,
	"david.marchand@redhat.com" <david.marchand@redhat.com>
Subject: Re: [dpdk-dev] [PATCH v3 3/3] net/vhost: fix interrupt mode
Date: Wed, 29 Jul 2020 15:27:25 +0200
Message-ID: <de8aaea4-a07c-8129-f90b-35ab5db15730@redhat.com> (raw)
In-Reply-To: <MN2PR11MB4063441B5B447ABC45A1D5969C700@MN2PR11MB4063.namprd11.prod.outlook.com>



On 7/29/20 3:24 PM, Xia, Chenbo wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Wednesday, July 29, 2020 8:54 PM
>> To: dev@dpdk.org; matan@mellanox.com; Xia, Chenbo
>> <chenbo.xia@intel.com>; Liu, Yong <yong.liu@intel.com>; Wang, Yinan
>> <yinan.wang@intel.com>
>> Cc: thomas@monjalon.net; Yigit, Ferruh <ferruh.yigit@intel.com>;
>> david.marchand@redhat.com
>> Subject: Re: [PATCH v3 3/3] net/vhost: fix interrupt mode
>>
>>
>>
>> On 7/29/20 11:20 AM, Maxime Coquelin wrote:
>>> At .new_device() time, only the first vring pair is now ready, other
>>> vrings are consfigured later.
>>>
>>> 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
>>> new one with the valid FD.
>>>
>>> Fixes: 604052ae5395 ("net/vhost: support queue update")
>>>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> ---
>>>  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 <unistd.h>
>>>  #include <pthread.h>
>>>  #include <stdbool.h>
>>> +#include <sys/epoll.h>
>>>
>>>  #include <rte_mbuf.h>
>>>  #include <rte_ethdev_driver.h>
>>> @@ -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;
>>> +
>>
>> Chenbo reported that we can have a NULL pointer dereference on handle when
>> using Virtio-user on the other end and quitting.
> 
> To clarity the root cause, it's because 'destroy_device' calls 'eth_vhost_uninstall_intr'
> when connection lost but l3fwd-power APP are still enabling intr.

Agree, l3fwd-power will need some rework, as even with fixing this, we
get flooded with error logs because the app tried to enable the intr in
loop once the frontend has disconnected. But this is also reproducible
in v20.05.

> Thanks,
> Chenbo
> 
>>
>>
>>
>>> +	if (handle->efds[rxq_idx] == handle->elist[rxq_idx].fd)
>>> +		return 0;
>>> +
>>> +	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);
>>> +
>>
>> I missed to check ret value here, will add it in v4.
>>
>>>  	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);
> 


      reply	other threads:[~2020-07-29 13:27 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-29  9:19 [dpdk-dev] [PATCH v3 0/3] Fix Vhost regressions Maxime Coquelin
2020-07-29  9:19 ` [dpdk-dev] [PATCH v3 1/3] vhost: fix guest notification setting Maxime Coquelin
2020-07-29  9:19 ` [dpdk-dev] [PATCH v3 2/3] net/vhost: fix queue update Maxime Coquelin
2020-07-29  9:20 ` [dpdk-dev] [PATCH v3 3/3] net/vhost: fix interrupt mode Maxime Coquelin
2020-07-29 11:27   ` David Marchand
2020-07-29 13:19     ` Maxime Coquelin
2020-07-29 12:53   ` Maxime Coquelin
2020-07-29 13:24     ` Xia, Chenbo
2020-07-29 13:27       ` Maxime Coquelin [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=de8aaea4-a07c-8129-f90b-35ab5db15730@redhat.com \
    --to=maxime.coquelin@redhat.com \
    --cc=chenbo.xia@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=matan@mellanox.com \
    --cc=thomas@monjalon.net \
    --cc=yinan.wang@intel.com \
    --cc=yong.liu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git