From 4ff3fdc519fedfffe79b1171ac8a1843427a6a1d Mon Sep 17 00:00:00 2001 From: peluse Date: Wed, 1 Sep 2021 16:17:46 -0400 Subject: [PATCH] lib/idxd: only select idxd device that are on the same socket Prior a regular round robin could result in strange performance if an idxd device from another socket was used. Signed-off-by: peluse Signed-off-by: Ziye Yang Change-Id: Id863c79067beabe73ef89d92b3fb3c436821b97a Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/9367 Tested-by: SPDK CI Jenkins Community-CI: Broadcom CI Reviewed-by: Monica Kenguva Reviewed-by: Jim Harris Reviewed-by: Tomasz Zawadzki --- CHANGELOG.md | 5 +++++ doc/accel_fw.md | 9 +++++++++ include/spdk/idxd.h | 8 ++++++++ lib/idxd/idxd.c | 6 ++++++ lib/idxd/idxd.h | 1 + lib/idxd/idxd_kernel.c | 1 + lib/idxd/idxd_user.c | 1 + lib/idxd/spdk_idxd.map | 1 + module/accel/idxd/accel_engine_idxd.c | 15 +++++++++++++-- 9 files changed, 45 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f46ab93a4..05247badc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## v21.10: (Upcoming Release) +### idxd + +Added `spdk_idxd_get_socket` to query the socket that the idxd device +is on. + ### nvmf Added `oncs` to `struct spdk_nvmf_ctrlr_data` so that the transport layer diff --git a/doc/accel_fw.md b/doc/accel_fw.md index b46d6c70c..8315d0973 100644 --- a/doc/accel_fw.md +++ b/doc/accel_fw.md @@ -86,6 +86,15 @@ of service parameters on the work queues that are not currently utilized by the module. Specialized use of DSA may require different configurations that can be added to the module as needed. +When a new channel starts, a DSA device will be assigned to the channel. The accel +idxd module has been tuned for the most likely best performance case. The result +is that there is a limited number of channels that can be supported based on the +number of DSA devices in the system. Additionally, for best performance, the accel +idxd module will only use DSA devices on the same socket as the requesting +channel/thread. If an error occurs on initialization indicating that there are no +more DSA devices available either try fewer threads or, if on a 2 socket system, +try spreading threads across cores if possible. + ### Software Module {#accel_sw} The software module is enabled by default. If no hardware engine is explicitly diff --git a/include/spdk/idxd.h b/include/spdk/idxd.h index a1d034a2a..706ced25f 100644 --- a/include/spdk/idxd.h +++ b/include/spdk/idxd.h @@ -61,6 +61,14 @@ struct spdk_idxd_device; */ struct idxd_batch; +/** + * Get the socket that this device is on + * + * \param idxd device to query + * \return socket number. + */ +uint32_t spdk_idxd_get_socket(struct spdk_idxd_device *idxd); + /** * Signature for configuring a channel * diff --git a/lib/idxd/idxd.c b/lib/idxd/idxd.c index 8ba085df1..5bd35a81d 100644 --- a/lib/idxd/idxd.c +++ b/lib/idxd/idxd.c @@ -81,6 +81,12 @@ struct device_config g_dev_cfg1 = { .total_engines = 4, }; +uint32_t +spdk_idxd_get_socket(struct spdk_idxd_device *idxd) +{ + return idxd->socket_id; +} + static inline void _submit_to_hw(struct spdk_idxd_io_channel *chan, struct idxd_ops *op) { diff --git a/lib/idxd/idxd.h b/lib/idxd/idxd.h index 77409540f..49d3e2591 100644 --- a/lib/idxd/idxd.h +++ b/lib/idxd/idxd.h @@ -168,6 +168,7 @@ struct spdk_idxd_impl { struct spdk_idxd_device { struct spdk_idxd_impl *impl; void *portals; + uint32_t socket_id; int wq_id; uint32_t num_channels; uint32_t total_wq_size; diff --git a/lib/idxd/idxd_kernel.c b/lib/idxd/idxd_kernel.c index f91e01e0f..a5b9230de 100644 --- a/lib/idxd/idxd_kernel.c +++ b/lib/idxd/idxd_kernel.c @@ -140,6 +140,7 @@ dsa_setup_single_wq(struct spdk_kernel_idxd_device *kernel_idxd, struct accfg_wq /* Update the active_wq_num of the kernel device */ kernel_idxd->wq_active_num++; kernel_idxd->idxd.total_wq_size += wq_ctx->wq_size; + kernel_idxd->idxd.socket_id = accfg_device_get_numa_node(dev); return 0; } diff --git a/lib/idxd/idxd_user.c b/lib/idxd/idxd_user.c index 4b624bbae..9f0b212ac 100644 --- a/lib/idxd/idxd_user.c +++ b/lib/idxd/idxd_user.c @@ -567,6 +567,7 @@ idxd_attach(struct spdk_pci_device *device) idxd = &user_idxd->idxd; user_idxd->device = device; idxd->impl = &g_user_idxd_impl; + idxd->socket_id = device->socket_id; pthread_mutex_init(&idxd->num_channels_lock, NULL); /* Enable PCI busmaster. */ diff --git a/lib/idxd/spdk_idxd.map b/lib/idxd/spdk_idxd.map index af3755dc3..226ca1230 100644 --- a/lib/idxd/spdk_idxd.map +++ b/lib/idxd/spdk_idxd.map @@ -16,6 +16,7 @@ spdk_idxd_batch_create; spdk_idxd_batch_cancel; spdk_idxd_batch_get_max; + spdk_idxd_get_socket; spdk_idxd_set_config; spdk_idxd_submit_compare; spdk_idxd_submit_crc32c; diff --git a/module/accel/idxd/accel_engine_idxd.c b/module/accel/idxd/accel_engine_idxd.c index 9bcaab28a..5465a8294 100644 --- a/module/accel/idxd/accel_engine_idxd.c +++ b/module/accel/idxd/accel_engine_idxd.c @@ -85,6 +85,7 @@ idxd_select_device(struct idxd_io_channel *chan) { uint32_t count = 0; struct idxd_device *dev; + uint32_t socket_id = spdk_env_get_socket_id(spdk_env_get_current_core()); /* * We allow channels to share underlying devices, @@ -101,6 +102,10 @@ idxd_select_device(struct idxd_io_channel *chan) dev = g_next_dev; pthread_mutex_unlock(&g_dev_lock); + if (socket_id != spdk_idxd_get_socket(dev->idxd)) { + continue; + } + /* * Now see if a channel is available on this one. We only * allow a specific number of channels to share a device @@ -109,12 +114,18 @@ idxd_select_device(struct idxd_io_channel *chan) chan->chan = spdk_idxd_get_channel(dev->idxd); if (chan->chan != NULL) { chan->max_outstanding = spdk_idxd_chan_get_max_operations(chan->chan); + SPDK_DEBUGLOG(accel_idxd, "On socket %d using device on socket %d\n", + socket_id, spdk_idxd_get_socket(dev->idxd)); return dev; } } while (count++ < g_num_devices); - /* we are out of available channels and devices. */ - SPDK_ERRLOG("No more DSA devices available!\n"); + /* We are out of available channels and/or devices for the local socket. We fix the number + * of channels that we allocate per device and only allocate devices on the same socket + * that the current thread is on. If on a 2 socket system it may be possible to avoid + * this situation by spreading threads across the sockets. + */ + SPDK_ERRLOG("No more DSA devices available on the local socket.\n"); return NULL; }