diff --git a/include/spdk/env.h b/include/spdk/env.h index b2b99ed7c..7214fe532 100644 --- a/include/spdk/env.h +++ b/include/spdk/env.h @@ -669,6 +669,8 @@ struct spdk_pci_device { struct _spdk_pci_device_internal { struct spdk_pci_driver *driver; bool attached; + /* optional fd for exclusive access to this device on this process */ + int claim_fd; bool pending_removal; /* The device was successfully removed on a DPDK interrupt thread, * but to prevent data races we couldn't remove it from the global @@ -895,13 +897,23 @@ int spdk_pci_device_get_serial_number(struct spdk_pci_device *dev, char *sn, siz * As long as this file remains open with the lock acquired, other processes will * not be able to successfully call this function on the same PCI device. * - * \param pci_addr PCI address of the device to claim + * The device can be un-claimed by the owning process with spdk_pci_device_unclaim(). + * It will be also unclaimed automatically when detached. * - * \return -1 if the device has already been claimed, an fd otherwise. This fd - * should be closed when the application no longer needs access to the PCI device - * (including when it is hot removed). + * \param dev PCI device to claim. + * + * \return -EACCES if the device has already been claimed, + * negative errno on unexpected errors, + * 0 on success. */ -int spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr); +int spdk_pci_device_claim(struct spdk_pci_device *dev); + +/** + * Undo spdk_pci_device_claim(). + * + * \param dev PCI device to unclaim. + */ +void spdk_pci_device_unclaim(struct spdk_pci_device *dev); /** * Release all resources associated with the given device and detach it. As long diff --git a/lib/env_dpdk/pci.c b/lib/env_dpdk/pci.c index b4af0d18e..fad7ea8bb 100644 --- a/lib/env_dpdk/pci.c +++ b/lib/env_dpdk/pci.c @@ -342,6 +342,7 @@ spdk_pci_device_init(struct rte_pci_driver *_drv, dev->detach = spdk_detach_rte; dev->internal.driver = driver; + dev->internal.claim_fd = -1; if (driver->cb_fn != NULL) { rc = driver->cb_fn(driver->cb_arg, dev); @@ -387,6 +388,11 @@ void spdk_pci_device_detach(struct spdk_pci_device *dev) { assert(dev->internal.attached); + + if (dev->internal.claim_fd >= 0) { + spdk_pci_device_unclaim(dev); + } + dev->internal.attached = false; dev->detach(dev); @@ -728,7 +734,7 @@ spdk_pci_addr_compare(const struct spdk_pci_addr *a1, const struct spdk_pci_addr #ifdef __linux__ int -spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr) +spdk_pci_device_claim(struct spdk_pci_device *dev) { int dev_fd; char dev_name[64]; @@ -741,20 +747,19 @@ spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr) .l_len = 0, }; - snprintf(dev_name, sizeof(dev_name), "/tmp/spdk_pci_lock_%04x:%02x:%02x.%x", pci_addr->domain, - pci_addr->bus, - pci_addr->dev, pci_addr->func); + snprintf(dev_name, sizeof(dev_name), "/tmp/spdk_pci_lock_%04x:%02x:%02x.%x", + dev->addr.domain, dev->addr.bus, dev->addr.dev, dev->addr.func); dev_fd = open(dev_name, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR); if (dev_fd == -1) { fprintf(stderr, "could not open %s\n", dev_name); - return -1; + return -errno; } if (ftruncate(dev_fd, sizeof(int)) != 0) { fprintf(stderr, "could not truncate %s\n", dev_name); close(dev_fd); - return -1; + return -errno; } dev_map = mmap(NULL, sizeof(int), PROT_READ | PROT_WRITE, @@ -762,7 +767,7 @@ spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr) if (dev_map == MAP_FAILED) { fprintf(stderr, "could not mmap dev %s (%d)\n", dev_name, errno); close(dev_fd); - return -1; + return -errno; } if (fcntl(dev_fd, F_SETLK, &pcidev_lock) != 0) { @@ -771,23 +776,44 @@ spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr) " process %d has claimed it\n", dev_name, pid); munmap(dev_map, sizeof(int)); close(dev_fd); - return -1; + /* F_SETLK returns unspecified errnos, normalize them */ + return -EACCES; } *(int *)dev_map = (int)getpid(); munmap(dev_map, sizeof(int)); + dev->internal.claim_fd = dev_fd; /* Keep dev_fd open to maintain the lock. */ - return dev_fd; + return 0; +} + +void +spdk_pci_device_unclaim(struct spdk_pci_device *dev) +{ + char dev_name[64]; + + snprintf(dev_name, sizeof(dev_name), "/tmp/spdk_pci_lock_%04x:%02x:%02x.%x", + dev->addr.domain, dev->addr.bus, dev->addr.dev, dev->addr.func); + + close(dev->internal.claim_fd); + dev->internal.claim_fd = -1; + unlink(dev_name); } #endif /* __linux__ */ #ifdef __FreeBSD__ int -spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr) +spdk_pci_device_claim(struct spdk_pci_device *dev) { /* TODO */ return 0; } + +void +spdk_pci_device_unclaim(struct spdk_pci_device *dev) +{ + /* TODO */ +} #endif /* __FreeBSD__ */ int diff --git a/lib/nvme/nvme_pcie.c b/lib/nvme/nvme_pcie.c index b250e4bd7..4f6550a48 100644 --- a/lib/nvme/nvme_pcie.c +++ b/lib/nvme/nvme_pcie.c @@ -39,6 +39,7 @@ #include "spdk/stdinc.h" #include "spdk/env.h" #include "spdk/likely.h" +#include "spdk/string.h" #include "nvme_internal.h" #include "nvme_uevent.h" @@ -101,9 +102,6 @@ struct nvme_pcie_ctrlr { /* Opaque handle to associated PCI device. */ struct spdk_pci_device *devhandle; - /* File descriptor returned from spdk_pci_device_claim(). Closed when ctrlr is detached. */ - int claim_fd; - /* Flag to indicate the MMIO register has been remapped */ bool is_remapped; }; @@ -808,25 +806,20 @@ struct spdk_nvme_ctrlr *nvme_pcie_ctrlr_construct(const struct spdk_nvme_transpo union spdk_nvme_cap_register cap; union spdk_nvme_vs_register vs; uint32_t cmd_reg; - int rc, claim_fd; + int rc; struct spdk_pci_id pci_id; - struct spdk_pci_addr pci_addr; - if (spdk_pci_addr_parse(&pci_addr, trid->traddr)) { - SPDK_ERRLOG("could not parse pci address\n"); - return NULL; - } - - claim_fd = spdk_pci_device_claim(&pci_addr); - if (claim_fd < 0) { - SPDK_ERRLOG("could not claim device %s\n", trid->traddr); + rc = spdk_pci_device_claim(pci_dev); + if (rc < 0) { + SPDK_ERRLOG("could not claim device %s (%s)\n", + trid->traddr, spdk_strerror(-rc)); return NULL; } pctrlr = spdk_zmalloc(sizeof(struct nvme_pcie_ctrlr), 64, NULL, SPDK_ENV_SOCKET_ID_ANY, SPDK_MALLOC_SHARE); if (pctrlr == NULL) { - close(claim_fd); + spdk_pci_device_unclaim(pci_dev); SPDK_ERRLOG("could not allocate ctrlr\n"); return NULL; } @@ -836,19 +829,18 @@ struct spdk_nvme_ctrlr *nvme_pcie_ctrlr_construct(const struct spdk_nvme_transpo pctrlr->ctrlr.trid.trtype = SPDK_NVME_TRANSPORT_PCIE; pctrlr->devhandle = devhandle; pctrlr->ctrlr.opts = *opts; - pctrlr->claim_fd = claim_fd; memcpy(&pctrlr->ctrlr.trid, trid, sizeof(pctrlr->ctrlr.trid)); rc = nvme_ctrlr_construct(&pctrlr->ctrlr); if (rc != 0) { - close(claim_fd); + spdk_pci_device_unclaim(pci_dev); spdk_free(pctrlr); return NULL; } rc = nvme_pcie_ctrlr_allocate_bars(pctrlr); if (rc != 0) { - close(claim_fd); + spdk_pci_device_unclaim(pci_dev); spdk_free(pctrlr); return NULL; } @@ -860,14 +852,14 @@ struct spdk_nvme_ctrlr *nvme_pcie_ctrlr_construct(const struct spdk_nvme_transpo if (nvme_ctrlr_get_cap(&pctrlr->ctrlr, &cap)) { SPDK_ERRLOG("get_cap() failed\n"); - close(claim_fd); + spdk_pci_device_unclaim(pci_dev); spdk_free(pctrlr); return NULL; } if (nvme_ctrlr_get_vs(&pctrlr->ctrlr, &vs)) { SPDK_ERRLOG("get_vs() failed\n"); - close(claim_fd); + spdk_pci_device_unclaim(pci_dev); spdk_free(pctrlr); return NULL; } @@ -938,8 +930,6 @@ nvme_pcie_ctrlr_destruct(struct spdk_nvme_ctrlr *ctrlr) struct nvme_pcie_ctrlr *pctrlr = nvme_pcie_ctrlr(ctrlr); struct spdk_pci_device *devhandle = nvme_ctrlr_proc_get_devhandle(ctrlr); - close(pctrlr->claim_fd); - if (ctrlr->adminq) { nvme_pcie_qpair_destroy(ctrlr->adminq); } @@ -951,6 +941,7 @@ nvme_pcie_ctrlr_destruct(struct spdk_nvme_ctrlr *ctrlr) nvme_pcie_ctrlr_free_bars(pctrlr); if (devhandle) { + spdk_pci_device_unclaim(devhandle); spdk_pci_device_detach(devhandle); } diff --git a/module/copy/ioat/copy_engine_ioat.c b/module/copy/ioat/copy_engine_ioat.c index 4eebe3d10..5c0e7f07b 100644 --- a/module/copy/ioat/copy_engine_ioat.c +++ b/module/copy/ioat/copy_engine_ioat.c @@ -256,7 +256,7 @@ probe_cb(void *cb_ctx, struct spdk_pci_device *pci_dev) } /* Claim the device in case conflict with other process */ - if (spdk_pci_device_claim(&pci_addr) < 0) { + if (spdk_pci_device_claim(pci_dev) < 0) { return false; } diff --git a/test/env/pci/pci_ut.c b/test/env/pci/pci_ut.c index 9e6105e85..c2dfac3c9 100644 --- a/test/env/pci/pci_ut.c +++ b/test/env/pci/pci_ut.c @@ -38,25 +38,19 @@ #include "env_dpdk/pci.c" static void -pci_claim_test(void) +pci_claim_test(struct spdk_pci_device *dev) { int rc = 0; pid_t childPid; int status, ret; - struct spdk_pci_addr pci_addr; - pci_addr.domain = 0x0; - pci_addr.bus = 0x5; - pci_addr.dev = 0x4; - pci_addr.func = 1; - - rc = spdk_pci_device_claim(&pci_addr); + rc = spdk_pci_device_claim(dev); CU_ASSERT(rc >= 0); childPid = fork(); CU_ASSERT(childPid >= 0); if (childPid == 0) { - ret = spdk_pci_device_claim(&pci_addr); + ret = spdk_pci_device_claim(dev); CU_ASSERT(ret == -1); exit(0); } else { @@ -206,6 +200,9 @@ pci_hook_test(void) rc = spdk_pci_device_map_bar(&ut_dev.pci, 1, &bar0_vaddr, &bar0_paddr, &bar0_size); CU_ASSERT(rc != 0); + /* test spdk_pci_device_claim() */ + pci_claim_test(&ut_dev.pci); + /* detach and verify our callback was called */ spdk_pci_device_detach(&ut_dev.pci); CU_ASSERT(!ut_dev.attached); @@ -235,7 +232,6 @@ int main(int argc, char **argv) } if ( - CU_add_test(suite, "pci_claim", pci_claim_test) == NULL || CU_add_test(suite, "pci_hook", pci_hook_test) == NULL ) { CU_cleanup_registry(); diff --git a/test/unit/lib/nvme/nvme_pcie.c/nvme_pcie_ut.c b/test/unit/lib/nvme/nvme_pcie.c/nvme_pcie_ut.c index 937bd4255..1c7dcd668 100644 --- a/test/unit/lib/nvme/nvme_pcie.c/nvme_pcie_ut.c +++ b/test/unit/lib/nvme/nvme_pcie.c/nvme_pcie_ut.c @@ -153,7 +153,13 @@ spdk_pci_device_cfg_write32(struct spdk_pci_device *dev, uint32_t value, uint32_ } int -spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr) +spdk_pci_device_claim(struct spdk_pci_device *dev) +{ + abort(); +} + +void +spdk_pci_device_unclaim(struct spdk_pci_device *dev) { abort(); }