From e8e46cb615a39df19113c892bf0dd9c6890ce61a Mon Sep 17 00:00:00 2001 From: Darek Stojaczyk Date: Tue, 9 Jun 2020 18:31:54 +0200 Subject: [PATCH] env_dpdk/pci: remove device detach callback You don't get notified when someone starts using your hooked device, so there's not much gain from knowing when someone stops. Remove that callback and also move DPDK device detach under the same lock which sets the pending_removal flag. This eliminates a data race window when hotremove notification could arrive after device was detached, but before it was scheduled to be removed. vmd and ioat nest the spdk_pci_device struct and abigail complains even though the parent structs only have forward declarations in public headers. Adding those two structs to the suppression list doesn't help though. Abidiff still complains about the pci device struct being changed, probably because ioat.h and vmd.h both include env.h. Abidiff suppresion list should eventually be split per-lib, but for now ignore struct spdk_pci_device changes globally. $ abidiff [...]/libspdk_ioat.so [...] 'struct spdk_pci_device at env.h:652:1' changed: type size changed from 1024 to 960 (in bits) 1 data member deletion: Change-Id: I9b113572c661f0e0786b6d625e16dc07fe77e778 Signed-off-by: Darek Stojaczyk Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/2939 Community-CI: Mellanox Build Bot Reviewed-by: Ben Walker Reviewed-by: Jim Harris Tested-by: SPDK CI Jenkins --- include/spdk/env.h | 1 - lib/env_dpdk/Makefile | 2 +- lib/env_dpdk/pci.c | 17 +++++++++++++---- lib/vmd/vmd.c | 6 ------ test/env/pci/pci_ut.c | 12 +----------- test/make/check_so_deps.sh | 2 ++ 6 files changed, 17 insertions(+), 23 deletions(-) diff --git a/include/spdk/env.h b/include/spdk/env.h index 4777d3842..b92a4cef6 100644 --- a/include/spdk/env.h +++ b/include/spdk/env.h @@ -667,7 +667,6 @@ struct spdk_pci_device { uint32_t len, uint32_t offset); int (*cfg_write)(struct spdk_pci_device *dev, void *value, uint32_t len, uint32_t offset); - void (*detach)(struct spdk_pci_device *dev); struct _spdk_pci_device_internal { struct spdk_pci_driver *driver; diff --git a/lib/env_dpdk/Makefile b/lib/env_dpdk/Makefile index df3a4aaa4..fccdfdf4c 100644 --- a/lib/env_dpdk/Makefile +++ b/lib/env_dpdk/Makefile @@ -34,7 +34,7 @@ SPDK_ROOT_DIR := $(abspath $(CURDIR)/../..) include $(SPDK_ROOT_DIR)/mk/spdk.common.mk -SO_VER := 4 +SO_VER := 5 SO_MINOR := 0 CFLAGS += $(ENV_CFLAGS) diff --git a/lib/env_dpdk/pci.c b/lib/env_dpdk/pci.c index 06cb5dbd0..a6078822e 100644 --- a/lib/env_dpdk/pci.c +++ b/lib/env_dpdk/pci.c @@ -129,6 +129,7 @@ detach_rte(struct spdk_pci_device *dev) } pthread_mutex_lock(&g_pci_mutex); + dev->internal.attached = false; /* prevent the hotremove notification from removing this device */ dev->internal.pending_removal = true; pthread_mutex_unlock(&g_pci_mutex); @@ -323,7 +324,6 @@ pci_device_init(struct rte_pci_driver *_drv, dev->unmap_bar = unmap_bar_rte; dev->cfg_read = cfg_read_rte; dev->cfg_write = cfg_write_rte; - dev->detach = detach_rte; dev->internal.driver = driver; dev->internal.claim_fd = -1; @@ -382,8 +382,18 @@ spdk_pci_device_detach(struct spdk_pci_device *dev) spdk_pci_device_unclaim(dev); } - dev->internal.attached = false; - dev->detach(dev); + if (strcmp(dev->type, "pci") == 0) { + /* if it's a physical device we need to deal with DPDK on + * a different process and we can't just unset one flag + * here. We also want to stop using any device resources + * so that the device isn't "in use" by the userspace driver + * once we detach it. This would allow attaching the device + * to a different process, or to a kernel driver like nvme. + */ + detach_rte(dev); + } else { + dev->internal.attached = false; + } cleanup_pci_devices(); } @@ -951,7 +961,6 @@ spdk_pci_hook_device(struct spdk_pci_driver *drv, struct spdk_pci_device *dev) assert(dev->unmap_bar != NULL); assert(dev->cfg_read != NULL); assert(dev->cfg_write != NULL); - assert(dev->detach != NULL); dev->internal.driver = drv; TAILQ_INSERT_TAIL(&g_pci_devices, dev, internal.tailq); } diff --git a/lib/vmd/vmd.c b/lib/vmd/vmd.c index ae018a294..14d9558c2 100644 --- a/lib/vmd/vmd.c +++ b/lib/vmd/vmd.c @@ -863,11 +863,6 @@ vmd_dev_cfg_write(struct spdk_pci_device *_dev, void *value, return 0; } -static void -_vmd_dev_detach(struct spdk_pci_device *dev) -{ -} - static void vmd_dev_detach(struct spdk_pci_device *dev) { @@ -907,7 +902,6 @@ vmd_dev_init(struct vmd_pci_device *dev) dev->pci.unmap_bar = vmd_dev_unmap_bar; dev->pci.cfg_read = vmd_dev_cfg_read; dev->pci.cfg_write = vmd_dev_cfg_write; - dev->pci.detach = _vmd_dev_detach; dev->hotplug_capable = false; if (dev->pcie_cap != NULL) { dev->cached_slot_control = dev->pcie_cap->slot_control; diff --git a/test/env/pci/pci_ut.c b/test/env/pci/pci_ut.c index 8287e78ac..eb9a46199 100644 --- a/test/env/pci/pci_ut.c +++ b/test/env/pci/pci_ut.c @@ -128,14 +128,6 @@ ut_enum_cb(void *ctx, struct spdk_pci_device *dev) return 0; } -static void -ut_detach(struct spdk_pci_device *dev) -{ - struct ut_pci_dev *ut_dev = (struct ut_pci_dev *)dev; - - ut_dev->attached = false; -} - static void pci_hook_test(void) { @@ -145,6 +137,7 @@ pci_hook_test(void) uint64_t bar0_paddr, bar0_size; int rc; + ut_dev.pci.type = "custom"; ut_dev.pci.id.vendor_id = 0x4; ut_dev.pci.id.device_id = 0x8; @@ -159,7 +152,6 @@ pci_hook_test(void) ut_dev.pci.unmap_bar = ut_unmap_bar; ut_dev.pci.cfg_read = ut_cfg_read; ut_dev.pci.cfg_write = ut_cfg_write; - ut_dev.pci.detach = ut_detach; /* hook the device into the PCI layer */ spdk_pci_hook_device(&ut_pci_driver, &ut_dev.pci); @@ -207,9 +199,7 @@ pci_hook_test(void) /* 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); CU_ASSERT(!ut_dev.pci.internal.attached); /* unhook the device */ diff --git a/test/make/check_so_deps.sh b/test/make/check_so_deps.sh index 6008f6bf1..5f38f56a1 100755 --- a/test/make/check_so_deps.sh +++ b/test/make/check_so_deps.sh @@ -278,6 +278,8 @@ function confirm_abi_deps() { name = spdk_net_impl [suppress_type] name = spdk_lvol +[suppress_type] + name = spdk_pci_device EOF for object in "$libdir"/libspdk_*.so; do