From dee8e1f4c0775fc839f3fc9f0c9e0aab0aee5643 Mon Sep 17 00:00:00 2001 From: Konrad Sztyber Date: Fri, 9 Dec 2022 13:59:50 +0100 Subject: [PATCH] accel: use iovecs for copy operations This patch is first in the series of patches aimed to make all accel operations describe their buffers with iovecs. The intention is to make it easier to handle tasks in a generic way. It doesn't mean that we change the API - all function signatures are preserved. If a function doesn't use iovecs, we use the aux_iovs array. However, this does mean that each accel module that provides support for a given operation will need to be adjusted to use iovecs. Additionally, update the unit test checking copy elision to verify the buffers of the copy operation that is left. Signed-off-by: Konrad Sztyber Change-Id: I9e6d8d1be3b8b9706cb4a6222dad30e8c373d8fb Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/15937 Tested-by: SPDK CI Jenkins Reviewed-by: Ben Walker Reviewed-by: Aleksey Marchuk --- include/spdk_internal/accel_module.h | 9 ++++++ lib/accel/accel.c | 42 ++++++-------------------- lib/accel/accel_sw.c | 21 ++++++++++++- module/accel/dsa/accel_dsa.c | 7 ++--- module/accel/ioat/accel_ioat.c | 20 ++++++++++-- test/unit/lib/accel/accel.c/accel_ut.c | 11 +++++-- 6 files changed, 66 insertions(+), 44 deletions(-) diff --git a/include/spdk_internal/accel_module.h b/include/spdk_internal/accel_module.h index 0a77963d0..038540d74 100644 --- a/include/spdk_internal/accel_module.h +++ b/include/spdk_internal/accel_module.h @@ -32,6 +32,14 @@ struct spdk_accel_crypto_key { TAILQ_ENTRY(spdk_accel_crypto_key) link; }; +enum spdk_accel_aux_iov_type { + SPDK_ACCEL_AUX_IOV_SRC, + SPDK_ACCEL_AUX_IOV_DST, + SPDK_ACCEL_AUX_IOV_SRC2, + SPDK_ACCEL_AUX_IOV_DST2, + SPDK_ACCEL_AUX_IOV_MAX, +}; + struct spdk_accel_task { struct accel_io_channel *accel_ch; spdk_accel_completion_cb cb_fn; @@ -76,6 +84,7 @@ struct spdk_accel_task { }; int flags; int status; + struct iovec aux_iovs[SPDK_ACCEL_AUX_IOV_MAX]; TAILQ_ENTRY(spdk_accel_task) link; TAILQ_ENTRY(spdk_accel_task) seq_link; }; diff --git a/lib/accel/accel.c b/lib/accel/accel.c index e572a065a..f7692c405 100644 --- a/lib/accel/accel.c +++ b/lib/accel/accel.c @@ -215,10 +215,15 @@ spdk_accel_submit_copy(struct spdk_io_channel *ch, void *dst, void *src, return -ENOMEM; } - accel_task->dst = dst; - accel_task->src = src; + accel_task->s.iovs = &accel_task->aux_iovs[SPDK_ACCEL_AUX_IOV_SRC]; + accel_task->d.iovs = &accel_task->aux_iovs[SPDK_ACCEL_AUX_IOV_DST]; + accel_task->d.iovs[0].iov_base = dst; + accel_task->d.iovs[0].iov_len = nbytes; + accel_task->d.iovcnt = 1; + accel_task->s.iovs[0].iov_base = src; + accel_task->s.iovs[0].iov_len = nbytes; + accel_task->s.iovcnt = 1; accel_task->op_code = ACCEL_OPC_COPY; - accel_task->nbytes = nbytes; accel_task->flags = flags; accel_task->src_domain = NULL; accel_task->dst_domain = NULL; @@ -978,6 +983,7 @@ accel_sequence_set_virtbuf(struct spdk_accel_sequence *seq, struct accel_buffer TAILQ_FOREACH(task, &seq->tasks, seq_link) { switch (task->op_code) { case ACCEL_OPC_DECOMPRESS: + case ACCEL_OPC_COPY: if (task->src_domain == g_accel_domain && task->src_domain_ctx == buf) { accel_update_iovs(task->s.iovs, task->s.iovcnt, buf); task->src_domain = NULL; @@ -987,17 +993,6 @@ accel_sequence_set_virtbuf(struct spdk_accel_sequence *seq, struct accel_buffer task->dst_domain = NULL; } break; - case ACCEL_OPC_COPY: - /* By the time we're here, we've already changed iovecs -> buf */ - if (task->src_domain == g_accel_domain && task->src_domain_ctx == buf) { - accel_update_buf(&task->src, buf); - task->src_domain = NULL; - } - if (task->dst_domain == g_accel_domain && task->dst_domain_ctx == buf) { - accel_update_buf(&task->dst, buf); - task->dst_domain = NULL; - } - break; case ACCEL_OPC_FILL: /* Fill should have src_domain cleared */ assert(task->src_domain == NULL); @@ -1223,25 +1218,6 @@ spdk_accel_sequence_finish(struct spdk_accel_sequence *seq, accel_sequence_merge_tasks(seq, task, &next); } - /* Since we store copy operations' buffers as iovecs, we need to convert them to scalar - * buffers, as that's what accel modules expect - */ - TAILQ_FOREACH(task, &seq->tasks, seq_link) { - if (task->op_code != ACCEL_OPC_COPY) { - continue; - } - - if (spdk_unlikely(task->s.iovcnt != 1 || task->d.iovcnt != 1)) { - SPDK_ERRLOG("Unable to set buffer of a copy operation due " - "to iovcnt!=1\n"); - return -EINVAL; - } - task->nbytes = spdk_min(task->s.iovs[0].iov_len, - task->d.iovs[0].iov_len); - task->src = task->s.iovs[0].iov_base; - task->dst = task->d.iovs[0].iov_base; - } - seq->cb_fn = cb_fn; seq->cb_arg = cb_arg; diff --git a/lib/accel/accel_sw.c b/lib/accel/accel_sw.c index 4b8853fb8..4679826e0 100644 --- a/lib/accel/accel_sw.c +++ b/lib/accel/accel_sw.c @@ -141,6 +141,23 @@ _sw_accel_copy(void *dst, void *src, size_t nbytes, int flags) } } +static int +_sw_accel_copy_iovs(struct iovec *dst_iovs, uint32_t dst_iovcnt, + struct iovec *src_iovs, uint32_t src_iovcnt, int flags) +{ + if (spdk_unlikely(dst_iovcnt != 1 || src_iovcnt != 1)) { + return -EINVAL; + } + + if (spdk_unlikely(dst_iovs[0].iov_len != src_iovs[0].iov_len)) { + return -EINVAL; + } + + _sw_accel_copy(dst_iovs[0].iov_base, src_iovs[0].iov_base, dst_iovs[0].iov_len, flags); + + return 0; +} + static void _sw_accel_copyv(void *dst, struct iovec *iov, uint32_t iovcnt, int flags) { @@ -462,7 +479,9 @@ sw_accel_submit_tasks(struct spdk_io_channel *ch, struct spdk_accel_task *accel_ case ACCEL_OPC_COPY: rc = _check_flags(accel_task->flags); if (rc == 0) { - _sw_accel_copy(accel_task->dst, accel_task->src, accel_task->nbytes, accel_task->flags); + rc = _sw_accel_copy_iovs(accel_task->d.iovs, accel_task->d.iovcnt, + accel_task->s.iovs, accel_task->s.iovcnt, + accel_task->flags); } break; case ACCEL_OPC_FILL: diff --git a/module/accel/dsa/accel_dsa.c b/module/accel/dsa/accel_dsa.c index b98c6e402..c12d8b7ed 100644 --- a/module/accel/dsa/accel_dsa.c +++ b/module/accel/dsa/accel_dsa.c @@ -135,15 +135,12 @@ _process_single_task(struct spdk_io_channel *ch, struct spdk_accel_task *task) switch (task->op_code) { case ACCEL_OPC_COPY: - siov.iov_base = task->src; - siov.iov_len = task->nbytes; - diov.iov_base = task->dst; - diov.iov_len = task->nbytes; if (task->flags & ACCEL_FLAG_PERSISTENT) { flags |= SPDK_IDXD_FLAG_PERSISTENT; flags |= SPDK_IDXD_FLAG_NONTEMPORAL; } - rc = spdk_idxd_submit_copy(chan->chan, &diov, 1, &siov, 1, flags, dsa_done, idxd_task); + rc = spdk_idxd_submit_copy(chan->chan, task->d.iovs, task->d.iovcnt, + task->s.iovs, task->s.iovcnt, flags, dsa_done, idxd_task); break; case ACCEL_OPC_DUALCAST: if (task->flags & ACCEL_FLAG_PERSISTENT) { diff --git a/module/accel/ioat/accel_ioat.c b/module/accel/ioat/accel_ioat.c index 6fc2dd27a..bbf43a15b 100644 --- a/module/accel/ioat/accel_ioat.c +++ b/module/accel/ioat/accel_ioat.c @@ -9,6 +9,7 @@ #include "spdk_internal/accel_module.h" #include "spdk/log.h" +#include "spdk/likely.h" #include "spdk/env.h" #include "spdk/event.h" @@ -128,6 +129,22 @@ ioat_supports_opcode(enum accel_opcode opc) } +static int +ioat_submit_copy(struct ioat_io_channel *ioat_ch, struct spdk_accel_task *task) +{ + if (spdk_unlikely(task->d.iovcnt != 1 || task->s.iovcnt != 1)) { + return -EINVAL; + } + + if (spdk_unlikely(task->d.iovs[0].iov_len != task->s.iovs[0].iov_len)) { + return -EINVAL; + } + + return spdk_ioat_build_copy(ioat_ch->ioat_ch, task, ioat_done, + task->d.iovs[0].iov_base, task->s.iovs[0].iov_base, + task->d.iovs[0].iov_len); +} + static int ioat_submit_tasks(struct spdk_io_channel *ch, struct spdk_accel_task *accel_task) { @@ -147,8 +164,7 @@ ioat_submit_tasks(struct spdk_io_channel *ch, struct spdk_accel_task *accel_task accel_task->dst, accel_task->fill_pattern, accel_task->nbytes); break; case ACCEL_OPC_COPY: - rc = spdk_ioat_build_copy(ioat_ch->ioat_ch, accel_task, ioat_done, - accel_task->dst, accel_task->src, accel_task->nbytes); + rc = ioat_submit_copy(ioat_ch, accel_task); break; default: assert(false); diff --git a/test/unit/lib/accel/accel.c/accel_ut.c b/test/unit/lib/accel/accel.c/accel_ut.c index eadf7ed0c..740dd127c 100644 --- a/test/unit/lib/accel/accel.c/accel_ut.c +++ b/test/unit/lib/accel/accel.c/accel_ut.c @@ -187,10 +187,7 @@ test_spdk_accel_submit_copy(void) /* submission OK. */ rc = spdk_accel_submit_copy(g_ch, dst, src, nbytes, flags, NULL, cb_arg); CU_ASSERT(rc == 0); - CU_ASSERT(task.dst == dst); - CU_ASSERT(task.src == src); CU_ASSERT(task.op_code == ACCEL_OPC_COPY); - CU_ASSERT(task.nbytes == nbytes); CU_ASSERT(task.flags == 0); CU_ASSERT(memcmp(dst, src, TEST_SUBMIT_SIZE) == 0); expected_accel_task = TAILQ_FIRST(&g_sw_ch->tasks_to_complete); @@ -1636,6 +1633,14 @@ test_sequence_copy_elision(void) seq = NULL; completed = 0; g_seq_operations[ACCEL_OPC_COPY].count = 0; + g_seq_operations[ACCEL_OPC_COPY].dst_iovcnt = 1; + g_seq_operations[ACCEL_OPC_COPY].src_iovcnt = 1; + g_seq_operations[ACCEL_OPC_COPY].src_iovs = &exp_iovs[0]; + g_seq_operations[ACCEL_OPC_COPY].dst_iovs = &exp_iovs[1]; + exp_iovs[0].iov_base = tmp[0]; + exp_iovs[0].iov_len = sizeof(tmp[0]); + exp_iovs[1].iov_base = buf; + exp_iovs[1].iov_len = sizeof(buf); dst_iovs[0].iov_base = tmp[1]; dst_iovs[0].iov_len = sizeof(tmp[1]);