From 3e0e07793978c98e3132cfabb9b4e749af3baf01 Mon Sep 17 00:00:00 2001 From: Konrad Sztyber Date: Tue, 28 Mar 2023 13:23:39 +0200 Subject: [PATCH] accel: copy memory domain context when merging tasks When changing src/dst buffers, we copied memory domain pointers, but we didn't copy memory domain context, which is obviously incorrect. It was probably missed, because we never append a copy with non-NULL memory domain. Added a unit test case to verify this behavior. Signed-off-by: Konrad Sztyber Change-Id: Ic174e0e72c33d3f437f0faddd3405638049f0c74 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/17425 Reviewed-by: Jim Harris Community-CI: Mellanox Build Bot Reviewed-by: Aleksey Marchuk Tested-by: SPDK CI Jenkins --- lib/accel/accel.c | 2 + test/unit/lib/accel/accel.c/accel_ut.c | 72 ++++++++++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/lib/accel/accel.c b/lib/accel/accel.c index 303ca499a..0d7d43e33 100644 --- a/lib/accel/accel.c +++ b/lib/accel/accel.c @@ -1752,6 +1752,7 @@ accel_sequence_merge_tasks(struct spdk_accel_sequence *seq, struct spdk_accel_ta next->s.iovs = task->s.iovs; next->s.iovcnt = task->s.iovcnt; next->src_domain = task->src_domain; + next->src_domain_ctx = task->src_domain_ctx; TAILQ_REMOVE(&seq->tasks, task, seq_link); TAILQ_INSERT_TAIL(&seq->completed, task, seq_link); break; @@ -1773,6 +1774,7 @@ accel_sequence_merge_tasks(struct spdk_accel_sequence *seq, struct spdk_accel_ta task->d.iovs = next->d.iovs; task->d.iovcnt = next->d.iovcnt; task->dst_domain = next->dst_domain; + task->dst_domain_ctx = next->dst_domain_ctx; /* We're removing next_task from the tasks queue, so we need to update its pointer, * so that the TAILQ_FOREACH_SAFE() loop below works correctly */ *next_task = TAILQ_NEXT(next, seq_link); diff --git a/test/unit/lib/accel/accel.c/accel_ut.c b/test/unit/lib/accel/accel.c/accel_ut.c index 2d0e3493a..05f50e772 100644 --- a/test/unit/lib/accel/accel.c/accel_ut.c +++ b/test/unit/lib/accel/accel.c/accel_ut.c @@ -975,8 +975,12 @@ struct ut_sequence_operation { int count; struct iovec *src_iovs; uint32_t src_iovcnt; + struct spdk_memory_domain *src_domain; + void *src_domain_ctx; struct iovec *dst_iovs; uint32_t dst_iovcnt; + struct spdk_memory_domain *dst_domain; + void *dst_domain_ctx; int (*submit)(struct spdk_io_channel *ch, struct spdk_accel_task *t); }; @@ -1002,6 +1006,15 @@ ut_sequnce_submit_tasks(struct spdk_io_channel *ch, struct spdk_accel_task *task sizeof(struct iovec) * op->dst_iovcnt), 0); } + CU_ASSERT_EQUAL(task->src_domain, op->src_domain); + CU_ASSERT_EQUAL(task->dst_domain, op->dst_domain); + if (op->src_domain != NULL) { + CU_ASSERT_EQUAL(task->src_domain_ctx, op->src_domain_ctx); + } + if (op->dst_domain != NULL) { + CU_ASSERT_EQUAL(task->dst_domain_ctx, op->dst_domain_ctx); + } + if (op->submit_status != 0) { return op->submit_status; } @@ -1491,6 +1504,7 @@ test_sequence_copy_elision(void) /* Override the submit_tasks function */ g_module_if.submit_tasks = ut_sequnce_submit_tasks; + g_module.supports_memory_domains = true; for (i = 0; i < ACCEL_OPC_LAST; ++i) { g_seq_operations[i].complete_status = 0; g_seq_operations[i].submit_status = 0; @@ -1869,11 +1883,69 @@ test_sequence_copy_elision(void) CU_ASSERT_EQUAL(g_seq_operations[ACCEL_OPC_COPY].count, 0); CU_ASSERT_EQUAL(g_seq_operations[ACCEL_OPC_DECRYPT].count, 1); + /* Check a sequence with memory domains and verify their pointers (and their contexts) are + * correctly set on the next/prev task when a copy is removed */ + seq = NULL; + completed = 0; + g_seq_operations[ACCEL_OPC_COPY].count = 0; + g_seq_operations[ACCEL_OPC_DECRYPT].count = 0; + g_seq_operations[ACCEL_OPC_DECRYPT].dst_iovcnt = 1; + g_seq_operations[ACCEL_OPC_DECRYPT].src_iovcnt = 1; + g_seq_operations[ACCEL_OPC_DECRYPT].src_iovs = &exp_iovs[0]; + g_seq_operations[ACCEL_OPC_DECRYPT].dst_iovs = &exp_iovs[1]; + g_seq_operations[ACCEL_OPC_DECRYPT].dst_domain = (void *)0x1; + g_seq_operations[ACCEL_OPC_DECRYPT].dst_domain_ctx = (void *)0x2; + g_seq_operations[ACCEL_OPC_DECRYPT].src_domain = (void *)0x3; + g_seq_operations[ACCEL_OPC_DECRYPT].src_domain_ctx = (void *)0x4; + 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]); + src_iovs[0].iov_base = tmp[0]; + src_iovs[0].iov_len = sizeof(tmp[0]); + rc = spdk_accel_append_copy(&seq, ioch, &dst_iovs[0], 1, (void *)0xdead, (void *)0xbeef, + &src_iovs[0], 1, (void *)0x3, (void *)0x4, 0, + ut_sequence_step_cb, &completed); + CU_ASSERT_EQUAL(rc, 0); + + dst_iovs[1].iov_base = tmp[2]; + dst_iovs[1].iov_len = sizeof(tmp[2]); + src_iovs[1].iov_base = tmp[1]; + src_iovs[1].iov_len = sizeof(tmp[1]); + rc = spdk_accel_append_decrypt(&seq, ioch, &key, &dst_iovs[1], 1, (void *)0xdead, (void *)0xbeef, + &src_iovs[1], 1, (void *)0xdead, (void *)0xbeef, 0, + sizeof(tmp[2]), 0, ut_sequence_step_cb, &completed); + CU_ASSERT_EQUAL(rc, 0); + + dst_iovs[2].iov_base = buf; + dst_iovs[2].iov_len = sizeof(buf); + src_iovs[2].iov_base = tmp[2]; + src_iovs[2].iov_len = sizeof(tmp[2]); + rc = spdk_accel_append_copy(&seq, ioch, &dst_iovs[2], 1, (void *)0x1, (void *)0x2, + &src_iovs[2], 1, (void *)0xdead, (void *)0xbeef, 0, + ut_sequence_step_cb, &completed); + CU_ASSERT_EQUAL(rc, 0); + + ut_seq.complete = false; + spdk_accel_sequence_finish(seq, ut_sequence_complete_cb, &ut_seq); + + poll_threads(); + + CU_ASSERT_EQUAL(completed, 3); + CU_ASSERT(ut_seq.complete); + CU_ASSERT_EQUAL(ut_seq.status, 0); + CU_ASSERT_EQUAL(g_seq_operations[ACCEL_OPC_COPY].count, 0); + CU_ASSERT_EQUAL(g_seq_operations[ACCEL_OPC_DECRYPT].count, 1); + /* Cleanup module pointers to make subsequent tests work correctly */ for (i = 0; i < ACCEL_OPC_LAST; ++i) { g_modules_opc[i] = modules[i]; } + g_module.supports_memory_domains = false; ut_clear_operations(); spdk_put_io_channel(ioch); poll_threads();