DPDK patches and discussions
 help / color / mirror / Atom feed
From: Andrew Rybchenko <arybchenko@solarflare.com>
To: Jeff Guo <jia.guo@intel.com>, <stephen@networkplumber.org>,
	<bruce.richardson@intel.com>, <ferruh.yigit@intel.com>,
	<konstantin.ananyev@intel.com>, <gaetan.rivet@6wind.com>,
	<jingjing.wu@intel.com>, <thomas@monjalon.net>,
	<motih@mellanox.com>, <matan@mellanox.com>,
	<harry.van.haaren@intel.com>, <qi.z.zhang@intel.com>,
	<shaopeng.he@intel.com>, <bernard.iremonger@intel.com>,
	<anatoly.burakov@intel.com>
Cc: <jblunck@infradead.org>, <shreyansh.jain@nxp.com>, <dev@dpdk.org>,
	<helin.zhang@intel.com>
Subject: Re: [dpdk-dev] [PATCH v2 4/4] vfio: enable vfio hotplug by req notifier handler
Date: Mon, 1 Oct 2018 12:47:05 +0300	[thread overview]
Message-ID: <765984fb-213e-002f-209a-9165011540fb@solarflare.com> (raw)
In-Reply-To: <1538316988-128382-5-git-send-email-jia.guo@intel.com>

On 9/30/18 5:16 PM, Jeff Guo wrote:
> When device is be hot-unplugged, the vfio kernel module will sent req
> notifier to request user space to release the allocated resources at
> first. After that, vfio kernel module will detect the device disappear,
> and then delete the device in kernel.
>
> This patch aim to add req notifier processing to enable hotplug for vfio.
> By enable the req notifier monitoring and register the notifier callback,
> when device be hot-unplugged, the hot-unplug handler will be called to
> process hotplug for vfio.
>
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
> v2->v1:
> refine some code logic.
> ---
>   drivers/bus/pci/linux/pci_vfio.c | 95 ++++++++++++++++++++++++++++++++++++++++
>   drivers/bus/pci/pci_common.c     | 10 +++++
>   2 files changed, 105 insertions(+)
>
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index 686386d..c780860 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -17,6 +17,8 @@
>   #include <rte_eal_memconfig.h>
>   #include <rte_malloc.h>
>   #include <rte_vfio.h>
> +#include <rte_eal.h>
> +#include <rte_bus.h>
>   
>   #include "eal_filesystem.h"
>   
> @@ -277,6 +279,89 @@ pci_vfio_setup_interrupts(struct rte_pci_device *dev, int vfio_dev_fd)
>   	return -1;
>   }
>   
> +static void
> +pci_vfio_req_handler(void *param)
> +{
> +	struct rte_bus *bus;
> +	int ret;
> +	struct rte_device *device = (struct rte_device *)param;
> +
> +	bus = rte_bus_find_by_device(device);
> +	if (bus == NULL) {
> +		RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n",
> +			device->name);
> +		return;
> +	}
> +
> +	/**

Why is doxygen style comment used here?

> +	 * vfio kernel module request user space to release allocated
> +	 * resources before device be deleted in kernel, so it can directly
> +	 * call the vfio bus hot-unplug handler to process it.
> +	 */
> +	ret = bus->hot_unplug_handler(device);
> +	if (ret)
> +		RTE_LOG(ERR, EAL, "Can not handle hot-unplug for "
> +			"device (%s)\n", device->name);

Consider to avoid format string split to simplify search using grep.

> +}
> +
> +/* enable notifier (only enable req now) */
> +static int
> +pci_vfio_enable_notifier(struct rte_pci_device *dev, int vfio_dev_fd)
> +{
> +	int ret;
> +	int fd = -1;
> +
> +	/* set up an eventfd for req notifier */
> +	fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
> +	if (fd < 0) {
> +		RTE_LOG(ERR, EAL, "Cannot set up eventfd, "
> +				"error %i (%s)\n", errno, strerror(errno));

Consider to avoid format string split to simplify search using grep.

> +		return -1;
> +	}
> +
> +	dev->req_notifier_handler.fd = fd;
> +	dev->req_notifier_handler.type = RTE_INTR_HANDLE_VFIO_REQ;
> +	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
> +	ret = rte_intr_callback_register(&dev->req_notifier_handler,
> +					 pci_vfio_req_handler,
> +					 (void *)&dev->device);
> +	if (ret) {
> +		RTE_LOG(ERR, EAL, "Fail to register req notifier handler.\n");

I think we should close(fd) here.

> +		return -1;
> +	}
> +
> +	ret = rte_intr_enable(&dev->req_notifier_handler);
> +	if (ret) {
> +		RTE_LOG(ERR, EAL, "Fail to enable req notifier.\n");

I think we should unregister notifier and close(fd) here.

> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +/*disable notifier (only disable req now) */

Space is missing before disable.

> +static int
> +pci_vfio_disable_notifier(struct rte_pci_device *dev)
> +{
> +	int ret;
> +
> +	ret = rte_intr_disable(&dev->req_notifier_handler);
> +	if (ret) {
> +		RTE_LOG(ERR, EAL, "fail to disable req notifier.\n");

I'd like to understand correct way handle errors here. Should we
terminate here or continue and unregister handler and close FD anyway?

> +		return -1;
> +	}
> +
> +	ret = rte_intr_callback_unregister(&dev->req_notifier_handler,
> +					   pci_vfio_req_handler,
> +					   (void *)&dev->device);
> +	if (ret) {
> +		RTE_LOG(ERR, EAL,
> +			 "fail to unregister req notifier handler.\n");
> +		return -1;
> +	}

Shoudn't we close eventfd?

> +	return 0;
> +}
> +
>   static int
>   pci_vfio_is_ioport_bar(int vfio_dev_fd, int bar_index)
>   {
> @@ -430,6 +515,7 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
>   	struct pci_map *maps;
>   
>   	dev->intr_handle.fd = -1;
> +	dev->req_notifier_handler.fd = -1;
>   
>   	/* store PCI address string */
>   	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
> @@ -521,6 +607,11 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev)
>   		goto err_vfio_res;
>   	}
>   
> +	if (pci_vfio_enable_notifier(dev, vfio_dev_fd) != 0) {
> +		RTE_LOG(ERR, EAL, "Error setting up notifier!\n");
> +		return -1;

I think we should do goto to make required cleanup.

> +	}
> +
>   	TAILQ_INSERT_TAIL(vfio_res_list, vfio_res, next);
>   
>   	return 0;
> @@ -546,6 +637,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
>   	struct pci_map *maps;
>   
>   	dev->intr_handle.fd = -1;
> +	dev->req_notifier_handler.fd = -1;
>   
>   	/* store PCI address string */
>   	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
> @@ -586,6 +678,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev)
>   
>   	/* we need save vfio_dev_fd, so it can be used during release */
>   	dev->intr_handle.vfio_dev_fd = vfio_dev_fd;
> +	dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd;
>   
>   	return 0;
>   err_vfio_dev_fd:
> @@ -658,6 +751,8 @@ pci_vfio_unmap_resource_primary(struct rte_pci_device *dev)
>   	snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
>   			loc->domain, loc->bus, loc->devid, loc->function);
>   
> +	pci_vfio_disable_notifier(dev);
> +

Is it OK to ignore disable failure here? Why? It would be good to see
explanations in comments. Does it leak eventfd now?

>   	if (close(dev->intr_handle.fd) < 0) {
>   		RTE_LOG(INFO, EAL, "Error when closing eventfd file descriptor for %s\n",
>   			pci_addr);
> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
> index f313fe9..2a8e5e9 100644
> --- a/drivers/bus/pci/pci_common.c
> +++ b/drivers/bus/pci/pci_common.c
> @@ -446,6 +446,16 @@ pci_hot_unplug_handler(struct rte_device *dev)
>   		return -1;
>   
>   	switch (pdev->kdrv) {
> +	case RTE_KDRV_VFIO:
> +		/**

Why is doxygen style comment is used here?

> +		 * vfio kernel module guaranty the pci device would not be
> +		 * deleted until the user space release the resource, so no
> +		 * need to remap BARs resource here, just directly notify
> +		 * the req event to the user space to handle it.
> +		 */
> +		rte_dev_event_callback_process(dev->name,
> +					       RTE_DEV_EVENT_REMOVE);
> +		break;
>   	case RTE_KDRV_IGB_UIO:
>   	case RTE_KDRV_UIO_GENERIC:
>   	case RTE_KDRV_NIC_UIO:

  reply	other threads:[~2018-10-01  9:48 UTC|newest]

Thread overview: 63+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-17 10:51 [dpdk-dev] [PATCH v1 0/5] Enable hotplug in vfio Jeff Guo
2018-08-17 10:51 ` [dpdk-dev] [PATCH v1 1/5] eal: add a new req notifier to eal interrupt Jeff Guo
2018-08-17 10:51 ` [dpdk-dev] [PATCH v1 2/5] eal: add a new req event to device event Jeff Guo
2018-08-20 10:37   ` Andrew Rybchenko
2018-08-21  6:56     ` Jeff Guo
2018-08-21  7:20       ` Andrew Rybchenko
2018-08-21  7:37         ` Jeff Guo
2018-08-17 10:51 ` [dpdk-dev] [PATCH v1 3/5] eal: modify device event callback process func Jeff Guo
2018-09-26 12:20   ` Burakov, Anatoly
2018-09-30 10:30     ` Jeff Guo
2018-09-26 12:20   ` Burakov, Anatoly
2018-09-30 10:31     ` Jeff Guo
2018-08-17 10:51 ` [dpdk-dev] [PATCH v1 4/5] pci: add req handler field to generic pci device Jeff Guo
2018-09-26 12:22   ` Burakov, Anatoly
2018-09-29  6:15     ` Jeff Guo
2018-10-01  7:51       ` Burakov, Anatoly
2018-08-17 10:51 ` [dpdk-dev] [PATCH v1 5/5] vfio: enable vfio hotplug by req notifier handler Jeff Guo
2018-09-26 12:28   ` Burakov, Anatoly
2018-09-29  5:51     ` Jeff Guo
2018-08-20  9:15 ` [dpdk-dev] [PATCH v1 0/5] Enable hotplug in vfio Gaëtan Rivet
2018-08-21  6:45   ` Jeff Guo
2018-08-21  8:17     ` Gaëtan Rivet
2018-09-30 14:16 ` [dpdk-dev] [PATCH v2 0/4] " Jeff Guo
2018-09-30 14:16   ` [dpdk-dev] [PATCH v2 1/4] eal: add a new req notifier to eal interrupt Jeff Guo
2018-10-01  9:46     ` Andrew Rybchenko
2018-10-02  4:30       ` Jeff Guo
2018-10-02  6:51         ` Andrew Rybchenko
2018-09-30 14:16   ` [dpdk-dev] [PATCH v2 2/4] eal: modify device event callback process func Jeff Guo
2018-10-01  9:46     ` Andrew Rybchenko
2018-10-02  4:45       ` Jeff Guo
2018-10-02  6:53         ` Andrew Rybchenko
2018-09-30 14:16   ` [dpdk-dev] [PATCH v2 3/4] pci: add req handler field to generic pci device Jeff Guo
2018-10-01  9:46     ` Andrew Rybchenko
2018-10-02  6:32       ` Jeff Guo
2018-09-30 14:16   ` [dpdk-dev] [PATCH v2 4/4] vfio: enable vfio hotplug by req notifier handler Jeff Guo
2018-10-01  9:47     ` Andrew Rybchenko [this message]
2018-10-02  5:42       ` Jeff Guo
2018-10-02 12:42 ` [dpdk-dev] [PATCH v2 0/4] Enable hotplug in vfio Jeff Guo
2018-10-02 12:42   ` [dpdk-dev] [PATCH v2 1/4] eal: add a new req notifier to eal interrupt Jeff Guo
2018-10-02 12:42   ` [dpdk-dev] [PATCH v2 2/4] eal: modify device event callback process func Jeff Guo
2018-10-02 12:42   ` [dpdk-dev] [PATCH v2 3/4] pci: add req handler field to generic pci device Jeff Guo
2018-10-02 12:42   ` [dpdk-dev] [PATCH v2 4/4] vfio: enable vfio hotplug by req notifier handler Jeff Guo
2018-10-02 12:44 ` [dpdk-dev] [PATCH v3 0/4] Enable hotplug in vfio Jeff Guo
2018-10-02 12:44   ` [dpdk-dev] [PATCH v3 1/4] eal: add a new req notifier to eal interrupt Jeff Guo
2018-10-02 12:45   ` [dpdk-dev] [PATCH v3 2/4] eal: modify device event callback process func Jeff Guo
2018-10-02 12:45   ` [dpdk-dev] [PATCH v3 3/4] pci: add req handler field to generic pci device Jeff Guo
2018-10-02 12:45   ` [dpdk-dev] [PATCH v3 4/4] vfio: enable vfio hotplug by req notifier handler Jeff Guo
2018-10-02 12:58 ` [dpdk-dev] [PATCH v3 0/4] Enable hotplug in vfio Jeff Guo
2018-10-02 12:58   ` [dpdk-dev] [PATCH v3 1/4] eal: add a new req notifier to eal interrupt Jeff Guo
2018-10-02 12:58   ` [dpdk-dev] [PATCH v3 2/4] eal: modify device event callback process func Jeff Guo
2018-10-02 12:58   ` [dpdk-dev] [PATCH v3 3/4] pci: add req handler field to generic pci device Jeff Guo
2018-10-02 12:58   ` [dpdk-dev] [PATCH v3 4/4] vfio: enable vfio hotplug by req notifier handler Jeff Guo
2018-10-02 14:15     ` Burakov, Anatoly
2018-10-04  5:05       ` Jeff Guo
2018-10-04  6:44 ` [dpdk-dev] [PATCH v4 0/4] Enable hotplug in vfio Jeff Guo
2018-10-04  6:44   ` [dpdk-dev] [PATCH v4 1/4] eal: add a new req notifier to eal interrupt Jeff Guo
2018-10-04  6:44   ` [dpdk-dev] [PATCH v4 2/4] eal: modify device event callback process func Jeff Guo
2018-10-04  6:44   ` [dpdk-dev] [PATCH v4 3/4] pci: add req handler field to generic pci device Jeff Guo
2018-10-15  9:12     ` Thomas Monjalon
2018-10-15 10:01       ` Thomas Monjalon
2018-10-04  6:44   ` [dpdk-dev] [PATCH v4 4/4] vfio: enable vfio hotplug by req notifier handler Jeff Guo
2018-10-04  9:11   ` [dpdk-dev] [PATCH v4 0/4] Enable hotplug in vfio Burakov, Anatoly
2018-10-15 21:47     ` Thomas Monjalon

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=765984fb-213e-002f-209a-9165011540fb@solarflare.com \
    --to=arybchenko@solarflare.com \
    --cc=anatoly.burakov@intel.com \
    --cc=bernard.iremonger@intel.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=gaetan.rivet@6wind.com \
    --cc=harry.van.haaren@intel.com \
    --cc=helin.zhang@intel.com \
    --cc=jblunck@infradead.org \
    --cc=jia.guo@intel.com \
    --cc=jingjing.wu@intel.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=matan@mellanox.com \
    --cc=motih@mellanox.com \
    --cc=qi.z.zhang@intel.com \
    --cc=shaopeng.he@intel.com \
    --cc=shreyansh.jain@nxp.com \
    --cc=stephen@networkplumber.org \
    --cc=thomas@monjalon.net \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).