Skip to contentSkip to conversation

crypto: iaa - rework the WQ selection tables

PATCH In review Vinicius Costa Gomes 2 participants 6 messages Oct 19 – Nov 11

Subsystems (2)
lore
Share & tools
lore

Vinicius Costa Gomes

author maintainer
#0 lore raw

cover crypto: iaa - rework the WQ selection tables

iaa_crypto maps each CPU to a single IAA device through a flat CPU index, which ignores packages (where the actual IAA devices live) and leaves other IAA devices in the package idle.

Patches 1-2 make the request carry the IAA workqueue representation ('iaa_wq') it holds a reference on, instead of looking it up again. Patch 3 replaces the per-CPU WQ table with per-package tables published with RCU, and patch 4 keeps each CPU on one WQ for decompression.

Patches 1-3 are meant more of a simplification, to make the code easier to reason about. Patch 4 is "low hanging" optimization (because decompressions are very fast, they benefit when the same CPU uses the same engine). Future work will introduce more complex optimizations.

---
Vinicius Costa Gomes (4):
crypto: iaa - carry the iaa_wq in the request
crypto: iaa - take the WQ from the request in the submit helpers
crypto: iaa - use RCU per-package WQ tables
crypto: iaa - keep each CPU on one WQ for decompression

 drivers/crypto/intel/iaa/iaa_crypto.h      |   7 -
 drivers/crypto/intel/iaa/iaa_crypto_main.c | 393 ++++++++++++-----------------
 2 files changed, 157 insertions(+), 243 deletions(-)

---
base-commit: 178aff4cf425b3fc51ff09c571fa1bfbc1eb8ea4
base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
change-id: 20261008-iaa-crypto-wq-tables-4e5911143017

Best regards,

Signature · 1 line
Vinicius
author maintainer
#1 lore raw

PATCH 1/4 crypto: iaa - carry the iaa_wq in the request

The submit helpers and the completion look the iaa_wq up again through the WQ's private pointer, after the entry point has already found it to take its reference. The submit helpers are also passed the WQ separately, so a request has no single record of which WQ it holds its reference on.

Store the iaa_wq in the request context when the reference is taken, and use it everywhere after that. This makes the association of a IAA workqueue and the request clear.

---
 drivers/crypto/intel/iaa/iaa_crypto_main.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
drivers/crypto/intel/iaa/iaa_crypto_main.c +8 -4
index c9ab4b83ae02..0f5bd244b00a 100644
@@ -159,6 +159,7 @@ static bool async_mode;
 static bool use_irq;
 
 struct iaa_req_ctx {
+	struct iaa_wq *iaa_wq;
 	u32 compression_crc;
 	struct page *bounce_src;
 	dma_addr_t bounce_src_dma;
@@ -1070,7 +1071,7 @@ static void iaa_desc_complete(struct idxd_desc *idxd_desc,
 
 	compression_ctx = crypto_tfm_ctx(ctx->tfm);
 
-	iaa_wq = idxd_wq_get_private(idxd_desc->wq);
+	iaa_wq = req_ctx->iaa_wq;
 	iaa_device = iaa_wq->iaa_device;
 	idxd = iaa_device->idxd;
 	pdev = idxd->pdev;
@@ -1177,7 +1178,7 @@ static int iaa_compress(struct crypto_tfm *tfm,	struct acomp_req *req,
 	struct device *dev;
 	int ret = 0;
 
-	iaa_wq = idxd_wq_get_private(wq);
+	iaa_wq = req_ctx->iaa_wq;
 	iaa_device = iaa_wq->iaa_device;
 	idxd = iaa_device->idxd;
 	pdev = idxd->pdev;
@@ -1328,7 +1329,7 @@ static int iaa_compress_verify(struct crypto_tfm *tfm, struct acomp_req *req,
 	struct device *dev;
 	int ret = 0;
 
-	iaa_wq = idxd_wq_get_private(wq);
+	iaa_wq = req_ctx->iaa_wq;
 	iaa_device = iaa_wq->iaa_device;
 	idxd = iaa_device->idxd;
 	pdev = idxd->pdev;
@@ -1405,6 +1406,7 @@ static int iaa_decompress(struct crypto_tfm *tfm, struct acomp_req *req,
 {
 	struct iaa_device_compression_mode *active_compression_mode;
 	struct iaa_compression_ctx *ctx = crypto_tfm_ctx(tfm);
+	struct iaa_req_ctx *req_ctx = acomp_request_ctx(req);
 	struct iaa_device *iaa_device;
 	struct idxd_desc *idxd_desc;
 	struct iax_hw_desc *desc;
@@ -1414,7 +1416,7 @@ static int iaa_decompress(struct crypto_tfm *tfm, struct acomp_req *req,
 	struct device *dev;
 	int ret = 0;
 
-	iaa_wq = idxd_wq_get_private(wq);
+	iaa_wq = req_ctx->iaa_wq;
 	iaa_device = iaa_wq->iaa_device;
 	idxd = iaa_device->idxd;
 	pdev = idxd->pdev;
@@ -1555,6 +1557,7 @@ static int iaa_comp_acompress(struct acomp_req *req)
 	}
 
 	iaa_wq = idxd_wq_get_private(wq);
+	req_ctx->iaa_wq = iaa_wq;
 
 	dev = &wq->idxd->pdev->dev;
 
@@ -1664,6 +1667,7 @@ static int iaa_comp_adecompress(struct acomp_req *req)
 	}
 
 	iaa_wq = idxd_wq_get_private(wq);
+	req_ctx->iaa_wq = iaa_wq;
 
 	dev = &wq->idxd->pdev->dev;
 
Signature · 1 line
2.56.0
author maintainer
#2 lore raw

PATCH 2/4 crypto: iaa - take the WQ from the request in the submit helpers

In reply to Vinicius Costa Gomes

iaa_compress(), iaa_compress_verify() and iaa_decompress() take the WQ to submit to as a parameter, and also find the iaa_wq in the request context. Nothing ties the two together.

Drop the parameter and use the request's iaa_wq->wq.

---
 drivers/crypto/intel/iaa/iaa_crypto_main.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)
drivers/crypto/intel/iaa/iaa_crypto_main.c +11 -10
index 0f5bd244b00a..7b02d3836623 100644
@@ -1049,7 +1049,6 @@ static int iaa_remap_for_verify(struct device *dev, struct iaa_wq *iaa_wq,
 				dma_addr_t *src_addr, dma_addr_t *dst_addr);
 
 static int iaa_compress_verify(struct crypto_tfm *tfm, struct acomp_req *req,
-			       struct idxd_wq *wq,
 			       dma_addr_t src_addr, unsigned int slen,
 			       dma_addr_t dst_addr, unsigned int *dlen);
 
@@ -1134,7 +1133,7 @@ static void iaa_desc_complete(struct idxd_desc *idxd_desc,
 			goto out;
 		}
 
-		ret = iaa_compress_verify(ctx->tfm, ctx->req, iaa_wq->wq, src_addr,
+		ret = iaa_compress_verify(ctx->tfm, ctx->req, src_addr,
 					  ctx->req->slen, dst_addr, &ctx->req->dlen);
 		if (ret) {
 			dev_dbg(dev, "%s: compress verify failed ret=%d\n", __func__, ret);
@@ -1162,7 +1161,6 @@ static void iaa_desc_complete(struct idxd_desc *idxd_desc,
 }
 
 static int iaa_compress(struct crypto_tfm *tfm,	struct acomp_req *req,
-			struct idxd_wq *wq,
 			dma_addr_t src_addr, unsigned int slen,
 			dma_addr_t dst_addr, unsigned int *dlen)
 {
@@ -1175,10 +1173,12 @@ static int iaa_compress(struct crypto_tfm *tfm,	struct acomp_req *req,
 	struct idxd_device *idxd;
 	struct iaa_wq *iaa_wq;
 	struct pci_dev *pdev;
+	struct idxd_wq *wq;
 	struct device *dev;
 	int ret = 0;
 
 	iaa_wq = req_ctx->iaa_wq;
+	wq = iaa_wq->wq;
 	iaa_device = iaa_wq->iaa_device;
 	idxd = iaa_device->idxd;
 	pdev = idxd->pdev;
@@ -1313,7 +1313,6 @@ static int iaa_remap_for_verify(struct device *dev, struct iaa_wq *iaa_wq,
 }
 
 static int iaa_compress_verify(struct crypto_tfm *tfm, struct acomp_req *req,
-			       struct idxd_wq *wq,
 			       dma_addr_t src_addr, unsigned int slen,
 			       dma_addr_t dst_addr, unsigned int *dlen)
 {
@@ -1326,10 +1325,12 @@ static int iaa_compress_verify(struct crypto_tfm *tfm, struct acomp_req *req,
 	struct idxd_device *idxd;
 	struct iaa_wq *iaa_wq;
 	struct pci_dev *pdev;
+	struct idxd_wq *wq;
 	struct device *dev;
 	int ret = 0;
 
 	iaa_wq = req_ctx->iaa_wq;
+	wq = iaa_wq->wq;
 	iaa_device = iaa_wq->iaa_device;
 	idxd = iaa_device->idxd;
 	pdev = idxd->pdev;
@@ -1400,7 +1401,6 @@ static int iaa_compress_verify(struct crypto_tfm *tfm, struct acomp_req *req,
 }
 
 static int iaa_decompress(struct crypto_tfm *tfm, struct acomp_req *req,
-			  struct idxd_wq *wq,
 			  dma_addr_t src_addr, unsigned int slen,
 			  dma_addr_t dst_addr, unsigned int *dlen)
 {
@@ -1413,10 +1413,12 @@ static int iaa_decompress(struct crypto_tfm *tfm, struct acomp_req *req,
 	struct idxd_device *idxd;
 	struct iaa_wq *iaa_wq;
 	struct pci_dev *pdev;
+	struct idxd_wq *wq;
 	struct device *dev;
 	int ret = 0;
 
 	iaa_wq = req_ctx->iaa_wq;
+	wq = iaa_wq->wq;
 	iaa_device = iaa_wq->iaa_device;
 	idxd = iaa_device->idxd;
 	pdev = idxd->pdev;
@@ -1582,8 +1584,7 @@ static int iaa_comp_acompress(struct acomp_req *req)
 	dev_dbg(dev, "map dst %llx req->dst %p dlen %d sg_len %d\n", dst_addr,
 		req->dst, req->dlen, sg_dma_len(req->dst));
 
-	ret = iaa_compress(tfm, req, wq, src_addr, req->slen, dst_addr,
-			   &req->dlen);
+	ret = iaa_compress(tfm, req, src_addr, req->slen, dst_addr, &req->dlen);
 	if (ret == -EINPROGRESS)
 		return ret;
 
@@ -1594,7 +1595,7 @@ static int iaa_comp_acompress(struct acomp_req *req)
 			goto out;
 		}
 
-		ret = iaa_compress_verify(tfm, req, wq, src_addr, req->slen,
+		ret = iaa_compress_verify(tfm, req, src_addr, req->slen,
 					  dst_addr, &req->dlen);
 		if (ret)
 			dev_dbg(dev, "asynchronous compress verification failed ret=%d\n", ret);
@@ -1719,8 +1720,8 @@ static int iaa_comp_adecompress(struct acomp_req *req)
 	dev_dbg(dev, "map dst %llx req->dst %p dlen %d sg_len %d\n", dst_addr,
 		req->dst, req->dlen, sg_dma_len(req->dst));
 
-	ret = iaa_decompress(tfm, req, wq, src_addr, req->slen,
-			     dst_addr, &req->dlen);
+	ret = iaa_decompress(tfm, req, src_addr, req->slen, dst_addr,
+			     &req->dlen);
 	if (ret == -EINPROGRESS)
 		return ret;
 
Signature · 1 line
2.56.0
author maintainer
#3 lore raw

PATCH 3/4 crypto: iaa - use RCU per-package WQ tables

In reply to Vinicius Costa Gomes

rebalance_wq_table() gives CPU n all the WQs of IAA device n / cpus_per_iaa. The flat index ignores package boundaries, so a CPU can be paired with a device on another package, and each CPU sends all its requests to one device while the other devices of its package stay idle. The table is also rebuilt in place, so a request issued during a probe or remove can read a NULL WQ and fail with -ENODEV.

Replace it with one table per package, holding the WQs of every IAA device on that package, and have each CPU round-robin over its package's table. Rebalance builds the whole set of tables and publishes it with rcu_replace_pointer(). The submit paths look the WQ up, and take their reference on it, inside an RCU read-side section.

- A package without IAA devices gets a copy of another package's table, so the lookup needs no fallback.
- Every table has room for all the WQs, a few pointers more than it needs, so they can be sized without counting per package.
- If a rebalance fails, the current tables are kept. On removal they are cleared instead, because they still hold the removed WQ. Removal then waits for a grace period before returning, because the idxd_wq can be freed after that.

---
 drivers/crypto/intel/iaa/iaa_crypto.h      |   7 -
 drivers/crypto/intel/iaa/iaa_crypto_main.c | 346 ++++++++++-------------------
 2 files changed, 117 insertions(+), 236 deletions(-)
drivers/crypto/intel/iaa/iaa_crypto.h +0 -7
index 56985e395263..aece8af011c2 100644
@@ -79,13 +79,6 @@ struct iaa_device {
 	atomic64_t			decomp_bytes;
 };
 
-struct wq_table_entry {
-	struct idxd_wq **wqs;
-	int	max_wqs;
-	int	n_wqs;
-	int	cur_wq;
-};
-
 #define IAA_AECS_ALIGN			32
 
 /*
drivers/crypto/intel/iaa/iaa_crypto_main.c +117 -229

507 lines not on this page · raw message

Signature · 1 line
2.56.0
author maintainer
#5 lore raw

PATCH 4/4 crypto: iaa - keep each CPU on one WQ for decompression

In reply to Vinicius Costa Gomes

Decompressions round-robin over the package's WQs like compressions do. Decompressions are latency-sensitive, and do better staying on a warm engine.

Have each CPU pick one WQ of its package's table the first time it decompresses, spread over the table by a per-table counter, and stay on it.

The CPU caches the index, not the WQ, so it cannot outlive the table it came from; rebalance makes every CPU pick again.

---
 drivers/crypto/intel/iaa/iaa_crypto_main.c | 40 +++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 6 deletions(-)
drivers/crypto/intel/iaa/iaa_crypto_main.c +34 -6
index 2ce545e8df42..4f2c99c099df 100644
@@ -35,12 +35,15 @@ static unsigned int nr_iaa;
 struct iaa_pkg_table {
 	struct rcu_head	rcu;
 	int		n_wqs;
+	atomic_t	decomp_next;
 	struct idxd_wq	*wqs[];
 };
 
 /* Indexed by logical package id, the count is fixed at boot */
 static struct iaa_pkg_table __rcu **iaa_pkg_tables;
 static DEFINE_PER_CPU(unsigned int, iaa_wq_idx);
+/* The WQ of its package's table a CPU decompresses on, -1 until it picks */
+static DEFINE_PER_CPU(int, iaa_decomp_wq_idx) = -1;
 
 static int iaa_cpu_pkg(int cpu)
 {
@@ -49,16 +52,35 @@ static int iaa_cpu_pkg(int cpu)
 	return (pkg >= 0 && pkg < topology_max_packages()) ? pkg : 0;
 }
 
-/* Round-robins over the WQs of the caller's package */
-static struct idxd_wq *iaa_next_wq(void)
+/* Pick the idx of the WQ on first usage on each CPU */
+static unsigned int iaa_decomp_wq_pick(struct iaa_pkg_table *table)
+{
+	int idx = this_cpu_read(iaa_decomp_wq_idx);
+
+	/* An index left over from a wider table just means picking again */
+	if (unlikely(idx < 0 || idx >= table->n_wqs)) {
+		idx = (unsigned int)atomic_inc_return(&table->decomp_next) %
+		      table->n_wqs;
+		this_cpu_write(iaa_decomp_wq_idx, idx);
+	}
+
+	return idx;
+}
+
+/* Compression round-robins over the package's WQs, decompression keeps one */
+static struct idxd_wq *iaa_next_wq(bool decomp)
 {
 	struct iaa_pkg_table *table;
+	unsigned int idx;
 
 	table = rcu_dereference(iaa_pkg_tables[iaa_cpu_pkg(raw_smp_processor_id())]);
 	if (!table)
 		return NULL;
 
-	return table->wqs[this_cpu_inc_return(iaa_wq_idx) % table->n_wqs];
+	idx = decomp ? iaa_decomp_wq_pick(table) :
+		       this_cpu_inc_return(iaa_wq_idx) % table->n_wqs;
+
+	return table->wqs[idx];
 }
 
 LIST_HEAD(iaa_devices);
@@ -779,7 +801,7 @@ static int rebalance_pkg_tables(void)
 	struct iaa_pkg_table **tables, *table, *first = NULL;
 	struct iaa_device *iaa_device;
 	struct iaa_wq *iaa_wq;
-	int pkg, n_wqs = 0;
+	int cpu, pkg, n_wqs = 0;
 
 	if (nr_iaa == 0) {
 		publish_pkg_tables(NULL);
@@ -820,6 +842,10 @@ static int rebalance_pkg_tables(void)
 	publish_pkg_tables(tables);
 	kfree(tables);
 
+	/* Have every CPU pick its decompression WQ again, from the new table */
+	for_each_possible_cpu(cpu)
+		WRITE_ONCE(per_cpu(iaa_decomp_wq_idx, cpu), -1);
+
 	return 0;
 err:
 	for (pkg = 0; pkg < topology_max_packages(); pkg++)
@@ -1412,6 +1438,7 @@ static int iaa_comp_acompress(struct acomp_req *req)
 	struct iaa_compression_ctx *compression_ctx;
 	struct crypto_tfm *tfm = req->base.tfm;
 	dma_addr_t src_addr, dst_addr;
+	bool decomp = false;
 	int ret = 0;
 	struct iaa_wq *iaa_wq;
 	struct idxd_wq *wq;
@@ -1439,7 +1466,7 @@ static int iaa_comp_acompress(struct acomp_req *req)
 
 	/* The table, and its WQs until iaa_wq_get() returns, need RCU */
 	rcu_read_lock();
-	wq = iaa_next_wq();
+	wq = iaa_next_wq(decomp);
 	ret = wq ? iaa_wq_get(wq) : -ENODEV;
 	rcu_read_unlock();
 	if (ret) {
@@ -1512,6 +1539,7 @@ static int iaa_comp_adecompress(struct acomp_req *req)
 	struct crypto_tfm *tfm = req->base.tfm;
 	dma_addr_t src_addr, dst_addr;
 	bool use_bounce_src = false;
+	bool decomp = true;
 	int ret = 0;
 	struct iaa_wq *iaa_wq;
 	struct device *dev;
@@ -1544,7 +1572,7 @@ static int iaa_comp_adecompress(struct acomp_req *req)
 
 	/* The table, and its WQs until iaa_wq_get() returns, need RCU */
 	rcu_read_lock();
-	wq = iaa_next_wq();
+	wq = iaa_next_wq(decomp);
 	ret = wq ? iaa_wq_get(wq) : -ENODEV;
 	rcu_read_unlock();
 	if (ret) {
Signature · 1 line
2.56.0
maintainer
#4 lore raw

Re: [PATCH 3/4] crypto: iaa - use RCU per-package WQ tables

In reply to Vinicius Costa Gomes
Quoted 22 lines · Vinicius Costa Gomes

rebalance_wq_table() gives CPU n all the WQs of IAA device n / cpus_per_iaa. The flat index ignores package boundaries, so a CPU can be paired with a device on another package, and each CPU sends all its requests to one device while the other devices of its package stay idle. The table is also rebuilt in place, so a request issued during a probe or remove can read a NULL WQ and fail with -ENODEV.

Replace it with one table per package, holding the WQs of every IAA device on that package, and have each CPU round-robin over its package's table. Rebalance builds the whole set of tables and publishes it with rcu_replace_pointer(). The submit paths look the WQ up, and take their reference on it, inside an RCU read-side section.

- A package without IAA devices gets a copy of another package's table, so the lookup needs no fallback.
- Every table has room for all the WQs, a few pointers more than it needs, so they can be sized without counting per package.
- If a rebalance fails, the current tables are kept. On removal they are cleared instead, because they still hold the removed WQ. Removal then waits for a grace period before returning, because the idxd_wq can be freed after that.

Quoted 5 lines
---
 drivers/crypto/intel/iaa/iaa_crypto.h      |   7 -
 drivers/crypto/intel/iaa/iaa_crypto_main.c | 346 ++++++++++-------------------
 2 files changed, 117 insertions(+), 236 deletions(-)

Please respond to this comment:

https://sashiko.dev/#/patchset/20261019-iaa-crypto-wq-tables-v1-0-fab67b2a1538%40intel.com

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt