From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 44E46374E for ; Tue, 2 Oct 2018 07:43:24 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 01 Oct 2018 22:43:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,330,1534834800"; d="scan'208,217";a="93800203" Received: from jguo15x-mobl.ccr.corp.intel.com (HELO [10.249.174.2]) ([10.249.174.2]) by fmsmga004.fm.intel.com with ESMTP; 01 Oct 2018 22:42:58 -0700 To: Andrew Rybchenko , 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 References: <1534503091-31910-1-git-send-email-jia.guo@intel.com> <1538316988-128382-1-git-send-email-jia.guo@intel.com> <1538316988-128382-5-git-send-email-jia.guo@intel.com> <765984fb-213e-002f-209a-9165011540fb@solarflare.com> From: Jeff Guo Message-ID: <3ca06b9f-027e-6a7e-5fa2-18059eb4cbfe@intel.com> Date: Tue, 2 Oct 2018 13:42:57 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 In-Reply-To: <765984fb-213e-002f-209a-9165011540fb@solarflare.com> Content-Language: en-US Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v2 4/4] vfio: enable vfio hotplug by req notifier handler 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: , X-List-Received-Date: Tue, 02 Oct 2018 05:43:26 -0000 thanks , andrew. On 10/1/2018 5:47 PM, Andrew Rybchenko wrote: > 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 >> --- >> 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 >> #include >> #include >> +#include >> +#include >> >> #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? > my fault, thanks. >> + * 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. > ok, i think i could try more to make it not split. >> +} >> + >> +/* 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. > ok. >> + 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. > you are right 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. > ok. >> + return -1; >> + } >> + >> + return 0; >> +} >> + >> +/*disable notifier (only disable req now) */ > > Space is missing before disable. > thanks. >> +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? > I think terminate and directly return anyway should be ok, just show the disable failure is make sense. >> + 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? > It should be, i think. >> + 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. > you are definitely right. >> + } >> + >> 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? > It might be leak eventfd, but anyway it should not ignore the check here. >> 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? > ok. >> + * 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: >